1585398 - Multiple generated passwords in the same session / principal after password merge

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Adrian Florinescu [:aflorinescu]]

[Environment:]

Windows 10, Ubuntu 16.04
70.0b11 2019-09-30
71.0a1 2019-10-01

[Description:]

For the Fx70 MVP, it shouldn't be possible to generate multiple secure passwords in the same session for the same principal (see bug 1569568 and bug 1551723). The generated secure password should be re-used throughout the session for the same principal.

[Steps:]

1. Open Firefox with a new profile.
2. Access https://bugs.mattn.ca/pwmgr/login_and_change_form.html
3. In the "Login form" provide an username and a password.
4. Press log in and save the credentials.
5. Press back to reload the form.
6. Remove the auto-filled password and right click/Fill Password/Generate password (at this step you can use whichever password field in the form)
7. Click on the blue icon key from the address bar and update the user/pass from step 4 in the door-hanger using user/generated_password.
8. Once the update+merge is done, choose another password field or just delete the password and to use it again and right click/Fill Password/Generate password

[Actual Result:]

The generated password from step 8 is different from the generated password @ step 6
debug log: https://pastebin.com/RXuEjFXZ
Guessing this is the problem: LoginManagerParent: Removing generated-password cache entry for origin: https://bugs.mattn.ca LoginManagerParent.jsm:212:12

[Expected Result:]

The generated password is the same throughout the session.

[Notes:]

1. Refreshing the page after step 6 will result in correct functionality (incorrect)
2. This behavior is active only after a merge.
3. Marking this as a regression, since 70.0b3 20190902120346 did not exhibit this behavior.

Comment 1

[Sam Foster [:sfoster] (he/him)]

This is the expected outcome of bug 1582780 and bug 1560042. We haven't created UI to generate new passwords for the same origin, but I don't think we want to actively prevent the user from saving multiple logins for the same site, and using the generated password feature in that process. If we skip the removal of the generated-password cache entry, the user could - perhaps unknowingly - end up with multiple logins on a single origin which all share the same generated password.

Comment 2

[Adrian Florinescu [:aflorinescu]]

I'd agree with you if bug 1576490 would be in scope for Fx70. As it is not, I'm afraid we are introducing "ifs" where a feature should be predictable: generated passwords are cached = 1 per session/principal unless you update and merge your passwords, which would make perfect sense if we'd have bug 1576490. Add to the above the general usability concerns in bug 1570319.

If you choose to update and merge password in the change password mid-process, then you will actually have only the autocomplete to rely on, which would imply reaching a point in which you use your current stored password to update the password, not to mention you lose the convenient editable field in the case of generated password.

Also, real edgy case, but it might be possible with this bug and bug 1584185 to reach a point mid password change where your current password, new password and re-entry password to be different from the expected values.

Taking into account the fact that the vast majority of users have only 1 login/site, I would expect not having multiple generation per principal not to be such a big deal, while on the other side, we already have a complex enough feature, hard to understand for a first time user.

Comment 3

[Sam Foster [:sfoster] (he/him)]

