Closed Bug 1016051 Opened 6 years ago Closed 4 years ago

Password manager won't add a username to a password-only signon

Categories

(Toolkit :: Password Manager, defect, P1)

defect
Points:
2

Tracking

()

VERIFIED FIXED
mozilla43
Iteration:
43.3 - Sep 21
Tracking Status
firefox42 + wontfix
firefox43 + verified

People

(Reporter: jwatt, Assigned: rittme)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

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.
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.
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...
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.
Summary: Make the password manager drop entries where the username is blank → Password manager won't add a username to a password-only signon
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.
Whiteboard: [blocked]
Priority: -- → P1
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]
Priority: P1 → --
Flags: qe-verify?
Flags: firefox-backlog+
Blocks: 1167657
No longer blocks: 1167657
Blocks: 1167657
Rank: 15
Priority: -- → P1
Flags: qe-verify? → qe-verify+
Blocks: 1175279
Blocks: 1193404
Whiteboard: [fxprivacy]
Assignee: nobody → bernardo
Status: NEW → ASSIGNED
Iteration: --- → 43.1 - Aug 24
Points: --- → 2
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Iteration: 43.1 - Aug 24 → 43.2 - Sep 7
Bug 1016051 - Allow username update from capture phase r=MattN
Attachment #8655580 - Flags: review?(MattN+bmo)
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)
Attachment #8655580 - Flags: review?(MattN+bmo)
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 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+
Iteration: 43.2 - Sep 7 → 43.3 - Sep 21
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)
Reminder to myself to address the review comments and land if Bernardo is busy.
Flags: needinfo?(MattN+bmo)
Tracking for 42 as it is something we are going to communicate on.
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.
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)
Flags: needinfo?(bernardo)
Flags: needinfo?(MattN+bmo)
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 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+
Thanks Bernardo.

I'll clarify the commit message, fix the one test message, and push this now.
Perfect, thank you Matt!
https://hg.mozilla.org/mozilla-central/rev/105633f695fa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
QA Contact: kjozwiak
Bernardo, could you fill the uplift request to 42? Thanks
Flags: needinfo?(bernardo)
This is too late for 42. Wontfix then.
Flags: needinfo?(bernardo)
Reproduced the issue on Nightly 2014-05-30 using the STR in comment 1.
Verified fixed on FF 43b1 Win 7.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.