Closed Bug 1296328 Opened 3 years ago Closed 3 years ago

Changing FxA password causes device registration to be lost

Categories

(Firefox :: Firefox Accounts, defect)

Unspecified
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox48 --- wontfix
firefox49 --- verified
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: pb, Assigned: pb, NeedInfo)

References

Details

(Whiteboard: [fxa-waffle])

Attachments

(1 file)

(apologies if this is the wrong component, wasn't sure of the correct place for stuff in services/fxaccounts)

When a user changes their password in FxA, the server destroys the device record. This leaves the user with a signed-in Sync session that doesn't show up in their list of connected devices.

This can easily be patched in the client by calling this._fxAccounts.updateDeviceRegistration() after this._fxAccounts.updateUserAccountData() when we process the fxaccounts:change_password web channel message, here:

https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsWebChannel.jsm#344

Fwiw, the root cause of this is really my somewhat muddled implementation of device registration in both client and server. The feature was specced so that device records would outlive session tokens and that is how our resetAccount stored procedure worked until I changed it, in:

https://github.com/mozilla/fxa-auth-db-mysql/pull/128

If resetAccount instead preserved the device record, this particular problem would not exist. However I think that would only introduce other problems because elsewhere the client behaves as if the device id is not permanent. So either way, I think a client patch is necessary.

There is also a PR on GitHub which discusses some server-side issues related to this problem:

https://github.com/mozilla/fxa-auth-db-mysql/pull/160
Does this also affect device registration for Firefox for Android (Bug 1250782) or iOS (Bug 1250783)?
Component: Firefox Sync: UI → FxAccounts
OS: Unspecified → All
Product: Cloud Services → Core
(I don't see a client-side device manager component… markh, is this the right spot?)
Flags: needinfo?(markh)
> Does this also affect device registration for Firefox for Android (Bug 1250782) or iOS (Bug 1250783)?

I've tested on Android and yes, the problem exists there too.

I don't have an iOS device to test on but my understanding is that device registration is not implemented on iOS yet, so I don't expect the same problem to exist there (yet).
(In reply to Richard Newman [:rnewman] from comment #2)
> (I don't see a client-side device manager component… markh, is this the
> right spot?)

Yep.

(In reply to Phil Booth [:pb] from comment #0)

> This can easily be patched in the client by calling
> this._fxAccounts.updateDeviceRegistration() after
> this._fxAccounts.updateUserAccountData() when we process the
> fxaccounts:change_password web channel message, here:

That sounds like an easy and upliftable fix.
Flags: needinfo?(markh)
Desktop patch. I lost hg access due to inactivity so waiting for that to be restored before I can push to try.
Attachment #8782852 - Flags: review?(markh)
Mercurial access has just been reinstated and I've pushed to try here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88d8cd8f1266

I'm literally just about to head to the airport for a week's PTO, so apologies in advance if anything breaks! I'll fix it when I get back or if someone else wants to do it before then, that's cool too. :)
Comment on attachment 8782852 [details] [diff] [review]
Bug-1296328-Update-FxA-device-registration-on-password-change.patch

Review of attachment 8782852 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks.
Attachment #8782852 - Flags: review?(markh) → review+
It looks like this is in 48, and I predict we will request uplift after landing.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/2f3f7824d792
Update FxA device registration on password change. r=markh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f3f7824d792
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hey :markh, thanks for getting this landed while I was away.

> ...I predict we will request uplift after landing.

Should we do this and, if so, what do I need to do to make it happen?
Flags: needinfo?(markh)
Comment on attachment 8782852 [details] [diff] [review]
Bug-1296328-Update-FxA-device-registration-on-password-change.patch

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Users changing their Sync password may get stale device information in synced-tabs and general sync UI.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: very low risk limited to when Sync users change their passwords.
[String/UUID change made/needed]: None
Flags: needinfo?(markh)
Attachment #8782852 - Flags: approval-mozilla-beta?
Attachment #8782852 - Flags: approval-mozilla-aurora?
Hi Phil, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(pbooth)
Flags: qe-verify+
Comment on attachment 8782852 [details] [diff] [review]
Bug-1296328-Update-FxA-device-registration-on-password-change.patch

Fixes a regression that is triggered when changing sync password, Aurora50+
Attachment #8782852 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8782852 [details] [diff] [review]
Bug-1296328-Update-FxA-device-registration-on-password-change.patch

Let's uplift this for Monday's 49 RC build. 
We want passwords to work, I'd love any sort of testing or verification of this fix and some smoke testing around it.
Attachment #8782852 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1300297
> Hi Phil, could you please verify this issue is fixed as expected on a latest Nightly build?

Alas, it is not. I've opened bug 1300297 to cover the problem in more detail, because it wasn't clear whether I should re-open this issue or not. With the patch here applied, things are a bit better than they would be otherwise. But there is a further patch attached to that bug which properly fixes the issue.

If that means we need to re-open this issue and back-out the patch, that's cool, I just didn't think I was the right person to make that decision. Hence the separate bug.
Depends on: 1300297
See Also: → 1300736
See Also: → 1300740
I've managed to investigate this issue on 49.0 build1 (20160905130425) using 
- Windows 10 x64
- Ubuntu 14.04 x64
- Mac OS X 10.9.5 
The fix looks good, there are no issues related to signed-in Sync sessions after the password changing, the device record is not lost anymore. 
Setting the tracking flag accordingly.
Removing the qe-verify flag, since this fix has already been verified on Fx49. Phil, if you feel there's any value in verifying it on Fx50 as well, please set it back.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Core → Firefox
Target Milestone: mozilla51 → Firefox 51
You need to log in before you can comment on or make changes to this bug.