417957 - Setting mail.auth_login and mail.server.default.auth_login to false breaks IMAP after restart

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Petri Kelottijärvi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: version 2.0.0.9 (20071031)

One of my mail providers use some kind of load-balancing proxy for its IMAP and POP3 servers. This breaks Thunderbird, in that it can't login. A workaround is published on the web, asking users of Glocalnet and BBB (Bredbandsbolaget) - two of Sweden's biggest ISPs - to toggle settings mail.auth_login and mail.server.default.auth_login to false. This works fine for POP3 accounts, but if one uses IMAP, login will initially work but then fail once Thunderbird is restarted. It seems as if these settings are set to false and TB is restarted, TB acts like the password for the IMAP server is zero-length. 

Reproducible: Always

Steps to Reproduce:
1. Add an IMAP account in TB
2. Go to Inbox, enter and save password.
3. In Options -> Advanced -> General -> Config Editor, change mail.auth_login and mail.server.default.auth_login to false.
4. Restart Thunderbird
Actual Results:  
After restart, this dialogue occurs between TB and my IMAP server:

* OK IMAP4 server ready
1 capability
* CAPABILITY IMAP4 IMAP4rev1 UIDPLUS IDLE LOGIN-REFERRALS NAMESPACE QUOTA CHILDREN AUTH=CRAM-MD5 AUTH=PLAIN
1 OK capabilities listed
2 login "user" ""
2 NO invalid user or password

Also, it takes ages for TB to time-out (around a minute or two), it repeats this dialogue 3-4 times, and this doesn't cause a password prompt to show.

Expected Results:  
This dialogue should occur (and indeed does occur until restarting TB):

* OK IMAP4 server ready
1 capability
* CAPABILITY IMAP4 IMAP4rev1 UIDPLUS IDLE LOGIN-REFERRALS NAMESPACE QUOTA CHILDREN AUTH=CRAM-MD5 AUTH=PLAIN
1 OK capabilities listed
2 login "user" "pass"
2 OK login successful
3 namespace

A newly-created (fresh) profile won't help, nor will using the latest nightly (version 3.0a1pre (2008021603)). signons.txt doesn't change after setting these options (and restarting), so it still contains the correct Base64 encoded password (I verified this using base64 from Cygwin).

The problem forcing me to use the auth_login options in the first place seems to be a faulty IMAP implementation, which causes a dialogue like this:
* OK IMAP4 server ready
1 capability
* CAPABILITY IMAP4 IMAP4rev1 UIDPLUS IDLE LOGIN-REFERRALS NAMESPACE QUOTA CHILDREN AUTH=PLAIN
1 OK capabilities listed
2 authenticate plain
+ go ahead
(20 characters of Base64 encoded user/pass data)
* BYE disconnecting
2 NO proxy authentication failed
3 login "user" "pass"

Comment 1

[Petri Kelottijärvi]

Oh, and the account in question works fine in Apple Mail:
* OK IMAP4 server ready
1 LOGIN user pass
1 OK login successful
2 CAPABILITY
* CAPABILITY IMAP4 IMAP4rev1 UIDPLUS IDLE LOGIN-REFERRALS NAMESPACE QUOTA CHILDREN AUTH=CRAM-MD5 AUTH=DIGEST-MD5 AUTH=PLAIN
2 OK capabilities listed

Comment 2

[Petri Kelottijärvi]

This is how it handles these settings for POP3, for comparison:

Both with and without these settings as false, TB asks for the password whenever login fails.

With these settings as true, it sends CAPA, gets a response including "SASL CRAM-MD5 PLAIN" (and "IMPLEMENTATION CPMS-7.3.130", fwiw). Then it tries to log in:
AUTH PLAIN
+ go ahead
(base64 u/p)
-ERR proxy authentication failed


With these settings as false, it simply does:
+OK POP3 server ready
USER user
+OK Password required
PASS pass
+OK x messages

Comment 3

[Petri Kelottijärvi]

