247417 - Master Password Dialog still displays for those non-store password POP accounts

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Karen Huang]

This bug is logged for tracing the rest issue/improvement for bug 245813:

So far, the fix of bug 245813 is:
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.

Actually, from user point of view, Master Password dialog shouldn't display 
for those non-store password POP accounts...(This is how Netscape 7.1 did)

Steps:
1) Setup 4 POP mail acocunts without "checked for new msgs at startup"
2) Setup 1 POP mail acocunt "check for new msgs every 1 minute"
3) Saved Passwords for 2 POP mail acocunts
4) Checked Preferences|Passwords|Use encryption when storing sensitive data
5) After select "Get Msgs", notice Master Password dialog displays for those 
stored password POP accounts.
6) After select "Cancel"
7) Select "Get Msgs" for those non-store password POP accounts.

Actual Results: Master Password Dialog still displays for those non-store 
password POP accounts

Expected results: Master Password Dialog shouldn't display for those non-store 
password POP accounts.

Comment 1

[Karen Huang]

I used latest Mozilla 1.7 branch build (06-17)...

Comment 2

[Andrew Schultz]

*** Bug 247623 has been marked as a duplicate of this bug. ***

Comment 3

[Andrew Schultz]

This bug regressed between linux trunk builds 2004092205 and 2004092305,
indicating this is a regression from bug 219976.

Comment 4

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

I'd say this is a wallet bug. the xpcom frontend for wallet uses the enumerator
to get all the urls, usernames and passwords. In clear text. So the master
password is needed. Only after that, it goes to search for the right url.
It should look for the right url first, and then decypher the username and password.

Comment 5

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

Attachment: delay decryption

Decyption should happen only if the uri's match.
Some might think of this as a hack. I think of all of wallet as a hack, so one
extra won't hurt.

Comment 6

[Darin Fisher]

Comment on attachment 151220 [details] [diff] [review]
delay decryption

>Index: extensions/wallet/src/nsPasswordManager.cpp

>-  nsCOMPtr<nsISimpleEnumerator> enumerator;
>-  rv = GetEnumerator(getter_AddRefs(enumerator));
>-  if (NS_FAILED(rv)) {
>-    return rv; // could not unlock the database
>+  nsPasswordManagerEnumerator* enumerator = new nsPasswordManagerEnumerator(PR_FALSE);

who frees this enumerator now?	it used to be COMPtr, which
would clean itself up after it goes out of scope.


>   while (hasMoreElements) {
>     rv = enumerator->GetNext(getter_AddRefs(passwordElem));
>     if (NS_FAILED(rv))
>       return rv;

here's an early return that appears to leak |enumerator|.


As for the core functionality change, I think you should poke
dveditz for a review.

Comment 7

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

Attachment: updated

Patch updated to darins comments. It's a comptr again.

Comment 8

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

Comment on attachment 154006 [details] [diff] [review]
updated

carrying timeless' review forward. dveditz can you sr?
darin: there isn't a real functionality changes. the patch only skips
decrypting passwords for entries that are thrown away anyway later, because the
url's don't match. The end result is the same.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 154006 [details] [diff] [review]
updated

sr=dveditz

Comment 10

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

checked in

Comment 11

[David :Bienvenu]

this should be checked into the aviary branch, right? If I don't hear any
dissenting voices, I'll check this into the aviary branch.

Comment 12

[David :Bienvenu]

*** Bug 185786 has been marked as a duplicate of this bug. ***

Comment 13

[David :Bienvenu]

*** Bug 236510 has been marked as a duplicate of this bug. ***

Comment 14

[David :Bienvenu]

Attachment: -up diff for aviary branch

this is what I'll check into the aviary branch.

Comment 15

[David :Bienvenu]

fixed on 1.0 branch

Comment 16

[David :Bienvenu]

Comment on attachment 154708 [details] [diff] [review]
-up diff for aviary branch

getting on radar for 1.7.12

Comment 17

[Frank Wein [:mcsmurf]]

*** Bug 306710 has been marked as a duplicate of this bug. ***

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 154708 [details] [diff] [review]
-up diff for aviary branch

out of scope for a security release