Closed
Bug 1016051
Opened 11 years ago
Closed 9 years ago
Password manager won't add a username to a password-only signon
Categories
(Toolkit :: Password Manager, defect, P1)
Toolkit
Password Manager
Tracking
()
People
(Reporter: jwatt, Assigned: rittme)
References
(Depends on 1 open bug)
Details
(Whiteboard: [fxprivacy])
Attachments
(1 file)
Somehow I not infrequently seem to get into a bad state where the password manager has remembered a blank username along with the (correct for the page) password. Firefox gets stuck in this state. It will not ask if you want to update the password details when you submit them again, so as far as a user is concerned Firefox has just decided not to remember passwords for the given page.
One way around this would be to have password manager clear out any login details with a blank username whenever it encounters them. I know how to do this manually with the Saved Passwords dialog, but not many users will know to try that even if they know of the dialog's existence.
Reporter | ||
Comment 1•11 years ago
|
||
I've identified one way in which the username can end up being empty. Go to http://www.ebuyer.com/ and create an account. Delete the login from the Saved Passwords dialog. Log out of ebuyer.com then use their password reset form to reset your password. The link that you're emailed takes you to a page where you only need to enter your new password, not your username. Submitting that and saving the new password in the password manager will result in an empty username being saved with the password.
Using these steps results in a slightly different experience to the one that prompted me to file this bug since it will result in the password field being prefilled when I visit ebuyer.com, but still without the username. Still, when I submit the login details I'm not prompted to same the new username to the password manager, so I always have to enter it manually.
Comment 2•11 years ago
|
||
There's code in LoginManagerContent.jsm that attempts to catch this case. It would be interesting to step through and see why it apparently isn't working here.
I'll make a SWAG that it's because the formSubmitURL differs between the two, and so we consider them separate logins. I've seen that kind of problem before, and I never really liked the way we do this (it's a big hammer to guard against a dubious security risk). If this is it, I wonder if weakening the check to ETLD+1 would help...
Comment 3•10 years ago
|
||
I can confirm this doesn't work on a simple test case. No notification is shown and the username isn't added to the record.
Blocks: passwords-2015-Q1
Summary: Make the password manager drop entries where the username is blank → Password manager won't add a username to a password-only signon
Comment 4•10 years ago
|
||
If it's a multistage login, we can address that with bug 433238.
If it's a general lone password field, our new capture dialog UI which allows editing/adding a username should address it.
Blocked by UX design of the new capture dialog.
Updated•10 years ago
|
Whiteboard: [blocked]
Updated•10 years ago
|
Priority: -- → P1
Comment 5•10 years ago
|
||
I misunderstood. This is when we have a username-less password saved already, and then we later get the username during a subsequent login.
Whiteboard: [blocked]
Updated•10 years ago
|
Priority: P1 → --
Updated•10 years ago
|
Flags: qe-verify?
Flags: firefox-backlog+
Updated•10 years ago
|
Rank: 15
Priority: -- → P1
Updated•9 years ago
|
Flags: qe-verify? → qe-verify+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bernardo
Status: NEW → ASSIGNED
Iteration: --- → 43.1 - Aug 24
Points: --- → 2
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Updated•9 years ago
|
Iteration: 43.1 - Aug 24 → 43.2 - Sep 7
Assignee | ||
Comment 7•9 years ago
|
||
Bug 1016051 - Allow username update from capture phase r=MattN
Attachment #8655580 -
Flags: review?(MattN+bmo)
Comment 8•9 years ago
|
||
Comment on attachment 8655580 [details]
MozReview Request: Bug 1016051 - Allow username update from capture phase r=MattN
https://reviewboard.mozilla.org/r/17937/#review16117
This is a partial review.
::: toolkit/components/passwordmgr/LoginManagerParent.jsm:526
(Diff revision 1)
> + } else if (formLogin.username && existingLogin.username != formLogin.username) {
As we discussed, change this condition to make it more clear.
::: toolkit/components/passwordmgr/LoginManagerParent.jsm:527
(Diff revision 1)
> + log("...empty username update, prompting to change.");
> + prompter = getPrompter();
> + prompter.promptToChangePassword(existingLogin, formLogin);
> } else {
> recordLoginUse(existingLogin);
> }
I think maybe we should still recordLoginUse since it is still getting used.
::: toolkit/components/passwordmgr/nsILoginMetaInfo.idl:43
(Diff revision 1)
> - * The time, in Unix Epoch milliseconds, when the login's password was
> - * last modified.
> + * The time, in Unix Epoch milliseconds, when the login was last modified.
> + *
> + * Contrary to what the name may suggest, this attribute takes into account
> + * not only the password, but other login attributes, like the username.
I kind of wonder if we should just define that this is only for changing the username or password fields.
::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:920
(Diff revision 1)
> - let logins = foundLogins.filter(l => l.username == login.username);
> + let logins = foundLogins.filter(l => l.username == login.username ||
> + (l.password == login.password &&
> + !l.username));
The UI button code needs to be updated to say "Update" instead of "Remember". Can you also add a test for this?
::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1197
(Diff revision 1)
> /*
> * promptToChangePassword
> *
Feel free to also add the extra "\*" and remove the two lines below it:
\* promptToChangePassword
\*
While you're touching this nearby code.
Attachment #8655580 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8655580 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8655580 [details]
MozReview Request: Bug 1016051 - Allow username update from capture phase r=MattN
Bug 1016051 - Allow username update from capture phase r=MattN
Comment 10•9 years ago
|
||
Comment on attachment 8655580 [details]
MozReview Request: Bug 1016051 - Allow username update from capture phase r=MattN
https://reviewboard.mozilla.org/r/17937/#review16441
::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1740
(Diff revisions 1 - 2)
> + * or with the same password as aLogin and empty username.
Split this sentence into two and add something like:
…and an empty username so the user can add a username.
Maybe the first sentence should be explaining at a higher level what this is doing. The point of this function is to see if there are existing logins instead of adding a new login and that should be communicated more clearly.
::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1749
(Diff revisions 1 - 2)
> + return aLoginList.filter(l => l.username == aLogin.username ||
> + (l.password == aLogin.password &&
> + !l.username));
Nit: indentation seems off.
::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1363
(Diff revision 2)
> /*
> * _updateLogin
> */
Nit: delete this comment
::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1363
(Diff revision 2)
> /*
> * _updateLogin
> */
Nit: you can nuke this
::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1366
(Diff revision 2)
> - _updateLogin : function (login, newPassword) {
> + _updateLogin(login, aNewLogin) {
define the default value of null for the second arg.
::: toolkit/components/passwordmgr/test/notification_common.js:46
(Diff revision 2)
> is(notification.options.passwordNotificationType, aKind);
> + if (aKind == "password-change") {
> + is(notification.mainAction.label, "Update");
> + } else if (aKind == "password-save") {
> + is(notification.mainAction.label, "Remember");
Please use the third argument to describe what is being checked.
::: toolkit/components/passwordmgr/test/test_notifications.html:249
(Diff revision 2)
> - // Check for no notification popup when existing pw-only login matches form.
> + // Check for update popup when existing pw-only login matches form.
> is(gotUser, "notifyu1", "Checking submitted username");
> is(gotPass, "notifyp1", "Checking submitted password");
> - popup = getPopup(popupNotifications, "password-save");
> + popup = getPopup(popupNotifications, "password-change");
Please make sure we stil have a test that no change doorhanger appears if the saved username is empty and there is no username field present and the password is the same.
Can you also double-check the history of when this test got added originally make sure there isn't a case we're not considering where this would be a regression? If this was for a specific site we can maybe make a recipe for that one instead.
Attachment #8655580 -
Flags: review?(MattN+bmo) → review+
Updated•9 years ago
|
Iteration: 43.2 - Sep 7 → 43.3 - Sep 21
Comment 11•9 years ago
|
||
Merge day and GTB for Beta 1 is Sept 21st for Firefox 42.0. If we want this for 42.0, best to land this prior to Beta 1
Flags: needinfo?(bernardo)
Comment 12•9 years ago
|
||
Reminder to myself to address the review comments and land if Bernardo is busy.
Flags: needinfo?(MattN+bmo)
Comment 13•9 years ago
|
||
Tracking for 42 as it is something we are going to communicate on.
status-firefox42:
--- → affected
status-firefox43:
--- → affected
tracking-firefox42:
--- → +
tracking-firefox43:
--- → +
Assignee | ||
Comment 14•9 years ago
|
||
https://reviewboard.mozilla.org/r/17937/#review16441
> Please make sure we stil have a test that no change doorhanger appears if the saved username is empty and there is no username field present and the password is the same.
>
> Can you also double-check the history of when this test got added originally make sure there isn't a case we're not considering where this would be a regression? If this was for a specific site we can maybe make a recipe for that one instead.
The test case just after this one, case 14, tests exactly that.
As for the original test case for this one, it's from bug 435531 and I didn't really see a specific reason why we would want this behaviour from that bug.
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8655580 [details]
MozReview Request: Bug 1016051 - Allow username update from capture phase r=MattN
Bug 1016051 - Allow username update from capture phase r=MattN
Attachment #8655580 -
Flags: review+ → review?(MattN+bmo)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bernardo)
Flags: needinfo?(MattN+bmo)
Comment 16•9 years ago
|
||
https://reviewboard.mozilla.org/r/17935/#review17351
::: toolkit/components/passwordmgr/test/test_notifications.html:249
(Diff revision 3)
> - // Check for no notification popup when existing pw-only login matches form.
> + // Check for update popup when existing pw-only login matches form.
> is(gotUser, "notifyu1", "Checking submitted username");
> is(gotPass, "notifyp1", "Checking submitted password");
> - popup = getPopup(popupNotifications, "password-save");
> - ok(!popup, "checking for no notification popup");
> + popup = getPopup(popupNotifications, "password-change");
> + ok(popup, "checking for no notification popup");
> + popup.remove();
> pwmgr.removeLogin(login2);
A test that this actually does the right thing when remember is clicked would have been good…
::: toolkit/components/passwordmgr/test/test_notifications.html:253
(Diff revision 3)
> - ok(!popup, "checking for no notification popup");
> + ok(popup, "checking for no notification popup");
This message is now incorrect
Comment 17•9 years ago
|
||
Comment on attachment 8655580 [details]
MozReview Request: Bug 1016051 - Allow username update from capture phase r=MattN
https://reviewboard.mozilla.org/r/17937/#review17353
Attachment #8655580 -
Flags: review?(MattN+bmo) → review+
Comment 18•9 years ago
|
||
Thanks Bernardo.
I'll clarify the commit message, fix the one test message, and push this now.
Assignee | ||
Comment 20•9 years ago
|
||
Perfect, thank you Matt!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•9 years ago
|
QA Contact: kjozwiak
Comment 22•9 years ago
|
||
Bernardo, could you fill the uplift request to 42? Thanks
Flags: needinfo?(bernardo)
Comment 24•9 years ago
|
||
Reproduced the issue on Nightly 2014-05-30 using the STR in comment 1.
Verified fixed on FF 43b1 Win 7.
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•