381269 - Multiple simultaneous master password prompts when checking multiple imap accounts on startup

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Arkady Sherman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070514 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070514 SeaMonkey/1.5a

Passwords for several imap mail accounts are stored with the same master password.
The "Check new messages at startup" option is checked.
When starting "Mail & Newsgroup" several prompts for Master password are appearing  simultaneously.
 

Reproducible: Always

Steps to Reproduce:
1. Several mail accounts should be present and passwords for them should be stored with Master password.
2. Check "Check new messages at startup" options for the accpunts
3. Start "Mail & Newsgroup"
Actual Results:  
Several prompts for Master password appear simultaneously at the same time.

Expected Results:  
Only one prompt for Master password should be asked.

Comment 1

[Nickolay_Ponomarev]

Note bug 369963.

Comment 2

[Serge Gautherie (:sgautherie)]

Can you reproduce with SeaMonkey v2.0a1pre ?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I believe that this is also a dupe of bug 356097.

Comment 4

[Stefan A. Möller]

I can reproduce it with SeaMonkey 2.0a1

Comment 5

[Jeffrey Barnes]

I can reproduce it with SeaMonkey 2.0a2 and POP3 instead of IMAP accounts.

Comment 6

[Bill Davidsen]

Happens in Linux, one prompt for each mail or news account, EVEN IF ONLY A BROWSER WINDOW IS OPENED.

Expected behavior:
1 - at most one prompt for master password
2 - prompt only when the MP is used the first time
    when opening only browser don't ask about mail
3 - maintainer would fix a bug which has been known for 2+ years!!

Comment 7

[Martin F.]

Even happens in SM2.0b1 after migrating from SM1.1.17 where no master password was used at all!

Comment 8

[Matthew Cheale]

I'm getting the same as commander_keen.  Never seen it before 2.0b1.

Comment 9

[Sam]

I have similar (same?) problem:
Restoring session with tabs logged into various accounts (email, nytimes, facebook, etc, etc).
It'll ask for master password multiple times ...
Eventually it gets flooded with requetsts (especially bad on Mac OS due to warning windows hanging from tabstrip - not separate windows), so I have to disable master password & restart this session.

Mac OSX.5.8/Firefox 3.5.4pre

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

I'll make this bug the target for the SM 2.0 interim solution. The real solution is developed in bug 338549 but will most probably not be ready in time for SM 2.0.

The patch I will attach in a minute will address the issue described by the summary of this bug and comment 6, unless the Master Password is canceled. It will not fix the issue described in comment 9 (which might be fixed by bug 475053 which hasn't landed on any branch yet AFAIK).

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Attachment: proposed patch

This is mostly TB's workaround as can be found here:
<https://bug501212.bugzilla.mozilla.org/attachment.cgi?id=386414>
(thanks to Peter Weilbacher for pointing this out in m.d.a.seamonkey!)

Additionally I defaulted the mail.biff.on_new_window pref to false which will disable the checking of MailNews accounts on opening the browser window (or any other one) for all profiles (the pref can be set to true again manually afterwards), something which is no big loss anyway (and doesn't work for IMAP, see bug 493478). Please note that this pref is already ifdef MOZ_SUITE so no TB review/approval required.

Please provide a fast review if you can; it'd be a shame if this missed the imminent SM 2.0 code freeze.

NB: I don't think the MailNews part needs a pref but if reviewers require it I will add it.

The effect of the patch is as follows:
1. if only the browser is opened, no extra Master Password prompt appears since MailNews biff is not triggered. If one or more websites trigger a Master Password prompt you will get multiple ones, though (as I said, that part is not addressed)
2. if MailNews is opened (either alone or together with the browser or later, e.g. from the browser) exactly one Master Password prompt appears. Only if that one is canceled the old behavior is triggered (multiple prompts).

I think this is the best we can do currently. The current behavior is unacceptable IMO. I think this should block but since I guess that that request would be denied I'm asking for wanted, mainly to get an idea whether the Council is in line with me here.

Comment 12

[NoOp]

#2 WFM - Thanks!

Tested with:
Build identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20091003 Lightning/1.0pre SeaMonkey/2.0pre

I reset all 4 POP3/SMTP accounts to check mail on startup & restarted SM using seamonkey -mail -browser. Only received one Master Password prompt at startup. Also checked: Tools|Password Manger|Manage Stored Passwords|Show Passwords - single prompt/works.

Comment 13

[Robert Kaiser]

Marking blocking+ so we have this on our radar for 2.0

Comment 14

[[e-mail]]

Um... doesn't this force a prompt even when, say, you only have RSS feeds?

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #14)
> Um... doesn't this force a prompt even when, say, you only have RSS feeds?

Yes, if a Master Password is set.

Comment 16

[Robert Kaiser]

From my POV, it's better to ask for a master pwd if one has only feeds than to leave the current situation, so let's try to get this in for RC1 (i.e. ideally within hours from now) and possibly refine it in followups. Would that be possible?

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

Patch hooking into final-ui-startup as suggested by Neil on IRC. Benefits:
- asks for Master Password before opening any window -> works not only for MailNews but also for browser window (e.g. pages with saved logins or client cert requests)
- no need to change mail.biff.on_new_window pref default
- controlled by new pref signon.startup.prompt (can be found in about:config)
- simple logic: if MP is set and pref is true (default), prompt on startup.

Comment 18

[NoOp]

Just tested in Windows:
Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1.4pre) Gecko/20091005 SeaMonkey/2.0pre

Same results as my #12 comments - works for me.

Test scenario was the same with the following exceptions: 1) only used 2 email accounts instead of 4, 2) OS was Windows 2000Pro SP4 in a VirtualBox VM.

