Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add DCLOGIN uri scheme #48

Merged
merged 10 commits into from
Sep 28, 2022
Merged

add DCLOGIN uri scheme #48

merged 10 commits into from
Sep 28, 2022

Conversation

uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated Show resolved Hide resolved
@r10s
Copy link
Member

r10s commented Jul 6, 2022

for cbor: one should also take into account, that the code needs to be generated somehow, by ppl not that deep into development maybe, cbor is not the best choice in that regard. it is not that know and has less support than url-parameter or base64 or json.

in general, i am wondering if json is a good format for this purpose.

using the url-parameter-format as for the other schemes has some advantages:

  • it is shorter, qr-codes can be smaller and will therefore be easier to scan in average
  • it uses less weird characters - and the ones used are url-save so that only the values needs encoding when used as an url-scheme (as we will do with some chance) - cmp.
    DCLOGIN:e=me@example.com&p=password vs .
    DCLOGIN:{"email":"me@example.com","password":"password"}#1 and in an url: DCLOGIN:e=me%40example.com&p=password vs. DCLOGIN:%7B%22email%22%3A%22me%40example.com%22%2C%22password%22%3A%22%22%7D#1
    the first variants are shorter and better to read (json could also be a bit shorter when using shorter keys, however, this is quite unusual)
  • the url-parameter-format is already in use for the other qr-codes, uri-(de)escaping is not great, but already needed anyway

of course, there are also drawbacks :) at least i want to bring up this point more clearly, i've seen it is already partly discussed above :)

interesting, while not totally matching: https://datatracker.ietf.org/doc/html/rfc5092

@flub
Copy link
Member

flub commented Jul 7, 2022

given the available size and the scanning by other apps i guess i feel more for using a plain URI format that's well known. Could flatten everything to query parameters etc, which is all well supported in many languages also for generation like r10s suggests.

@Simon-Laux
Copy link
Member Author

Simon-Laux commented Jul 9, 2022

So you are all more in favour of implementing the uri encoding/decoding?

for cbor: one should also take into account, that the code needs to be generated somehow, by ppl not that deep into development maybe, cbor is not the best choice in that regard. it is not that know and has less support than url-parameter or base64 or json.

