Closed Bug 1560042 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Password Manager, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox70 --- verified

People

(Reporter: MattN, Assigned: sfoster)

References

Details

(Whiteboard: [passwords:generation] [skyline])

Attachments

(5 files)

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.
Whiteboard: [passwords:generation] [skyline]
Flags: qe-verify+
Priority: P2 → P1

(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?

Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)

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).

(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?

Flags: needinfo?(MattN+bmo)

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.

(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?

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.

  • General approach implemented in the prompt's persistData method
  • b-c test sketched out, not done here
  • Plz ignore logging junk for now.
Attachment #9082099 - Attachment description: Bug 1560042 - WIP Update matching login and remove empty-username login when persisting changes from update doorhanger → Bug 1560042 - WIP. Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN

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.

Attachment #9082099 - Attachment description: Bug 1560042 - WIP. Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN → Bug 1560042 - Part2: Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN
Attachment #9082099 - Attachment description: Bug 1560042 - Part2: Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN → Bug 1560042 - Part 3: Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN
Attachment #9082099 - Attachment description: Bug 1560042 - Part 3: Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN → Bug 1560042 - (WIP) Part 3: Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN
Attachment #9083807 - Attachment description: Bug 1560042 - Part 1: Share verifyLogins test helper. r?MattN → Bug 1560042 - Part 1: Share verifyLogins test helper, new cleanupDoorhanger helper. r?MattN
Attachment #9082099 - Attachment description: Bug 1560042 - (WIP) Part 3: Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN → Bug 1560042 - Part 3: Update matching login and remove empty-username login when persisting changes from update doorhanger. r?MattN

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.

Attachment #9084175 - Attachment description: Bug 1560042 - Part 2: Add test coverage for empty-username/doorhanger case → Bug 1560042 - Part 2: Add test coverage for empty-username/doorhanger case. r?MattN
Pushed by sfoster@mozilla.com:
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

(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.

Blocks: 1575091

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

Flags: needinfo?(sfoster)

(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.

Pushed by sfoster@mozilla.com:
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

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
  1. 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!

Flags: needinfo?(sfoster)
Flags: needinfo?(MattN+bmo)
  1. 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?

Flags: needinfo?(sfoster) → needinfo?(tbabos)

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.

Flags: needinfo?(tbabos)

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

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.

Depends on: 1579539

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(MattN+bmo)
See Also: → 1605322
You need to log in before you can comment on or make changes to this bug.