Comment 19

[NoOp]

Sorry, should note that my Windows test was per comment #11 - not Jens attachment 404871 [details] [diff] [review].

Comment 20

[[e-mail]]

Comment on attachment 404871 [details] [diff] [review]
patch v2

>+// prompt for Master Password on startup
>+pref("signon.startup.prompt",               true);
Well, I would have possibly put this before the general.startup prefs, or maybe with the browser.formfill prefs (on the shaky ground that formfill both used to be part of wallet).

>+    var prefBranch = Components.classes["@mozilla.org/preferences-service;1"].
>+                     getService(Components.interfaces.nsIPrefBranch);
Nit: .getService to line up with .classes

>+    if (!prefBranch.getBoolPref("signon.startup.prompt"))
(Or just write one big if statement.)

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2a

If nothing else is needed from my side, please feel free to take whatever action necessary to push this once reviews are finished.

Before I forget, this change probably should be announced on the newsgroups since it's a bit surprising if you for example mainly use the browser, have a MP set for saved website logins and then trigger a SM launch through clicking a link in an external application (assuming SM is the system default web browser) that leads to a site that won't trigger the MP prompt itself. I can do that once this is in.

Comment 22

[[e-mail]]

Comment on attachment 404944 [details] [diff] [review]
patch v2a

>+    if (prefBranch.getBoolPref("signon.startup.prompt")) {
Sorry, you misunderstood me. I'll combine these two patches into a single patch that uses the bits you got right.

Comment 23

[[e-mail]]

Attachment: patch v2b [Checkin: Comment 27]

Comment 24

[Frank Wein [:mcsmurf]]

Looks like the checkin for this bug broke this test:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | Exception thrown - [Exception... "'User canceled master password entry, login not added.' when calling method: [nsILoginManagerStorage::addLogin]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///e:/builds/slave/comm-1.9.1-win32-unittest/build/objdir/mozilla/dist/bin/components/nsLoginManager.js :: anonymous :: line 454"  data: no]

Comment 25

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #24)
> Looks like the checkin for this bug broke this test:

Since that's in Toolkit I think the solution for that is to simply set the signon.startup.prompt pref to false for all tests to revert to the previous behavior for them but I'm not a test framework expert and don't know how to achieve that.

Comment 26

[[e-mail]]

Maybe sgautherie can help; KaiRo is probably too busy to ask.

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 404957 [details] [diff] [review]
patch v2b
[Checkin: Comment 27]


http://hg.mozilla.org/comm-central/rev/0e28d1ce1aeb

Comment 28

[NoOp]

WFM:
Build identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20091007 Lightning/1.0pre SeaMonkey/2.0pre

Thanks Jens! Also, I like the fact that the MP prompt is prior to opening any browser and/or email window. 

Note: This provides an path to an added future security feature whereby the MP must be supplied first before opening any client window. The existing solution can of course be bypassed by simply canceling the initial MP prompt, but it would be nice to see this evolve into a locking mechanism where neither browser nor email client could be opened without first supplying the MP.

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)
> Since that's in Toolkit I think the solution for that is to simply set the
> signon.startup.prompt pref to false for all tests to revert to the previous
> behavior for them but I'm not a test framework expert and don't know how to
> achieve that.

