Closed Bug 1575091 Opened 4 months ago Closed 4 months ago

Update _generatedPasswordsByPrincipalOrigin when merging and deleting logins

Categories

(Toolkit :: Password Manager, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

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

Attachments

(1 file)

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.

Depends on: 1560042
Priority: -- → P2
Whiteboard: [passwords:generation] [skyline]

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.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED

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

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

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

Attachment #9087517 - Attachment description: Bug 1575091 (WIP) - Update _generatedPasswordsByPrincipalOrigin for auto-saved, generated-password login changes. r?MattN → Bug 1575091 - Update _generatedPasswordsByPrincipalOrigin for auto-saved, generated-password login changes. r?MattN
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/610592f70d74
Update _generatedPasswordsByPrincipalOrigin for auto-saved, generated-password login changes. r=MattN

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

Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf6ddf6d9a59
Update _generatedPasswordsByPrincipalOrigin for auto-saved, generated-password login changes. r=MattN
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.