IRC: Add option for SASL authentication. #793
Conversation
|
cc: @asdf8dfafjk |
rht
changed the title
rht
changed the title
|
Looks good, but looks like it has a merge conflict. @andersk feel free to rebase and merge if it helps you upgrade dependencies. |
|
Rebased. |
|
What’s the motivation here? Adding SASL support sounds good, but dropping non-SASL support sounds limiting? |
|
I will add back the PLAIN authentication, so as not to restrict user's choice. |
rht
force-pushed
the
irc-fix
branch
2 times, most recently
from
3d5958c
to
f1c095a
Compare
|
Rebased and the SASL authentication is now optional. |
rht
changed the title
|
|
||
| logging.basicConfig(level=logging.DEBUG) |
There was a problem hiding this comment.
This looks like a temporary debugging measure that should be removed.
There was a problem hiding this comment.
This is the log when I didn't specify the SASL password to Libera.Chat. This message wouldn't show up unless I set the level to logging.DEBUG.
DEBUG:irc.client:FROM SERVER: :sodium.libera.chat NOTICE zz_zulip :*** Notice -- SASL authentication to a NickServ account wi
th a verified email address is required to connect from your current network. Please see https://libera.chat/guides/sasl for
configuration assistance.
...
DEBUG:irc.client:FROM SERVER: ERROR :Closing Link: rein.zulipdev.org (SASL authentication to a NickServ account with a verifi
ed email address is required to connect from your current network. Please see https://libera.chat/guides/sasl for configurati
on assistance.)
There was a problem hiding this comment.
Well that’d be a side effect of logging all data sent from the server.
Instead, add an on_error handler to IRCBot.
def on_error(self, connection: ServerConnection, event: Event) -> None:
logging.error("error from server: %s", event.target)There was a problem hiding this comment.
I added on_error that raises Exception for all errors. I'm not sure why there is no default on_error on the IRCBot that reports the errors.
Currently testing by running the bridge for some period of time, to see that there are errors that are recoverable.
| @@ -1 +1 @@ | |||
| irc==18.0 | |||
| irc==20.3.0 | |||
There was a problem hiding this comment.
Should this be >= 20.3.0 or ~= 20.3?
There was a problem hiding this comment.
I changed it to ~=20.3.
rht
force-pushed
the
irc-fix
branch
5 times, most recently
from
800a27c
to
fba82f5
Compare
| @@ -144,3 +144,6 @@ def on_dccchat(self, c: ServerConnection, e: Event) -> None: | |||
| except ValueError: | |||
| return | |||
| self.dcc_connect(address, port) | |||
|
|
|||
| def on_error(self, c: ServerConnection, e: Event) -> None: | |||
| raise IRCError(e.target) | |||
There was a problem hiding this comment.
Don’t steal someone else’s exception class like this. It’s intended to be raised by the library, so its constructor interface belongs to the library and could change at the library’s discretion. For example, its constructor could gain more required arguments, like the corresponding irc.server.IRCError already has.
This additionally reverts to using sync IRC client, because upstream https://github.com/jaraco/irc only supports it for the sync client.
|
Merged without the error handling change since that needs more work and should arguably be separate anyway. Feel free to open a new PR for it. |
This reverts to using sync IRC client, because upstream https://github.com/jaraco/irc only supports it for the sync client.