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

Account Management Framework #276

Closed
wants to merge 29 commits into from

Conversation

@DanielOaks
Copy link
Member

DanielOaks commented Nov 7, 2016

The ACC command framework provides a standardized way for clients to create and manage accounts. In particular, the ACC REGISTER and ACC VERIFY subcommands should allow a network to provide a consistent interface to register new accounts, no matter what services software is used on the backend. They also let clients build nice interfaces for account registration. This framework is extensible, letting us provide more commands and interfaces in future.

This is the same spec as in #152 but cleaned up and with some bugs fixed. Asked kaniini if I could submit some fixes a while back and he said I should just throw a new PR in.

edit: I think the last thing missing here is the ability to know whether the net uses your nickname as the account name or not. I'll take a look at that and get something worked out there

extensions/reg-core-3.3.md Outdated Show resolved Hide resolved
extensions/reg-core-3.3.md Outdated Show resolved Hide resolved
extensions/reg-core-3.3.md Outdated Show resolved Hide resolved
extensions/reg-core-3.3.md Outdated Show resolved Hide resolved
extensions/reg-core-3.3.md Outdated Show resolved Hide resolved
Thanks to @ProgVal for the suggestions.
@DanielOaks
Copy link
Member Author

DanielOaks commented Nov 17, 2016

@ProgVal I'll look at making sure the error codes are in the given list, but how do those changes look to you? Should clean up the language in those sections a fair bit

These changes allow REG to be better used on networks that explicitly link accounts to nicknames.
@ProgVal
Copy link
Contributor

ProgVal commented Nov 18, 2016

Why do you want to prevent the client from setting a fingerprint that is not the one of its current certificate?
We will have to allow that at some point if we want to allow users to change certificates or use more than one.

@DanielOaks
Copy link
Member Author

DanielOaks commented Nov 18, 2016

Different networks can use whatever fingerprint methods they want for certs. In addition, allowing clients to register with fingerprints other than one that they explicitly control sounds a bit dodgy in terms of spoofing other users. Say that you know the fingerprint of my cert – you register an account with the fingerprint in the certfp's cred field and now I can't register an account linked to my cert because one already exists for it.

Because of that and the difficulty of conveying which algorithm is being used. Does certificate fingerprinting use standardised algorithms like SHA1/SHA256 always or are there cases where people can do whatever special stuff they want, say using both SHA1 and SHA256 to better protect against dodgy collisions? Is there a 100% solid way that we know to convey exactly which method is being used to generate the fingerprint, or should we instead also define a numeric that dumps what your certfp is on connection, so that clients can then use that later in the REG CREATE command?

Just using the client's current cert makes the most sense to me, though if there are answers for all these then I'm open to investigating it further.

@grawity
Copy link
Contributor

grawity commented Nov 18, 2016

Existing services already allow "claiming" any fingerprint anyway. But I like the restriction because it can reduce accidents – if you're only allowed to use your current cert, then registering implies you can successfully connect with it and won't end up with an unusable account.

@DanielOaks
Copy link
Member Author

DanielOaks commented Nov 18, 2016

Updating your cert would be handled separately, if appropriate could be through another REG command we describe later (similar to updating your email address perhaps).

@ProgVal
Copy link
Contributor

ProgVal commented Nov 18, 2016

But to update the cert, you have to be able to give a fingerprint at some point. Unless you find a way of connecting with the old and the new cert at the same time (IRC in TLS in TLS?), but that may be hard to implement in some/most clients.

@DanielOaks
Copy link
Member Author

DanielOaks commented Nov 19, 2016

I think restricting registration to the certificate you currently have makes sense, for reasons @grawity put above. For updating the cert on an existing account that could definitely make sense, but I think looking at that is the job of another spec (which may also look at updating passphrase/etc for the account as well).

@ProgVal
Copy link
Contributor

ProgVal commented Nov 19, 2016

Ok

extensions/reg-core-3.3.md Outdated Show resolved Hide resolved
@jwheare jwheare added the accounts label Jan 7, 2017
@jwheare jwheare modified the milestone: Roadmap Jan 7, 2017

A `REG VERIFY` command consists of the following format:

REG VERIFY <accountname> <auth_code>

This comment has been minimized.

Copy link
@Zarthus

Zarthus Jan 7, 2017

I think this should include the (optional?) password if we're going to immediately log the user in [L111]. Or indicate that this command is only available while already logged in (how do you log in?) (in which case the sentence below "the irc server should also send an rpl_loggedin ..." would no longer be valid)

There are many users who accidentally paste their authentication code in chat.

@Zarthus
Copy link

Zarthus commented Jan 7, 2017

