coretasks: better SASL handling #1928
Conversation
This only works for SASL 3.2 spec. In 3.1, the supported mechanisms are only advertised after (and if) the client tries to use an unsupported mechanism first, and THEN the server sends a 908 RPL_SASLMECHS numeric. (And the 908 isn't required, so…)
dgw
force-pushed
the
fail-sasl-on-unadvertised-mech
branch
from
a8a007b
to
58eb6e9
Compare
Handles SASL 3.1 advertised mechanisms, sent after Sopel's configured mechanism fails, *sometimes* (the 908 numeric is sadly optional).
dgw
force-pushed
the
fail-sasl-on-unadvertised-mech
branch
from
e73d0c5
to
70e442e
Compare
|
@Exirel Yes, I saw your aside on IRC about the repeated SASL mech/password lookup code. What do you think now? 😁 |
| def _get_sasl_pass_and_mech(bot): | ||
| password = None | ||
| mech = None | ||
| if bot.config.core.auth_method == 'sasl': | ||
| password = bot.config.core.auth_password | ||
| mech = bot.config.core.auth_target | ||
| elif bot.config.core.server_auth_method == 'sasl': | ||
| password = bot.config.core.server_auth_password | ||
| mech = bot.config.core.server_auth_sasl_mech | ||
| return password, mech |
There was a problem hiding this comment.
| def _get_sasl_pass_and_mech(bot): | |
| password = None | |
| mech = None | |
| if bot.config.core.auth_method == 'sasl': | |
| password = bot.config.core.auth_password | |
| mech = bot.config.core.auth_target | |
| elif bot.config.core.server_auth_method == 'sasl': | |
| password = bot.config.core.server_auth_password | |
| mech = bot.config.core.server_auth_sasl_mech | |
| return password, mech | |
| def _get_sasl_pass_and_mech(settings): | |
| password = None | |
| mech = None | |
| if settings.core.auth_method == 'sasl': | |
| password = settings.core.auth_password | |
| mech = settings.core.auth_target | |
| elif settings.core.server_auth_method == 'sasl': | |
| password = settings.core.server_auth_password | |
| mech = settings.core.server_auth_sasl_mech | |
| return password, mech |
| def sasl_mechs(bot, trigger): | ||
| # Presumably we're only here if we said we actually *want* sasl, but still | ||
| # check anyway in case the server glitched. | ||
| password, mech = _get_sasl_pass_and_mech(bot) |
There was a problem hiding this comment.
| password, mech = _get_sasl_pass_and_mech(bot) | |
| password, mech = _get_sasl_pass_and_mech(bot.config) |
| password = bot.config.core.server_auth_password | ||
| mech = bot.config.core.server_auth_sasl_mech | ||
| # check anyway in case the server glitched. | ||
| password, mech = _get_sasl_pass_and_mech(bot) |
There was a problem hiding this comment.
OK it's kind of a nitpick here: if all you need is the settings, then just provide the settings. I've added other suggestion to reflect that change, so you can commit them in one go.
| password, mech = _get_sasl_pass_and_mech(bot) | |
| password, mech = _get_sasl_pass_and_mech(bot.config) |
|
@Exirel Review re-requested because of the new commit making three SASL handlers "unblockable", which I noticed while browsing through About changing the helper to take |
We stop at the most accurate higher level of objects. The function wants the settings for SASL. Today it's in core, maybe tomorrow that'll be in core and another section, or only in another section, etc. |
Or maybe it'll be in |
Description
Took a WIP fix for #1780 I found lying around the branches and extended it.
Checklist
make qa(runsmake qualityandmake test)Notes
Better late than never (just look how long this PR sat open…)