1575091 - Update _generatedPasswordsByPrincipalOrigin when merging and deleting logins

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

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

LoginManagerParent._generatedPasswordsByPrincipalOrigin is used to keep track of generated passwords by origin. When these logins are saved/removed/merged we should update this cache so that the storageGUID points at the correct login.

Comment 1

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

I have a WIP patch that just removes the _generatedPasswordsByPrincipalOrigin cache entry for removed logins if the guid / storageGUID matches. I think that is the expected behavior? When the auto-saved login is merged with another, the auto-saved login is removed and the cache entry will be removed along with it. The updated login will be available in the autocomplete and context menu if the user wants to fill a password field on the same origin. If they want to create a new password, they'll get a different generated password than the one merged/deleted.

There isn't a practical way to tie this behavior to the result of the promptToChangePassword() call in _onGeneratedPasswordFilledOrEdited, as LoginManagerParent doesnt know what the outcome was - it could be the auto-saved login is untouched, updated or merged & deleted. By observing passwordmgr-storage-changed, we'll catch deletions of the auto-saved login from the autocomplete menu and about:logins for free, which AFAICT is a good thing.

Comment 2

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

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

I have a WIP patch that just removes the _generatedPasswordsByPrincipalOrigin cache entry for removed logins if the guid / storageGUID matches. I think that is the expected behavior? When the auto-saved login is merged with another, the auto-saved login is removed and the cache entry will be removed along with it. The updated login will be available in the autocomplete and context menu if the user wants to fill a password field on the same origin. If they want to create a new password, they'll get a different generated password than the one merged/deleted.

One question is what to do in the case where an auto-saved login is given a username? At what point does it "graduate" from being an auto-saved login to a regular saved login. And when to remove that cache entry allowing a new password to be generated for that origin.

If this login will never get a username, it means we can't auto-save (persist to storage) generated passwords for that origin as long as this login exists (this is already true for a saved login with no username on the same origin as a generated password.) But for the current session, any attempts to fill with a generated password will be re-using the password associated with this previously auto-saved login.

Comment 3

[Matthew N. [:MattN]]

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

One question is what to do in the case where an auto-saved login is given a username? At what point does it "graduate" from being an auto-saved login to a regular saved login. And when to remove that cache entry allowing a new password to be generated for that origin.

My understanding is that it's a limitation of the MVP that we are fine with so we shouldn't do anything for that case now. We have a bug for allowing to request new passwords which will be implemented in the future.

Comment 4

[Matthew N. [:MattN]]

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

I have a WIP patch that just removes the _generatedPasswordsByPrincipalOrigin cache entry for removed logins if the guid / storageGUID matches. I think that is the expected behavior? When the auto-saved login is merged with another, the auto-saved login is removed and the cache entry will be removed along with it. The updated login will be available in the autocomplete and context menu if the user wants to fill a password field on the same origin. If they want to create a new password, they'll get a different generated password than the one merged/deleted.

The updated login won't be anywhere if the user manually deleted the auto-saved login… I'm not sure we should delete the whole cache record in that case… can we null the storageGUID instead?

There isn't a practical way to tie this behavior to the result of the promptToChangePassword() call in _onGeneratedPasswordFilledOrEdited, as LoginManagerParent doesnt know what the outcome was - it could be the auto-saved login is untouched, updated or merged & deleted. By observing passwordmgr-storage-changed, we'll catch deletions of the auto-saved login from the autocomplete menu and about:logins for free, which AFAICT is a good thing.

You could dispatch another observer notification from the doorhanger code when a merge happened.

Comment 5

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

Attachment: Bug 1575091 - Update _generatedPasswordsByPrincipalOrigin for auto-saved, generated-password login changes. r?MattN

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/610592f70d74
Update _generatedPasswordsByPrincipalOrigin for auto-saved, generated-password login changes. r=MattN

Comment 7

[Noemi Erli[:noemi_erli]]

Backed out changeset 610592f70d74 (Bug 1575091) for failing in test_autocomplete_new_password.html

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=610592f70d7423ba8bf402dcd98a028c526975ac&selectedJob=263272273

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=263278012&repo=autoland&lineNumber=32404

Backout: https://hg.mozilla.org/integration/autoland/rev/faf094c9cd08f5ad349b3bb8ee6d9b9c36061f15

Comment 8

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

Thanks for the backout. Turns out, mcac os needs different keys sent for deleting an item from the autocomplete item. New patch fixes this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf6ddf6d9a59
Update _generatedPasswordsByPrincipalOrigin for auto-saved, generated-password login changes. r=MattN

Comment 10

[Andrei Ciure[:aciure]]

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