1471945 - IRC Connect with username/password + /quote PASS doesn't work

Status: RESOLVED DUPLICATE of bug 1016494

(Thunderbird :: Instant Messaging, defect)

Description

[Tom Butler]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180626184830

Steps to reproduce:

Add an IRC account on a server that requires a port, username and password

Enter the username and server on the first dialog, then the password on the next. Configure the port and check "show server messages".

This results in the following back from the server (depending on how the server is configured):

17:45:17 - *** You need to send your password. Configure your client to send a server password.
17:45:17 - *** To connect now, you can use /quote PASS :, or /quote PASS /: to connect to a specific network.

There are two different issues here:

1) I would expect this configuration to run the equivalent of the command:

/server [host] [port] [username]:[password]

This does not happen and I cannot login.

2) Doing as it says by typing "/quote PASS [password]"  (or omitting /quote and just /PASS) does nothing.

If I use "Copy Debug log" I can see that the password is sent as a private message to the server address, I'm not sure if this is the correct behaviour.

If I do this with another IRC client I get an immediate response (even if it's invalid password). As it stands there does not seem to be a way to connect to an IRC server which  has a username and a password.

[28/06/2018, 17:47:20 GMT+1] LOG   (@ prpl-irc: sendString resource:///modules/socket.jsm:263)
Sending:
PRIVMSG [HOST-NAME] :/quote PASS [USERNAME]:[PASSWORD]

[28/06/2018, 17:47:20 GMT+1] DEBUG (@ prpl-irc: onTransportStatus resource:///modules/socket.jsm:561)
onTransportStatus(STATUS_SENDING_TO)

Comment 1

[Tom Butler]

I've managed to debug this a little. The problem is in: omni.ja/components/irc.js:1846

I added some additional debug output at line 1842:


  // Implement section 3.1 of RFC 2812
  _connectionRegistration: function() {
    // Send the Client Capabilities list command.
    this.sendMessage("CAP", "LS");

     this.ERROR("hasServerPassword:");
     this.ERROR(this.prefs.prefHasUserValue("serverPassword"));
     this.ERROR("serverPassword:");
     this.ERROR(this.getString("serverPassword"));

    if (this.prefs.prefHasUserValue("serverPassword")) {
      this.sendMessage("PASS", this.getString("serverPassword"),
                       "PASS <password not logged>");
    }

    // Send the nick message (section 3.1.2).
    this.changeNick(this._requestedNickname);



The this.prefs.prefHasUserValue("serverPassword") returns false even if there is a password set and this.getString("serverPassword") is an empty string.


I have a password set in the GUI but it's not being stored under the preference key "serverPassword".

Comment 2

[Tom Butler]

Attachment: patch for irc.js to fix password

Looking at gtalk.js and others I have fixed the bug in irc.js

1842,1843c1842,1843
<     if (this.prefs.prefHasUserValue("serverPassword")) {
<       this.sendMessage("PASS", this.getString("serverPassword"),
---
>     if (this.imAccount.password) {
>       this.sendMessage("PASS", this.imAccount.password,


This is the first time I've ever looked at Firefox or Thunderbird's code so go easy on me, I don't know if this is the correct way to solve this but it certainly works in TB60 beta and looks more like how the other account types work.

I will submit another patch shortly with a new field.

Comment 3

[Tom Butler]

Attachment: patch for irc.js to fix password and include separate password box

A bAlong with correctly using the server password, I've added a username field (Sorry I don't know what I need to edit to add the translation, and there must be a translation for "username" already). This is more in line with other IRC clients which also ask for a "username" field. 

I don't know whether this is really the correct approach because I believe the "username" field in other clients is usually also used as the ident, but at least it allows Thunderbird to be more easily used with servers that require usernames and passwords.

With the first patch you still have to enter [username]:[password] into the single password box.

Comment 4

[Jorg K (CEST = GMT+2)]

You need to provide a proper HG patch and get it reviewed by a module peer, in this case :florian, :clokep or :freaktechnik.

You can follow https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch, but don't bother with MozReview, a normal text patch, like you already attached, will do. However, it needs to have a different format, like:

# HG changeset patch
# User Richard Marti <[e-mail]>
# Date 1530958243 -7200
# Parent  fb4fd4704e5e7336cc38bc9aa92f130430d69d48
Bug 1474079 - Remove <resources> from activity.xml. r=jorgk

diff --git a/mail/components/activity/content/activity.xml b/mail/components/activity/content/activity.xml
--- a/mail/components/activity/content/activity.xml
+++ b/mail/components/activity/content/activity.xml

Comment 5

[Tom Butler]

Attachment: Fix server password support in IRC

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined:  Still cannot connect to IRC servers which require passwords
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

I do not have access to the try server so didn't run the automated tests. However, have performed manual tests by running Thunderbird with this change.

This patch correctly sends the IRC password by using this.imAccount.password rather than the preference "serverPassword" which does not exist.

I am assuming this is a better approach than creating the serverPassword preference. 

If so, I don't envisage this causing any issues. I will post a variant shortly with slightly more functionality.

Comment 6

[Martin Giger [:freaktechnik]]

You may want to note past work in this direction in bug 1016494. The reason the password is kept separate is because server passwords are not always username/account specific afaik.

Comment 7

[Tom Butler]

Thanks for pointing out that bug. I had searched but hadn't found that. Looking at the bug, the preferred option is to make serverPassword a GUI option. 

Is it now possible to add a masked field as an preference option for the account?

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8990688 [details] [diff] [review]
Fix server password support in IRC

Please get your patch reviewed (see comment #4) and landed before requesting a backport.

Comment 9

[Patrick Cloke [:clokep]]

Tom,

Thanks for attempting to fix this!

Using the "serverPassword" preference is done on purpose. Most people want to use the password they type in to identify themself to services (not as a server password). We want this to be done via SASL mechanisms, instead of using the PASS command because it is more secure. Because of this, we separated out when PASS is sent to a separate preference that should not be needed most of the time!

What server are you trying to connect to that requires this? What is the "username" provided above? Is that your nick or the field to give for USER or something else?

Note that how clients map specific commands (e.g. /server) to the protocol differs for each client. Thunderbird doesn't expect you to use these commands at all during the registration process, once you fill in the proper fields it should "just work".

As Martin noted above, I think this might be a duplicate of bug 1016494. I never finished implementing the idea of "masked" fields in the UI, but we can discuss how to fix that bug specifically in there.

I'm going to mark this bug as a duplicate, but please continue to work on it in bug 1016494 if you would like to see this fixed! Feel free to ask any questions you might have in there (or over IRC, my nick is clokep or clokep_work, I'm usually in #maildev, #instantbird, etc. on irc.mozilla.org -- I'm traveling this week though so I might be spotty on IRC).