Attachment: Patch for mozilla/mailnews/imap/src/nsImapProtocol.cpp, cvs tag BRANCH_1_8

Some code for looking up the password and doing some checking was only executed when the preference mail.auth_login was set to true. It should run whether or not mail.auth_login is set to true, or Thunderbird won't login if false after a restart.

Comment 4

[Petri Kelottijärvi]

Attachment: Patch for mozilla/mailnews/imap/src/nsImapProtocol.cpp, cvs tag HEAD

Same as attachment 316292 [details] [diff] [review] except for HEAD. Some password looking-up code was not executed unless mail.auth_login was set to true.

Comment 5

[Emre Birol]

Is this patch file applicable? I see ! characters in the file.

Comment 6

[David :Bienvenu]

that's because it's not a -u diff - Petri, a -u diff would be helpful; we're much more used to reading unified diffs.

Comment 7

[Magnus Melin [:mkmelin]]

And preferably standing in the mozilla dir, "cvs diff -up9"

Comment 8

[Petri Kelottijärvi]

Attachment: Patch for BRANCH_1_8

You live and learn :) Is this better?

Comment 9

[Petri Kelottijärvi]

Attachment: Patch for HEAD

Same as attachment 316391 [details] [diff] [review] but on HEAD.

Comment 10

[Petri Kelottijärvi]

Attachment: Patch for BRANCH_1_8

Sorry about the attachment spamming, but it does help if I upload the new diff instead of the old one.

Comment 11

[Petri Kelottijärvi]

Attachment: Patch for HEAD

Once again, my mistake. This is the new diff.

Comment 12

[David :Bienvenu]

I'll try to look at this soon.

Comment 13

[David :Bienvenu]

Comment on attachment 316397 [details] [diff] [review]
Patch for HEAD

If I'm reading the code correctly, couldn't you move the lines of code that get the mail.auth_login pref down next to where it's used?

With your change, won't we prompt for the password, and then, in some situations, tell the user they can't logon because they've picked authentication settings not supported by the server? I think we'd like to avoid that.

Comment 14

[Petri Kelottijärvi]

That would be correct, I think. However that would mean the password-getting code would be duplicated in the function. Is that OK? The code seems relatively stable, but perhaps it should be moved into a separate function to prevent future mis-synchronization of the code?

Comment 15

[David :Bienvenu]

How about making a helper function to get the password, to avoid the duplication? Or is that what you were suggesting? 

Comment 16

[David :Bienvenu]

Comment on attachment 316396 [details] [diff] [review]
Patch for BRANCH_1_8

minusing based on prev discussion.

Comment 17

[Petri Kelottijärvi]

That is what I meant. Rebuilding, will take a while on my machine.

Comment 18

[David :Bienvenu]

great, thanks. You might take a look at bug 227685 and see if it looks like the same problem.

Comment 19

[Petri Kelottijärvi]

Attachment: Patch for nsImapProtocol.cpp, HEAD

Comment 20

[Petri Kelottijärvi]

Attachment: Patch for nsImapProtocol.h, HEAD

Comment 21

[Petri Kelottijärvi]

If GetMsgWindow failed, previously PRBool TryToLogon() returned an nsresult with the error code. This could potentially cause TryToLogon()'s caller to assume the login had succeeded. I changed it into a break if the call to GetPassword() fails.

Comment 22

[David :Bienvenu]

Comment on attachment 322228 [details] [diff] [review]
Patch for nsImapProtocol.cpp, HEAD

Thx for working on this...

GetPassword doesn't  return an rv at the end of the method, and since you check that return rv, that could be bad :-)

Personally, I'd just remove aMsgWindow from nsImapProtocol::TryToLogon, not pass in a ref to the comptr in GetPassword, and leave it as a local comptr in GetPassword - and I'd change the name to just msgWindow, not aMsgWindow, since it's not an arg to an interface method.

Comment 23

[Petri Kelottijärvi]

Attachment: nsImapProtocol.cpp, HEAD

