Bug 1268761 - Have Firefox re-register the Sync device when new fields are added. r=markh, a=lizzard
authorEdouard Oger <eoger@fastmail.com>
Thu, 05 May 2016 14:11:00 -0400
changeset 332973 e1a617887fb7a59315cfd2e2870e9d7a6119e10a
parent 332972 ddc847fa5f2c8fb1aa1eb03c0195d47b459a4bb2
child 332974 ad804098d30c0b227c73e344c9addd9e5a7c3e09
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, lizzard
bugs1268761
milestone48.0a2
Bug 1268761 - Have Firefox re-register the Sync device when new fields are added. r=markh, a=lizzard
services/fxaccounts/FxAccounts.jsm
services/fxaccounts/FxAccountsCommon.js
services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
services/fxaccounts/tests/xpcshell/test_storage_manager.js
--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -347,16 +347,19 @@ function FxAccountsInternal() {
 /**
  * The internal API's prototype.
  */
 FxAccountsInternal.prototype = {
   // The timeout (in ms) we use to poll for a verified mail for the first 2 mins.
   VERIFICATION_POLL_TIMEOUT_INITIAL: 15000, // 15 seconds
   // And how often we poll after the first 2 mins.
   VERIFICATION_POLL_TIMEOUT_SUBSEQUENT: 30000, // 30 seconds.
+  // The current version of the device registration, we use this to re-register
+  // devices after we update what we send on device registration.
+  DEVICE_REGISTRATION_VERSION: 1,
 
   _fxAccountsClient: null,
 
   // All significant initialization should be done in this initialize() method,
   // as it's called after this object has been mocked for tests.
   initialize() {
     this.currentTimer = null;
     this.currentAccountState = this.newAccountState();
@@ -594,19 +597,20 @@ FxAccountsInternal.prototype = {
       this._handleTokenError(err)
     ).then(result => currentState.resolve(result));
   },
 
   getDeviceId() {
     return this.currentAccountState.getUserAccountData()
       .then(data => {
         if (data) {
-          if (data.isDeviceStale || !data.deviceId) {
-            // A previous device registration attempt failed or there is no
-            // device id. Either way, we should register the device with FxA
+          if (!data.deviceId || !data.deviceRegistrationVersion ||
+              data.deviceRegistrationVersion < this.DEVICE_REGISTRATION_VERSION) {
+            // There is no device id or the device registration is outdated.
+            // Either way, we should register the device with FxA
             // before returning the id to the caller.
             return this._registerOrUpdateDevice(data);
           }
 
           // Return the device id that we already registered with the server.
           return data.deviceId;
         }
 
@@ -1466,19 +1470,22 @@ FxAccountsInternal.prototype = {
   // Attempt to update the auth server with whatever device details are stored
   // in the account data. Returns a promise that always resolves, never rejects.
   // If the promise resolves to a value, that value is the device id.
   updateDeviceRegistration() {
     return this.getSignedInUser().then(signedInUser => {
       if (signedInUser) {
         return this._registerOrUpdateDevice(signedInUser);
       }
-    }).catch(error => this._logErrorAndSetStaleDeviceFlag(error));
+    }).catch(error => this._logErrorAndResetDeviceRegistrationVersion(error));
   },
 
+  // 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) {
     try {
       // Allow tests to skip device registration because:
       //   1. It makes remote requests to the auth server.
       //   2. _getDeviceName does not work from xpcshell.
       //   3. The B2G tests fail when attempting to import services-sync/util.js.
       if (Services.prefs.getBoolPref("identity.fxaccounts.skipDeviceRegistration")) {
         return Promise.resolve();
@@ -1506,17 +1513,17 @@ FxAccountsInternal.prototype = {
       }
 
       log.debug("registering new device details");
       return this.fxAccountsClient.registerDevice(
         signedInUser.sessionToken, deviceName, this._getDeviceType(), deviceOptions);
     }).then(device =>
       this.currentAccountState.updateUserAccountData({
         deviceId: device.id,
-        isDeviceStale: null
+        deviceRegistrationVersion: this.DEVICE_REGISTRATION_VERSION
       }).then(() => device.id)
     ).catch(error => this._handleDeviceError(error, signedInUser.sessionToken));
   },
 
   _getDeviceName() {
     return Utils.getDeviceName();
   },
 
@@ -1534,71 +1541,71 @@ FxAccountsInternal.prototype = {
         if (error.errno === ERRNO_DEVICE_SESSION_CONFLICT) {
           return this._recoverFromDeviceSessionConflict(error, sessionToken);
         }
       }
 
       // `_handleTokenError` re-throws the error.
       return this._handleTokenError(error);
     }).catch(error =>
-      this._logErrorAndSetStaleDeviceFlag(error)
+      this._logErrorAndResetDeviceRegistrationVersion(error)
     ).catch(() => {});
   },
 
   _recoverFromUnknownDevice() {
     // FxA did not recognise the device id. Handle it by clearing the device
     // id on the account data. At next sync or next sign-in, registration is
     // retried and should succeed.
     log.warn("unknown device id, clearing the local device data");
     return this.currentAccountState.updateUserAccountData({ deviceId: null })
-      .catch(error => this._logErrorAndSetStaleDeviceFlag(error));
+      .catch(error => this._logErrorAndResetDeviceRegistrationVersion(error));
   },
 
   _recoverFromDeviceSessionConflict(error, sessionToken) {
     // FxA has already associated this session with a different device id.
     // Perhaps we were beaten in a race to register. Handle the conflict:
     //   1. Fetch the list of devices for the current user from FxA.
     //   2. Look for ourselves in the list.
-    //   3. If we find a match, set the correct device id and the stale device
-    //      flag on the account data and return the correct device id. At next
+    //   3. If we find a match, set the correct device id and device registration
+    //      version on the account data and return the correct device id. At next
     //      sync or next sign-in, registration is retried and should succeed.
     //   4. If we don't find a match, log the original error.
     log.warn("device session conflict, attempting to ascertain the correct device id");
     return this.fxAccountsClient.getDeviceList(sessionToken)
       .then(devices => {
         const matchingDevices = devices.filter(device => device.isCurrentDevice);
         const length = matchingDevices.length;
         if (length === 1) {
           const deviceId = matchingDevices[0].id;
           return this.currentAccountState.updateUserAccountData({
             deviceId,
-            isDeviceStale: true
+            deviceRegistrationVersion: null
           }).then(() => deviceId);
         }
         if (length > 1) {
           log.error("insane server state, " + length + " devices for this session");
         }
-        return this._logErrorAndSetStaleDeviceFlag(error);
+        return this._logErrorAndResetDeviceRegistrationVersion(error);
       }).catch(secondError => {
         log.error("failed to recover from device-session conflict", secondError);
-        this._logErrorAndSetStaleDeviceFlag(error)
+        this._logErrorAndResetDeviceRegistrationVersion(error)
       });
   },
 
-  _logErrorAndSetStaleDeviceFlag(error) {
+  _logErrorAndResetDeviceRegistrationVersion(error) {
     // Device registration should never cause other operations to fail.
-    // If we've reached this point, just log the error and set the stale
-    // device flag on the account data. At next sync or next sign-in,
+    // If we've reached this point, just log the error and reset the device
+    // registration version on the account data. At next sync or next sign-in,
     // registration will be retried.
     log.error("device registration failed", error);
     return this.currentAccountState.updateUserAccountData({
-      isDeviceStale: true
+      deviceRegistrationVersion: null
     }).catch(secondError => {
       log.error(
-        "failed to set stale device flag, device registration won't be retried",
+        "failed to reset the device registration version, device registration won't be retried",
         secondError);
     }).then(() => {});
   },
 
   _handleTokenError(err) {
     if (!err || err.code != 401 || err.errno != ERRNO_INVALID_AUTH_TOKEN) {
       throw err;
     }
--- a/services/fxaccounts/FxAccountsCommon.js
+++ b/services/fxaccounts/FxAccountsCommon.js
@@ -224,30 +224,30 @@ exports.ERROR_MSG_METHOD_NOT_ALLOWED    
 // JSON file in the profile dir and in the login manager.
 // In order to prevent new fields accidentally ending up in the "wrong" place,
 // all fields stored are listed here.
 
 // The fields we save in the plaintext JSON.
 // See bug 1013064 comments 23-25 for why the sessionToken is "safe"
 exports.FXA_PWDMGR_PLAINTEXT_FIELDS = new Set(
   ["email", "verified", "authAt", "sessionToken", "uid", "oauthTokens", "profile",
-  "deviceId", "isDeviceStale"]);
+  "deviceId", "deviceRegistrationVersion"]);
 
 // Fields we store in secure storage if it exists.
 exports.FXA_PWDMGR_SECURE_FIELDS = new Set(
   ["kA", "kB", "keyFetchToken", "unwrapBKey", "assertion"]);
 
 // Fields we keep in memory and don't persist anywhere.
 exports.FXA_PWDMGR_MEMORY_FIELDS = new Set(
   ["cert", "keyPair"]);
 
 // A whitelist of fields that remain in storage when the user needs to
 // reauthenticate. All other fields will be removed.
 exports.FXA_PWDMGR_REAUTH_WHITELIST = new Set(
-  ["email", "uid", "profile", "deviceId", "isDeviceStale", "verified"]);
+  ["email", "uid", "profile", "deviceId", "deviceRegistrationVersion", "verified"]);
 
 // The pseudo-host we use in the login manager
 exports.FXA_PWDMGR_HOST = "chrome://FirefoxAccounts";
 // The realm we use in the login manager.
 exports.FXA_PWDMGR_REALM = "Firefox Accounts credentials";
 
 // Error matching.
 exports.SERVER_ERRNO_TO_ERROR = {};
