182490 - New password manager is ignoring valid password entries

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P1)

Description

[Craig]

User-Agent:       Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.2) Gecko/20021126
Build Identifier: Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.2) Gecko/20021126

If you have N different username/password combinations for a site, the new
password manager is effectively discarding N-1 of them!

Reproducible: Always

Steps to Reproduce:
1. Use Mozilla 1.2beta. Username/password stored as www.example.org/p1, etc
2. Install Mozilla 1.2. Go to a site. All possibilities listed.
3. Having logged in once, try to log in again.

Actual Results:  
Mozilla automatically fills in only the previously successful data.

Expected Results:  
Offered all the possible choices as before.

Format has changed from www.example.org/p1, etc, to http://www.example.org/p1.
Logging in to a site once causes the data to be converted FOR THAT ONE DATA PAIR
ALONE, causing all others to be ignored from then on.

There should either be (have been):
a) A program run at install time to change the format of all entries or
b) Mozilla should look up both data formats to get all relevant
username/password pairs or
c) The format conversion that's performed now should be performed on all data
matching that site

Comment 1

[Stephen P. Morse]

This is a side effect of darin's patch in bug 175495.  Reassigning.

Comment 2

[Stephen P. Morse]

Confirming since the symptom sounds like a logical consequence of the
above-mentioned patch, even though I haven't actually tested it.

Comment 3

[Stephen P. Morse]

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

Comment 4

[Darin Fisher]

ouch!  working on a fix.

Comment 5

[Stephen P. Morse]

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

Comment 6

[Darin Fisher]

Attachment: v1 patch

this is a rather ugly patch, but it solves the bug.

i tried my best to only add code for the case in which legacy password entries
exist.	my basic solution is to build a composite si_SignonURLStruct at the top
of si_GetUser and then release this structure before exiting the function. 
this composite URL struct holds all of the candidate users from both the "new"
entries as well as the "legacy" entries.

before this solution, i tried hacking si_GetUser to iterate over two separate
lists, but that got very complicated because of the fact that the selected user
must be moved to the front of the list, and if the selected user is a legacy
user, then it cannot be promoted to the "new" user list until the user has
pressed the login button.  because si_GetUser is called several times (to
access various fields), we have to remember the fact that a user was chosen. 
so, it would have required significant changes to existing code paths to fix
this bug.
for this reason, i opted for a much less intrusive solution.

the function si_GetCompositeURL replaces the si_GetURL call from si_GetUser. 
and before si_GetUser returns it calls si_ReleaseCompositeURL.	if there aren't
any legacy password entries then si_GetCompositeURL becomes equivalent to
si_GetURL and si_ReleaseCompositeURL does nothing.

Comment 7

[Darin Fisher]

oh, and i setup a netscape-internal testcase at:

http://unagi.mcom.com/bugs/bug_182490/test.html

just run an older version of mozilla (like netscape 7.0) to populate your
password manager.  take mozilla 1.2 (or any recent nightly build) and you should
be able to confirm this bug.  with this patch, this testcase works as expected.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 108290 [details] [diff] [review]
v1 patch

+PRIVATE si_SignonCompositeURLStruct * si_composite_url=0;

I guess I don't really understand how this is used, but it's not clear to me
that it's ok to have this global state, what if you're simultanously working
with multiple sites using the password manager? Will this code do the right
thing then, if so, then all is well, if not, this needs to change. Also, I'd
really like to see the name of that global variable reflect the fact that it's
a global variable.

- In si_ReleaseCompositeURL():

...
+    si_composite_url->primaryUrl = NULL;
+    si_composite_url->legacyUrl = NULL;
+    si_composite_url->chosen_user = NULL;
+    si_composite_url->signonUser_list.Clear();
+
+    delete si_composite_url;

At least the clearing of signonUser_list is unnecessary code here, the
destructor that will run when you call delete will do that for you, no? And do
you really need to null out those other members here?

Other than that this looks fine at the code level, but I don't really know the
first thing about this code so I can't really speak for what this change really
does.

sr=jst

Comment 9

[Darin Fisher]

i spoke with jst via IRC.  the global state variable is OK, or at least neither
of us could come up with a way that it would cause trouble.  as for the cleanup
code before calling "delete", i'll clean that up when i checkin.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 108290 [details] [diff] [review]
v1 patch

a=asa for checkin to 1.3a

Comment 11

[Darin Fisher]

fixed-on-trunk

Comment 12

[David :Bienvenu]

this fix seems to have caused bug 184571, which is a bug that breaks password
management in mailnews in strange ways.