That should not be too complicated.
But I failed to understand why your SM checkin disturbs that TK test, by just code reading.
Then, my only guess would be that maybe the TK test should log out when starting?
I'll probably have to run the test and try to figure it out anyway...

Comment 30

[Robert Kaiser]

(In reply to comment #28)
> Note: This provides an path to an added future security feature whereby the MP
> must be supplied first before opening any client window. The existing solution
> can of course be bypassed by simply canceling the initial MP prompt, but it
> would be nice to see this evolve into a locking mechanism where neither browser
> nor email client could be opened without first supplying the MP.

Exactly that's why I dislike that solution and why it can only be temporary - it gives the wrong impression that the profile or the application would be secured by the master password, which is completely untrue and isn't planned for any Mozilla product right now. But if you want to discuss that, let's please take it to the newsgroups and don't make this bug report longer than needed.

Comment 31

[Bill Davidsen]

(In reply to comment #28)
> WFM:
> Build identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre)
> Gecko/20091007 Lightning/1.0pre SeaMonkey/2.0pre
> 
> Thanks Jens! Also, I like the fact that the MP prompt is prior to opening any
> browser and/or email window. 
> 
> Note: This provides an path to an added future security feature whereby the MP
> must be supplied first before opening any client window.

The master password should protect the security device, not the whole application. If you want to propose an application security password (which I personally think is inappropriate) then do so. One of the original problems was that when opening the browser the unrequested mail process asked for many master passwords, asking when not needed makes this worse.

You want to ask ONCE and only WHEN NEEDED. There is no benefit to asking until necessary, and none to asking more than once.

Coding similar things in C/pthreads I would do something like:
if passwd needed
 if MP present
  wait for semiphore (or similar)
  while MP not validated
    ask MP and validate
    exit-loop if CANCEL
  end-loop
 endif
 if MP validated use saved passwd
fi

By use of a semiphore or similar requests are serialized. And I do realize that the logic might not fit the use case, it's for discussion.

I'd also suggest a [NO MP] button in addition to [CANCEL] to allow manual passwd entry in all cases.

Comment 32

[Robert Kaiser]

Isn't this bug fixed now? If so, could we please mark it as that to clear it off the radars?

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #29)
> But I failed to understand why your SM checkin disturbs that TK test, by just
> code reading.
> Then, my only guess would be that maybe the TK test should log out when
> starting?
> I'll probably have to run the test and try to figure it out anyway...

I filed bug 521263 to investigate the test issue.

Comment 34

[Jens Hatlak (:InvisibleSmiley)]

The TB hack has been removed in bug 560746. I quite like the feature, even after the underlying problem has been fixed, so I vote for just changing the default of our pref to false.

Comment 35

[Robert Kaiser]

(In reply to comment #34)
> The TB hack has been removed in bug 560746. I quite like the feature, even
> after the underlying problem has been fixed, so I vote for just changing the
> default of our pref to false.

I'm very much for removing it on our side as well, as it gives the wrong impressions of all of SeaMonkey's data being encoded with this password.
Still, due to us still having to deal with HTTP Auth as well, it might be reasonable to just switch the default for now but have a bug filed for eventual hack removal.

Comment 36

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #35)
> (In reply to comment #34)
> > The TB hack has been removed in bug 560746. I quite like the feature, even
> > after the underlying problem has been fixed, so I vote for just changing the
> > default of our pref to false.
> 
> I'm very much for removing it on our side as well, as it gives the wrong
> impressions of all of SeaMonkey's data being encoded with this password.

Sure, this shouldn't be advertised. I'm OK with removing the pref UI we added, I only suggest to keep the pref and functionality itself. Use case: With this pref I can safely let SM start, e.g. during system boot, and type the MP once I'm at the keyboard. Without the pref/functionality the MP doesn't block the loading of the application so it can easily happen that when I've entered the MP that I get a timeout response from all the mail servers I'm querying at startup.

That said, if the decision is made that the pref and functionality will be removed I'd probably provide an extension to solve the issue for me if I can, or patch SM myself if I can't.

Comment 37

[Robert Kaiser]

Hmm, actually, should we file a followup or multiple followups based on the last comment(s)?