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

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

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

[Justin Dolske [:Dolske]]

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

[Matthew N. [:MattN]]

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.

Comment 4

[Chris Karlof [:ckarlof]]

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.

Comment 5

[Chris Karlof [:ckarlof]]

I misunderstood. This is when we have a username-less password saved already, and then we later get the username during a subsequent login.

Comment 7

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1016051 - Allow username update from capture phase r=MattN

Bug 1016051 - Allow username update from capture phase r=MattN

Comment 8

[Matthew N. [:MattN]]

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.

Comment 9

[Bernardo Rittmeyer [:rittme]]

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

[Matthew N. [: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.

Comment 11

[Erin Lancaster [:elan]]

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

Comment 12

[Matthew N. [:MattN]]

Reminder to myself to address the review comments and land if Bernardo is busy.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Tracking for 42 as it is something we are going to communicate on.

Comment 14

[Bernardo Rittmeyer [:rittme]]

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 15

[Bernardo Rittmeyer [:rittme]]

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 16

[Matthew N. [:MattN]]

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

[Matthew N. [:MattN]]

Comment on attachment 8655580 [details]
MozReview Request: Bug 1016051 - Allow username update from capture phase r=MattN

https://reviewboard.mozilla.org/r/17937/#review17353

Comment 18

[Matthew N. [:MattN]]

Thanks Bernardo.

I'll clarify the commit message, fix the one test message, and push this now.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/105633f695fa

Comment 20

[Bernardo Rittmeyer [:rittme]]

Perfect, thank you Matt!

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/105633f695fa

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Bernardo, could you fill the uplift request to 42? Thanks

Comment 23

[Sylvestre Ledru [:Sylvestre]]

This is too late for 42. Wontfix then.

Comment 24

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on Nightly 2014-05-30 using the STR in comment 1.
Verified fixed on FF 43b1 Win 7.