1296328 - Changing FxA password causes device registration to be lost

Status: VERIFIED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Phil Booth [:pb]]

(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

Comment 1

[Richard Newman [:rnewman]]

Does this also affect device registration for Firefox for Android (Bug 1250782) or iOS (Bug 1250783)?

Comment 2

[Richard Newman [:rnewman]]

(I don't see a client-side device manager component… markh, is this the right spot?)

Comment 3

[Phil Booth [:pb]]

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

Comment 4

[Mark Hammond [:markh] [:mhammond]]

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

Comment 5

[Phil Booth [:pb]]

Attachment: Bug-1296328-Update-FxA-device-registration-on-password-change.patch

Desktop patch. I lost hg access due to inactivity so waiting for that to be restored before I can push to try.

Comment 6

[Phil Booth [:pb]]

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 7

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

It looks like this is in 48, and I predict we will request uplift after landing.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/2f3f7824d792
Update FxA device registration on password change. r=markh

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f3f7824d792

Comment 11

[Phil Booth [:pb]]

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?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

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

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Phil, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d1a9befbdc3

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 17

[Phil Booth [:pb]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/642545b6e7b9

Comment 19

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.

Comment 20

[Andrei Vaida [:avaida]]

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.