1560042 - Merge logins if a user adds an existing username to a generated password in the doorhanger

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement, P1)

Description

[Matthew N. [:MattN]]

Scenario:

• User has loginA with "uname"/"pword" saved
• User generates a new password on that site
  ** Login with ""/pword is saved as loginB if no login with username "" was already saved
• Doorhanger appears allowing the user to add a username to the saved password
• User changes the "" to "uname" in the doorhanger
• Primary button remains as update regardless of the username field

Expected result:

• Existing login loginA is updated with the new password.
• New login loginB is deleted.

Comment 1

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

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #0)

• User has loginA with "uname"/"pword" saved
• User generates a new password on that site
  ** Login with ""/pword is saved as loginB if no login with username "" was already saved
• Doorhanger appears allowing the user to add a username to the saved password

Is this the dismissed doorhanger we create when the login with generated password is auto-saved. Or when the form is submitted. Or both?

Comment 2

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

Also, would it be fair to say this behavior change will be applied when using the doorhanger to edit any login with an empty username? Are there downsides to doing that which would mean we need a positive way to identify an auto-saved generated-password login vs. a login saved when no username field was available (or the user cleared the username field in the doorhanger before saving).

Comment 3

[Matthew N. [:MattN]]

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

Is this the dismissed doorhanger we create when the login with generated password is auto-saved. Or when the form is submitted. Or both?

I think it would have to be both.

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

Also, would it be fair to say this behavior change will be applied when using the doorhanger to edit any login with an empty username? Are there downsides to doing that which would mean we need a positive way to identify an auto-saved generated-password login vs. a login saved when no username field was available (or the user cleared the username field in the doorhanger before saving).

I was thinking to reduce scope and only do it for generated passwords which would be using a password change doorhanger whereas I think the more common case when not generating would be a save doorhanger, right? How do we handle that case now when not generating (both for update and save prompts)? Do we give an error?

Comment 4

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

I was thinking to reduce scope and only do it for generated passwords which would be using a password change doorhanger whereas I think the more common case when not generating would be a save doorhanger, right? How do we handle that case now when not generating (both for update and save prompts)? Do we give an error?

It looks like we handle it without error. So, if you started out with empty username, and type a username that matches an existing login, we switch the main action button to "Update" and apply those changes to the matching login.

There is code in there to abort and log an error if there are more than 1 potential logins it looks like you are trying to update, but I've not yet figured out how to get into that state, except via auto-saved generated password logins. So my current approach is to remove the auto-saved login from the matches array, around https://searchfox.org/mozilla-central/rev/96403eac4ff6f96f89a0b120b29bd46540d5705e/toolkit/components/passwordmgr/LoginManagerPrompter.jsm#1122 and then remove the auto-saved login from storage when necessary.

Comment 5

[Matthew N. [:MattN]]

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

I was thinking to reduce scope and only do it for generated passwords which would be using a password change doorhanger whereas I think the more common case when not generating would be a save doorhanger, right? How do we handle that case now when not generating (both for update and save prompts)? Do we give an error?

It looks like we handle it without error. So, if you started out with empty username, and type a username that matches an existing login, we switch the main action button to "Update" and apply those changes to the matching login.

It's not clear to me that you tested both update and save doorhangers cases as I mentioned as I think they are different.

Consider if you have two saved logins: login1(user1/pass1) and login2(""/pass2) and submit a change from pass2 to pass3, if you edit the doorhanger to (user1/pass3) which logins get modified and which exist after?

Comment 6

[Matthew N. [:MattN]]

In bug 1548861 I'm caching the GUID of the login that was auto-saved (when applicable) but after a merge that cache should be updated to include the GUID of the original login that got overwritten. That is necessary so that an edit after an explicit merge in the doorhanger causes storage to get updates to the password field. I guess that should be considered for this bug.

Comment 7

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

Attachment: Bug 1560042 - Part 3: Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN

• General approach implemented in the prompt's persistData method
• b-c test sketched out, not done here
• Plz ignore logging junk for now.

Comment 8

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

I got the b-c tests defined and some initial logic added to persistData (It will all need a clean up before it goes in for review-proper). A few of the tests fail currently, and I'm not sure if the test expectations are wrong, or if the logic is wrong (or both.) There are a couple of cases where if the user changed both the username and password in the prompt, its not clear to me what the outcome should be. As it stands the _filterUpdatableLogins will not match logins in this case, even if the login object has the guid of a previously saved login that we could trivially update.

Comment 9

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

Attachment: Bug 1560042 - Part 1: Share verifyLogins test helper, new cleanupDoorhanger helper. r?MattN

Comment 10

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

Attachment: Bug 1560042 - Part 2: Add test coverage for empty-username/doorhanger case. r?MattN

Depends on D41083

Comment 11

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

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

I'm seeing some failures for browser_promptToChangePassword.js so this is still not done but its getting there.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cdae11ea2306
Part 1: Share verifyLogins test helper, new cleanupDoorhanger helper. r=MattN
https://hg.mozilla.org/integration/autoland/rev/ada7b53d87d6
Part 2: Add test coverage for empty-username/doorhanger case. r=MattN
https://hg.mozilla.org/integration/autoland/rev/0fb712c93395
Part 3: Update matching login and remove empty-username login when persisting changes from update doorhanger. r=MattN

Comment 13

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

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #6)

