Closed Bug 1605322 Opened 1 year ago Closed 1 year ago

Cannot update a saved login where storage has both: ""/submitted-password and submitted-username/other-password

Categories

(Toolkit :: Password Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla73
Tracking Status
firefox-esr68 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- verified

People

(Reporter: tbabos, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Keywords: regressionwindow-wanted, Whiteboard: [passwords:generation])

Attachments

(1 file)

Affected versions
Nightly 73 - Build ID (20191220041517) - contains fix for Bug 1604472

Affected platforms
Windows 10 x64

Steps to reproduce

  1. Launch Firefox
  2. Go to reddit.com
  3. Save a set of credentials
  4. Re-load login form
  5. Generate a secure password
  6. Open a Private Window and go to reddit login form
  7. Fill in the username with the saved credential
  8. Select the saved "No username" entry for the password
  9. Click on Log in
  10. Choose to "Update" the entry in the displayed doorhanger

Expected result
The entries should be merged in about:logins.
The initial username should have the generated password
The "No username" entry should be deleted given it was merged

Actual Result
Both entries are still displayed in about:logins.
The username does not have the new(generated) password updated
The "No username" entry is still displayed in about:logins because it was not merged

Note
Happen the other way around for credentials saved in Private Browsing and merged in normal.

Log
LoginManagerPrompter: persistData: Matched 2 logins LoginManagerPrompter.jsm:310:11
LoginManagerPrompter: multiple logins, loginToRemove: undefined LoginManagerPrompter.jsm:319:13
Unexpected match of multiple logins. LoginManagerPrompter.jsm:323
persistData resource://gre/modules/LoginManagerPrompter.jsm:323
callback resource://gre/modules/LoginManagerPrompter.jsm:377
_onButtonEvent resource://gre/modules/PopupNotifications.jsm:1872
oncommand chrome://browser/content/browser.xhtml:1

This seems like follow-up work to be done, doesn't seem to be a regression given that due to Bug 1604472, it never really worked.

See Also: → 1604472

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

I'm looking into this now.

Priority: -- → P2

So far it's looking this is a regression from bug 1560042. That bug moved the return when there are multiple logins higher: https://searchfox.org/mozilla-central/rev/94a19efaaf1eefa58e36047f73b49ed0c5162163/toolkit/components/passwordmgr/LoginManagerPrompter.jsm#313,318-325

Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
See Also: → 1560042

This problem is not specific to private browsing or password generation, it's only more common with password generation because of the auto-saved empty username.

Summary: Credentials saved in private browsing can't be merged in normal browsing and the other way around → Cannot update a saved login where storage has both: ""/submitted-password and submitted-username/other-password

This means that we will no longer have other-password after the user clicks Update in the doorhanger but the doorhanger made it clear this was an update and showed submitted-username/submitted-password as the desired result.

We can't change the other login to submitted-username/submitted-password as then we would have duplicate usernames for the same site.

For now I implemented a fix to properly update the login with a username but not to delete the empty-username login as that would require properly merging all the metadata (which we don't have logic to do yet AFAIK) and because we can't be sure the user wants it deleted. From the perspective of password generation, sites loaded in PB and not-in-PB are separate (e.g. they don't share the cached generated password) so the logic for deleting a login upon merge can't easily run in this case.

The problem happens without pwgen also:

STR 2 without generation (the comment 0 STR are still useful for verification of a slightly different scenario):

  1. Save login ""/password1 for https://www.reddit.com
  2. Save login user2/password2 for https://www.reddit.com
  3. Load the Reddit login form
  • user2/password2 should get autofilled
  1. Replace "password2" with "password1" in the password field
  2. Submit the form
  • It's expected to get an error from Reddit if you didn't substitute the user# or password# above
  1. Prompt appears to update user2 with password1
  2. Click Update in the doorhanger

Expected result:

  • about:logins has user2/password1 and ""/password1
    ** Perhaps in the future we could merge them into one but I'm not sure that is always wanted since empty-username logins are treated special for autofill sometimes.
  • I'm thinking this is better than not saving the change from the doorhanger at all but let me know if you disagree… it's possible there is a case where the user wouldn't expect this result based on the doorhanger as it kinda relies on a user knowing we don't want to save two passwords for the same username with the same origin/formActionOrigin/httpRealm.
  • I'm thinking this is better than not saving the change from the doorhanger at all but let me know if you disagree… it's possible there is a case where the user wouldn't expect this result based on the doorhanger as it kinda relies on a user knowing we don't want to save two passwords for the same username with the same origin/formActionOrigin/httpRealm.

The user may not know they have another saved login with no username which had the other password already. So they are probably already confused as they thought they already saved this new password. In that scenario this patch helps, as it allows them to update the password on the 2nd login rather than just bailing out.

But as we aren't able to distinguish between a login intentionally saved with no username, and a login saved with no username as we weren't able to find one at submit time, we don't really know for sure... I'm inclined to agree that one balance this improves the situation, giving a better result more often, but fully expect a regression to be filed against this bug before too long as it trades off one outcome for another.

Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/b29e131d10dc
Replace other-password with submitted when login storage has ""/submitted-password and submitted-username/other-password. r=sfoster
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Let's give this bake time in 73. Wontfix for 72.

Flags: qe-verify+

Verified - Fixed on latest Nighty on Windows 10, with these changes we have the following behavior (noting it down for future reference):

Normal Browsing Mode:

  • "No username"/"generated password" should be merged as it has until now with any saved credentials. (Only one entry remains with "username1"/"generated password")
  • "No username"/"password1" will be merged with existing credentials resulting in the following: "username2/password1" and keep "No username"/"password1" in about:logins ( This is scenario described by Matt in Comment 6 )

Private Browsing Mode:

  • same as Normal Browsing

Credentials saved in Normal and merged in Private (and the other way around):

  • "No username"/"password1" will be merged with existing credentials resulting in the following: "username2/password1" and keep "No username"/"password1" in about:logins ( This is scenario described by Matt in Comment 6 )
  • "No username"/"generated password" will be merged with existing credentials resulting in the following: "username2/generated password" and keep "No username"/"generated password" in about:logins (This is scenario described by Matt in Comment 6 and applies to password generations as well)

Marking this as verified on 73.0a1 (2020-01-03) on Windows 10 x64. Will update the testcases to reflect the changes.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.