*** Original post on bio 1811 by Rob McAuley <rmcauley AT gmail.com> at 2012-11-19 21:58:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***

*** Original post on bio 1811 as attmnt 2106 by rmcauley AT gmail.com at 2012-11-19 21:58:00 UTC *** The password field for an IRC account does not adapt to server password authentication, such as that used to authenticate with ZNC servers. Attached is a protocol dump of what happens when no password is specified, so that you can see what ZNC is sending back.

*** Original post on bio 1811 as attmnt 2107 at 2012-11-19 23:23:00 UTC *** If we get the ZNC error message, we take the current password and set it to the serverPassword, clear out the current password and reconnect. This patch also handles if there is an invalid password.

*** Original post on bio 1811 at 2012-11-20 10:22:33 UTC *** Is the patch correct? I'm asking because the server message tells about sending "username:password" with the PASS command and you're only copying the account password into the serverPassword preference, leaving out the user's name.

*** Original post on bio 1811 at 2012-11-20 11:24:31 UTC *** (In reply to comment #2) > Is the patch correct? I'm asking because the server message tells about sending > "username:password" with the PASS command and you're only copying the account > password into the serverPassword preference, leaving out the user's name. That's up to the user to configure. I can't guess whether they already put in username: or not into the password field. (Also, for the test server Rob set up for me, I don't type in username:password.)

Comment on attachment 8353868 [details] [diff] [review] Patch *** Original change on bio 1811 attmnt 2107 at 2012-11-20 11:44:23 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353868 [details] [diff] [review] Patch *** Original change on bio 1811 attmnt 2107 at 2012-11-20 13:16:53 UTC *** This is a good way of handling it :) Not part of this bug, but I'm wondering whether it might not be a good idea after all to expose the server password in the advanced options (and store it in the password manager). It may appear a bit "magical" for the password to "disappear" from the account options, and yet still work...