uri encoding/decoding is easily forgotten and then people wonder why it does not work. (like it worked with this simple password, but does not work with my super secure passwords that contain special characters (which I don't know that they need special encoding so I blame it on DC))

So going for a binary format forces people to think about the documentation. Also I would create a web tool to generate it.

it is not that know and has less support than url-parameter or base64 or json.

msgpack(https://msgpack.org) and cbor(https://cbor.io/impls.html) have implementations for most popular programming languages.

The thing about json (or binary json) is that it is easier for us to work with, than uri encoding which can have a someencoding edge cases like macOS encoding the #. (see deltachat/deltachat-core-rust#1969)
or the difference between encodeURIComponent and encodeURI (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent#description) which can be quite confusing when implementing it.

The idea of the version part is that we can change the format, if we decide on the uri-scheme format from the start we could also put the version into a query parameter, I believe we should still have a version, especially if we later might want to allow stuff like oauth authentication in those links/qr-codes, and then we need a way for the old versions of deltachat to tell to the user "this requires a newer version of DC, please update".

an uri based version was already proposed in the forum:

dclogin://password:name@example.com?smtpserver=loginname:password@smtp.example.com:1234&imapserver=loginname:password@imap.example.com:5678&smtpsecurity=ssltls&imapsecurity=ssltls&authmethod=auto&certchecks=auto

which follows the url conventions a bit, kinda like (https://datatracker.ietf.org/doc/html/rfc5092)

There is still the question of how to shorten the parameters, but I don't think it is even needed (we have ~1000 chars, with all parameters set we might get 400-500), also shortening them to single letters makes the format more complicated again.

I'd be in favour of the url based variant though I'm worried the escaping stuff will get too complicated.

@r10s
Copy link
Member

r10s commented Jul 9, 2022

thanks for all the considerations!

maybe, the need of a webtool seems to underline that things are designed too complex :)

also, having a webtool does not really help if you want to generate the code automatically somehow. also, this is questionable security wise as one has to enter the password on some platform (even if that is not uploaded somewhere, one has to explain that etc.) (or install the tool locally, which is also some effort)
i also do not think that urlencode is often forgotten or a real issue to most devs - and even if, the dev has to learn that then, i mean this is a cornerstone of the whole web, it is needed everywhere and also very visible. you need to learn that anyway soon as a web developer. in contrast to that, cbor. msgpack or whatever is a much bigger hurdle for much fewer impact when new to web development.

i would not die on that hill, tbh, i am also quite a bit unsure about the whole scope of DCLOGIN and why it is needed now, however, i would go the easy path.

another thing: consider also to adapt the existing DCACCOUNT scheme as that is handled more and more by the different UIs. from the view of the UI, both things work the same i think?

EDIT: ahh: and there is currently also a BACKUP-TRANSFER QR-code evolving: deltachat/deltachat-core-rust#3489 - at least that targets a similar field.

@Simon-Laux
Copy link
Member Author

, i am also quite a bit unsure about the whole scope of DCLOGIN and why it is needed now,

It's needed for a project.

another thing: consider also to adapt the existing DCACCOUNT scheme as that is handled more and more by the different UIs. from the view of the UI, both things work the same i think?

data at some web endpoint (DCACCOUNT) vs data inside in the qrcode (DCLOGIN). Yes UI is similar, maybe different wording but apart from that it can share the same code base.

EDIT: ahh: and there is currently also a BACKUP-TRANSFER QR-code evolving: deltachat/deltachat-core-rust#3489 - at least that targets a similar field.

yeah similar field, but also very different

So I will update this pr to propose a uri based approach soon, binary brings it's own complexities so we can try the uri encoding stuff.

@Simon-Laux
Copy link
Member Author

Changed it to a fully URI based approach, what do you think now?

uri-schemes.md Outdated
dclogin://user:password@host?options
dclogin:user:password@host/?options
# example: (email: me@example.com, password: securePassword)
dclogin://me:securePassword@example.com?v=1
Copy link
Member

@adbenitez adbenitez Jul 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if it is a thing but IIRC some servers use only the local part of the email address as the username to log in, how would that be specified here? or this is only for servers that the login is with the email address and that the server address ends with @host and host is the right imap/smtp host? I guess it will have then to do the DC autoconfig to detect right host and ports

personally would have preferred something like dclogin://addr:password?mail_server=...&send_server=...&(other dc config options here)

Copy link
Member

@adbenitez adbenitez Jul 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then calling it dcsetup: would makes more sense since you could setup several config options for the user, would be more useful for admins setting up servers for easy onboarding, example if creating the DC account is triggered from inside another service /account and then you could preconfigure the same username that the user has in that other service

| ---------- | ------------------------- | --------------------------------------------------- | --------------------- |
| `v` | `version` | defines the format version, more explanation below | `v=1` |
| ---------- | ------------------------- | --------------------------------------------------- | ---------------- |
| `ih` | `imap_host` | IMAP host | `ih=imap.example.com` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh ok so actually the host can be specified in options, then splitting the email address in local part and host kinda feels unnecessary 🤔

Copy link
Member Author

@Simon-Laux Simon-Laux Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's because url spec has username:password@host as format, so to be consistent with other schemes/standards

Copy link
Member

@r10s r10s Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following this "auth spec" seems not to be needed and does not really match anyway as we have to deal with potentially two different usernames and passwords. and if we follow: the part before the colon is the username, so that would be the full email address in many, but of course not all cases.

so, trying to reconstruct the email address and username from different parts is doable, but would results in quite some "if" and unneeded complexity.

it seems much easier to have a dedicated field for the email address: DCLOGIN:?a=foo%40bar.de&ipw= password just map everything 1:1 to the existing config fields. having that as a parameter also eliminates doubts how and if to urlencode, it is just all the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need to "simplify" everything that much,

maybe DCLOGIN:email@example.com?pw=1234&v=1, but I don't see the big benefit in specifying everything in the query part, though we could do it, minimum version (no advanced options) would then look like:

DCLOGIN:?a=email@example.com&p=1234&v=1

and when reusing the existing imap password field (though that might be more annoying to explain than an extra property):

DCLOGIN:?a=email@example.com&ipw=1234&v=1

BTW: the v=1 is there for future proofing, that we can increase this number to force specific advanced options like oauth2 or similar in the future, if it's a number not supported by dc it will tell the user to update the app.

Copy link
Contributor

@missytake missytake Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also can have separate passwords for IMAP & SMTP, right? So what about recommending DCLOGIN://user:password@example.com/?v=1 as default, not having a p= option at all, but instead smtp_password= and imap_password= options, which override the password in the "authority" field?

That would feel most intuitive for me, and kind of maps the login field with the advanced login settings.

edit: I just saw that there are smtp_password and imap_password parameters already 🙃 I like to have the password in the front, as it makes it clearer what overrides which imho.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really willing to change everything at this stage again, r10s is right that it might be more complicated to do it in the userinfo part (because URI encoding and stuff like that, passwords tend to contain special characters)

uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated Show resolved Hide resolved
uri-schemes.md Outdated
Comment on lines 143 to 151
dclogin:user@host?p=password&v=1[&options]
dclogin:user@host/?p=password&v=1[&options]
dclogin://user@host/?p=password&v=1[&options]
# example: (email: me@example.com, password: securePassword)
dclogin://me@example.com?p=securePassword&v=1
# example: (email: myself@example.com, password: url/Encoded\@passw@rd)
dclogin://myself@example.com?p=url%2FEncoded%5C%40passw%40rd&v=1
# example: (email: myself@example.com, password: 123456, insecure smtp at different server)
dclogin://myself@example.com?p=123456&v=1&sh=mail.example.com&sc=3&ss=plain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're diverging a bit far from the URI syntax.

I'd refer to the URI spec and say:

  • must have userinfo
  • must have host
  • must be single / for path
  • query parameters ad defined

This means example 1 and 2 should be considered invalid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so path is allowed to be emtpy, may as well omit / in that case.

Copy link
Member Author

@Simon-Laux Simon-Laux Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So force using :// at beginning and no / for the path?
It is parsed by an URL parser lib we already use in core so path is just ignored anyway but we could still remove that example from the spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That the existing parser lib parses it is already great. As usual being strict in the spec but liberal in parsing is a good thing probably.

Yes to requiring :// as separator (though if parsing without works then that's great), so I'd remove the examples without it. All the other examples are already following the URI spec I think.

And maybe it's also worth specifically describing the syntax wrt to URI schema. Something like:

The generic URI schema is:

URI = scheme ":" ["//" authority] path ["?" query] ["#" fragment]
authority = [userinfo "@"] host [":" port]

For DCLOGIN this means:
- *scheme* is always dclogin
- There must be an *authority* section
  - The *userinfo* part is required.
- The *path* may be omitted or be a single `/`.
- The fragment must be omitted.
- The query parameters ... (as currently described)

The benefit of this is that it explicitly acknowledges the URI format and signals that we're aiming to be compatible. Though I won't mind too much if you don't agree with this.

| short name | stands for | description | example |
| ---------- | ------------------------- | --------------------------------------------------- | --------------------- |
| `v` | `version` | defines the format version, more explanation below | `v=1` |
| `p` | `password` | required in version 1, password of the account | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not require p= in the options if it is supplied in the userinfo part.

@Simon-Laux Simon-Laux merged commit 6a79c5f into master Sep 28, 2022
@Simon-Laux Simon-Laux deleted the add-dclogin-scheme branch September 28, 2022 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants