need_sasl message is misleading for unverified accounts

[felsokning]

Steps to Reproduce:
Register a nickname. Do not verify the account. Connect from an auth block with the need_sasl flag, with SASL properly configured.

Repro:

01:01 -!- SASL authentication succeeded
01:01 !orwell.freenode.net *** Notice -- You need to identify via SASL to use this server

Suspected Root Cause:
The check that throws the error looks for the config to be enabled and a char array to be instantiated.

suser is defined in client.h, for everyone following along at home.

When a user signs-on, it looks like you pass a flag, *, to validate the user is registered and that initiates the copy of the logon name to the suser char array, seen here.

I suspect you have a check for authUsed == authRequired which will also populate the char array if the correct/expected auth mechanism is used but I didn't get too far/deep into the auth negotiation part of it. In principle, that part should still fail the same way (for incorrect auth path) and is, principally, why I suggested a check for verification should be included and tossing a different error (via another path, maybe on login?).

Expected Behaviour:
The client should receive a message that indicates the actual cause of the failure.

Actual Behaviour:
The client connects and, subsequently, the check happens for the char array being populated and only then is the client disconnected by the server (after having already made a successful connection).

(edited reproduction steps – @ilbelkyr)
(edited suspected root cause and expected behaviour - @felsokning)

[ilbelkyr]

The ircd doesn't know about verified/unverified; instead, services do not propagate logins for unverified accounts to the ircd. However, you can still log in, including via SASL.

(The SVSLOGIN message uses a * parameter to indicate "no change"; thus, while the user will have authenticated against services, their ircd-side account name will remain unset.)

Successfully authenticating is not a bug, but the generic "you need to identify via SASL" message is indeed very unhelpful in this case.

[felsokning]

Aye, I modified it a bit. I suspect you use that path for auth coditions that aren't met (specifically, not using SASL when it's required based on the connection) and that should definitely remain intact.

[ilbelkyr]

We check whether the user has an account name set, which indicates they've authenticated to a verified account; at the point we check that, this is only possible via SASL, hence calling the option need_sasl. (It's not actually about enforcing SASL as much as it is about enforcing having a (verified) account.)

I suspect the easiest thing to do would be to keep track of whether the user has successfully completed a SASL exchange, and if they have (but their account name is still unset), assume they're not verified.

…Actually, the truly easiest thing would be to adjust the message to say "You need to use SASL with a verified account to use this server" no matter what. This wouldn't be as good for networks that don't use email verification, but freenode does. It's also a little less helpful to users in figuring out what exactly the issue is, but admittedly still better than the current message.