Closed Bug 1448165 Opened 6 years ago Closed 6 years ago

Move deviceId and deviceRegistrationVersion under a device key in signedInUser.json

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(4 files)

Bug 1448165 p1 - Simplify and factorize FxAccounts signout mechanism.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1448165 p2 - Remove un-used ON_FXA_UPDATE_NOTIFICATION.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1448165 p4 - Remove skipDeviceRegistration pref.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1448165 p3 - Migrate deviceId and deviceRegistrationVersion under device.
59 bytes, text/x-review-board-request
markh
: review+
Details 
This is spun of bug 1447479 where we need to associate a device with message indexes.
Oops made a mistake, I meant bug 1442133!
Comment on attachment 8961561 [details]
Bug 1448165 p4 - Remove skipDeviceRegistration pref.

https://reviewboard.mozilla.org/r/230416/#review235944

Looks good!
Attachment #8961561 - Flags: review?(tchiovoloni) → review+
Priority: -- → P1
Comment on attachment 8961560 [details]
Bug 1448165 p2 - Remove un-used ON_FXA_UPDATE_NOTIFICATION.

https://reviewboard.mozilla.org/r/230414/#review236896
Attachment #8961560 - Flags: review?(markh) → review+
Comment on attachment 8961561 [details]
Bug 1448165 p4 - Remove skipDeviceRegistration pref.

https://reviewboard.mozilla.org/r/230416/#review236900

I don't understand why we need to do this - shouldn't everything end up back in place correctly next login? (and IIUC, these are the only 2 callers of resetConfigURLs, so if we really do want to do this, we should consider removing that too)
Comment on attachment 8961562 [details]
Bug 1448165 p3 - Migrate deviceId and deviceRegistrationVersion under device.

https://reviewboard.mozilla.org/r/230418/#review236902

This looks good in generaly, but I still need to look over how part4 and part1 interact - and I need to run - but I think I didn't see how we "migrate" the existing registration to the new format. Did I just miss it, or does that need to be added?

