Closed Bug 955721 Opened 7 years ago Closed 7 years ago

Login fails using znc without setting serverPassword

Categories

(Chat Core :: IRC, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: clokep)

Details

Attachments

(3 files, 2 obsolete files)

*** Original post on bio 2270 by Alexis Metaireau <alexis+bugs AT mozilla.com> at 2013-12-09 14:54:00 UTC ***

When configuring instantbird for the first time, if you're using a znc bouncer, it's hard to figure out what's going wrong.

Part of the reason is that the options that has to be sent to the bouncer aren't exposed in the ui.

The current way to solve this is to go in the about:config section and enter

messenger.account.account1.options.username : username/network
messenger.account.account1.options.serverPassword : pass

Here is the message you get from the server:

[09/12/2013 15:36:57] DEBUG (@ prpl-irc resource://gre/components/irc.js:687)
{"rawMessage":":irc.znc.in 464 alexis :Password required","command":"464","params":["alexis","Password required"],"servername":"irc.znc.in"}
[09/12/2013 15:36:57] LOG (@ prpl-irc resource:///modules/socket.jsm:182)
Disconnect
*** Original post on bio 2270 at 2013-12-09 15:01:40 UTC ***

We can probably send PASS in response to 464, but I don't know if the connection is still open at this point. (I'm not sure if that disconnect is from us or from the server.) We'd probably want to ensure we didn't already send PASS (since 464 is sent for both missing and wrong passwords).
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Other → All
Hardware: x86 → All
Summary: Better integration with a (znc) bouncer → Login fails using znc without setting serverPassword
*** Original post on bio 2270 by qlum <qlumreg AT gmail.com> at 2013-12-25 16:24:17 UTC ***

I can confirm the situation above, here is a debug log of me trying to connect using the method described on the znc wiki:http://wiki.znc.in/Pidgin to connect with pidgin: http://pastebin.mozilla.org/3860411

and this is what happens after using the fix described in the first post: http://pastebin.mozilla.org/3860413
Fallen gave me a ZNC account, so I'll try to reproduce this tonight.
My guess is that https://github.com/znc/znc/commit/981963a41e8b47f8a4f4ed16b5d8d90ae0626f54 is the commit that broke us. Still investigating though.
Attached file Debug log
In my debug logs the account was disconnected before the NOTICE was received since we actually force Instantbird to disconnect when receiving a 464 call [1]. You can see this in the stack of the disconnect:
> Socket.disconnect@resource:///modules/socket.jsm:185:1
> ircSocket.prototype.disconnect@file:///c:/Users/clokep/comm-central/obj-i686-pc-mingw32/mozilla/dist/bin/components/irc.js:707:5
> ircAccount.prototype.gotDisconnected@file:///c:/Users/clokep/comm-central/obj-i686-pc-mingw32/mozilla/dist/bin/components/irc.js:1651:5
> ircBase.commands[464]@resource:///modules/ircBase.jsm:1282:1
> ircHandlers._handleMessage@resource:///modules/ircHandlers.jsm:102:1
> ircHandlers.handleMessage@resource:///modules/ircHandlers.jsm:117:1
> ircSocket.prototype.onDataReceived@file:///c:/Users/clokep/comm-central/obj-i686-pc-mingw32/mozilla/dist/bin/components/irc.js:688:11
> Socket._handleQueue@resource:///modules/socket.jsm:463:7
> Socket._activateQueue@resource:///modules/socket.jsm:457:5
> Socket.onDataAvailable@resource:///modules/socket.jsm:447:7

I've attached a debug log to help with review.

[1] http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircBase.jsm#1279
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached patch Parse 464 not NOTICE (obsolete) — Splinter Review
This switches from parsing the NOTICE call to handling the 464 call.
Attachment #8388949 - Flags: review?(aleth)
Attached file Debug log with patch
This is the debug log with the patch applied. You can see we successfully connect (001). I find it odd that we receive two 464 calls and thus send the PASS command twice. But I'd like to keep the ZNC hacks to a minimum so I'm going to go with "this is OK" unless aleth or Florian greatly disagree.

I have a feeling SendRequiredPasswordNotice() is being called twice [1][2].

By the way, it seems that ZNC supports SASL. This would be a "safer" mechanism to use than the PASS command. [3] (We currently only support PLAIN, but if someone points me to an IRC server that supports other mechanisms and files a new bug I'll happily add them!)

[1] https://github.com/znc/znc/blob/a8e7b908c0225d3bcc398b51547dee060cde8d4b/src/Client.cpp#L852
[2] https://github.com/znc/znc/blob/a8e7b908c0225d3bcc398b51547dee060cde8d4b/src/Client.cpp#L157
[3] http://wiki.znc.in/Sasl
(In reply to Patrick Cloke [:clokep] from comment #7)

> But I'd like to keep the ZNC hacks to a minimum so I'm
> going to go with "this is OK" unless aleth or Florian greatly disagree.

I don't understand much of the ZNC-specific situation so I'll trust you and aleth, but the one question I had when reading quickly here is: it seems a change in ZNC caused our code to no longer work, and you are adapting our code to work again with newer ZNC versions. Is this at the cost of breaking compatibility with older ZNCs, or have you found a way that works for both old and new versions of ZNC?

Thanks for debugging this!
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> but the one question I had when reading quickly here is: it seems a
> change in ZNC caused our code to no longer work, and you are adapting our
> code to work again with newer ZNC versions. Is this at the cost of breaking
> compatibility with older ZNCs, or have you found a way that works for both
> old and new versions of ZNC?

I thought about this a bit and just assumed we had been ignoring the 464...but now that I think about it more, that doesn't make sense. I went back through the ZNC logs a bit more and found a commit [1] which is clearly what was breaking us. They added a 464 response about 2 years ago.

This patch would NOT work with versions of ZNC before that, I'll put up a new patch.

[1] https://github.com/znc/znc/commit/283fe7a72b6d6f081063fcfe5d4c597729ecaa35
Attachment #8388949 - Flags: review?(aleth) → review-
This is a more backwards compatible version which eats the new 464 message and lets the old NOTICE handler do what it always has. Note the extra check for "Password required" is because if you type in the WRONG password the same numeric is used, but the message is "Invalid Password". These exact wordings will be specific to ZNC, but that's OK since there's a check if it's from ZNC already.
Attachment #8388949 - Attachment is obsolete: true
Attachment #8389772 - Flags: review?(aleth)
Comment on attachment 8389772 [details] [diff] [review]
Ignore 464 if from znc and "Password required" message

Review of attachment 8389772 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! r+ with the expanded comment.

::: chat/protocols/irc/ircNonStandard.jsm
@@ +121,5 @@
>  
> +    "464": function(aMessage) {
> +      // :Password required
> +      // If we receive a ZNC error message requesting a password, eat it since
> +      // a NOTICE AUTH will follow causing us to send the password.

Please add something like "the extra check for "Password required" is because if you type in the WRONG password the same numeric is used, but the message is "Invalid Password", in which case the standard 464 handler will disconnect." to avoid confusion in the future ;)
Attachment #8389772 - Flags: review?(aleth) → review-
Updated comment.
Attachment #8389772 - Attachment is obsolete: true
Attachment #8389879 - Flags: review?(aleth)
Attachment #8389879 - Flags: review?(aleth) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/ebe146fc7ac2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.