1453643 - Port bug 1408610 to other authentication methods: Resend auth string when imap server requests it, , to avoid imap folders deleted and synced messages redownloaded

Status: RESOLVED FIXED

(MailNews Core :: Networking, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1408610 +++

This is a spin-off of bug 1176399 comment #81

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1453643-review-changes0.patch

Gene, should we land this "as is" or cover other authentication methods, as per bug 1176399 comment #83 (quote):
===
there may be other auth methods that have the same problem. It was fixed for auth plain (e.g., yahoo) and now for oauth2/gmail. 
===

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1453643-review-changes0.patch - JK v1

I put the code into its own function to avoid repetition and so that we can easily add this to other auth methods as well. Which ones could you test?

Comment 3

[Jorg K (CEST = GMT+2)]

I don't think this has much to do with the redownloading. It just straightens out the error path when you can't authenticate.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+1) from comment #3)
> I don't think this has much to do with the redownloading. It just
> straightens out the error path when you can't authenticate.

So you would see that solution coming via the issues surrounding bug 1422568?

Comment 5

[Jorg K (CEST = GMT+2)]

There are tree pieces:
1) Fix the master password prompting issues, bug 1176399 and bug 584014.
2) This bug here. This bug here will ensure that whatever went wrong in authentication doesn't
   trigger bug 1422568, so it just makes sure the authentication failure is properly detected.
3) Bug 1422568 itself.

At least that's how I understand it. Gene knows more.

Comment 6

[gene smith]

(In reply to Jorg K (GMT+1) from comment #3)
> I don't think this has much to do with the redownloading. It just
> straightens out the error path when you can't authenticate.

Actually, from what I have observed, this does prevent the folders (actually mbox/maildir files) from being deleted so they are never re-downloaded. I haven't researched exactly what causes them to be deleted when imap error "BAD" or other errors are returned for every response, but I have verified that they are deleted for gmail and were also deleted for yahoo before the similar patch was applied.

Without this patch and with the "master password" patch in place and with master password defined, I would see all the gmail folders vanish right after the master password was entered, then they re-downloaded. With this patch in place, the "retry" password/access token is fetched OK and sent (in the 2nd loop iteration) so folders never get deleted. If both are bad, a login failure is flagged and no folders are deleted.

Then again, there may be other reasons why gmail and yahoo folder vanish/re-download that I haven't seen, but I doubt it.

I haven't had a chance to review the consolidated plain/oauth2 patch. Will do that now.

I really don't think I have any accounts that use the other possible supported auth methods to test. So unless users are reporting similar problems with servers using other auth methods (NTLM, gss, cram, external etc) I would suggest not trying to "fix" them at this time.

Comment 7

[Jorg K (CEST = GMT+2)]

I don't doubt what you've said, however, until we fully understand how the re-download happens, we can be 100% sure, although empirical evidence is strong.

Comment 8

[gene smith]

Your patch attached above prevents tb's imap state from entering "authenticated" when a bad password/access token is sent, so list/xlist/lsub commands won't occur and then respond BAD. The error occurs when lsub commands responds with BAD causing the list of subscribed folders to be empty. When tb sees a folder that was previously subscribed but is now not subscribed, it causes it to disappear. Without the patch, I see all folder disappear from the gmail folder pane. Sometimes they come back immediately but usually they don't come back until I restart tb and they are "discovered" again. 

Of course, in this context, "disappearing", "vanishing" or "deleting" the folder means removing the online store and msf file for the folder. It is still present on the server and is not lost.

The actual "deletion" of a newly unsubscribed folder occurs here:
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#1759

There is way to prevent the deletion of folders when they are unsubscribed that might also be a workaround for this authentication bug. In advanced server setting you can set it so that not-subscribed folders remain visible. When this mode is enabled, the lsub command never occurs (so no BAD lsub response occurs) and every folder, subscribed or not, remains visible and is never deleted due to being falsely unsubscribed. (The BAD response still occurs for list and xlist but they don't cause a problem.)

(In reply to Jorg K (GMT+1) from comment #7)
> I don't doubt what you've said, however, until we fully understand how the
> re-download happens, we can be 100% sure, although empirical evidence is
> strong.

The "re-download" occurs on another properly authenticated connection while running or at tb restart when the previously "deleted" folders are re-discovered via an error free list/xlist/lsub folder discovery sequence. This occurs even if the folder offline storage and msf file are manually deleted, so re-downloading is normal and to be expected. The reason for the folder deletion is what I tried to rationalize above. (Maybe I am parsing your comment too literally.)

Comment 9

[gene smith]

Comment on attachment 8967522 [details] [diff] [review]
1453643-review-changes0.patch - JK v1

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5748,5 @@
> +    if (!GetServerStateParser().WaitingForMoreClientInput())
> +      break;
> +
> +    // Server is asking for authentication string again. So we send the
> +    // same string again although we already know that it will be

The comment about "already know that it will be rejected" was specifically for yahoo where bad password on the first try failed and a good password on the resent also failed. In general this may not be true, although I haven't tested this on gmail. So maybe it could be changed to "we know that it might be rejected".

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c023f62fd9c1
Enable proper retry on oauth2 authenication failure. r=jorgk

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8967522 [details] [diff] [review]
1453643-review-changes0.patch - JK v1

I've landed that with the comment adjusted as requested.

Gene, thanks a lot for the investigation!

Comment 12

[Jorg K (CEST = GMT+2)]

I studied your comment #8 in detail now. Thanks. I'm sure re-downloads due to authentication issues for Yahoo and Gmail are now under control.

Should we move the discussion to bug 1422568 where the description reads:
"Anecdotal evidence in Support forums indicates that Thunderbird is removing folders from IMAP accounts when the mail server is non contactable.  Usually de to firewall or anti virus issues post update."

A similar report be the same reporter in bug 1423588.

Comment 13

[gene smith]

(In reply to Jorg K (GMT+1) from comment #12)
> I studied your comment #8 in detail now. Thanks. I'm sure re-downloads due
> to authentication issues for Yahoo and Gmail are now under control.
> 
> Should we move the discussion to bug 1422568 where the description reads:
> "Anecdotal evidence in Support forums indicates that Thunderbird is removing
> folders from IMAP accounts when the mail server is non contactable.  Usually
> de to firewall or anti virus issues post update."
> 
> A similar report be the same reporter in bug 1423588.

I'm pretty sure both of these are fixed by the patch for this bug. Bug 1423588 talks about changing the password and then all the gmail files re-download, which is what would have happen when the now bad, but still stored, password/access token is sent in the "authenticate oauth2" imap command without the patch for this bug.

Bug 1422568 talks about "non-contactable" causing the deletion/re-download. I've never seen that happen just because of a bad/intermittent connection to the server (re: bug 1422568 comment 3 and 4). I don't know what anti-virus could do that would cause this to happen either; then again, I don't know much about how AV messes with tb (don't use it on linux).

I could be wrong, but I tend to think both of these bugs are actually duplicates of this now fixed bug. Of course, more evidence would change my mind.

Comment 14

[Jorg K (CEST = GMT+2)]

I'll comment on bug 1422568. Let's move the discussion there.

Comment 15

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/417637b7cad86df52532cc7061f0cb683cbebb6a