::: services/fxaccounts/FxAccounts.jsm:1641
(Diff revision 1)
>    // If you change what we send to the FxA servers during device registration,
>    // you'll have to bump the DEVICE_REGISTRATION_VERSION number to force older
>    // devices to re-register when Firefox updates
> -  _registerOrUpdateDevice(signedInUser) {
> +  async _registerOrUpdateDevice(signedInUser) {
> -    try {
> -      // Allow tests to skip device registration because:
> +    // Allow tests to skip device registration because:

nit: Let's tweak this comment - remove (3), which probably means we can make the entire comment a single sentence.
> I don't understand why we need to do this - shouldn't everything end up back in place correctly next login? (and IIUC, these are the only 2 callers of resetConfigURLs, so if we really do want to do this, we should consider removing that too)

I think I wanted to refactor FxAccounts.jsm further to call .signOut() in a few more places.
I still think this is a valuable change though: signing-out shouldn't reset these config URLs in the first place imo (tcsc agrees). It also makes the signOut code a tiny bit less complex.

> Did I just miss it, or does that need to be added?

Check out |FxAccounts.getDeviceId| in p4, this is where the migration happens.
(In reply to Edouard Oger [:eoger] from comment #14)
> > I don't understand why we need to do this - shouldn't everything end up back in place correctly next login? (and IIUC, these are the only 2 callers of resetConfigURLs, so if we really do want to do this, we should consider removing that too)
> 
> I think I wanted to refactor FxAccounts.jsm further to call .signOut() in a
> few more places.
> I still think this is a valuable change though: signing-out shouldn't reset
> these config URLs in the first place imo (tcsc agrees). It also makes the
> signOut code a tiny bit less complex.

See bug 1249520 comment 9 for the reason we drop this config at signout. The tl;dr is that if IIUC, your patch will mean it is effectively impossible for Firefox to pickup a different config from the server if that config changes - signing out and back in again isn't an ideal solution to that, but at least it's *something*.
Comment on attachment 8961559 [details]
Bug 1448165 p1 - Simplify and factorize FxAccounts signout mechanism.

https://reviewboard.mozilla.org/r/230412/#review237382

::: services/fxaccounts/FxAccounts.jsm:770
(Diff revision 2)
> +    if (!tokenInfos) {
> +      return Promise.resolve();
> +    }
>      // let's just destroy them all in parallel...
>      let promises = [];
>      for (let tokenInfo of Object.values(tokenInfos || {})) {

you can probably remove the `|| {}` here?
Attachment #8961559 - Flags: review?(markh) → review+
Comment on attachment 8961559 [details]
Bug 1448165 p1 - Simplify and factorize FxAccounts signout mechanism.

https://reviewboard.mozilla.org/r/230412/#review237390

::: services/fxaccounts/FxAccounts.jsm:820
(Diff revision 2)
> -
> -    const options = { service: "sync" };
> -
> -    if (deviceId) {
> -      log.debug("destroying device, session and unsubscribing from FxA push");
> -      return this.deleteDeviceRegistration(sessionToken, deviceId);
> +      log.error("Could not unsubscribe from push.", err);
> +    }
> +    if (sessionToken) {
> +      log.debug("Destroying session and device.");
> +      try {
> +        await this.fxAccountsClient.signOut(sessionToken, {service: "sync"});

Actually, looking at bug 1227527 comment 15, it's not clear this is deleting the device entirely (ie, it seems like it might just be signing out of the "sync" service) - please make sure you check with someone from FxA about this change (ie, it may be that we want to hit /account/device/destroy rather than /session/destroy to completely kill everything.
Comment on attachment 8961562 [details]
Bug 1448165 p3 - Migrate deviceId and deviceRegistrationVersion under device.

https://reviewboard.mozilla.org/r/230418/#review237388

These are a great improvement, thanks!

::: services/fxaccounts/FxAccounts.jsm:670
(Diff revision 2)
> -      .then(data => {
> -        if (data) {
> -          if (!data.deviceId || !data.deviceRegistrationVersion ||
> -              data.deviceRegistrationVersion < this.DEVICE_REGISTRATION_VERSION) {
> -            // There is no device id or the device registration is outdated.
> +    if (!data) {
> +      // Without a signed-in user, there can be no device id.
> +      return null;
> +    }
> +    // Try migrating first. Remove this in Firefox 65+.
> +    if (data.deviceId) {

oops - sorry I missed this, but I think we should add a test for this.

::: services/fxaccounts/FxAccounts.jsm:1644
(Diff revision 2)
>    // you'll have to bump the DEVICE_REGISTRATION_VERSION number to force older
>    // devices to re-register when Firefox updates
> -  _registerOrUpdateDevice(signedInUser) {
> -    try {
> -      // Allow tests to skip device registration because:
> -      //   1. It makes remote requests to the auth server.
> +  async _registerOrUpdateDevice(signedInUser) {
> +    // Allow tests to skip device registration because it makes remote requests
> +    // to the auth server and _getDeviceName does not work from xpcshell.
> +    if (Services.prefs.getBoolPref("identity.fxaccounts.skipDeviceRegistration", false)) {

I'm not too bothered, but maybe we should rename this pref to something that indicates it is a test-only pref, as IIUC, setting it in Firefox itself will mean we don't signout correctly. OTOH though, it has no default value, so people aren't going to accidentally find it, so it's easy to argue it's not worthwhile. Your call.
Attachment #8961562 - Flags: review?(markh) → review+
Comment on attachment 8961561 [details]
Bug 1448165 p4 - Remove skipDeviceRegistration pref.

https://reviewboard.mozilla.org/r/230416/#review237392

IMO we need to consider how a user will get config updates before landing this part.
Attachment #8961561 - Flags: review-
(In reply to Mark Hammond [:markh] from comment #15)
> See bug 1249520 comment 9 for the reason we drop this config at signout. The
> tl;dr is that if IIUC, your patch will mean it is effectively impossible for
> Firefox to pickup a different config from the server if that config changes
> - signing out and back in again isn't an ideal solution to that, but at
> least it's *something*.

Makes sense, thanks for finding this out! I'll drop the commit.

(In reply to Mark Hammond [:markh] from comment #17)
> Actually, looking at bug 1227527 comment 15, it's not clear this is deleting
> the device entirely (ie, it seems like it might just be signing out of the
> "sync" service) - please make sure you check with someone from FxA about
> this change (ie, it may be that we want to hit /account/device/destroy
> rather than /session/destroy to completely kill everything.

I've confirmed with the FxA team that the behavior of /session/destroy has been changed in the past to remove the device associated with the session token.
Comment on attachment 8961561 [details]
Bug 1448165 p4 - Remove skipDeviceRegistration pref.

https://reviewboard.mozilla.org/r/230416/#review237762

Awesome!
Attachment #8961561 - Flags: review?(markh) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b06f03dfb623
p1 - Simplify and factorize FxAccounts signout mechanism. r=markh
https://hg.mozilla.org/integration/autoland/rev/ae3f2df2b2ee
p2 - Remove un-used ON_FXA_UPDATE_NOTIFICATION. r=markh
https://hg.mozilla.org/integration/autoland/rev/5489b1559fc9
p3 - Migrate deviceId and deviceRegistrationVersion under device. r=markh
https://hg.mozilla.org/integration/autoland/rev/f7e80091988f
p4 - Remove skipDeviceRegistration pref. r=markh,tcsc
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: