245813 - master password prompt per saved host before asking for POP password

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

I recently upgraded from 1.3.1 to 1.7 RC2.  
There's a new behavior that is really terrible.
Now, when I try to read my POP email, it prompts me for my master password.  
But I have NOT stored the password for this POP server, so I bonk cancel. 
I have to do that 5 times before it finally stops asking me for my master
password and gets around to asking me for my POP server password.  
Really irritating.  

This bug is similar to, but not a dup of, bug 236510

I'm happy to answer any questions you have about this.

I have 4 POP email accounts and 5 news server accounts configured in the 
profile.  I use the password manager to store the passwords for 2 of the
4 POP mail accounts, but NOT to store the password for this particular
account.  

I think that it should not prompt me for the master password for this 
account at all.  If you think it really must do so, for some reason,
then it should do so at most once.  When I bonk cancel, it should take
the hint.

Comment 1

[Justin Wood (:Callek)]

Can you verify that it exists in 1.7rc3

Other than that, moving "branch" off `1.4` to `other branch` (1.7)

Bumping severity up a notch, and adding regression keyword.  Should this exist
in 1.7rc3 I'll request blocking, if it does not exist, I'll peek for a dupe, and
if I find none, I'll resolved.

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> Can you verify that it exists in 1.7rc3 ?

If it does, please check with a new profile too. Thanks.

Comment 3

[Andrew Schultz]

Nelson: are you seeing this behavior when you explicitly check the account for
new mail or when you open mail (perhaps it's trying to check all your accounts)?

With linux trunk 2004061006, I get a master password dialog even though I didn't
store the password, but I don't think the password manager even knows what is
stored until it has the master password (if you try to manage passwords without
typing the master password, the list is empty).

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Yes, it exists in RC3 too.  In fact, it's worse.  It's 7 prompts in RC3.

In answer to comment 3, here are the exact steps to reproduce.
1. fire up mozilla.
2. Window->Mail and Newsgroups
3. Right click on this email account, and in the pop-up menu, choose 
   "Get Messages for Account".
4. observe and cancel (in sequence) *7* master password prompts.
5. Finally get regular mail server password prompt.

Comment 5

[Justin Wood (:Callek)]

Would it be possible to test this with a fresh profile (where you add these mail
accounts to your new profile) and then set passwords same as you have on your
current profile.

Could be a mal-set pref of some sort causing this, and if its reproduceable the
way I just described, may help the mailnews devs track it down faster.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Here's another idea.  Review the changes between RC2 and RC3 that could
have affected the wallet.  Back 'em out.  The look for other changes in that
area that happened recently,  back 'em out too.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> Here's another idea.

It could be interesting to try with a current Trunk (v1.8a1 or after) build too.
You should be able to install both and share your profile(s).

Comment 8

[Andrew Schultz]

> In answer to comment 3, here are the exact steps to reproduce.

Using these steps, I get only one master password dialog.

(In reply to comment #6)
> Here's another idea.  Review the changes between RC2 and RC3 that could
> have affected the wallet.  Back 'em out.  The look for other changes in that
> area that happened recently,  back 'em out too.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_7_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=05%2F14%2F2004&maxdate=06%2F08%2F2004&cvsroot=%2Fcvsroot

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Perhaps this info is relevant:  I have 7 saved passwords.
2 are passwords used with composer, to upload composed web pages,
2 are for POP email accounts
3 are for logins to web sites

I'd *guess* that I'm getting one prompt for each of those, and 
I'd guess that somewhere between RC2 and RC3, some code as changed 
so that I now get prompted for all types of passwords, whereas 
before, I was only prompted for POP and web site logins.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Two additional discoveries:

1, I added an eighth password, one for another web site.
Sure enough, now when I try to read email on my POP account,
I get prompted *8 times* for my master password.  

2, I also get prompted 8 times now by doing the following:
a. Edit->preferences.
b. Privacy & Security-> passwords
c. click Manage Passwords.
d. when it prompts for the master password, click cancel.
e. repeat step d until it stops asking for the master password.

Clearly, somewhere there is some logic that walks down the content 
of the .s file, and for each password entry, it makes some call that 
asks for the master password if it wasn't recently given.
It should give up and STOP when any of those requests is cancelled.

Comment 11

[Andrew Schultz]

I can reproduce the behavior in comment 10.  It regressed between linux trunk
2004021208 and 2004021308, apparently bug 186237.

but the original bug is more complicated than that.  With 2 passwords and
checking mail on an account without a password stored:
1.4:          no master password prompt
1.5-1.6:      1 master password prompt
1.7rc3-trunk: 2 master password prompts

1 -> 2 prompts regressed between 2004021208 and 2004021308

Comment 12

[Daniel Veditz [:dveditz]]

This looks like it's rev 1.191 in nsMsgIncomingServer.cpp to fix bug 219976
which added a call to nsIPasswordManagerInternal::FindPasswordEntry() checked in
Sept 2003

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/mailnews/base/util&command=DIFF_FRAMESET&file=nsMsgIncomingServer.cpp&rev1=1.190&rev2=1.191&root=/cvsroot

However, when I do a bonsai blame on the file it looks like the call has been
there since at least rev 1.152

Comment 13

[Daniel Veditz [:dveditz]]

Nope, Andrew is right, bug 186237 caused it to keep enumerating after the first
failure, so it enumerates through each saved host (number of unique host
entries, not passwords) asking for the master password to unencrypt each one.

The more sites for which you use password manager the worse it gets (I get 101
prompts), but probably the more likely you are to just enter the password.

Even if FindPasswordEntry were working correctly you'd get one master password
prompt. That's OK in this bug since it is asking for the POP password, but the
comments in the Biff case seem to imply they don't want the prompt, so fixing
this bug only makes bug 236510 bearable but doesn't fix it.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

The .s file has the name of the site/account unencrypted and unencoded.
So, it should be possible to traverse the list, and not even ATTEMPT 
to decrypt or decode anything UNTIL a match is found on the site/account 
name.  Since I have no password stored for this account, I should get
zero prompts to the master password to access this account.

It did work that way in 1.3 (and also 1.4 according to comment 11 above).
So, apparently there have been several regressions.  
This bug should be marked fixed when it works like 1.3/1.4 again.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

the problem is here:
http://lxr.mozilla.org/seamonkey/source/extensions/wallet/src/nsPasswordManager.cpp#207

This code enumerates all entries. The enumerator decrypts everything. Only after
that, it checks if the urls match.

Fixing it would require a sane way to access the singsign database.

(this code is ugly... real ugly)

Comment 16

[Brendan Eich [:brendan]]

Can we use a distinguished XPCOM failure result code to indicate that the user
has canceled the master password dialog, and break out of the (or all such) loops?

/be

Comment 17

[chris hofmann]

or just back up to pre-1.4 behavior with some patch back outs?

Comment 18

[Daniel Veditz [:dveditz]]

Attachment: backs out mysterious extra bit of bug 186237

This reverts a couple lines made during the change to bug 186237. Result is
that if you cancel out of the master password prompt on a mail account with no
password you go directly to the mail password prompt.

For an account that *does* have a stored password you have to cancel three
times before getting the mail password prompt.

This does not appear to regress bug 186237, in fact I'm not sure why these
lines were changed in that bug since nothing in there uses
nsPasswordManager::FindPasswordEntry.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 150853 [details] [diff] [review]
backs out mysterious extra bit of bug 186237

should I get mcalmus to review as well since this affects part of his original
patch? Is he still active?

Comment 20

[Daniel Veditz [:dveditz]]

Attachment: same but remove extraneous NS_SUCCEEDED test

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 150853 [details] [diff] [review]
backs out mysterious extra bit of bug 186237

With the NS_SUCCEEDED(rv) test removed, as discussed over IRC.

/be

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 150856 [details] [diff] [review]
same but remove extraneous NS_SUCCEEDED test

marking implied sr= from comment 21

Comment 23

[Daniel (Leaf) Nunes]

Comment on attachment 150856 [details] [diff] [review]
same but remove extraneous NS_SUCCEEDED test

a=leaf on behalf of drivers

Comment 24

[Daniel Veditz [:dveditz]]

Fix checked into trunk and 1.7 branch (couple hours ago, forgot to update bug).

Comment 25

[Karen Huang]

Used latest Mozilla 1.7 branch 06-17 build:

Bug verification is as following:
So far, after click "Cancel" from the Master Password Dialog.
It displays 3 times for those stored password POP accounts.
And it displays 1 time for those non-store password POP accounts.

For this bug itself, Master password does not prompt per saved host anymore..
The rest issues will be tracked on bug 236510 & bug 247417...
Changing Keywords from "fixed1.7" to "verified1.7"

Will leave bug status "as is" until this bug be verified on trunk.

Comment 26

[Andrew Schultz]

I filed bug 247623 for the fact that mail prompts for the master password at all
when accessing an account without a password stored.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I marked this fixed-aviary1.0 when I ported the patch to the branch, but the
code actually isn't used there -- it's forked, and substantially different.  Did
anyone test for the presence of this bug in Thunderbird?)

Comment 28

[Mike Calmus]

The fix for Bug #186237 had a similar effect on the password manager dialog. I'm
not sure whether it's completely appropriate to put it here, but in any event I
have a patch to fix it for that situation as well.

Comment 29

[Mike Calmus]

Attachment: Patch to fix similar issue with Passowrd Manager dialog

This patch allows the code to propagate more specific failure reasons to the
top layer so the exception can deal with different situations as apporpriate. I
didn't know and couldn't find a way to parse out the exception other than a
String test so if there's a better way, I'd love to use it.

Comment 30

[timeless]

Comment on attachment 151330 [details] [diff] [review]
Patch to fix similar issue with Passowrd Manager dialog

>} catch(e) {
>+      /* The user cancelled the master password dialog */
>+      if (e.indexOf("NS_ERROR_NOT_AVAILABLE") != -1) {

Use:
e.result==Components.results.NS_ERROR_NOT_AVAILABLE

Comment 31

[Mike Calmus]

Attachment: Same patch using correct exception syntax

Changed to reflect Comment #30.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 151353 [details] [diff] [review]
Same patch using correct exception syntax

sr=dveditz

Comment 33

[Mike Calmus]

Comment on attachment 151353 [details] [diff] [review]
Same patch using correct exception syntax

Is someone able to check in the patch now that it has been fully r and sr'd?

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #33)
> (From update of attachment 151353 [details] [diff] [review])
> Is someone able to check in the patch now that it has been fully r and sr'd?

checked in on trunk

Comment 35

[Justin Wood (:Callek)]

Bug 247417 seems to have added more fixes to this