Seems to be missing a way to drop the existing nickname. What if I misspell my email?

@DanielOaks
Copy link
Member Author

DanielOaks commented Jan 7, 2017

That's handled by a separate spec. This is just about registering your account

@prawnsalad
Copy link

prawnsalad commented Jan 7, 2017

C: REG CREATE * mailto:dan@example.com passphrase :testpassphrase123
S: 920 dan- dan- :Account registered
S: 927 dan- dan- dan@example.com :A verification code was sent

From the client dev perspective, receiving the 920 when verification is still required can cause issues in the UI.

  • User attempts to register, UI shows a loading sign
  • Client receives the 920, UI shows "congrats" screen
  • Client must wait for an arbitrary length of time incase it receives a 927, UI must now change to "Sorry, you're not actually finished. verify now".

Two possible ways to improve this would be to either send 927 before 920, or to send one or the other but never both. The latter would be cleaner IMO.

@DanielOaks
Copy link
Member Author

DanielOaks commented Jan 7, 2017

920's sent when it's successful, so if they do mailto: then it'd send that after they send the REG VERIFY command, not after REG CREATE. If they REG CREATE with skipping verification, they wouldn't get 927 since it requires no verification code to be sent/etc.

@glguy
Copy link

glguy commented Feb 5, 2019

If the supported callback and credential types were advertised in CAP LS a client could know what options to present to the user in the case that account registration was to be done before connection-registration. 005 is too late for this.

Examples with and without CAP 302

CAP * LS :acc
CAP * LS :acc=REGCREDTYPES=passphrase,certfp;REGCALLBACKS=mailto,sms

Making this extension opt-in via CAP negotiation ensures that the new, more descriptive error messages using FAIL aren't sent to clients that don't understand them and expect the current error numerics. Many clients I've used, particularly mobile ones, completely ignore messages they don't expect or understand. Changing error messages in cases like joining a channel that requires registration could cause that failure to turn into a silent failure.

It would be slightly more work for server implementations to know whether they should send a standard numeric error or the new FAIL error, but it should just be a simple case of checking negotiated capabilities.

It will be no more work for clients to have a negotiated capability because clients will need to continue to support servers that don't implement this extension. The numeric errors sent when joining a registered-users-only channel will continue to exist in the wild. A client negotiating this capability would be declaring the ability to handle the new error message format.

extensions/acc-core.md Outdated Show resolved Hide resolved
@DanielOaks DanielOaks changed the title Add register-and-verify extension ACC Command Framework Apr 3, 2019
@DanielOaks
Copy link
Member Author

DanielOaks commented Apr 3, 2019

So, this is essentially a complete rewrite of the ACC Command Framework spec with the (lots of) feedback kept in mind. No longer relies on ISUPPORT, no longer defines a ton of error numerics and makes use of the FAIL command instead, etc. I think this is much easier to implement, I rewrote the confusing / arbitrarily complex text, and it should support the use cases a lot better in general now.

Let us know what you think.

@xPaw
Copy link

xPaw commented Apr 3, 2019

Is there a reason why this is called acc and not just account or something like that?

Even looking at the title The ACC command framework gives me no meaningful information at to what this framework is supposed to do.

@DanielOaks
Copy link
Member Author

DanielOaks commented Apr 3, 2019

The account-notify spec already defines the ACCOUNT verb. I can change the title to something better regardless of the actual command name, though.

@DanielOaks DanielOaks changed the title ACC Command Framework Account Management Framework Apr 3, 2019
@DanielOaks
Copy link
Member Author

DanielOaks commented Apr 3, 2019

Bit of a better name, if we wanna brandy alternative suggestions about we can though.

@DanielOaks
Copy link
Member Author

DanielOaks commented Apr 8, 2019

Oragono now implements the updated spec as of the latest git

@DanielOaks
Copy link
Member Author

DanielOaks commented Jul 3, 2019

Mention from the channel on something this could use: "it has no provision afaict for separated verification measures (think "go to this URL and solve a captcha" or "generate this proof of work")"

@DanielOaks
Copy link
Member Author

DanielOaks commented Jul 3, 2019

This tries to do a lot and I think misses the mark in some ways and would probably be better getting rewritten. Especially this long with no real implementations sorta shows that it's probably better to get a more well-defined look at what specifically we should solve in a single spec (e.g. lump registration as well as authentication changing, maybe ^ and whatever other registration/cred-specific things come up), rather than trying to add on later with patch specs like this one's tried to.

I dunno, I'll submit something to -ideas along the lines of a problem statement so we can better nail down what we want out of this and see if there's any actual interest from the not-client side.

@DanielOaks DanielOaks closed this Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.