Re: bug#29108: 25.3; ERC SASL support

[J.P.]

A couple potential UX concerns likely need addressing:

1. Say a user wants to change an SASL option after quitting a successful
   session. (Maybe they've added a cert fingerprint and want to try out
   EXTERNAL.) So they set that new option (globally or locally, doesn't
   matter) and attempt to /reconnect. Unfortunately, the module won't
   notice the change and will end up reusing the same parameters again.
   Granted, this isn't the end of the world, especially if the user
   knows that trying from scratch with a fresh `erc-tls' invocation is
   the way to go. But we should probably still make it a point to
   mention this somewhere.

2. Say a user who's normally cloaked is struggling to configure SASL.
   They manage to connect and obtain their desired nick, but only
   provisionally (the timeout is usually something like 30 seconds).
   Unfortunately, if they don't see the warning or can't get their act
   together before being renicked and autojoined, they'll end up sharing
   their IP.

   But what about if they've configured SASL correctly?

   It's not unheard of for a server to drop a client during registration
   for reasons unrelated to authentication, like resource pressure. But
   this module currently only stashes SASL params after making a
   successful (logical) connection, meaning perfectly good settings
   might be thrown away. In and of itself, that's arguably fine, but not
   so much when combined with auto-reconnect timers (which are then
   forced to rely solely on SASL defaults and whatever entry-point
   params and global options happen to be in play). Which ultimately
   leads to provisional nicks and leaked IPs.

   Luckily, potential remedies appear to exist. One might involve local
   handler hooks to just drop the connection on all nick-rejection
   numerics. Another might be adding some conditional reconnect logic to
   the module-init procedure that only proceeds when SASL options from a
   previously successful session are safely on the books.


BTW, also spotted a small UI issue. Something like this should fix it:

diff --git a/lisp/erc/erc-sasl.el b/lisp/erc/erc-sasl.el
index d8ef600351..227ca008ad 100644
--- a/lisp/erc/erc-sasl.el
+++ b/lisp/erc/erc-sasl.el
@@ -347,7 +347,7 @@ erc-sasl--authenticate-handler
    (s905 . "ERR SASLTOOLONG (credentials too long) %s")
    (s906 . "ERR_SASLABORTED (authentication aborted) %s")
    (s907 . "ERR_SASLALREADY (already authenticated) %s")
-   (s908 . "RPL_SASLMECHS (unsupported mechanism %m) %s")))
+   (s908 . "RPL_SASLMECHS (unsupported mechanism: %m) %s")))
 
 (define-erc-module sasl nil
   "Non-IRCv3 SASL support for ERC.
@@ -411,8 +411,9 @@ erc-sasl--destroy
 (define-erc-response-handler (908)
   "Handle a RPL_SASLALREADY response." nil
   (erc-display-message parsed '(notice error) 'active 's908
-                       '?m (alist-get 'mechanism erc-sasl--options)
-                       '?s (erc-response.contents parsed))
+                       ?m (alist-get 'mechanism erc-sasl--options)
+                       ?s (string-join (cdr (erc-response.command-args parsed))
+                                       " "))
   (erc-sasl--destroy proc))
 
 (cl-defmethod erc--register-connection (&context (erc-sasl-mode (eql t)))