Closed
Bug 973166
Opened 11 years ago
Closed 8 years ago
Sync reverts changed passwords if old password entered before sync complete
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: theo148, Assigned: lina)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140130030202
Steps to reproduce:
1. Set up Firefox Sync (easier to reproduce with two installations on a dual boot).
2. Synchronise saved passwords.
3. Change and sync the saved password to an online account.
4. Reboot (or otherwise switch to) another Firefox installation.
5. Attempt to log in to account with changed password before sync occurs (setting a master password makes this easier to notice, not sure if it affects reproducibility).
Actual results:
The login attempt failed with an incorrect password message (in this case, Google informed me that I had changed my password a while ago). After synchronisation was completed, the new password was overwritten with the old password on all devices set up with sync, making it impossible to log in with the saved password.
Expected results:
Ideally, sync should have picked up and used the new password immediately, although of course that's not possible under all circumstances. It probably should have eventually picked up the new password, and definitely should not have reverted the password on all synced devices.
Reporter | ||
Updated•11 years ago
|
Component: Untriaged → Sync
OS: Windows 8.1 → All
Hardware: x86_64 → All
Comment 1•11 years ago
|
||
In step 5, we're perhaps updating the timestamp of the old password - does that maybe make sync prefer it to the new one when a Sync is eventually triggered?
Component: Sync → Firefox Sync: Backend
Product: Firefox → Mozilla Services
Version: Trunk → unspecified
Reporter | ||
Comment 2•11 years ago
|
||
That sounds like it would make sense. Is there any debugging information I could provide to confirm this?
Comment 3•11 years ago
|
||
https://wiki.mozilla.org/User_Services/Sync/Debug_Sync
also, the latest nightly will output to the web console.
Comment 4•8 years ago
|
||
Could you see if this might have been fixed or changed recently?
Flags: needinfo?(eoger)
Assignee | ||
Comment 6•8 years ago
|
||
This is likely because the login manager bumps the "last used" timestamp if the login attempt fails, but we also use that timestamp for reconciliation. We could try adding a "last success" timestamp to the login manager for this.
Status: UNCONFIRMED → NEW
Component: Firefox Sync: Backend → Sync
Ever confirmed: true
Product: Cloud Services → Firefox
Comment 7•8 years ago
|
||
Matt, does the password manager reliably know if a login attempt succeeded?
Flags: needinfo?(MattN+bmo)
Comment 8•8 years ago
|
||
No, that's a hard problem that has never got resourced. I've seen recent Chromium issues about them getting it wrong sometimes.
Flags: needinfo?(MattN+bmo)
Comment 9•8 years ago
|
||
Maybe an addition of a changed time stamp?
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 10•8 years ago
|
||
We listen for the `modifyLogin` notification to track changes. Is that fired when a password is used, too?
Flags: needinfo?(kit)
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #10)
> We listen for the `modifyLogin` notification to track changes. Is that fired
> when a password is used, too?
Yes. :-) Easy fix, so taking this bug.
Assignee: nobody → kit
Status: NEW → ASSIGNED
Flags: needinfo?(kit)
Priority: P2 → P1
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8870210 [details]
Bug 973166 - Only track passwords with changes to synced fields.
https://reviewboard.mozilla.org/r/141670/#review145312
::: services/sync/modules/engines/passwords.js:368
(Diff revision 1)
> +
> case "addLogin":
> case "removeLogin":
> // Skip over Weave password/passphrase changes.
> subject.QueryInterface(Ci.nsILoginMetaInfo).QueryInterface(Ci.nsILoginInfo);
> if (Utils.getSyncCredentialsHosts().has(subject.hostname)) {
I expect we want this check to remain in the above case too?
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8870210 [details]
Bug 973166 - Only track passwords with changes to synced fields.
https://reviewboard.mozilla.org/r/141670/#review145312
> I expect we want this check to remain in the above case too?
We do! Accursed fallthrough. :-P I suspect this also needs a test, then, where we simulate such a change.
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8870210 [details]
Bug 973166 - Only track passwords with changes to synced fields.
https://reviewboard.mozilla.org/r/141670/#review145552
LGTM! It would be nice if you could add a test to exercise these changes :)
Attachment #8870210 -
Flags: review?(eoger) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Edouard Oger [:eoger] from comment #15)
> It would be nice if you could add a test to exercise these changes :)
Added!
Comment 18•8 years ago
|
||
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d243e41d8f9
Only track passwords with changes to synced fields. r=eoger
Comment 19•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in
before you can comment on or make changes to this bug.
Description
•