(In reply to Adrian Florinescu [:adrian_sv] from comment #2)

If you choose to update and merge password in the change password mid-process, then you will actually have only the autocomplete to rely on, which would imply reaching a point in which you use your current stored password to update the password, not to mention you lose the convenient editable field in the case of generated password.

But autocomplete is the primary mechanism for selecting and filling fields with saved logins. After the doorhanger interaction which saved the generated password with the username you entered, that same username will be the first entry in the autocomplete popup. Selecting it will fill the previously generated password into the field. That seems like exactly what we want and would expect, so I don't regard this as a regression. I do agree that there is some ambiguity with how we label the generated password item in the autocomplete - we don't distinguish between generating a new one and filling with an existing one for this origin. That might be a useful bug to have on file so we can ask UX to think about a solution.

Also, real edgy case, but it might be possible with this bug and bug 1584185 to reach a point mid password change where your current password, new password and re-entry password to be different from the expected values.

If you have STR for that, lets open a new bug.

Comment 4

[Adrian Florinescu [:aflorinescu]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

But autocomplete is the primary mechanism for selecting and filling fields with saved logins.

From the technical point makes sense, I am just pointing that it might be weird for users to reach the point where they get to use a current password(merged) to change the current password(populated before the merge), before actually changing the current password(site) :). - take in account that users need to figure the "No username" entry, which then poofs away when merged.

Also, real edgy case, but it might be possible with this bug and bug 1584185 to reach a point mid password change where your current password, new password and re-entry password to be different from the expected values.

If you have STR for that, lets open a new bug.

A new bug would just be a dupe after bug 1584185, since it's the same root cause, with just a slight variation introduced by the fact that you can have a new generated password when updating and this particular variation will be already mostly handled in fx71 by fixing bug 1576490.

Just to make clear my standing point, having multiple generated passwords is something I think we must have 71+, but Fx70 scope doesn't cover bug 1551723 and bug 1569568, which would make this bug a regression in terms of Fx70 and probably part of the feature in Fx71. Functionally, if this bug is fixed for Fx70 (we do not remove generated-password cache entry for origin when merging), it might result into a stale "No username" entry in case the user chooses to generate again in the same session/principal + the risk that the user uses the secure password for multiple logins, which would not be "secure".

I marked this issue as critical from the point of view that this bug might introduce further confusion in Fx70 to an user who already has to cruise through understanding the empty auto-saved password and hitting possible edge cases, in which expected result is not always intuitive and my standing quality point of view is that we need the work flow to be predictable and intuitive in general, but more so in regards to a password manager auto-generation, all in order to gain and retain an user's trust .

In regards to Fx70, time is ticking away, so I think we need a decision here.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Matt is on PTO until next week so we may not hear from him. Lets get some UX input on this.

Comment 6

[rgaddis]

FWIW Adrian, I do agree with you; in lieu of not having bug 1576490, being able to generate multiple passwords (with no obvious heuristics in place to delineate them or compared to other matching saved logins) can incite confusion.

If it's reasonable to lock it down to one generated password per a given domain (per session) if I understand you correctly, then that may ease some concern/confusion ahead of the additional planned improvements that could be made for 71.

The password generation feature is MVP for 70; it had a lot of the scope trimmed in terms of UX heuristic improvements and engineering enhancements, and is somewhat intentionally hard to evoke as it will largely be hidden behind a context menu. Thus, I could also view this as not as critical as stated... but just one humble man's opinion. :)

Given that our deadline for uplift to 70 is tomorrow, this may require your escalation if you feel that a better solution needs to be accounted for regarding this use case.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1585398 - Dont clear the generated password cache when merging the autosaved login. r?jaws

Comment 8

[Sam Foster [:sfoster] (he/him)]

Per comment #6 and team discussion, the patch reverts the behavior where we clear the generated password cache entry when merging the auto-saved login. This means the generated password will always be the same per session on a given origin.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d92166867a3c6bcf187d364b124afb535568a22

Comment 9

[Adrian Florinescu [:aflorinescu]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #8)

Per comment #6 and team discussion, the patch reverts the behavior where we clear the generated password cache entry when merging the auto-saved login. This means the generated password will always be the same per session on a given origin.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d92166867a3c6bcf187d364b124afb535568a22

Thanks Sam. We looked a bit over the try build, we haven't noticed anything amiss with the reset change.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb54503c983f
Dont clear the generated password cache when merging the autosaved login. r=jaws

Comment 11

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9098738 [details]
Bug 1585398 - Dont clear the generated password cache when merging the autosaved login. r?jaws

Beta/Release Uplift Approval Request

• User impact if declined: There is the potential for confusion around how generated passwords work without this patch as we do not provide any other mechanisms for creating multiple generated passwords for a given site (origin). Our team felt internal consistency was important.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: We do have a browser-chrome test for this. But comment #0 bug outlines STR for manual verification
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Changes have a very limited scope, covered by tests that verify the expected outcome.
  This is a minor tweak for how we maintain state in a session for generated passwords.
  We haven't had verification on Nightly, but the taskcluster build with this patch has been tested by QA.
• String changes made/needed: None

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/cb54503c983f

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Adrian, can you verify the fix on Monday?
And julien or pascal - I'd like to get this into Monday's build, can you check on it and approve the uplift if this gets verification? Thanks!

Comment 14

[Adrian Florinescu [:aflorinescu]]

Verified on 71.0a1 2019-10-06 on Windows 10, 7, Ubuntu 16.04, Mac Osx 10.14.6.

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9098738 [details]
Bug 1585398 - Dont clear the generated password cache when merging the autosaved login. r?jaws

password generation fix, verified in nightly, approved for 71.0b13

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b2a2101f9386

Comment 17

[Adrian Florinescu [:aflorinescu]]

Verified on Ubuntu 16.04, Windows 10, Mac 10.14.6 using 70.0b13 2019-10-07 .