*** Original post on bio 1811 at 2012-11-20 13:18:08 UTC *** (In reply to comment #4) > Not part of this bug, but I'm wondering whether it might not be a good idea > after all to expose the server password in the advanced options (and store it > in the password manager). It may appear a bit "magical" for the password to > "disappear" from the account options, and yet still work... We need to implement the masked option in the UI then, this was the "long term" solution in the other bug.

*** Original post on bio 1811 at 2012-11-20 13:38:07 UTC *** (In reply to comment #4) > in the password manager). It may appear a bit "magical" for the password to > "disappear" from the account options, and yet still work... Also an issue if the password had a typo and needs to be changed, and the user doesn't know about the existence of the pref.

Comment on attachment 8353868 [details] [diff] [review] Patch *** Original change on bio 1811 attmnt 2107 at 2012-11-21 00:05:53 UTC *** This patch is based on the assumption that the user's configuration is wrong and needs to be fixed/migrated. I dislike that assumption. I think when receiving the ZNC AUTH message, we should just send the PASS command from the ZNC handler (I expected to see something in the file where we handle random services whose messages we want to eat rather than in ircBase.jsm), and set a variable somewhere that tells us to not attempt to identify at all with nickserv (clokep said: just set "isAuthenticated" to true to not send anything to NickServ). Disconnecting the account and reconnecting from scratch really feels like a hack we would have in an add-on but avoid in the core. I discussed this briefly with clokep, he said "I don't think we can send the PASS command at that point." which is probably right in the general IRC case, but we know we are connected to a ZNC bouncer, and it specifically asked us to do just that, so I think we should just do it. If the approach I'm suggesting here turns out to be impossible or horribly difficult to implement, I may retract this r-.

*** Original post on bio 1811 at 2012-11-21 13:47:06 UTC *** (In reply to comment #7) > This patch is based on the assumption that the user's configuration is wrong > and needs to be fixed/migrated. I dislike that assumption. It is, however, quite possibly the case. 22:00:31 <aleth> Can you think of a situation where you'd need a separate password (later) to the one you use for ZNC auth? 22:01:12 <LiquidRain> If ZNC is not configured to handle NickServ auth, then yes. 22:01:27 <LiquidRain> however I would imagine most ZNC users are competent enough to config ZNC to do that for them 22:03:45 <LiquidRain> I think it's probably a better idea to have it not try to do nickserv for us after that 22:04:03 <LiquidRain> In my case my ZNC password is different from my Nickserv password 22:04:29 <LiquidRain> (and ZNC already has me authorized under my nick before I even connect using IB) Migrating the account password to the server password (maybe checking if the latter is empty) is the right thing to do, certainly for those who don't have ZNC set up to handle NickServ (if we don't need to make that assumption, we shouldn't) it allows them to also set the account password. But of course we can also just decide sending the account password when requested is good enough. > ircBase.jsm), and set a variable somewhere that tells us to not attempt to > identify at all with nickserv (clokep said: just set "isAuthenticated" to true > to not send anything to NickServ). Using isAuthenticated for this will not help if the user changes their nick and we try to reauthenticate.

*** Original post on bio 1811 as attmnt 2113 at 2012-11-21 16:07:00 UTC *** This is a different version that Florian asked for. It does NOT attempt to stop authentication to NickServ, currently.

Comment on attachment 8353874 [details] [diff] [review] WIP v1 *** Original change on bio 1811 attmnt 2113 at 2012-11-21 16:21:26 UTC *** I like this much better, thanks! (In reply to comment #9) > It does NOT attempt to stop authentication to NickServ, currently. Is it because it's WIP, or because it's difficult/impossible to do for some reason?

*** Original post on bio 1811 as attmnt 2115 at 2012-11-21 16:37:00 UTC *** Avoids authenticating with NickServ if logging into ZNC.

Comment on attachment 8353874 [details] [diff] [review] WIP v1 *** Original change on bio 1811 attmnt 2113 at 2012-11-21 16:37:19 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1811 by Rob McAuley <rmcauley AT gmail.com> at 2012-11-21 17:25:15 UTC *** As the person who commented in IRC that my NickServ password is different, I do very much appreciate IB not trying to auth with NickServ for me on connect. ZNC handles that for me, and IB would only generate errors from NickServ every time it connects to my ZNC server. The holy grail here honestly would be to just let us handle username & server password ourselves in an exposed pref in Advanced, as even Pidgin lets us do, but it seemed like that wasn't the direction you wanted to take IB when it was discussed in chat.

*** Original post on bio 1811 at 2012-11-21 17:29:28 UTC *** (In reply to comment #12) > The holy grail here honestly would be to just let us handle username & server > password ourselves in an exposed pref in Advanced, as even Pidgin lets us do, > but it seemed like that wasn't the direction you wanted to take IB when it was > discussed in chat. With the patch as is, this uses the password you would have typed in for the account (or the serverPassword hidden preference). The username is still a hidden preference though. Is this not adequate? Yes, you still have to go into about:config for the username, but I really don't want to expose that in the UI. I think 90% of users won't care to change it.

*** Original post on bio 1811 by Rob McAuley <rmcauley AT gmail.com> at 2012-11-21 17:36:43 UTC *** I read the patch - and yes, it's adequate, but only adequate. :) My comment was just that, a comment, based on some of the discussion above re: NickServ.

*** Original post on bio 1811 at 2012-11-21 18:25:27 UTC *** (In reply to comment #12) > The holy grail here honestly would be to just let us handle username & server > password ourselves in an exposed pref in Advanced Would you like to write an add-on for that? By the way, thanks for the feedback!

Comment on attachment 8353876 [details] [diff] [review] Patch v2 *** Original change on bio 1811 attmnt 2115 at 2012-11-21 18:46:56 UTC *** I like this, but feel aleth should have a look too. >Bug 955244 (bio 1811) - IRC password does not work with ZNC/server password auth, r=aleth. (+ he's already the reviewer in the check-in comment ;)) I have 2 "possible nits" but I'm not even sure they should be addressed as both these points are debatable. >diff --git a/chat/protocols/irc/ircNonStandard.jsm b/chat/protocols/irc/ircNonStandard.jsm >+ "NOTICE": function(aMessage) { >+ // NOTICE <msgtarget> <text> >+ // If we receive a ZNC error message about passwords. >+ if (aMessage.params[0] == "AUTH" && >+ aMessage.params[1] == "*** You need to send your password. Try /quote PASS <username>:<password>") { You could reverse the test to return early and reduce the indent level (in that case, also change the comment to something like "We only handle ZNC password error messages here"). This may not be wanted if you expect there will be more code in that method soon. >+ if (this.imAccount.password) { >+ // Send the password now, if it is available. >+ this.shouldAuthenticate = false; >+ this.sendMessage("PASS", this.imAccount.password, >+ "PASS <password not logged>"); >+ } >+ else { >+ // Otherwise, put the account in an error state. >+ this.gotDisconnected(Ci.prplIAccount.ERROR_AUTHENTICATION_IMPOSSIBLE, >+ _("connection.error.passwordRequired")); >+ } >+ return true; >+ } >+ return false; >+ }, >+ >diff --git a/chat/protocols/irc/ircServices.jsm b/chat/protocols/irc/ircServices.jsm > "001": function(aMessage) { // RPL_WELCOME > // If SASL authentication failed, attempt IDENTIFY. >- if (this.imAccount.password && !this.isAuthenticated) { >+ if (this.imAccount.password && this.shouldAuthenticate && >+ !this.isAuthenticated) { I think this.imAccount.password may trigger a master password prompt, so we may want to test this.shouldAuthenticate before that. This comment only makes sense if: - this is the first time this.imAccount.password is used in the connection process - we expect other future use cases of shouldAuthenticate, as in the ZNC case we can reasonably expect that there's a password anyway...

*** Original post on bio 1811 as attmnt 2120 at 2012-11-22 04:37:00 UTC *** This is the same patch as attachment 8353876 [details] [diff] [review] (bio-attmnt 2115), but unbitrotted from bug 955198 (bio 1766).

Comment on attachment 8353876 [details] [diff] [review] Patch v2 *** Original change on bio 1811 attmnt 2115 at 2012-11-22 14:01:05 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353881 [details] [diff] [review] Patch v2 unbitrotted for commit *** Original change on bio 1811 attmnt 2120 at 2012-11-22 14:09:26 UTC *** >+ "NOTICE": function(aMessage) { >+ // NOTICE <msgtarget> <text> >+ // If we receive a ZNC error message about passwords. >+ if (aMessage.params[0] == "AUTH" && >+ aMessage.params[1] == "*** You need to send your password. Try I agree this test should be reversed to return early if this handler has nothing to do. We should also improve the comment so our thinking here is not forgotten - something like "If we receive a ZNC password request, the server password pref was not set by the user, so we try to login to ZNC with the account password". I couldn't test this of course. I am assuming that if the server password *is* set we don't receive this password request message from ZNC (because we have sent PASS)? Looks good otherwise!

*** Original post on bio 1811 as attmnt 2122 at 2012-11-22 15:37:00 UTC *** Meets nits from aleth.

Comment on attachment 8353868 [details] [diff] [review] Patch *** Original change on bio 1811 attmnt 2107 at 2012-11-22 15:37:16 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353876 [details] [diff] [review] Patch v2 *** Original change on bio 1811 attmnt 2115 at 2012-11-22 15:37:16 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353881 [details] [diff] [review] Patch v2 unbitrotted for commit *** Original change on bio 1811 attmnt 2120 at 2012-11-22 15:37:16 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353883 [details] [diff] [review] Patch v3 *** Original change on bio 1811 attmnt 2122 at 2012-11-22 15:41:18 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1811 at 2012-11-25 01:09:52 UTC *** Committed as http://hg.instantbird.org/instantbird/rev/ef98956be637

*** Original post on bio 1811 as attmnt 2131 at 2012-11-25 13:42:00 UTC *** Fix breakage caused by a bad merge between this and bug 955198 (bio 1766). :(

Comment on attachment 8353892 [details] [diff] [review] Breakage fix *** Original change on bio 1811 attmnt 2131 at 2012-11-25 13:44:50 UTC *** Should have caught this on review... Thanks for the fix.

*** Original post on bio 1811 at 2012-11-25 23:38:02 UTC *** (In reply to comment #21) > Created attachment 8353892 [details] [diff] [review] (bio-attmnt 2131) [details] > Breakage fix Committed as http://hg.instantbird.org/instantbird/rev/af7d99904021