Closed Bug 1176399 Opened 9 years ago Closed 6 years ago

Multiple requests for master password when GMail OAuth2 is enabled

Categories

(MailNews Core :: Networking, defect)

defect
Not set
major

Tracking

(thunderbird_esr52 wontfix, thunderbird_esr6062+ fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 wontfix, thunderbird63 fixed)

RESOLVED FIXED
Thunderbird 63.0
Tracking Status
thunderbird_esr52 --- wontfix
thunderbird_esr60 62+ fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- wontfix
thunderbird63 --- fixed

People

(Reporter: leopoldo.saggin, Assigned: Fallen)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, Whiteboard: [oauth2])

Attachments

(1 file, 7 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150605094246

Steps to reproduce:

I really do not know if the component is correct but I cannot find atopic/component where to insert my report.
I have 8-10 different accounts (depending on the PC I'm using).
So I need to save my passwords and protect them by activating a "Main (meta) password" by using:
Menu -> Options -> Security -> Password -> Use Main password.
This was ok until version 31.7 even if I have been using my 6 Google calendars (embedded by CalDEV method) into Lightning and even if I have been using GoogleTaskSync 0.5.2 which requires an OAuth2 authentication.


Actual results:

With version 38.0.1 I took the opportunity to activate the OAuth2 authentication also for my GMail account in TB v. 38.0.1.
Unfortunately, in this condition, TB asks me the Main Password 3 times (!) before I can access TB!



Expected results:

IMHO it should have asked the main password only once.
So I decided to deactivated the main password of TB and I installed the old extension "Profile Password v.0.7.17" extension by Paolo Kaosmos
https://nic-nac-project.org/~kaosmos/profilepassword-en.html
By using it, I had to insert my "meta/main" password only once.
Leopoldo Saggin aka Topoldo
I assume that you are talking about subsequent restarts of Thunderbird with OAuth2, and not the initial configuration.
Component: Preferences → Networking
Product: Thunderbird → MailNews Core
Summary: password → Multiple requests for master password when GMail OAuth2 is enabled
Whiteboard: [oauth2]
Yes, of course!
In the occasion of the release of the "new" Thunderbird 38.0.1 I decided to reinstall it from scratch. During this new installation I re-created my accounts (which I populated in a second time with my old data) and, while recreating my GMail account I accepted to use OAuth2 as the auth method.
At the end of the process of reinstallation of extensions, data etc... I also added a master password to protect all my saved pasword.
From then on, when every time I open TB, it asks me 3 times for the master password before letting me to operate.

BUT yesterday night I had to reinstall everything again from the beginning because I missed some configuration and some data. So I decided to test one more the problem of master password.
In this case I noticed that TB asks me only TWICE for the master password.
Asbjørn, is this something you can test for us?
Flags: needinfo?(lordcrc)
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #3)
> Asbjørn, is this something you can test for us?

Sorry for the late reply, I was on vacation. I can confirm that I get the same behavior on Thunderbird 38.0.1 (Windows). With OAuth and one GMail account, I get asked twice upon starting Thunderbird. When switching off OAuth I only get asked once (as expected).

I'd be happy to test other builds, just let me know which one as I've never installed test/nightly builds of Thunderbird so not sure which one(s) to get.
Flags: needinfo?(lordcrc)
I have what I believe is the same issue, except that I get 5 "Password Required" pop-ups.  This started happening after I downloaded TBird v38.1.0 and connecting to Google for my calendars for the first time(never had the Lightning add-on previously).  If I just enter my password in the last pop-up and cancel the other 4 then both mail and calendar seem to work.

I 'unsubscribed' a couple of my google calendars and now I get 2 pop-ups on start, and a third pops up after I enter my p/w once.  I can still cancel the 2 extra ones after I enter my p/w once and mail and calendar both seem to function properly.

Not sure what authentication is used but I had also added "Provider for Google Calendar 1.04" per the online instructions I was following to set up Lightning.
Confirming from multiple reports.
Status: UNCONFIRMED → NEW
Ever confirmed: true
A workaround for this is to install the "Startupmaster" extension.

I believe I've mentioned this before in another bug, but the problem is that our nsIMsgAsyncPrompter is not asynchronous enough :-) It is assumed that multiple asynchronous requests can be made, but the actual password prompt must be synchronous. This is not the case for OAuth, because a window needs to be opened and such.

I'd love for this to be fixed by extending nsIMsgAsyncPrompter to allow for asynchronous password requests, preferably in a backwards compatible way. This would also allow me to fix the issue for the Provider for Google Calendar.
(In reply to Philipp Kewisch [:Fallen] from comment #7)

> A workaround for this is to install the "Startupmaster" extension.

As I reported in my first message of this thread, my workaround (still in use) was to deactivate the Thunderbird main (meta) password and the installation of the old (but still valid) extension "Profile Password" v.0.7.17 by Paolo Kaosmos you can get at:
https://nic-nac-project.org/~kaosmos/profilepassword-en.html
By using it, I had to insert my "meta/main" password only once. Besides, this extension gives a little bit more security to TB application.
Topoldo
I can confirm this bug.

Instead of disabling the master password request, it would be a better workaround, to disable receiving mails at program start (in account options for OAuth accounts).
Hello/ Bonjour

@AlexIhrig@thunderbird-mail 

Disable receiving mails at program start does not change the problem,  or start TB offline either.

If you disable Gmail OAuth (TB38) is ok
The solution with the Startup Master extension also works well;-)
https://addons.mozilla.org/fr/thunderbird/addon/startupmaster/

(sorry for the translation :-(  )

/------------------------------------\
@AlexIhrig@thunderbird-mail 
Désactiver la relève  ne change rien au problème, ni démarrer TB en hors connexion non plus.

Si on désactive OAUTH  de Gmail (TB38) c'est ok 
La solution avec l'extension Startup Master fonctionne aussi très bien ;-)
https://addons.mozilla.org/fr/thunderbird/addon/startupmaster/
(In reply to Philipp Kewisch [:Fallen] from comment #7)
> ...the problem is that
> our nsIMsgAsyncPrompter is not asynchronous enough :-) It is assumed that
> multiple asynchronous requests can be made, but the actual password prompt
> must be synchronous. 

Thanks Philip, et.al.  Seems to me that the master password dialog needs to simply happen and complete before letting anyone use it's results, either because one happens before the other, or because one must wait until the other is done.  

I looked at the workaround, but there is a stiff warning that it's not very secure right at the start of the info about it.  And yes, this having to put a pw in 3 times is a pain, and is the sort of thing that will drive people away from TB.  Hope you can fix this.  It's been around for quite awhile broken like this!
I was getting duplicate prompts until a week or two ago with Firefox Nightly, but the behavior has changed in the past week or so. I may still get duplicate prompts if I wait long enough but I haven't seen it for a few days.

There also appears to have been a change in when the password prompts appears (it shows up later), but I can't tell whether that is a Nightly change or a change in the behavior in the web sites in my pinned tabs.

There are three bugs on this issue. I'll let someone else decide which should be marked as duplicate.

Bug 794736: Firefox/Security: Two master password popup windows come up
Bug 981579: Toolkit/Password Manager: Asked for master password twice
Bug 1176399: MainNews Core/Networking: Multiple requests for master password when GMail OAuth2 is enabled
On Thunderbird, I solved all my problems concerning master password duplicate requests due to Google mail account + Google Calendar (mapped into Lightning) requests by installing the following extension:
https://addons.mozilla.org/it/thunderbird/addon/startupmaster/
Regards,
Topoldo/Leopoldo
Does it happen with nightly build?  (without the addon of course)
see https://archive.mozilla.org/pub/thunderbird/nightly/latest-comm-central/

If you go back to one of the version 31.x series, does the problem go away?
see https://www.mozilla.org/en-US/thunderbird/releases/
Flags: needinfo?(lordcrc)
Flags: needinfo?(leopoldo.saggin)
(In reply to Philipp Kewisch [:Fallen] from comment #7)
> A workaround for this is to install the "Startupmaster" extension.
> 
> I believe I've mentioned this before in another bug, but the problem is that
> our nsIMsgAsyncPrompter is not asynchronous enough :-) It is assumed that
> multiple asynchronous requests can be made, but the actual password prompt
> must be synchronous. This is not the case for OAuth, because a window needs
> to be opened and such.

looks like that would be bug 349641 comment 84


> I'd love for this to be fixed by extending nsIMsgAsyncPrompter to allow for
> asynchronous password requests, preferably in a backwards compatible way.
> This would also allow me to fix the issue for the Provider for Google
> Calendar.

No mention nsIMsgAsyncPrompter in bug 643265 and bug 584014, the key "master password" bugs in Thunderbird.  And nowhere else in the *entire* history of bugzilla do I find mention of nsIMsgAsyncPrompter (except an unrelated bug 647000)

It's pretty clear no patch is coming from the respective module owners and peers via bug 177175 (which is quite the cesspool). So what is our path forward?  Can we do it ourselves, or ply them with beer?
Depends on: 643265
Flags: needinfo?(philipp)
Flags: needinfo?(lordcrc)
Flags: needinfo?(leopoldo.saggin)
Flags: needinfo?(jorgk)
See Also: → 849540
Sorry, I know nothing about this. But bug 177175 was reviewed by Honza, so perhaps ask him.
Flags: needinfo?(jorgk)
OTOH, there is bug 1271851 where dolske recently says "We've also been doing _nothing_ for long enough that we really need to make some incremental progress towards improving things so that future improvements don't also get stuck in this hopeless logjam".

Mattn is also active in triage of password manager bug reports
nsIMsgAsyncPrompter is our code at https://dxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgAsyncPrompter.idl

I don't think we need to ask anyone, we just need to change the interface to be able to handle asynchronous password prompts.
Flags: needinfo?(philipp)
Attached patch Fix - v1 (obsolete) — Splinter Review
I was thinking about something like this.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #8813917 - Flags: review?(mkmelin+mozilla)
Blocks: 1257058
Review ping for this bug? I think we should get this into 52.
(In reply to Philipp Kewisch [:Fallen] from comment #24)
>  I think we should get this into 52.

And even 45 if it's deemed not too risky - because, beyond just the mere prompting, it has the potential to help some other issues like crashes.
I'll try to get to it this week.
Comment on attachment 8813917 [details] [diff] [review]
Fix - v1

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

I set up two gmail accounts, and set the master password. 

With this patch applied, I get only one master password prompt (OK!), but only one of the accounts is showing any folders (just shows the account name, no folders to expand). Presumably the "second" one is not really authenticated. Nothing of interest in the error console. I can see the problem account folders briefly if using trunk with the profile earlier. But the folders and subfolders disappear once logins would be done.
Attachment #8813917 - Flags: review?(mkmelin+mozilla) → review-
Progress!  Back to Philipp.

Can you put up a try build with the next patch?
Flags: needinfo?(philipp)
Attached patch Fix - v2 (obsolete) — Splinter Review
I was not able to reproduce this directly, but I can imagine what is happening: the _loginUrl is actually just oauth://accounts.google.com so this will be the same for both accounts, which will consolidate the prompts. I've added the username in the mix now. It does not have to be a valid url, just a unique token.

I am still slightly concerned that in the case where prompts are correctly consolidated (i.e. one account, possibly master password, two different sources try to trigger an oauth login). If I am right to assume that in this case the same instance of msgOAuth2Module will be used it is ok, but if there are two instances then consolidating requests will cause one of them to be missing a token.

Anyway, I've started a try run with my latest patches:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=28d2d61cb151fdbb400fad592fddca6c81a7dd58
Attachment #8813917 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8827172 - Flags: review?(mkmelin+mozilla)
This could be a MAJOR step forward. I  did not test calendar. Only email accounts.  I hope others report similar success for calendar.

Try build results:
- happily only one master password prompt (normally I get one per google account)
- 2 oauth gmail accounts, all accounts logged in
- 2 normal login gmail accounts, all logged in OK 
- no new ill effects and no other changes that I could determine 

Only the normal crashes if you try to shut down THunderbird with a login prompt open...

try build:  xul.dll@0x17c7cf8 | nss3.dll@0x79584 | nss3.dll@0x6eb37 | _o__CIpow bp-cddb2b23-94f0-4cf2-9b70-6b03b2170121

45.6.0:  shutdownhang | _PR_MD_WAIT_CV | _PR_WaitCondVar | mozilla::CondVar::Wait | nsEventQueue::GetEvent | nsThread::ProcessNextEvent | NS_ProcessNextEvent | nsThread::Shutdown | nsThreadManager::Shutdown More Reports Search
bb26b697-34a6-4d84-bde6-c86452170121 bug 1307963
Comment on attachment 8827172 [details] [diff] [review]
Fix - v2

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

Sorry for the review delay!
This version works for me, when I enter the password. 

If I cancel the master password prompt I get left in a dead state, where I can't do anything in the affected accounts. 
Without the patch, when I do the same thing, the master password dialog would pop back up again if I try to view a folder.

In the error console I get
"NS_ERROR_ABORT: User canceled master password entry"
Attachment #8827172 - Flags: review?(mkmelin+mozilla) → review-
> In the error console I get
> "NS_ERROR_ABORT: User canceled master password entry"

I reproduce this also on Windows
Flags: needinfo?(philipp)
(amending) after cancelling the master password prompt appears after some period of time (1-2 minutes).  But the affected accounts are still dead - no prompt for password
Attached patch Fix - v3 (obsolete) — Splinter Review
Thanks for the extensive testing and sorry for the delay. Not sure this is still in time given potential regression risks, but I've found the issues in the previous patch.

I've tested successful login, cancelled login, successfull login after initially cancelling, saving passwords using master passwords. It all seems to work.
Attachment #8827172 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8841218 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8841218 [details] [diff] [review]
Fix - v3

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

Sorry for the review lag here. Anyway, there still appears to be problems after cancelling. 

My setup is I have two gmail accounts. After starting I get the master password prompt and press cancel. Now if I decide I want to see an inbox I select the inbox, but get no prompt. However, some other folders seem to trigger the prompt. I think it's only the first time a folder is selected the prompt will show itself. After that nothing if I cancel.

::: mailnews/base/src/msgAsyncPrompter.js
@@ +59,3 @@
>            consumer.onPromptCanceled();
> +        }
> +      } catch (ex) {

Apparently we end up here

runnablePrompter:run: [Exception... "User canceled master password entry"  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "JS frame :: file:///opt/comm-central/src/obj-x86_64-pc-linux-gnu/dist/bin/components/crypto-SDR.js :: decrypt :: line 134"  data: no]
Attachment #8841218 - Flags: review?(mkmelin+mozilla) → review-
Thanks for the thorough review, I'm surprised this issue didn't show for me. I'll look into this asap.
Flags: needinfo?(philipp)
Attached patch Fix - v4 (obsolete) — Splinter Review
There we go. The first prompter is not canceled in case of an exception, and I also found that we are trying to create empty login manager entries if the refresh token is null (e.g. cancelling the master password prompt after logging in)
Attachment #8841218 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8849555 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8849555 [details] [diff] [review]
Fix - v4

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

Everything works now, yay!

Just please fix the indention to use 2 spaces, not 4

::: mailnews/base/src/msgOAuth2Module.js
@@ +139,5 @@
>  
>    connect(aWithUI, aListener) {
> +    let oauth = this._oauth;
> +    let promptlistener = {
> +        onPromptStartAsync: function(callback) {

two space indents please :)

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8334,5 @@
> +NS_IMETHODIMP nsImapProtocol::OnPromptStartAsync(nsIMsgAsyncPromptCallback *aCallback)
> +{
> +    bool result = false;
> +    OnPromptStart(&result);
> +    return aCallback->OnAuthResult(result);

here too

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +779,5 @@
> +NS_IMETHODIMP nsPop3Protocol::OnPromptStartAsync(nsIMsgAsyncPromptCallback *aCallback)
> +{
> +    bool result = false;
> +    OnPromptStart(&result);
> +    return aCallback->OnAuthResult(result);

and here

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +2497,5 @@
> +NS_IMETHODIMP nsNNTPProtocol::OnPromptStartAsync(nsIMsgAsyncPromptCallback *aCallback)
> +{
> +    bool result = false;
> +    OnPromptStart(&result);
> +    return aCallback->OnAuthResult(result);

and here
Attachment #8849555 - Flags: review?(mkmelin+mozilla) → review+
Attached patch Fix - v4 (spaces fixed). (obsolete) — Splinter Review
Ready to land, right?
Attachment #8849555 - Attachment is obsolete: true
Attachment #8855936 - Flags: review+
Is bug 177175 related in any way? Could parts of this fix be used for FF?
(In reply to Jorg K (GMT+2) from comment #41)
> Created attachment 8855936 [details] [diff] [review]
> Fix - v4 (spaces fixed).
> 
> Ready to land, right?

Looks good, yes. Thanks for taking care. We may want to test this a bit before we uplift, but it would definitely be a candidate for the next minor version.

(In reply to Albert Scheiner [:alberts] from comment #42)
> Is bug 177175 related in any way? Could parts of this fix be used for FF?

I'm sure Firefox could fix it in a similar way, but since they have the power to fix it in toolkit they may want to do that instead. Thunderbird uses a services that queues all auth prompts, this specific bugs only extends that service to handle async auth prompts.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c553f7084ca92a5a93b66c5d0a82b78e385d356b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
perha(In reply to Philipp Kewisch [:Fallen] from comment #43)
> (In reply to Jorg K (GMT+2) from comment #41)
> > Created attachment 8855936 [details] [diff] [review]
> > Fix - v4 (spaces fixed).
> > 
> > Ready to land, right?
> 
> Looks good, yes. Thanks for taking care. We may want to test this a bit
> before we uplift, but it would definitely be a candidate for the next minor
> version.

I'd like to see us take this and bug 1176399 for the first 54 beta - if I had to put a date on it, I'd say building Monday 4/24. So by then it would have been on nightly for 2 weeks.
Well, no uplift was ever requested. And since bug 682474 (*not* bug 1176399 which is this bug here) is in Calendar, I don't have the authority to just uplift it since I want to :-(
Attachment #8855936 - Flags: approval-comm-beta+
Attachment #8855936 - Flags: approval-comm-aurora+
Attachment #8855936 - Flags: approval-comm-esr52?
See Also: → 177175
One small wrinkle in this action (asking for the Master password twice) is that the Master Password dialogue actually appears (pops up) 3 times. On the first dialogue, it appears and disappears in a flash. Then the other 2 dialogues open one after the other to request the Master password twice. I would suspect that the first dialogue isn't doing anything and could be removed.
I know it's on the tracking list, and now we have enough testing, so time to get this and it's friend bug 682474 on the next esr52.
> status-thunderbird54: affected → fixed

This applies to TB 54.0b1 32-bit on both, Linux and Win7. TB started in safe mode.
For me this isn't fixed, I still get multiple Master passwords prompts.
I do have multiple email accounts (among them one Gmail IMAP with OAuth2) and a Gmail calendar also with OAuth2.
On Linux I do get 2 Master Password prompts, on Win7 I do get 6 prompts. The accounts are the same for both installations.
The Startup Master add-on still takes care of the problem.
(In reply to Christian Riechers from comment #52)

> On Linux I do get 2 Master Password prompts, on Win7 I do get 6 prompts.

Correction:
There are 6 prompts on both, Linux and Windows.
Are you using any other extensions aside from Lightning?
Yes, I do. However, I thought that starting Thunderbird in safe mode would eliminate interference from other extensions.
Attachment #8855936 - Flags: approval-comm-esr52? → approval-comm-esr52+
TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/fdb3acad393d

Note that there were changes to an IDL file here. I guess they were OK to uplift since we don't support binary extensions any more. To get this landed, I had to add ba= and IGNORE IDL to the commit message.
Depends on: 1373161
Fixed now with latest 52.2.0. If you enter the master password right away, then you are only asked once to enter it.

However, there is maybe? a related bug. If you do not enter the master password right away but wait, say about 10 minutes, then the same problem of multiple password requests comes back again - depends on how long you wait, but you get 3-4 requests to enter the master password over and over. Is there another bug for that?
I just backed this out from TB 52.2 since it was causing issues with Gmail. When we fix those issues, we'll keep your report in mind.
(In reply to Jorg K (GMT+2) from comment #59)
> I just backed this out from TB 52.2 since it was causing issues with Gmail.
> When we fix those issues, we'll keep your report in mind.

I was hoping to avoid this, but I believe we must also back this out for beta.  ref bug 1380040
Flags: needinfo?(jorgk)
TB 52.2.1 still has the bug. Not sure why it worked right after it was "fixed" but now always asking 2 times for Master password. Sorry if this is the "expected" result after backing out, but just in case it wasn't actually backed out.
Yes, this was backed out from TB 52.2.1 to fix some Gmail problems, so the multiple master password prompts are back.

Wayne, yes, I will back the "combo" (this bug plus two related calendar bugs) out from TB 55 beta.
Flags: needinfo?(jorgk)
Backout from trunk:
https://hg.mozilla.org/comm-central/rev/8f404af70ca26af39dd3f1472d8fd357508a1ce6

Sorry, I had to back this out since the next merge date is coming and I can't continue to back it out from all the betas we ship.

Further references: Gijs' comments from bug 1374244 comment #4.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm running 56b3 and i'm having this problem, maybe since 56b1. Mac, 10.12.6. One IMAP and two gmail accounts. I know that this was fixed maybe a year or two ago, so it seems like it's come back. Damned if I can't find the bugs in question from back then.
For the record, following are my analysis from June regarding the regression symptoms

SUMMARY

Started with 52.2.0 (does not exist in 52.1.1) which shipped patch for bug 1176399.  Regression reported in bug 1362881 by several 54.0b1 linux beta users, but it was not picked up as being serious.  Best techincal info at https://bugzilla.mozilla.org/show_bug.cgi?id=1374244#c4

Assumptions:
* Only users of oauth are affected. But of those, the low volume of support requests (estimate ~10-15 in 7 days)  suggest most (perhaps the vast majority) users are not affected.  But, this assumption could be wrong.

Some of the symptoms (not all users have the same symptoms) :
* folder iterations of [Gmail].sbd [Gmail-1].sbd [Gmail-2].sbd
* gmail account redownloads
* some folders don't appear in folder pane after startup
* folders do appear every second or third startup
* some users only affected if check_all_folders_for_new=true

Bugs in chrono order:
* Bug 1362881 - Thunderbird does not always load emails (and folders) from gmail account
* Bug 1373161 - check_all_folders_for_new=true resets gmail imap folders on startup in 52.2
* Bug 1373493 - TB 52.2.0 see "IMAP command is invalid" or CRASHES @ nsImapProtocol::CreatePossibleTrashName
* Bug 1374244 - Thunderbird 52.2.0 loses my local copies of gmail folders and email on startup and then redownloads everything

Bugs summary

Bug 1362881 - Thunderbird does not always load emails (and folders) from gmail account
                           paulo 54.0b1 2017-05-07 paulo linux, jank linux, dennis linux, s.fezzo win, =4)
                           about every second time I open Thunderbird, it does not show the Gmail folders

Bug 1373161 - check_all_folders_for_new=true resets gmail imap folders on startup in 52.2
                            ephbase 52.2.0 2017-06-15 00:48PDT ephbase win7

              for some users check_all_folders_for_new=true is not related
              freakrob also crashes, 
              u597066 creating numerous iterations of each folder and file, such as [Gmail].sbd [Gmail-1].sbd [Gmail-2].sbd
              Ache gets window telling about wrong IMAP command for non-gmail account, also tends to crash (bug 1373493)

Bug 1373493 - TB 52.2.0 is broken with Gmail IMAP AND CRASHES nsImapProtocol::CreatePossibleTrashName (but check_all_folders_for_new=false)
              Ache 52.2.0 2017-06-15 20:08pdt , u597066, ku
              error "IMAP command is invalid"
              crash bug 557252 CreatePossibleTrashName 

Bug 1374244 - Thunderbird 52.2.0 loses my local copies of gmail folders and email on startup and then redownloads everything
              gijs 2017-06-19
Blocks: 1346268
Depends on: 1374244, 1362881, 1373493
Now that the dust has settled for a few months, what options do we have to move forward?

Is situation similar to "Gene's" bug 1408610 (yahoo) where error checking is insufficient and imap operations are allowed to proceed when they should not have?
Flags: needinfo?(philipp)
Flags: needinfo?(jorgk)
Flags: needinfo?(jorgk)
Updated to 57b2 and still having the same issue. Password dialog is smaller, but problem still remains.
See Also: → 584014
This clearly left a bad tast with Jorg.

(In reply to Jorg K (GMT+1) from comment #64)
> Backout from trunk:
> https://hg.mozilla.org/comm-central/rev/
> 8f404af70ca26af39dd3f1472d8fd357508a1ce6
> 
> Sorry, I had to back this out since the next merge date is coming and I
> can't continue to back it out from all the betas we ship.
> 
> Further references: Gijs' comments from bug 1374244 comment #4.

Gene, is this enough info for you to have a go?  I'm sure Jorg and Philipp can advise.

Note - given the severity of the previously mentioned regressions, any potential fixes must be thorougly tested before landing on beta.
The following occurred to me. This bug 1176399 was fixed and then backed out again since it caused constant re-download of Gmail folders for some users. Reading bug 1374244 comment #4 I get the impression that due to timing issues, the server is considered unavailable for a (very) brief time and that triggers bug 1422568 and friends. Good theory?
Theory sounds plausible to me. And I personally have had gmail redownload on many occassions - seemingly random.

Gene you had several comments in bug 1422568 where Matt collected some good info, what do you think?
Flags: needinfo?(philipp)
I can confirm that disabling check for mail at startup for all accounts is a valid workaround. When is there going to be an actual
fix? What logs do I need to submit to help with this?

(In reply to AlexIhrig from comment #10)
> I can confirm this bug.
> 
> Instead of disabling the master password request, it would be a better
> workaround, to disable receiving mails at program start (in account options
> for OAuth accounts).
Are we still talking about the multiple master passwords requested of the user t startup? I ask since, on my MacBook Pro with El Capitain 10.11.6 and TB 52.7.0 (64-bit), "the" problem disappeared about a month ago or so - wonderful. I'm set to receive mails at program start and TB only asks for the master password one time.
(In reply to Wayne Mery (:wsmwk) from comment #69)
> 
> Gene, is this enough info for you to have a go?  I'm sure Jorg and Philipp
> can advise.
> 
> Note - given the severity of the previously mentioned regressions, any
> potential fixes must be thorougly tested before landing on beta.

On my trunk build with many test accounts running, including one gmail with oauth2, I just enabled the master password feature. I only see one prompt for that password on tb start (same as comment 73 above). Is the issue that there are sometimes or always multiple prompts? Also, I haven't seen any gmail folder disappear and reload even when the network is down or intermittent.

Does this require the use of calendars or lighting add-on to duplicate. I don't have these enabled. 

The attached patch above that was reportedly backed out is not in my trunk tree which was pulled about a month ago. I wouldn't expect it to be.

Is there a known way to duplicate this bug? Maybe only occurs on windows and/or osx? I am running linux.
You need to set up two gmail accounts to reproduce.
Ok, I got another gmail account. Now I do see two prompts. I can fill them both in with the master password and I don't see any more prompts when I access the gmail accounts and other accounts. 
I then restart tb and enter the master password. I can then cancel the next prompt without entering anything and all still seems OK. 
I restart again and immediately cancel the master p/w prompt multiple times (hold down escape keep for a while). It keeps coming back and I enter the password and there are no more prompts and all seems OK.
This is all without the attached patch. Is this the gist of the bug or am I still missing something?
Flags: needinfo?(gds)
I applied the above "Fix - v4(spaces fixed)" patch. Now I only see 1 master password login prompt and it works (*). I have restarted multiple times with the patch in place and I still don't see any problems with missing gmail folders or missing folder on any other accounts.

Not certain, but when I look at bug 1374244 it is not clear that patch "Fix - v4(spaces fixed)" actually caused the missing gmail folders that had to be re-downloaded. Maybe this patch just makes it worse?

(*) I do notice that if I wait a minute or so to enter the password then another prompt window appears right under the first one and it too expects the password after it is entered in the first prompt window.
With multiple emails in Thunderbird on Ubuntu I got multiple prompts for a master password. Now that I disabled check for email at startup I only enter the master password once.
(In reply to Xander {MP} from comment #78)
> With multiple emails in Thunderbird on Ubuntu I got multiple prompts for a
> master password. Now that I disabled check for email at startup I only enter
> the master password once.

Do you have more than one gmail account? (I assume you mean you disabled the check for only your gmail account(s)? 

I assume you are *not* running with the attached patch above and are running a the latest 52 release, I think 52.7?

Also, do you ever see the problem that has also been report where your gmail or other account folders vanish and then re-download?

I disabled the check for new mail on both of my gmail accounts and I still see two master password prompts after about a minute with the above attached patch applied, but still just one if I enter the password quickly.
(In reply to Wayne Mery (:wsmwk) from comment #67)
> Now that the dust has settled for a few months, what options do we have to
> move forward?
> 
> Is situation similar to "Gene's" bug 1408610 (yahoo) where error checking is
> insufficient and imap operations are allowed to proceed when they should not
> have?

It does looks somewhat similar to the yahoo auth bug. I can verify that I only see the problem (gmail folders vanish and re-download) with "check for new msgs at startup" set. What I'm seeing is, after the master p.w. is entered, two back-to-back oauth2 requests are sent by tb on different ports with imap message tag 2. They have different base64 encoded content. The first one receives a immediate good response from gmail and tb uses that port to send some xlist and other imap commands that work ok. Then a base64 response occurs to the 2nd auth request that is apparently bad. Tb ignores the base64 string response and goes and and sends imap commands on the port. Gmail responds with BAD for each command after that on that port (just more basic list or xlist commands). I suspect the tb determines that the folders are not subscribed so they are removed. Eventually, on other ports, tb sends oauth2 auth requests that are accepted and imap commands begin succeeding and everything seems to work.

Looking closer, here's what happens on the port (connection) that logs in OK. Some of the string are base64 encoded but I am showing them decoded, but sometime partially indicated by "...". 

gm = gmail imap server, tb = thunderbird client.

tb: 2 AUTHENTICATE xoauth2 user-gd.smth@gmail.comauth=Bearer ya29...
gm: * CAPABILITY IMAP4rev1 UNSELECT etc
gm: 2 OK gd.smth@gmail.com authenticated (Success)
tb: 3 namespace 
<Normal error-free request/response on this port continue>

Here's what happens on the port (connection that fails authentication:

tb: 2 AUTHENTICATE xoauth2 user-gd.smth@gmail.comauth=Bearer null   <--gm sees "null" as, a bad auth string.
gm: + {"status":"400","schemes":"Bearer","scope":"https://mail.google.com/"}
tb: 3 namespace
gm: 2 NO [AUTHENTICATIONFAILED] Invalid credentials (Failure)
tb: 4 ID
gm: 4 ID {<gmail ID info>)   <-- imap id does not require auth state
gm: 5 xlist "" "%"
gm: 5 BAD Unknown command 203mb150275753ywm
gm: 5 xlist "" "%/%"
<All further commands fail with "BAD" on this connection, tb deletes folder seen as unsubscribed>

https://hg.mozilla.org/comm-central/rev/83f249160f6c (Yahoo auth plain bug 1408610)

With the above patch for AUTH plain adapted for AUTH oauth2, the "null" auth string shown above now produces a failure that tb understands and tb no long tries to use that string or send imap commands on the never authenticated port. This eliminates the problem:

tb: 2 AUTHENTICATE xoauth2 user-gd.smth@gmail.comauth=Bearer null   <--gm sees "null" as, a bad auth string.
gm: + {"status":"400","schemes":"Bearer","scope":"https://mail.google.com/"}
tb: 2 AUTHENTICATE xoauth2 user-gd.smth@gmail.comauth=Bearer null   <--tb now retries the "null" string.
gm: 2 NO [AUTHENTICATIONFAILED] Invalid credentials (Failure)
<With the fix in place, tb now re-sends the auth string like gm is expecting. Then it sees the "NO" response to the auth string so tb no longer attempts to use bad auth string or the unauthenticated port.>

So even though I have no idea where this "null" string is coming from, when it or other bad auth strings happen, they will cause no harm (e.g., removed folders) with the fix in place.

I will attach a patch in my next comment.
This is almost a clone of the yahoo fix that was for "auth plain" slightly above in the same function.
Attachment #8966010 - Flags: review?(jorgk)
(In reply to gene smith from comment #80)

There were some typos here in the tb/gm column fixed below:
> 
> Here's what happens on the port (connection that fails authentication):
> 
> tb: 2 AUTHENTICATE xoauth2 user-gd.smth@gmail.comauth=Bearer null   <--gm sees "null" as, a bad auth string.
> gm: + {"status":"400","schemes":"Bearer","scope":"https://mail.google.com/"}
> tb: 3 namespace
> gm: 2 NO [AUTHENTICATIONFAILED] Invalid credentials (Failure)
> tb: 4 ID
> gm: 4 ID {<gmail ID info>)   <-- imap id does not require auth state
> tb: 5 xlist "" "%"
> gm: 5 BAD Unknown command 203mb150275753ywm
> tb: 5 xlist "" "%/%"
> <All further commands fail with "BAD" on this connection, tb deletes folder seen as unsubscribed>
Just to try to be clearer, without my patch above but with the above master password patch in place and with 2 gmail accounts and "check for new mail at startup" enabled, I always see gmail folders vanish for at least one account (and usually re-download after the account is double clicked a few times). In this case, the "null" authentication string mentioned above always occurs for a least one of the gmail account. With only "check for new mail at startup" disabled, the null string does not occur and I only see complete and valid authentication strings so no folder deletion occurs.

With both patches above in place, 2 gmail accounts and "check for new mail at startup" enabled, the gmail accounts do not have folders vanish and need to be download. The "null" auth string still occurs and is sent but it causes no problems that I can see since tb now detects the login failure on that port and quickly switches to another port and this time obtains a non-null and valid auth string to use.

There may be other random cases where the auth string is corrupted by the network and causes the folder deletion that is not related to this errant "null" string. This may account for random reports of "all my gmail folder vanished and then re-downloaded".

Also, as a final note, 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. Also, another obvious question is "why does tb ever delete a folder when not asked to?".
Comment on attachment 8966010 [details] [diff] [review]
Fix to allow proper retry on oauth2 authentication failure

Thanks for the investigation, Gene.

I have no problem to implement here what we already implemented for plain autu/Yahoo:
https://dxr.mozilla.org/comm-central/rev/98d0311d8838edcc1e07ab7b723538083a300380/mailnews/imap/src/nsImapProtocol.cpp#5903

That said, the other patch in this bug is, how shall I say it, not well understood, at least not by me, and has been questioned in bug 1374244 comment #4. So unless someone fully understands and fully addresses all the concerns raised, I don't think we can go forward here.

We should also investigate what's behind the "null" auth string that you observed.

Magnus, I'm happy with Gene's additional code which has been tried and tested, but I'd really like to understand Philipp's patch and Gijs' comments.
Attachment #8966010 - Flags: review?(jorgk)
Attachment #8966010 - Flags: review+
Attachment #8966010 - Flags: feedback?(mkmelin+mozilla)
(In reply to Jorg K (GMT+1) from comment #84)

> Magnus, I'm happy with Gene's additional code which has been tried and
> tested, but I'd really like to understand Philipp's patch and Gijs' comments.

My patch is independent of this one. We have an async prompter that does this:

> on new password request:
>   queue the request, possibly folding duplicate requests using a hash key
> 
> while loop:
>   get top request from queue
>   synchronously request password (which triggers master password prompt)
>   notify responses for all requests of the same hash key

The problem is the synchronous password request. For gmail, we have the following workflow:


> on new password request:
>   queue the request, possibly folding duplicate requests using a hash key
> 
> while loop:
>   get top request from queue
>   asynchronously request password, which doesn't complete in this while loop
>   notify responses for all requests of the same hash key (before the password prompt completed)

The master password prompt is triggered once from the first gmail account during "asynchronously request password", then the next request is taken from the queue, which prompts for the master password again. What my patch is makes the "synchronously request password" step async, and waits for completion, so:

> on new password request:
>   queue the request, possibly folding duplicate requests using a hash key
> 
> while loop:
>   get top request from queue
>   await (asynchronously request password)
>   notify responses for all requests of the same hash key


As a result, in theory, there will be no two master password prompts because the next request from the queue is not taken until the first request (which includes prompting for master password) is either completed or cancelled.
Thanks for the explanation, sadly I don't follow it:

(In reply to Philipp Kewisch [:Fallen]  from comment #85)
> We have an async prompter that does this:
> >   synchronously request password (which triggers master password prompt)
I this the existing state for non-Gmail? Where is the code? You say "async prompter", but does a sync request. So I'm confused.

> The problem is the synchronous password request. For gmail, we have the
> following workflow:
OK, is there different processing for Gmail? Where's the code?

> >   asynchronously request password, which doesn't complete in this while loop
OK, so this time the async prompter does an async request. I can follow that.

> What my patch is makes the "synchronously request password" step async, and waits for
> completion, ...
Hmm, I thought it was async already, al least for Gmail? I'm more confused.

I can follow the idea that waiting for the first reply makes sense to satisfy further replies.

Maybe given the severity of the problem, it makes sense to document it completely.

How about Gijs' comments. Is he mistaken? Quoting from bug 1374244 comment #4:
===
It looks to me like, before, oauth connect didn't directly call into the prompt code, and now it does, which I don't understand. Like, it didn't call onPromptStart() directly anyway, but now it's been made to call onPromptStartAsync? Also, it then calls back to the aListener passed to connect() *before* calling back into the prompt thing's onAuthResult, which also seems odd. If anything, I would expect that the aListener callback should wait until after the onAuthResult call has been resolved and whatever code depends on that had run to completion.

More generally, it seems oauth.connect() used to return synchronously (and call onSuccess() synchronously!) in the case where a token was readily available, and now it doesn't. Perhaps some caller doesn't wait, but should?

The timing differences could mean that there's some kind of race - which folders get recreated / deleted seems to vary every time I start up, which would support that theory.
===
(In reply to Jorg K (GMT+1) from comment #86)
> Thanks for the explanation, sadly I don't follow it:
> 
> (In reply to Philipp Kewisch [:Fallen]  from comment #85)
> > We have an async prompter that does this:
> > >   synchronously request password (which triggers master password prompt)
> I this the existing state for non-Gmail? Where is the code? You say "async
> prompter", but does a sync request. So I'm confused.
The prompter is "async", in the sense that you can give it multiple password requests, and it doesn't complete them synchronously, but queues them up and therefore does them asynchronously. 

The "synchronous" part is when the password prompt is brought up. The prompting code is assumed to be synchronous (show modal password dialog, return), so it continues operation once the nsIMsgAsyncPrompter::onPromptStart function call returns. For gmail, the prompt itself is asynchronous (open browser window, wait for it to complete, call back with results).

Once nuance, what I described is how it works for Lightning's gmail oauth. It turns out it is slightly different for IMAP, where the async prompter is just not used at all, so the master password dialogs just get triggered right away.

The patch uses the async prompter in both cases, but additionally adds nsIMsgAsyncPrompter::onPromptStartAsync which allows for an async password prompt.

> 
> > The problem is the synchronous password request. For gmail, we have the
> > following workflow:
> OK, is there different processing for Gmail? Where's the code?

The code is in msgOAuth2Module.js and nsSmtpProtocol.cpp. Processing is different as per above nuance.

> Maybe given the severity of the problem, it makes sense to document it
> completely.
I don't mean this in a bad way, but nsIMsgAsyncPrompter.idl is pretty self-documenting, I don't know what exactly needs to be documented?



> How about Gijs' comments. Is he mistaken? Quoting from bug 1374244 comment
Could be right in that the wrong listeners are called, but I'd have to take a closer look at the code again to confirm.
(In reply to Philipp Kewisch [:Fallen]  from comment #87)
> I don't mean this in a bad way, but nsIMsgAsyncPrompter.idl is pretty
> self-documenting, I don't know what exactly needs to be documented?
I don't mean this in a bad way, but what's missing is an overall design document. It's nice that the wheels are documented, but that doesn't help fixing a car that's flawed by design. We need a document that explains the principles and then points to the respective code for the details.

That should also include a few "use" cases, like:
TB starts. Such and such code decides to get now messages for all accounts. Server 1 is contacted via protocol XXX and server asks for password. That's retrieved via YYY. That's how the master password prompt is triggered at ZZZ. Meanwhile another server 2 is contacted and TTT happens. To synchronise the various password retrievals all protected by the same master password, we do ... and so forth. For Gmail we have the following additional complications ... Etc.

Such a document should then allow a developer with "regular" stills to investigate for example why the workaround given here (don't retrieve messages at start, let the timer get them after a while) executes a different code path that avoids multiple prompts.

Looks like you're already trying to explain some of the principles here in this bug, at least in fragments. Surely that's incomplete since there are more protocols requiring passwords, most prominently Gmail's IMAP (not SMTP).

As a complete novice to this area, I can only observe these facts:
1) TB like FF stores passwords for servers or sites.
2) These passwords can be protected by a master password that you should only
   need to enter once per session.
3) Gmail's setup may be different during the first establishment of the password
   (open browser window, etc.), but once the password and/or OAuth2 token is/are stored,
   I don't see the difference any more.

Don't get me wrong, you most likely came over from Calender which also connects to Google and tried to fix the Mailnews issues. That's laudable. The general lack of documentation is not your fault either. But for knowledge transfer and to attract more contributors, we need much more documentation, especially like here, where we don't have linear processing and many system components need to play together.
A general design document is a good idea, I had understood you wanted to document this specific issue and how the async prompter works. I agree with you that this would help attract additional developers.
(In reply to Philipp Kewisch [:Fallen]  from comment #85)
> (In reply to Jorg K (GMT+1) from comment #84)
> 
> As a result, in theory, there will be no two master password prompts because
> the next request from the queue is not taken until the first request (which
> includes prompting for master password) is either completed or cancelled.

I assume this means "there will not be two master password prompts" and not "there will now be two master password prompts"?
I mentioned this before, but I still see two master password prompts if I wait a while (maybe 30-60s) before entering the first one. If I immediately enter the master password, I get no more prompts. This is with your patch in place.
(In reply to Jorg K (GMT+1) from comment #88)
> (In reply to Philipp Kewisch [:Fallen]  from comment #87)
> 
> As a complete novice to this area, I can only observe these facts:
> 1) TB like FF stores passwords for servers or sites.
> 2) These passwords can be protected by a master password that you should only
>    need to enter once per session.
> 3) Gmail's setup may be different during the first establishment of the
> password
>    (open browser window, etc.), but once the password and/or OAuth2 token
> is/are stored,
>    I don't see the difference any more.

As someone who can hardly spell "oathu2" it may be a bit different than other stored passwords. The password for gmail visible in password manager is not the same as the accessToken sent out in the imap "AUTHENTICATE xoauth2 ***" command. It varies some each time it is sent for the same account. I also see some https network activity each time the accessToken is obtained but have not yet attempted to decode it. I have been unable to find anywhere in the tb oauth2 code where it produces an accessToken with the raw unencoded value "null" like I am seeing. So maybe this "null" accessToken is being fetched over the network from the google servers?
Comment on attachment 8966010 [details] [diff] [review]
Fix to allow proper retry on oauth2 authentication failure

We'll deal with the IMAP changes in bug 1453643.
Attachment #8966010 - Attachment is obsolete: true
Attachment #8966010 - Flags: feedback?(mkmelin+mozilla)
Not sure why but this seem to fix the problem independent of the changes in bug 1453643. With this change (not a formal patch) only the valid non-null password/access token is sent so there is no problems with deleted folders and re-downloading. However, the patch in bug 1453643 still needs to occur.

diff --git a/mailnews/base/src/msgOAuth2Module.js b/mailnews/base/src/msgOAuth2Module.js
--- a/mailnews/base/src/msgOAuth2Module.js
+++ b/mailnews/base/src/msgOAuth2Module.js 

  buildXOAuth2String() {
-    return btoa("user=" + this._username + "\x01auth=Bearer " +
-      this._oauth.accessToken + "\x01\x01");
+    if (this._oauth.accessToken == null)
+      return null;
+    else
+      return btoa("user=" + this._username + "\x01auth=Bearer " +
+        this._oauth.accessToken + "\x01\x01");
   },
 };
Just noticed that the above change only causes the bad password to not be sent out so folders are not deleted/re-downloaded. However, the null return still causes an "Authentication failed for gmail" message to pop up. With this additional change (again, for info only) a retry occurs that successfully obtains a non-null password and no pop-up message occurs:

diff --git a/mailnews/imap/src/nsSyncRunnableHelpers.cpp b/mailnews/imap/src/nsSyncRunnableHelpers.cpp
--- a/mailnews/imap/src/nsSyncRunnableHelpers.cpp
+++ b/mailnews/imap/src/nsSyncRunnableHelpers.cpp

 void OAuth2ThreadHelper::GetXOAuth2String(nsACString &base64Str)
 {
+  bool retried = false;
   MOZ_ASSERT(!NS_IsMainThread(), "This method cannot run on the main thread");

   // Acquire a lock early, before reading anything. Guarantees memory visibility
   // issues.
   MonitorAutoLock lockGuard(mMonitor);

   // Umm... what are you trying to do?
   if (!mOAuth2Support)
     return;
-
+retry:
   nsCOMPtr<nsIRunnable> runInit =
     NewRunnableMethod("OAuth2ThreadHelper::GetXOAuth2String", this, &OAuth2ThreadHelper::Connect);
   NS_DispatchToMainThread(runInit);
   mMonitor.Wait();

   // Now we either have the string, or we failed (in which case the string is
   // empty).
+  if (!retried && mOAuth2String.IsEmpty())
+  {
+    retried = true;
+    goto retry;
+  }
   base64Str = mOAuth2String;
 }

Also, just to be clear, these changes are with the attached "Fix - v4 (spaces fixed)" patch in place.
comment 94 fully digested?
Flags: needinfo?(gds)
See Also: → 1453643
(In reply to Wayne Mery (:wsmwk) from comment #95)
> comment 94 fully digested?

Wayne, I don't know what you mean by "fully digested?" or what info you are asking for. I will just say that comment 94 and comment 93 are just some observations on what appears to fix the patch for me. Since there seem to be others much more expert in this area (javascript, async calls, etc) than me, I posted this just for information purposes.
Flags: needinfo?(gds)
If a mailto: in my browser opens Thunderbird.
Thunderbird tries to store the new created mail in my IMAP account.
After many tries and errors it finally ask for the master password.

If i open thunderbird manually I can see at the bottom of the screen that the program tries to connect to my 3 IMAP accounts.
After that the master password pop-up appears.
The echo of the pressed keys is slow.

Recently I have a old dual-core pc with Linux mint.
No problems at all, there.

I have that problem only on my Windows system.(i7)
Originally, I opened or confirmed an issue (this one, I believe) where TB would ask multiple times for the master password before letting you in. This may not have been related to the GMAIL issue. The problem happens when you DO NOT log in right away. I sat here this morning on El Capitan on a Mac and opened TB 52.7.0 (64-bit). Instead of logging in, I waited about 15 minutes. When I attempted to log into TB using the master password, I was presented three times with the login dialogue before gaining access. This problem still exists. If you do not wait to log in, TB only asks once for the master password. I have not tried waiting one minute, then two minutes, then three minutes, etc., to see what would happen.
Yes in comment 90 and comment 79 I noticed the same thing. With or without the patch.
Attached patch 1176399-masterpassword-1.diff (obsolete) — Splinter Review
I've dusted off the patch and rebased it, and it seems to do what it's supposed to. I do get multiple prompts if I cancel though.

Making some builds here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=61e36e1532657d488d4de9065f8541b7d3fd3c12
Attachment #8855936 - Attachment is obsolete: true
(In reply to Geoff Lankow (:darktrojan) from comment #102)
> I've dusted off the patch and rebased it, and it seems to do what it's
> supposed to. I do get multiple prompts if I cancel though.
The issue isn't so much whether it works, that has been confirmed before. The issue is whether the approach is correct.

See comment #84 to comment #88 here. Can you follow the new logic and disprove Gijs' comments (bug 1374244 comment #4)?
I've spent an age trying to wrap my head around this. I understand what it's trying to do - call each auth in series, so that the master password is available to each subsequent check - but I cannot see how it could cause bug 1374244. The only thing I can think of is if OAuth2Module.connect throws an exception and the caller [1] drops into that NS_FAILED block. From that I don't know what would happen. Gijs's suggestion of a race condition makes me wonder if there is something in there.

I can't reproduce bug 1374244, either with v52.2.0 or the updated version I made above.

[1] https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsSyncRunnableHelpers.cpp#565
Thanks for analysing this!

(In reply to Geoff Lankow (:darktrojan) from comment #104)
> ... - but I cannot see how it could cause bug 1374244.
(that is the re-download)
I though Gene's comment #80 to comment #84 illustrates what happened with the patch in place, even if that's not a full explanation. Note that his patch attached in comment #81 got landed (in a refactored form) in bug 1453643.

What about Gene's suggestions in comment #93 and comment #94?
As a simple user, i see differences between Thunderbird on Windows 10 and Linux mint.
The windows machine starts to log in on the accounts and asks later in the process for the master password.
The Linux machine, halts at the start en prompts direct for the password.
The old Linux barrel computer wins from the ssd/Windows/i7 machine in start-up speed.
Every extra account makes the problem on the Windows version bigger.
For 3 IMAP accounts it takes sometimes 20 seconds before prompting for the password.(fiber internet 100/100)
I applied Geoff's patch and now see one password prompt even when I wait a while (maybe 2-3 min) before entering the master password. However, if I wait 20-30 min I see two password prompt boxes, apparently one for each gmail account (I have two)?

However, I still see the transient "NULL" auth string (base64 encoded of course) being sent as described in comment 82 above. The need still exists to return null (not the string "NULL") from buildXOAuth2String() in mailnews/base/src/msgOAuth2Module.js when a non-empty token cannot be obtained (see comment 93 above). Note: Without the fix in Bug 1453643 the "NULL" string would often cause the deletion and redownload of folders.

Also, right after the master password is entered, usually buildXOAuth2String() returns null and 1-3 times before a valid token is returned. This causes spurious "Authentication failure while connecting to server imap.gmail.com" pop-ups to occur. This change to mailnew/imap/src/nsSyncRunnableHelpers.cpp (similar to comment 94 above) allows up to 5 retries (forgive the goto):

=============
diff --git a/mailnews/imap/src/nsSyncRunnableHelpers.cpp b/mailnews/imap/src/nsSyncRunnableHelpers.c
--- a/mailnews/imap/src/nsSyncRunnableHelpers.cpp
+++ b/mailnews/imap/src/nsSyncRunnableHelpers.cpp
@@ -512,33 +512,43 @@ bool OAuth2ThreadHelper::SupportsOAuth2(

   // After synchronously initializing, if we didn't get an object, then we don't
   // support XOAuth2.
   return mOAuth2Support != nullptr;
 }

 void OAuth2ThreadHelper::GetXOAuth2String(nsACString &base64Str)
 {
+  const uint16_t RETRIES = 5;
+  uint16_t retries = RETRIES;
   MOZ_ASSERT(!NS_IsMainThread(), "This method cannot run on the main thread");

   // Acquire a lock early, before reading anything. Guarantees memory visibility
   // issues.
   MonitorAutoLock lockGuard(mMonitor);

   // Umm... what are you trying to do?
   if (!mOAuth2Support)
     return;

+retry:
   nsCOMPtr<nsIRunnable> runInit =
     NewRunnableMethod("OAuth2ThreadHelper::GetXOAuth2String", this, &OAuth2ThreadHelper::Connect);
   NS_DispatchToMainThread(runInit);
   mMonitor.Wait();

   // Now we either have the string, or we failed (in which case the string is
   // empty).
+  if (retries && mOAuth2String.IsEmpty())
+  {
+    retries--;
+    goto retry;
+  }
+  printf("gds: tried this many times to get authToken = %d\n", RETRIES-retries+1);
   base64Str = mOAuth2String;
 }

 void OAuth2ThreadHelper::Init()
 {
   MOZ_ASSERT(NS_IsMainThread(), "Can't touch JS off-main-thread");
   MonitorAutoLock lockGuard(mMonitor);
=============

One other thing also causes spurious "Authentication failure while connecting to server imap.gmail.com" pop-ups to occur. Where GetXOAuth2String() is called in nsImapProtocol.cpp, the tcp connection to the server is already established for gmail and the master password prompt is present and waiting. The socket timer was set to 100s and has not been disabled since the connection is not yet "cached". This means that the if the user waits more than 100s to enter the master password, the function SendData() will attempt to send the password/accessToken but it silently fails (*) and then the attempt to decode the response loudly fails and triggers the pop-up. A solution to this is to disable the socket timer before calling GetXOAuth2String() in nsImapProtocol::AuthLogin():

=============
@@ -6049,32 +6072,47 @@ nsresult nsImapProtocol::AuthLogin(const
   {
     MOZ_LOG(IMAP, LogLevel::Debug, ("XOAUTH2 auth"));

     // Get the XOAuth2 base64 string.
     NS_ASSERTION(mOAuth2Support,
       "What are we doing here without OAuth2 helper?");
     if (!mOAuth2Support)
       return NS_ERROR_UNEXPECTED;
+
+    // Disable socket timer since user may wait an unlimited time to enter
+    // a master password.
+    if (m_transport)
+      m_transport->SetTimeout(nsISocketTransport::TIMEOUT_READ_WRITE, PR_UINT32_MAX);
+
     nsAutoCString base64Str;
     mOAuth2Support->GetXOAuth2String(base64Str);
     mOAuth2Support = nullptr; // Its purpose has been served.
     if (base64Str.IsEmpty())
     {
       MOZ_LOG(IMAP, LogLevel::Debug, ("OAuth2 failed"));
       return NS_ERROR_FAILURE;
     }

     // Send the data on the network.
     nsAutoCString command (GetServerCommandTag());
     command += " AUTHENTICATE XOAUTH2 ";
     command += base64Str;
     command += CRLF;

     rv = SendDataParseIMAPandCheckForNewMail(command.get(), nullptr);
=============

Note: Proper functioning of SetTimeout() depends on the fix proposed in bug Bug 1469117, attachment "netwerk.diff -- proposed fix for socket timer"

Note: Non-oauth2 accounts don't establish a connection to the imap server until after the master password is entered so it is not necessary to disable the socket timeout for them.

(*) For some reason, if the socket has timed out, when SendData() is called it does not detect the error and returns NS_OK but nothing is actually sent (and the imap log indicates that the data was sent, even though it wasn't). The error is not detected until later when the response "line" is attempted to be decoded.
(In reply to erik from comment #106)
> As a simple user, i see differences between Thunderbird on Windows 10 and
> Linux mint.
> The windows machine starts to log in on the accounts and asks later in the
> process for the master password.
> The Linux machine, halts at the start en prompts direct for the password.

I have been testing this on linux and what I see is that tb establishes 2 or 3 connections to the gmail server and does a successful CAPABILITY request on each and then the connections go idle while the master password prompt is waiting. Then when the user enters the password, the access token is obtained from ??? but is not yet ready and an empty token is returned, so retries are needed. Then the token is sent to the server and all should be well (except for several issues described and addressed in comment 107 above). 

I don't see anything in the code where the connection and authentication behavior differs by OS, but I could be wrong.
(In reply to gene smith from comment #107)
> I applied Geoff's patch and now see one password prompt even when I wait a
> while (maybe 2-3 min) before entering the master password. However, if I
> wait 20-30 min I see two password prompt boxes, apparently one for each
> gmail account (I have two)?

Two more, or the original one plus another?

> However, I still see the transient "NULL" auth string (base64 encoded of
> course) being sent as described in comment 82 above. The need still exists
> to return null (not the string "NULL") from buildXOAuth2String() in
> mailnews/base/src/msgOAuth2Module.js when a non-empty token cannot be
> obtained (see comment 93 above). Note: Without the fix in Bug 1453643 the
> "NULL" string would often cause the deletion and redownload of folders.

I can't see how that would happen. Even if this._username and this._oauth.accessToken are weird there should still be the string parts and the \1 bytes. Unless the btoa call is malfunctioning. What?!


> Where
> GetXOAuth2String() is called in nsImapProtocol.cpp, the tcp connection to
> the server is already established for gmail and the master password prompt
> is present and waiting. 

> Note: Non-oauth2 accounts don't establish a connection to the imap server
> until after the master password is entered so it is not necessary to disable
> the socket timeout for them.

This really isn't my area, but why are we setting up network connections before we're ready to use them? Perhaps if we stop doing that, some of our problems here will go away.
(In reply to Geoff Lankow (:darktrojan) from comment #109)
> (In reply to gene smith from comment #107)
> > I applied Geoff's patch and now see one password prompt even when I wait a
> > while (maybe 2-3 min) before entering the master password. However, if I
> > wait 20-30 min I see two password prompt boxes, apparently one for each
> > gmail account (I have two)?
> 
> Two more, or the original one plus another?

Original one plus another after a 20-30m wait. Not sure when the 2nd one appears but I usually only see 1 if I enter the master pwd within the 1st few minutes. However, just now when I immediately entered the password, I saw two again!

> 
> > However, I still see the transient "NULL" auth string (base64 encoded of
> > course) being sent as described in comment 82 above. The need still exists
> > to return null (not the string "NULL") from buildXOAuth2String() in
> > mailnews/base/src/msgOAuth2Module.js when a non-empty token cannot be
> > obtained (see comment 93 above). Note: Without the fix in Bug 1453643 the
> > "NULL" string would often cause the deletion and redownload of folders.
> 
> I can't see how that would happen. Even if this._username and
> this._oauth.accessToken are weird there should still be the string parts and
> the \1 bytes. Unless the btoa call is malfunctioning. What?!
> 

This is what gets sent when the access token is empty, where stuff after "xoauth2" is actually base64 encoded:
tb: 2 AUTHENTICATE xoauth2 user-gd.smth@gmail.comauth=Bearer null=\1\1

So the username and Bearer string are encoded OK. It's just the this._oauth.accessToken that is empty/null and get encoded as the literal string "null".

Here is what gets sent when the token is good, where the good AccessToken is ya29...:
tb: 2 AUTHENTICATE xoauth2 user-gd.smth@gmail.comauth=Bearer ya29...\1\1

See comment 80 for my original discussion of this.

I don't understand this code well enough to understand where the access token comes from. Is it created within tb or does tb go to google (via web/https and not via imap) to get it? It is not always the same string and it is not the same as the "password" string stored in the password manager.

> This really isn't my area, but why are we setting up network connections
> before we're ready to use them? Perhaps if we stop doing that, some of our
> problems here will go away.

I really don't know why it presently works that way. However, other than the socket timing out after 100 sec, I don't think it is the cause of why I often see a null string this._oauth.accessToken in BuildXOAuth2String(). All I know for sure is that the accessToken is not obtained over the imap connection and if you retry the BuildXOAuth2String() call, you eventually get a not empty accessToken.
Okay, I can make buildXOAuth2String return "" when there's no token. It looks like the IMAP code handles this well and the SMTP code doesn't:

https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#5994
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsSmtpProtocol.cpp#1651

> I don't understand this code well enough to understand where the access token comes from. Is it created within tb or does tb go to google (via web/https and not via imap) to get it? It is not always the same string and it is not the same as the "password" string stored in the password manager.

The tokens are negotiated over HTTP. I *think* what's stored in the password manager is the refresh token, which is used to get a new auth token when the old one expires.
Up to now I haven't really looked at what happens when I don't have the master password feature turned on. Do I see null accessTokens obtained then too? The answer is yes. On each tb startup I see a null access token set by BuildXOAuth2String() and with two "tries" needed to obtain a non-empty token. (This is with my diff's above applied that do up to 5 retries to obtain a non-empty token.
(In reply to Geoff Lankow (:darktrojan) from comment #111)
> Okay, I can make buildXOAuth2String return "" when there's no token. It

Not sure what you mean by "no token". Delete the password in password mgr?
Wouldn't it return something like (Base64)"user-gd.smth@gmail.comauth=Bearer\1\1" when there is no accessToken?

> The tokens are negotiated over HTTP. I *think* what's stored in the password
> manager is the refresh token, which is used to get a new auth token when the
> old one expires.

That's what I thought but wasn't sure. So any null/empty accessToken string would come from Google?

(In reply to Geoff Lankow (:darktrojan) from comment #113)
> I'm making a new build with the change to buildXOAuth2String.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=21af80bbc89440f744734c96d09996381dfaebf7

 
   buildXOAuth2String() {
+    if (!this._oauth.accessToken) {
+      return "";
+    }
+
     return btoa("user=" + this._username + "\x01auth=Bearer " +
       this._oauth.accessToken + "\x01\x01");
   },
 };

Building it here now too. Looks like it will work. However, maybe will still see pop-up errors when the accessToken is empty?  I will try it.
Yes, it works just like my diff above. However, when I disable retries in  OAuth2ThreadHelper::GetXOAuth2String() in nsSyncRunnableHelpers.cpp, you still spurious pop-ups saying "Authentication failure while connecting to server gmail...".

However, these failure are innocuous and don't cause any harm; just misleading.

This is what I was pointing out in comment 94 above. But I still have no idea why retries help. The last time I started tb with 5 max retires enabled, 3 tries were needed to obtain a non-null token for one one connection; 2 on another. Others just required 1 try.
Blocks: 1180374
As the original reporter of this bug, I'm back.
For all this time I installed the MasterPassword+ addon which allowed to manage my 10 email accounts (one of which was yahoo, another was Microsoft and a third was Google) and my 7 Google Calendars (synced by using CalDAV) without any problem, ie I was asked for only one master password.
Now, with the introduction of TB60, MasterPassword+ stopped to work and I went back to the original, TB built-in, master password.
I was very surprised that the problem of the double request is still alive: two overlapping windows appear asking me for the introduction of the master password. Sometime I was successful to login answering *just* to 2 requests; some others I was asked more than twice. I'm honest, anyway: when I was reqired more than twice, it could be I was wrong and I did not write the correct password.
Anyway there are still at least 2 requests.
So I decided to remove the master password at all (actually its protection is basic) at least until a solution will be provided. Anyway, in my ignorance, I was wondering why don't the developers think to try to follow/implement/try to adapt MasterPassword+ technique to Thunderbird to bypass the problem?
Topoldo
I filed an issue for MasterPassword+ at https://github.com/vanowm/MasterPasswordPlus/issues/140
While MasterPassword+ does not work anymore with Thunderbird 60 even if you use compatibility flag because problems about how its options are managed, I found that another (less powerful) extension ie StartupMaster 1.6.5 which is VERY similar - at least for the end user point of view - to the original master pasword as implemented by Thunderbird, works!
It is sufficient to set the flag of compatibility for the extensions or edit its install.rdf changing the manimum version of TB from 56.0 to 60.0 (or higher) and, al least in my hands, it works like charm.
Only one master password requested (not 2 or more!) for my 10 emails accounts and 7 Google Calendars as I reported a couple of posts above.
Regards,
Topoldo
I have found a bug in the OAuth handling. Potentially it is *the* bug that caused the problem, but I am not sure. onPromptAuthAvailable was being called outside of the OAuth transaction, thus there was no access token to send. I knew there was something fishy about that.
Attachment #8987423 - Attachment is obsolete: true
Attachment #9000196 - Flags: review?(mkmelin+mozilla)
Also, I've skipped the step of going back over the XPCOM boundary to build an auth string, since both the callers do that immediately anyway.
Comment on attachment 9000196 [details] [diff] [review]
1176399-masterpassword-2.diff

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

Thanks for the detective work! Seems ok as far as I can tell, so lets land this for wider testing. r=mkmelin

::: mailnews/base/public/msgIOAuth2Module.idl
@@ -50,5 @@
> -  /**
> -   * Return the base64-encoded string to send as the client initial response for
> -   * SASL XOAUTH2.
> -   */
> -  ACString buildXOAuth2String();

For backporting removing this may be a bit problematic. I wonder if it's used by any add-on.
Attachment #9000196 - Flags: review?(mkmelin+mozilla) → review+
"Master Password+", which is a good candidate, doesn't use it.

BTW, we re-land bug 682474 and bug 1359967? Maybe Geoff can unrot(?)/merge the patches and attach them to bug 682474.
Flags: needinfo?(geoff)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/749cb8eaaea2
Multiple requests for master password when GMail OAuth2 is enabled. r=mkmelin
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Still an issue on latest TB 60.0 build 20180731173940 (32-bit).  Multiple master password prompts overlay each other for each account (5 in my case).  You can enter the master password once, and then simply click "OK" on the other 4 windows.  Irritating that this has been a problem for years.
(In reply to Thomas Brown from comment #125)
> Still an issue on latest TB 60.0 build 20180731173940 (32-bit).
We are aware of that. The bug has just been fixed in TB 63 available as Daily. You will see more activity in this bug when it gets backported to TB 60.
In fact, I'll do that right now:
TB 60.0 beta 11: https://hg.mozilla.org/releases/comm-beta/rev/89062153e38c0605b540ddbc27a5b41797698a78 (BETA_60_CONTINUATION)
TB 60.1 ESR: https://hg.mozilla.org/releases/comm-esr60/rev/ffd9a9c6d757fa3cf7bad86cdfbf7cc71ee5cb6f

Note, of course it won't work unless you use a binary containing those changes.
Attachment #9000196 - Flags: approval-comm-esr60+
I tried out the daily 63.0a1 2018-08-18 and it is fixed for me (3 accounts - 2 gmail, 1 imap), I only get one master password prompt. Woohoo! Great work!
Thanks for the confirmation. The fix will ship in TB 60.0 beta 11 about next week, and if all goes well, in TB 60.1 ESR in September. Note that the problem was "fixed" once before, but caused re-download of entire Gmail folders. We believe that we've eliminated that unwanted side-effect now.

P.S.: Moving NI to bug 682474.
Flags: needinfo?(geoff)
Geoff, please take a look at my ESR 60 push:
https://treeherder.mozilla.org/#/jobs?repo=comm-esr60&revision=34cd910fdbfcaff6ab8bb2984bef992c4eefb6b9

A lot of test failures that look as if they came from here :-(
Flags: needinfo?(geoff)
More precisely: Xpcshell test failures. The patch applied to comm-esr60 after a little tweak, so I don't think it's an uplift problem.
Actually, it's most likely fixed here:
https://hg.mozilla.org/try-comm-central/rev/a323054cbe5015a3a7037617da9eaf55ee50dce6
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cbca46851015a413a000962d8cdba0a04ee11c7d

Should we modernise and use 'await' along with scrapping Task and making it an async function like in trunk?
Your try run's good. Just push that fix and don't worry about it. :)
Flags: needinfo?(geoff)
Fixed for me in 60b11 from channel update.
(In reply to Chris Andrichak from comment #136)
> Fixed for me in 60b11 from channel update.

Hi,
Can you tell me how I get the fix please?
Thanks,
Download the latest beta from here https://www.thunderbird.net/en-GB/channel/ or wait for TB 60.1 to be released.
I still had the StartupMaster extension installed with TB 60.0b11, and thought I give it a try and remove it.
Without the extension I still get the multiple master password prompts with TB 60.0b11 64-bit on Linux.
I do have both, a Gmail IMAP account with OAuth2 authentication, and a Google Calendar accessed via CalDAV.
Same with TB 60.2.1 64-bit on Linux. I do have both, a Gmail IMAP account with OAuth2 authentication, and a Google Calendar accessed via CalDAV. I do get two master password prompts.
Blocks: 1465154
(In reply to Christian Riechers from comment #140)
> Same with TB 60.2.1 64-bit on Linux. I do have both, a Gmail IMAP account
> with OAuth2 authentication, and a Google Calendar accessed via CalDAV. I do
> get two master password prompts.

I have the same configuration, and I do not get two prompts.
(In reply to Geoff Lankow (:darktrojan) from comment #141)
> I have the same configuration, and I do not get two prompts.

The two master password prompts also happen in TB63.0 beta safe mode. So it may not be related to Google OAuth2 at all.
I do also have an AOL and Yahoo account. Both are using OAuth2 authentication. The problem may be there.
You need to log in before you can comment on or make changes to this bug.