Closed Bug 1582780 Opened 3 months ago Closed 3 months ago

Password is automatically updated in storage when password is generated with existent username (without user action)

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- disabled
firefox70 --- verified
firefox71 --- verified

People

(Reporter: adrian_sv, Assigned: sfoster)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [passwords:generation] [skyline])

Attachments

(2 files)

[Environment:]

Windows 10, Ubuntu 16.04

71.0a1 20190919184237
70.0b8 20190919164641

[Steps:]
  1. Create new profile (optional - just use a login form with one entry for example)
  2. Access any login form (for example: https://www.reddit.com/login/) and save a set of credentials.
  3. Refresh the login form.
  4. After the u/p field are autofilled, delete autofilled password, right-click and generate a secure password.
[Actual Result:]
  1. about:login will return username = user / password = 123456
    after step 4. about:login will return username = user / password = sE6rBWw4HW6gQXr
    Door-hanger will show: Do you want to update: user / sE6rBWw4HW6gQXr
[Expected Result:]

Blank username / generated password saved
Old username/password remain unchanged at this point.
On futher user action: door-hanger update action, the username/password and blank/generated_password get merged.

[Note:]

Mozregression will show: Bug 1570485 - Show save doorhanger when we didn't auto-save and form username isn't empty.
However, please note that what I've considered as good builds, were affected by lack of other fixes, so uncertain how reliable the regression range is.

Attached file 70b7 logs.txt

a bit of typo, the log above is from 70.0b8

[Expected Result:]

Blank username / generated password saved
Old username/password remain unchanged at this point.
On futher user action: door-hanger update action, the username/password and blank/generated_password get merged.

We currently handle the case where the preexisting saved login has an empty username by creating a dismissed change-password doorhanger when we fill the password field with a generated password. Nothing is auto-saved at this point. If you open the doorhanger and click "Update" on the doorhanger, that empty-username login will be updated with the new generated password.

I think we should treat the case where the form has a username value that matches an existing saved login in the same way. Filling a generated password should create a dismissed doorhanger. The username field in that doorhanger is pre-filled with the username value from the form - which matches that of the saved login. If you click "update" at this point - or later when the form is submitted and we show the doorhanger - that saved login will be updated with the new password. I'm suggesting we shouldn't auto-save an empty-username login in this scenario. We auto-save the new empty-username login when there is ambiguity about which login is being edited. We don't have that ambiguity in this case.

If that is the case, the change is to add a autoSaveLogin = false; at #864 just like we do above for the formLoginWithoutUsername match.

Do you agree MattN?

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

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

I think we should treat the case where the form has a username value that matches an existing saved login in the same way. Filling a generated password should create a dismissed doorhanger. The username field in that doorhanger is pre-filled with the username value from the form - which matches that of the saved login. If you click "update" at this point - or later when the form is submitted and we show the doorhanger - that saved login will be updated with the new password. I'm suggesting we shouldn't auto-save an empty-username login in this scenario. We auto-save the new empty-username login when there is ambiguity about which login is being edited.

We also auto-save the new empty-username login so that the user doesn't lose access to their new password if the doorhanger doesn't appear upon "submission". I don't think what you're suggesting is consistent with what we have been planning for 70 so I don't agree. We should auto-save with an empty username and then merge if the user uses the doorhanger.

Flags: needinfo?(MattN+bmo)

If we did more of the prep work to improve capture like I wanted to before working on pwgen then I may agree with you but I'm still seeing a fair number of bug reports about no capture doorhanger.

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

We also auto-save the new empty-username login so that the user doesn't lose access to their new password if the doorhanger doesn't appear upon "submission". I don't think what you're suggesting is consistent with what we have been planning for 70 so I don't agree. We should auto-save with an empty username and then merge if the user uses the doorhanger.

That's clear and makes sense, thanks.

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

We already look to see if this is an edit to the auto-saved login, in the patch I'm just keeping track of that and only updating the matching stored login if its the autosaved login we created. Otherwise, we'll auto-save a new login for the generated password, and create a doorhanger to update it. With the merge-logins stuff we already fixed in LoginManagerPrompter that will populate the doorhanger with the username, and merge the new password into the existing login if the user OKs the update from the doorhanger.

Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27fa829b139f
Dont modify a empty-username login onGeneratedPasswordEditedOrFilled unless it is the auto-saved login. r=MattN
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

As I was eager beaver to verify this fix, I've grabbed a taskcluster build (71.0a1 20190924094348) and checked the fix. It looks good, at least on Ubuntu atm, so could we request an uplift to 70b?

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Flags: needinfo?(sfoster)
Flags: needinfo?(MattN+bmo)

Comment on attachment 9094338 [details]
Bug 1582780 - Dont modify a empty-username login onGeneratedPasswordEditedOrFilled unless it is the auto-saved login. r?MattN

Beta/Release Uplift Approval Request

  • User impact if declined: A user's saved password can get overwritten prematurely (without a confirmation in the doorhanger) if they use password generation and the username in the form matches
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Verified and small change with test
  • String changes made/needed: None
Flags: needinfo?(sfoster)
Flags: needinfo?(MattN+bmo)
Attachment #9094338 - Flags: approval-mozilla-beta?

Comment on attachment 9094338 [details]
Bug 1582780 - Dont modify a empty-username login onGeneratedPasswordEditedOrFilled unless it is the auto-saved login. r?MattN

Fix for Skyline feature, verified in nightly build. Let's uplift for beta 10.

Attachment #9094338 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified - fixed on latest Beta 70.0b10 (64-bit) on WIndows 7, Mac OS 10.14 and Ubuntu 18.04.

  • a new entry will no username/generate password will be craeted
  • initial saved credentials remain unchanged
  • updating the password via the doorhanger will merge the entries and update the existing username with the generated password
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Depends on: 1585398
Regressions: 1585398
You need to log in before you can comment on or make changes to this bug.