Last Comment Bug 417957 - Setting mail.auth_login and mail.server.default.auth_login to false breaks IMAP after restart
: Setting mail.auth_login and mail.server.default.auth_login to false breaks IM...
Status: RESOLVED FIXED
: fixed1.8.1.15
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla1.9
Assigned To: David :Bienvenu
:
Mentors:
: 227685 (view as bug list)
Depends on:
Blocks: 417976 227685
  Show dependency treegraph
 
Reported: 2008-02-16 11:30 PST by Petri Kelottijärvi
Modified: 2009-01-22 10:17 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch for mozilla/mailnews/imap/src/nsImapProtocol.cpp, cvs tag BRANCH_1_8 (4.87 KB, patch)
2008-04-17 14:58 PDT, Petri Kelottijärvi
no flags Details | Diff | Splinter Review
Patch for mozilla/mailnews/imap/src/nsImapProtocol.cpp, cvs tag HEAD (4.67 KB, patch)
2008-04-17 15:01 PDT, Petri Kelottijärvi
no flags Details | Diff | Splinter Review
Patch for BRANCH_1_8 (4.87 KB, patch)
2008-04-18 02:20 PDT, Petri Kelottijärvi
no flags Details | Diff | Splinter Review
Patch for HEAD (4.67 KB, patch)
2008-04-18 02:21 PDT, Petri Kelottijärvi
no flags Details | Diff | Splinter Review
Patch for BRANCH_1_8 (5.17 KB, patch)
2008-04-18 02:34 PDT, Petri Kelottijärvi
no flags Details | Diff | Splinter Review
Patch for HEAD (4.84 KB, patch)
2008-04-18 02:36 PDT, Petri Kelottijärvi
no flags Details | Diff | Splinter Review
Patch for nsImapProtocol.cpp, HEAD (5.56 KB, patch)
2008-05-23 02:13 PDT, Petri Kelottijärvi
mozilla: review-
mozilla: superreview-
Details | Diff | Splinter Review
Patch for nsImapProtocol.h, HEAD (1.34 KB, patch)
2008-05-23 02:13 PDT, Petri Kelottijärvi
no flags Details | Diff | Splinter Review
nsImapProtocol.cpp, HEAD (6.02 KB, patch)
2008-05-23 15:19 PDT, Petri Kelottijärvi
mozilla: review-
mozilla: superreview-
Details | Diff | Splinter Review
nsImapProtocol.h, HEAD (1.31 KB, patch)
2008-05-23 15:19 PDT, Petri Kelottijärvi
no flags Details | Diff | Splinter Review
Combined patch nsImapProtocol.cpp/h, HEAD (7.29 KB, patch)
2008-05-23 16:12 PDT, Petri Kelottijärvi
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
patch that I'll land (7.05 KB, patch)
2008-05-24 11:05 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
[checked in]Combined patch for branch 1.8 (7.54 KB, patch)
2008-05-28 01:02 PDT, Petri Kelottijärvi
mozilla: review+
mozilla: superreview+
dmose: approval1.8.1.15+
Details | Diff | Splinter Review

Description Petri Kelottijärvi 2008-02-16 11:30:43 PST
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 2008-02-16 11:43:11 PST
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 2008-02-17 03:45:23 PST
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 2008-04-17 14:58:18 PDT
Created attachment 316292 [details] [diff] [review]
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 2008-04-17 15:01:19 PDT
Created attachment 316293 [details] [diff] [review]
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 2008-04-17 15:18:12 PDT
Is this patch file applicable? I see ! characters in the file.
Comment 6 David :Bienvenu 2008-04-17 15:20:41 PDT
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 2008-04-17 22:07:43 PDT
And preferably standing in the mozilla dir, "cvs diff -up9"
Comment 8 Petri Kelottijärvi 2008-04-18 02:20:27 PDT
Created attachment 316391 [details] [diff] [review]
Patch for BRANCH_1_8

You live and learn :) Is this better?
Comment 9 Petri Kelottijärvi 2008-04-18 02:21:40 PDT
Created attachment 316392 [details] [diff] [review]
Patch for HEAD

Same as attachment 316391 [details] [diff] [review] but on HEAD.
Comment 10 Petri Kelottijärvi 2008-04-18 02:34:51 PDT
Created attachment 316396 [details] [diff] [review]
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 2008-04-18 02:36:02 PDT
Created attachment 316397 [details] [diff] [review]
Patch for HEAD

Once again, my mistake. This is the new diff.
Comment 12 David :Bienvenu 2008-05-04 16:48:40 PDT
I'll try to look at this soon.
Comment 13 David :Bienvenu 2008-05-16 14:15:17 PDT
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 2008-05-16 14:20:05 PDT
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 2008-05-16 14:22:47 PDT
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 2008-05-16 14:55:20 PDT
Comment on attachment 316396 [details] [diff] [review]
Patch for BRANCH_1_8

minusing based on prev discussion.
Comment 17 Petri Kelottijärvi 2008-05-21 13:56:43 PDT
That is what I meant. Rebuilding, will take a while on my machine.
Comment 18 David :Bienvenu 2008-05-21 14:06:08 PDT
great, thanks. You might take a look at bug 227685 and see if it looks like the same problem.
Comment 19 Petri Kelottijärvi 2008-05-23 02:13:02 PDT
Created attachment 322228 [details] [diff] [review]
Patch for nsImapProtocol.cpp, HEAD
Comment 20 Petri Kelottijärvi 2008-05-23 02:13:46 PDT
Created attachment 322229 [details] [diff] [review]
Patch for nsImapProtocol.h, HEAD
Comment 21 Petri Kelottijärvi 2008-05-23 03:57:34 PDT
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 2008-05-23 12:36:10 PDT
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 2008-05-23 15:19:12 PDT
Created attachment 322311 [details] [diff] [review]
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 2008-05-23 15:19:50 PDT
Created attachment 322312 [details] [diff] [review]
nsImapProtocol.h, HEAD
Comment 25 David :Bienvenu 2008-05-23 15:23:54 PDT
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 2008-05-23 16:12:32 PDT
Created attachment 322322 [details] [diff] [review]
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 2008-05-23 16:38:12 PDT
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 2008-05-24 02:32:07 PDT
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 2008-05-24 07:18:16 PDT
thx, Petri, I'll leave it as is, then, and add a comment.
Comment 30 David :Bienvenu 2008-05-24 11:05:03 PDT
Created attachment 322378 [details] [diff] [review]
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 2008-05-24 14:21:36 PDT
Great! Guess this means I can read my mail with a regular release soon, then :)
Comment 32 David :Bienvenu 2008-05-25 11:23:06 PDT
I think we should consider a 1.8.1 branch patch for this as well.
Comment 33 Petri Kelottijärvi 2008-05-25 12:05:53 PDT
Will take a look at that.
Comment 34 David :Bienvenu 2008-05-27 08:36:51 PDT
*** Bug 227685 has been marked as a duplicate of this bug. ***
Comment 35 Petri Kelottijärvi 2008-05-28 01:02:00 PDT
Created attachment 322737 [details] [diff] [review]
[checked in]Combined patch for branch 1.8
Comment 36 David :Bienvenu 2008-05-28 09:38:04 PDT
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 2008-05-28 12:30:07 PDT
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 2008-05-28 12:31:39 PDT
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 2008-06-05 11:29:12 PDT
I've got to rebuild my 2.0 tree, but I'll land this after I do that.
Comment 40 David :Bienvenu 2008-06-05 12:53:08 PDT
fixed for 1.8.1.15

Note You need to log in before you can comment on or make changes to this bug.