--- a/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
+++ b/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
@@ -19,16 +19,18 @@ log.level = Log.Level.Debug;
 Services.prefs.setCharPref("identity.fxaccounts.loglevel", "Trace");
 Log.repository.getLogger("FirefoxAccounts").level = Log.Level.Trace;
 
 Services.prefs.setCharPref("identity.fxaccounts.remote.oauth.uri", "https://example.com/v1");
 Services.prefs.setCharPref("identity.fxaccounts.oauth.client_id", "abc123");
 Services.prefs.setCharPref("identity.fxaccounts.remote.profile.uri", "http://example.com/v1");
 Services.prefs.setCharPref("identity.fxaccounts.settings.uri", "http://accounts.example.com/");
 
+const DEVICE_REGISTRATION_VERSION = 42;
+
 function MockStorageManager() {
 }
 
 MockStorageManager.prototype = {
   initialize(accountData) {
     this.accountData = accountData;
   },
 
@@ -104,16 +106,17 @@ function MockFxAccounts(device = {}) {
       registerPushEndpoint() {
         return new Promise((resolve) => {
           resolve({
             endpoint: "http://mochi.test:8888"
           });
         });
       },
     },
+    DEVICE_REGISTRATION_VERSION
   });
 }
 
 add_task(function* test_updateDeviceRegistration_with_new_device() {
   const deviceName = "foo";
   const deviceType = "bar";
 
   const credentials = getTestUser("baz");
@@ -159,17 +162,17 @@ add_task(function* test_updateDeviceRegi
   do_check_eq(spy.registerDevice.args[0][1], deviceName);
   do_check_eq(spy.registerDevice.args[0][2], "desktop");
   do_check_eq(spy.registerDevice.args[0][3].pushCallback, "http://mochi.test:8888");
 
   const state = fxa.internal.currentAccountState;
   const data = yield state.getUserAccountData();
 
   do_check_eq(data.deviceId, "newly-generated device id");
-  do_check_false(data.isDeviceStale);
+  do_check_eq(data.deviceRegistrationVersion, DEVICE_REGISTRATION_VERSION);
 });
 
 add_task(function* test_updateDeviceRegistration_with_existing_device() {
   const deviceName = "phil's device";
   const deviceType = "desktop";
 
   const credentials = getTestUser("pb");
   const fxa = new MockFxAccounts({ name: deviceName });
@@ -210,17 +213,17 @@ add_task(function* test_updateDeviceRegi
   do_check_eq(spy.updateDevice.args[0][1], credentials.deviceId);
   do_check_eq(spy.updateDevice.args[0][2], deviceName);
   do_check_eq(spy.updateDevice.args[0][3].pushCallback, "http://mochi.test:8888");
 
   const state = fxa.internal.currentAccountState;
   const data = yield state.getUserAccountData();
 
   do_check_eq(data.deviceId, credentials.deviceId);
-  do_check_false(data.isDeviceStale);
+  do_check_eq(data.deviceRegistrationVersion, DEVICE_REGISTRATION_VERSION);
 });
 
 add_task(function* test_updateDeviceRegistration_with_unknown_device_error() {
   const deviceName = "foo";
   const deviceType = "bar";
 
   const credentials = getTestUser("baz");
   const fxa = new MockFxAccounts({ name: deviceName });
@@ -268,17 +271,17 @@ add_task(function* test_updateDeviceRegi
   do_check_eq(spy.updateDevice.args[0][2], deviceName);
   do_check_eq(spy.updateDevice.args[0][3].pushCallback, "http://mochi.test:8888");
 
 
   const state = fxa.internal.currentAccountState;
   const data = yield state.getUserAccountData();
 
   do_check_null(data.deviceId);
-  do_check_false(data.isDeviceStale);
+  do_check_eq(data.deviceRegistrationVersion, DEVICE_REGISTRATION_VERSION);
 });
 
 add_task(function* test_updateDeviceRegistration_with_device_session_conflict_error() {
   const deviceName = "foo";
   const deviceType = "bar";
 
   const credentials = getTestUser("baz");
   const fxa = new MockFxAccounts({ name: deviceName });
@@ -334,17 +337,17 @@ add_task(function* test_updateDeviceRegi
   do_check_eq(spy.getDeviceList.args[0].length, 1);
   do_check_eq(spy.getDeviceList.args[0][0], credentials.sessionToken);
   do_check_true(spy.getDeviceList.time >= spy.updateDevice.time);
 
   const state = fxa.internal.currentAccountState;
   const data = yield state.getUserAccountData();
 
   do_check_eq(data.deviceId, credentials.deviceId);
-  do_check_true(data.isDeviceStale);
+  do_check_eq(data.deviceRegistrationVersion, null);
 });
 
 add_task(function* test_updateDeviceRegistration_with_unrecoverable_error() {
   const deviceName = "foo";
   const deviceType = "bar";
 
   const credentials = getTestUser("baz");
   delete credentials.deviceId;
@@ -393,72 +396,100 @@ add_task(function* test_updateDeviceRegi
 add_task(function* test_getDeviceId_with_no_device_id_invokes_device_registration() {
   const credentials = getTestUser("foo");
   credentials.verified = true;
   delete credentials.deviceId;
   const fxa = new MockFxAccounts();
   yield fxa.internal.setSignedInUser(credentials);
 
   const spy = { count: 0, args: [] };
+  fxa.internal.currentAccountState.getUserAccountData =
+    () => Promise.resolve({ email: credentials.email,
+                            deviceRegistrationVersion: DEVICE_REGISTRATION_VERSION });
   fxa.internal._registerOrUpdateDevice = function () {
     spy.count += 1;
     spy.args.push(arguments);
     return Promise.resolve("bar");
   };
 
   const result = yield fxa.internal.getDeviceId();
 
   do_check_eq(spy.count, 1);
   do_check_eq(spy.args[0].length, 1);
   do_check_eq(spy.args[0][0].email, credentials.email);
   do_check_null(spy.args[0][0].deviceId);
   do_check_eq(result, "bar");
 });
 
-add_task(function* test_getDeviceId_with_device_id_and_stale_flag_invokes_device_registration() {
+add_task(function* test_getDeviceId_with_registration_version_outdated_invokes_device_registration() {
   const credentials = getTestUser("foo");
   credentials.verified = true;
   const fxa = new MockFxAccounts();
   yield fxa.internal.setSignedInUser(credentials);
 
   const spy = { count: 0, args: [] };
   fxa.internal.currentAccountState.getUserAccountData =
-    () => Promise.resolve({ deviceId: credentials.deviceId, isDeviceStale: true });
+    () => Promise.resolve({ deviceId: credentials.deviceId, deviceRegistrationVersion: 0 });
   fxa.internal._registerOrUpdateDevice = function () {
     spy.count += 1;
     spy.args.push(arguments);
     return Promise.resolve("wibble");
   };
 
   const result = yield fxa.internal.getDeviceId();
 
   do_check_eq(spy.count, 1);
   do_check_eq(spy.args[0].length, 1);
   do_check_eq(spy.args[0][0].deviceId, credentials.deviceId);
   do_check_eq(result, "wibble");
 });
 
-add_task(function* test_getDeviceId_with_device_id_and_no_stale_flag_doesnt_invoke_device_registration() {
+add_task(function* test_getDeviceId_with_device_id_and_uptodate_registration_version_doesnt_invoke_device_registration() {
   const credentials = getTestUser("foo");
   credentials.verified = true;
   const fxa = new MockFxAccounts();
   yield fxa.internal.setSignedInUser(credentials);
 
   const spy = { count: 0 };
+  fxa.internal.currentAccountState.getUserAccountData =
+    () => Promise.resolve({ deviceId: credentials.deviceId, deviceRegistrationVersion: DEVICE_REGISTRATION_VERSION });
   fxa.internal._registerOrUpdateDevice = function () {
     spy.count += 1;
     return Promise.resolve("bar");
   };
 
   const result = yield fxa.internal.getDeviceId();
 
   do_check_eq(spy.count, 0);
   do_check_eq(result, "foo's device id");
 });
 
+add_task(function* test_getDeviceId_with_device_id_and_with_no_registration_version_invokes_device_registration() {
+  const credentials = getTestUser("foo");
+  credentials.verified = true;
+  const fxa = new MockFxAccounts();
+  yield fxa.internal.setSignedInUser(credentials);
+
+  const spy = { count: 0, args: [] };
+  fxa.internal.currentAccountState.getUserAccountData =
+    () => Promise.resolve({ deviceId: credentials.deviceId });
+  fxa.internal._registerOrUpdateDevice = function () {
+    spy.count += 1;
+    spy.args.push(arguments);
+    return Promise.resolve("wibble");
+  };
+
+  const result = yield fxa.internal.getDeviceId();
+
+  do_check_eq(spy.count, 1);
+  do_check_eq(spy.args[0].length, 1);
+  do_check_eq(spy.args[0][0].deviceId, credentials.deviceId);
+  do_check_eq(result, "wibble");
+});
+
 function expandHex(two_hex) {
   // Return a 64-character hex string, encoding 32 identical bytes.
   let eight_hex = two_hex + two_hex + two_hex + two_hex;
   let thirtytwo_hex = eight_hex + eight_hex + eight_hex + eight_hex;
   return thirtytwo_hex + thirtytwo_hex;
 };
 
 function expandBytes(two_hex) {
--- a/services/fxaccounts/tests/xpcshell/test_storage_manager.js
+++ b/services/fxaccounts/tests/xpcshell/test_storage_manager.js
@@ -9,16 +9,18 @@ Cu.import("resource://gre/modules/Task.j
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/FxAccountsStorage.jsm");
 Cu.import("resource://gre/modules/FxAccountsCommon.js");
 Cu.import("resource://gre/modules/Log.jsm");
 
 initTestLogging("Trace");
 log.level = Log.Level.Trace;
 
+const DEVICE_REGISTRATION_VERSION = 42;
+
 // A couple of mocks we can use.
 function MockedPlainStorage(accountData) {
   let data = null;
   if (accountData) {
     data = {
       version: DATA_FORMAT_VERSION,
       accountData: accountData,
     }
@@ -123,47 +125,47 @@ add_storage_task(function* checkNewUser(
 });
 
 // Initialized without account data but storage has it available.
 add_storage_task(function* checkEverythingRead(sm) {
   sm.plainStorage = new MockedPlainStorage({
     uid: "uid",
     email: "someone@somewhere.com",
     deviceId: "wibble",
-    isDeviceStale: true
+    deviceRegistrationVersion: null
   });
   if (sm.secureStorage) {
     sm.secureStorage = new MockedSecureStorage(null);
   }
   yield sm.initialize();
   let accountData = yield sm.getAccountData();
   Assert.ok(accountData, "read account data");
   Assert.equal(accountData.uid, "uid");
   Assert.equal(accountData.email, "someone@somewhere.com");
   Assert.equal(accountData.deviceId, "wibble");
-  Assert.equal(accountData.isDeviceStale, true);
+  Assert.equal(accountData.deviceRegistrationVersion, null);
   // Update the data - we should be able to fetch it back and it should appear
   // in our storage.
   yield sm.updateAccountData({
     verified: true,
     kA: "kA",
     kB: "kB",
-    isDeviceStale: false
+    deviceRegistrationVersion: DEVICE_REGISTRATION_VERSION
   });
   accountData = yield sm.getAccountData();
   Assert.equal(accountData.kB, "kB");
   Assert.equal(accountData.kA, "kA");
   Assert.equal(accountData.deviceId, "wibble");
-  Assert.equal(accountData.isDeviceStale, false);
+  Assert.equal(accountData.deviceRegistrationVersion, DEVICE_REGISTRATION_VERSION);
   // Check the new value was written to storage.
   yield sm._promiseStorageComplete; // storage is written in the background.
-  // "verified", "deviceId" and "isDeviceStale" are plain-text fields.
+  // "verified", "deviceId" and "deviceRegistrationVersion" are plain-text fields.
   Assert.equal(sm.plainStorage.data.accountData.verified, true);
   Assert.equal(sm.plainStorage.data.accountData.deviceId, "wibble");
-  Assert.equal(sm.plainStorage.data.accountData.isDeviceStale, false);
+  Assert.equal(sm.plainStorage.data.accountData.deviceRegistrationVersion, DEVICE_REGISTRATION_VERSION);
   // "kA" and "foo" are secure
   if (sm.secureStorage) {
     Assert.equal(sm.secureStorage.data.accountData.kA, "kA");
     Assert.equal(sm.secureStorage.data.accountData.kB, "kB");
   } else {
     Assert.equal(sm.plainStorage.data.accountData.kA, "kA");
     Assert.equal(sm.plainStorage.data.accountData.kB, "kB");
   }