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

fix ircv3/ircv3-ideas#73 #490

Closed
wants to merge 2 commits into from

Conversation

slingamn
Copy link
Contributor

Add account-required cap value to draft/account-registration as per
discussion here:

ircv3/ircv3-ideas#73

Add `account-required` cap value to `draft/account-registration` as per
discussion here:

ircv3/ircv3-ideas#73
@emersion
Copy link
Contributor

One last thought: it would be a bit nicer to have this not tied to account-registration. This would allow servers to advertise that they require an account even if they don't support account registration.

@progval
Copy link
Contributor

progval commented Mar 15, 2022

Wouldn't it make more sense as value on the sasl cap?

@emersion
Copy link
Contributor

That would work, but the sasl cap values are already used for mechanism names, so we'd be mixing up two different things here. There is a theoretical potential for a conflict between the additional cap values we define (here, account-required) and the SASL mechanism names. Not sure we should care, maybe this is fine.

@progval
Copy link
Contributor

progval commented Mar 15, 2022

mechanisms are in caps uppercase, this kind of value isn't

@slingamn
Copy link
Contributor Author

This would allow servers to advertise that they require an account even if they don't support account registration.

This is actually covered by the existing semantics, I think: if you advertise account-required without advertising before-connect, then there is no way to register an account.

@emersion
Copy link
Contributor

mechanisms are in caps uppercase, this kind of value isn't

That's a fair point.

This is actually covered by the existing semantics, I think: if you advertise account-required without advertising before-connect, then there is no way to register an account.

Hm, right. It still feels a bit weird to advertise account-registration when neither REGISTER nor VERIFY are supported at all…

@slingamn
Copy link
Contributor Author

From IRC discussion, using the SASL cap value for this violates a MUST of the current ratified SASL 3.2 specification:

The value, if present, MUST be a comma (,) separated list of SASL authentication mechanisms.

since account-required is not a mechanism.

It still feels a bit weird to advertise account-registration when neither REGISTER nor VERIFY are supported at all…

It's a little weird. Here's how I'm thinking about it: the SASL spec covers, narrowly, how to tunnel SASL authentication inside the IRC C2S protocol. This spec covers policy issues related to accounts (whether they can be registered, what is required to register them, whether they're required to connect, etc.).

@emersion
Copy link
Contributor

@jwheare, do you have any preferences here?

@jwheare
Copy link
Member

jwheare commented Mar 16, 2022

I feel like this shouldn't be tied to either the SASL or draft/account-registration capabilities. It's not tied to the AUTHENTICATE, REGISTER or VERIFY flows. It might be followed up by a REGISTER flow but that's not mandatory.

I'm not sure I understand where the issue is with just using the FAIL ACCOUNT_REQUIRED error code. Catering to a use case where a client has decided to only pre connect for a list of CAPs to cache and then disconnect seems odd to me. Surely such clients could cache the presence of the FAIL code too?

@progval
Copy link
Contributor

progval commented Mar 16, 2022

@jwheare They don't get the FAIL code until they have sent NICK+USER+CAP END.

@emersion
Copy link
Contributor

@jwheare My client displays a connection UI with the following fields: "server", "nickname", "password (optional)". What I'd like to do is peek at the server caps once the "server" field is filled, and:

  • Hide the "password (optional)" field if the server doesn't support SASL
  • Make the "password (optional)" field mandatory if the server advertises that authentication is required

because it's confusing for users to see "password (optional)" when connecting to a server where the password is mandatory.

@jwheare
Copy link
Member

jwheare commented Mar 16, 2022

If the consensus is that this really really needs to be in the capabilities list, then a separate informational CAP ala STS is probably the best option imo. I'm still not sure if I'm fully on board for the necessity for this, but if we have multi stakeholder demand I'm not going to block it.

@emersion
Copy link
Contributor

@slingamn, would you be fine with that?

@slingamn
Copy link
Contributor Author

I have no objection, but I think I'll leave it to someone else to draft :-)

@slingamn slingamn closed this Mar 23, 2022
emersion added a commit to emersion/ircv3-specifications that referenced this pull request Mar 25, 2022
@kylef
Copy link
Contributor

kylef commented Mar 26, 2022

I'm still not sure if I'm fully on board for the necessity for this

I'm also in a similar boat, would an appropriate numeric or standard reply give you the same information pretty quickly during your registration attempt that having an additional capability is not needed?

For example:

C: CAP LS 302
C: NICK kylef
C: USER a b c d
S: CAP responses ...
S: ... registration is required ...

# user fixes registration and eventually triggers CAP END

@emersion
Copy link
Contributor

The goal here is to figure out whether authentication is required before connection registration. User enters hostname to connect to, client performs a CAP LS, checks for sasl/account-required, and then shows/hides the password field accordingly. I feel like most people skeptical about this new cap don't really understand the use-case? I can post UI screenshots if that helps.

@kylef
Copy link
Contributor

kylef commented Mar 26, 2022

@emersion In my example above, the client would receive the information that registration is requored alongside the CAP LS output. The client has that informtion about the same time, doesn't this suffice for your use-case?

@emersion
Copy link
Contributor

I don't know which nickname/username/realname the user wants to pick at that point. I can't send NICK nor USER. I only know about the hostname, nothing else.

@emersion
Copy link
Contributor

I've posted some screenshots at #492 (comment).

emersion added a commit to emersion/ircv3-specifications that referenced this pull request Apr 6, 2022
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.

5 participants