Gah :D You are indeed correct. Does it look better with an NS_OK slapped onto it? Should've looked looked for whether aMsgWindow was used any more in TryToLogon() but...

Comment 24

[Petri Kelottijärvi]

Attachment: nsImapProtocol.h, HEAD

Comment 25

[David :Bienvenu]

Comment on attachment 322311 [details] [diff] [review]
nsImapProtocol.cpp, HEAD

almost there...

you don't need to check msgWindow here - now we know it's null.

+    if (!msgWindow)
+    {
+      rv = GetMsgWindow(getter_AddRefs(msgWindow));
+      if (NS_FAILED(rv)) return rv;
+    }

Also (sorry I didn't spot this before), you don't need to pass in an nsCAutoString ref, it should just be nsCString & (nsCAutoString is a subclass that allocates a buffer on the stack).

Finally, can you just generate a single diff that contains both the .cpp and .h files? - that makes it easier for me to apply and test

Comment 26

[Petri Kelottijärvi]

Attachment: Combined patch nsImapProtocol.cpp/h, HEAD

Thank you for your help, it is invaluable. It's complicated touching code that is both unknown to me and when not having written a single line of code for several years.

Does this diff work for you?

Comment 27

[David :Bienvenu]

Comment on attachment 322322 [details] [diff] [review]
Combined patch nsImapProtocol.cpp/h, HEAD

Petri, no problem, thanks again for working on this.

+  nsresult rv = NS_OK;
+  nsCOMPtr<nsIMsgWindow> msgWindow;

you might as well declare rv right where you use it, i.e.,

remove it from the top,

and say

nsresult rv = GetMsgWindow(getter_AddRefs(msgWindow));

But I can make that change before I check this in; no need for an other patch from you.

The other thing that maybe can be cleaned up is all the temporary copies of password flying around, some of which I probably introduced :-) But now that I look at it some more, I think what you have is right. Maybe we could pass in m_lastPasswordSent directly into PromptForPassword, and avoid using the temporary pwd, as long as in the cancel case, PromptForPassword doesn't change the out string. I'll try that here, I guess...

Comment 28

[Petri Kelottijärvi]

GetPasswordWithUI() seems to truncate the password if the prompt is cancelled:
779       if (!*okayValue) // if the user pressed cancel, just return an empty string;
780       {
781         aPassword.Truncate();
782         return NS_MSG_PASSWORD_PROMPT_CANCELLED;

So the string reference passed to PromptForPassword() should perhaps not be used in that case.

Comment 29

[David :Bienvenu]

thx, Petri, I'll leave it as is, then, and add a comment.

Comment 30

[David :Bienvenu]

Attachment: patch that I'll land

thx again, Petri. This is the patch that I'll land, either later today or tomorrow.

Comment 31

[Petri Kelottijärvi]

Great! Guess this means I can read my mail with a regular release soon, then :)

Comment 32

[David :Bienvenu]

I think we should consider a 1.8.1 branch patch for this as well.

Comment 33

[Petri Kelottijärvi]

Will take a look at that.

Comment 35

[Petri Kelottijärvi]

Attachment: [checked in]Combined patch for branch 1.8

Comment 36

[David :Bienvenu]

Petri, thx for the patch. It looks fine at first glance, but I need to rebuild my 2.0 tree to test it, and that will take some time. 

Comment 37

[David :Bienvenu]

Comment on attachment 322737 [details] [diff] [review]
[checked in]Combined patch for branch 1.8

thx Petri, this looks right, and works fine.

Comment 38

[David :Bienvenu]

Comment on attachment 322737 [details] [diff] [review]
[checked in]Combined patch for branch 1.8

this allows the user to turn off auth login (a hidden pref, I believe) and still be able to logon.

auth login is usually turned off by the user when, for whatever reason, it doesn't function with Thunderbird.

Comment 39

[David :Bienvenu]

I've got to rebuild my 2.0 tree, but I'll land this after I do that.

Comment 40

[David :Bienvenu]

fixed for 1.8.1.15