In bug 1548861 I'm caching the GUID of the login that was auto-saved (when applicable) but after a merge that cache should be updated to include the GUID of the original login that got overwritten. That is necessary so that an edit after an explicit merge in the doorhanger causes storage to get updates to the password field. I guess that should be considered for this bug.

I've filed bug 1575091 for this. We also need to remove cache entries when a login is deleted.

Comment 14

[Arthur Iakab [arthur_iakab]]

Backed out 3 changesets (bug 1560042) for causing browser-chrome failures on browser_temporary_permissions.js CLOSED TREEE

Backout revision https://hg.mozilla.org/integration/autoland/rev/696fbe6740c241264c45eb168c0c1bf2fe5c0f74

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=262383050&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-browser-chrome-e10s-9%2Cm%28bc9%29&revision=0fb712c93395afcad05dd5bd8023b7fa5fd23446

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=262383050&repo=autoland

Sam can you please take a look?

Comment 15

[Cosmin Sabou [:CosminS]]

Please also take a look over these assertion failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-browser-chrome-e10s-8%2Cm%28bc8%29&tochange=696fbe6740c241264c45eb168c0c1bf2fe5c0f74&fromchange=0fb712c93395afcad05dd5bd8023b7fa5fd23446&selectedJob=262395783

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=262383078&repo=autoland&lineNumber=28749

Comment 16

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

I think the osx failures are coming from the use of KEY_End rather than KEY_ArrowRight to get to the end of the input selection.
New try push with this and a couple other fixes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0abf3fac831e8b6d2344b550e672db0c8c2c602b

Comment 17

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

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

I think the osx failures are coming from the use of KEY_End rather than KEY_ArrowRight to get to the end of the input selection.
New try push with this and a couple other fixes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0abf3fac831e8b6d2344b550e672db0c8c2c602b

Whack-a-mole with the setInputValue function. This try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d7900ef0c5842ccd0e5c95df3a83fa23f4ff051 uses input.select(); EventUtils.synthesizeKey("KEY_Backspace") to hopefully clear that field and produce the necessary input events - across all platforms.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4618c959415a
Part 1: Share verifyLogins test helper, new cleanupDoorhanger helper. r=MattN
https://hg.mozilla.org/integration/autoland/rev/009180b641f4
Part 2: Add test coverage for empty-username/doorhanger case. r=MattN
https://hg.mozilla.org/integration/autoland/rev/201884f5d601
Part 3: Update matching login and remove empty-username login when persisting changes from update doorhanger. r=MattN

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4618c959415a
https://hg.mozilla.org/mozilla-central/rev/009180b641f4
https://hg.mozilla.org/mozilla-central/rev/201884f5d601

Comment 20

[Timea Cernea [:tbabos][inactive]]

Hi Sam and Matt,

We tried to verify the fix using different scenarios and got the following results:

The scenario described in Comment 0 is now fixed:
If the existing username from loginA is added in the update doorhanger for the newly generated password(loginB) - loginA password is updated and loginB is deleted

However, there are several scenarios where the merge should be in play:
LoginA= Complete credentials with username and password
LoginB= No username/GeneratedPasw

1. LoginA autocomplete before generated password: FIXED

• Have LoginA saved and auto-filled in the fields
• Delete password and generate a new one via the context menu or select LoginB for the password

2. Auto-filled generated password and existing username addition: BROKEN

• Have loginA saved credentials
• Have loginB no username generated password entry
• Autocomplete password for loginB
• Type in the same username as in loginA in the field and click on Update in the doorhanger -> entries will NOT BE MERGED

Please note that there are many variables that change the behavior of the merge process (same context, order of auto-fill, order of saving credentials) and we still need to go through them and investigate, given that we often get mixed results and start over from the bottom. We would like to get an opinion on the above scenarios, results and get some guidance on how to proceed further.
Attaching bellow a screenshot of the error we get in the browser console whenever the merge is failing.
Thanks!

Comment 21

[Timea Cernea [:tbabos][inactive]]

Attachment: Browser Console error

Comment 22

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

2. Auto-filled generated password and existing username addition: BROKEN

• Have loginA saved credentials
• Have loginB no username generated password entry
• Autocomplete password for loginB
• Type in the same username as in loginA in the field and click on Update in the doorhanger -> entries will NOT BE MERGED

I'm not clear which fields we are talking about in this scenario, but if I understand this correctly, the issue here is that the doorhanger for the auto-saved login is always created with an empty username and the doorhanger username field does not update when opened if the user has since entered a username in the form? Can you confirm the username field in the doorhanger is still empty in this scenario when you click Update?

Comment 23

[Timea Cernea [:tbabos][inactive]]

Attachment: Video of merge issue Broken scenario

No, the username on the update doorhanger will have LoginA but will still not get updated and merged in about:logins. Reproduced the issue in the attached video for a more clear view.

Comment 24

[Timea Cernea [:tbabos][inactive]]

This remains to be investigated further during our Beta70 testing process.

Comment 25

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

I can see what is going on here. When a matching username is added to the form, the onFormSubmit handler finds the existing login matching the username and sees the new password so calls promptToChangePassword. But it compares the guid and password value of the generated pw to the matched login which don't match, so it does not pass autoSavedStorageGUID.

Comment 26

[Adrian Florinescu [:aflorinescu]]

This issue has already been verified indirectly as part of Password Generation feature testing in Fx70.