Bug 1337244 - Delete previous device registration when setting-up a new FxA user. r?markh draft
authorEdouard Oger <eoger@fastmail.com>
Tue, 14 Feb 2017 20:07:36 +0100
changeset 485263 fc911fbd7e4149f8edbfc3a0c73f3c459130f316
parent 485258 2737f66ad6ac74a688bde788b319122f2001b92b
child 545974 9243f9088eb9ddcbeb63ec2bf0804ced35b6baa5
push id45686
push userbmo:eoger@fastmail.com
push dateThu, 16 Feb 2017 13:09:31 +0000
reviewersmarkh
bugs1337244
milestone54.0a1
Bug 1337244 - Delete previous device registration when setting-up a new FxA user. r?markh MozReview-Commit-ID: wp4OJ1nwKH
services/fxaccounts/FxAccounts.jsm
services/fxaccounts/tests/xpcshell/test_accounts.js
services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
services/sync/tps/extensions/tps/resource/auth/fxaccounts.jsm
--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -64,16 +64,17 @@ var publicProperties = [
   "requiresHttps",
   "resendVerificationEmail",
   "resetCredentials",
   "sessionStatus",
   "setProfileCache",
   "setSignedInUser",
   "signOut",
   "updateDeviceRegistration",
+  "deleteDeviceRegistration",
   "updateUserAccountData",
   "whenVerified",
 ];
 
 // An AccountState object holds all state related to one specific account.
 // Only one AccountState is ever "current" in the FxAccountsInternal object -
 // whenever a user logs out or logs in, the current AccountState is discarded,
 // making it impossible for the wrong state or state data to be accidentally
@@ -541,17 +542,23 @@ FxAccountsInternal.prototype = {
    *          verified: true/false
    *        }
    * @return Promise
    *         The promise resolves to null when the data is saved
    *         successfully and is rejected on error.
    */
   setSignedInUser: function setSignedInUser(credentials) {
     log.debug("setSignedInUser - aborting any existing flows");
-    return this.abortExistingFlow().then(() => {
+    return this.getSignedInUser().then(signedInUser => {
+      if (signedInUser) {
+        return this.deleteDeviceRegistration(signedInUser.sessionToken, signedInUser.deviceId);
+      }
+    }).then(() =>
+      this.abortExistingFlow()
+    ).then(() => {
       let currentAccountState = this.currentAccountState = this.newAccountState(
         Cu.cloneInto(credentials, {}) // Pass a clone of the credentials object.
       );
       // This promise waits for storage, but not for verification.
       // We're telling the caller that this is durable now (although is that
       // really something we should commit to? Why not let the write happen in
       // the background? Already does for updateAccountData ;)
       return currentAccountState.promiseInitialized.then(() => {
@@ -562,17 +569,17 @@ FxAccountsInternal.prototype = {
 
         return this.updateDeviceRegistration();
       }).then(() => {
         Services.telemetry.getHistogramById("FXA_CONFIGURED").add(1);
         this.notifyObservers(ONLOGIN_NOTIFICATION);
       }).then(() => {
         return currentAccountState.resolve();
       });
-    })
+    });
   },
 
   /**
    * Update account data for the currently signed in user.
    *
    * @param credentials
    *        The credentials object containing the fields to be updated.
    *        This object must contain |email| and |uid| fields and they must
@@ -818,18 +825,18 @@ FxAccountsInternal.prototype = {
     // For now we assume the service being logged out from is Sync, so
     // we must tell the server to either destroy the device or sign out
     // (if no device exists). We might need to revisit this when this
     // FxA code is used in a context that isn't Sync.
 
     const options = { service: "sync" };
 
     if (deviceId) {
-      log.debug("destroying device and session");
-      return this.fxAccountsClient.signOutAndDestroyDevice(sessionToken, deviceId, options);
+      log.debug("destroying device, session and unsubscribing from FxA push");
+      return this.deleteDeviceRegistration(sessionToken, deviceId);
     }
 
     log.debug("destroying session");
     return this.fxAccountsClient.signOut(sessionToken, options);
   },
 
   /**
    * Check the status of the current session using cached credentials.
@@ -1531,16 +1538,41 @@ FxAccountsInternal.prototype = {
   updateDeviceRegistration() {
     return this.getSignedInUser().then(signedInUser => {
       if (signedInUser) {
         return this._registerOrUpdateDevice(signedInUser);
       }
     }).catch(error => this._logErrorAndResetDeviceRegistrationVersion(error));
   },
 
+  // Delete the Push Subscription and the device registration on the auth server.
+  // Returns a promise that always resolves, never rejects.
+  async deleteDeviceRegistration(sessionToken, deviceId) {
+    try {
+      // Allow tests to skip device registration because it makes remote requests to the auth server.
+      if (Services.prefs.getBoolPref("identity.fxaccounts.skipDeviceRegistration")) {
+        return Promise.resolve();
+      }
+    } catch (ignore) {}
+
+    try {
+      await this.fxaPushService.unsubscribe();
+      if (sessionToken && deviceId) {
+        await this.fxAccountsClient.signOutAndDestroyDevice(sessionToken, deviceId);
+      }
+      this.currentAccountState.updateUserAccountData({
+        deviceId: null,
+        deviceRegistrationVersion: null
+      });
+    } catch (err) {
+      log.error("Could not delete the device registration", err);
+    }
+    return Promise.resolve();
+  },
+
   handleDeviceDisconnection(deviceId) {
     return this.currentAccountState.getUserAccountData()
       .then(data => data ? data.deviceId : null)
       .then(localDeviceId => {
         if (!localDeviceId) {
           // We've already been logged out (and that logout is probably what
           // caused us to get here via push!), so don't make noise here.
           log.info(`Push request to disconnect, but we've already disconnected`);
--- a/services/fxaccounts/tests/xpcshell/test_accounts.js
+++ b/services/fxaccounts/tests/xpcshell/test_accounts.js
@@ -65,16 +65,19 @@ MockStorageManager.prototype = {
     return Promise.resolve();
   },
 
   getAccountData() {
     return Promise.resolve(this.accountData);
   },
 
   updateAccountData(updatedFields) {
+    if (!this.accountData) {
+      return Promise.resolve();
+    }
     for (let [name, value] of Object.entries(updatedFields)) {
       if (value == null) {
         delete this.accountData[name];
       } else {
         this.accountData[name] = value;
       }
     }
     return Promise.resolve();
@@ -251,16 +254,40 @@ add_task(function* test_get_signed_in_us
   let localOnly = true;
   yield account.signOut(localOnly);
 
   // user should be undefined after sign out
   result = yield account.getSignedInUser();
   do_check_eq(result, null);
 });
 
+add_task(function* test_set_signed_in_user_deletes_previous_device() {
+  _("Check setSignedInUser tries to delete a previous registered device");
+  let account = MakeFxAccounts();
+  let deleteDeviceRegistrationCalled = false;
+  let credentials = {
+    email: "foo@example.com",
+    uid: "1234@lcip.org",
+    assertion: "foobar",
+    sessionToken: "dead",
+    kA: "beef",
+    kB: "cafe",
+    verified: true
+  };
+  yield account.setSignedInUser(credentials);
+
+  account.internal.deleteDeviceRegistration = () => {
+    deleteDeviceRegistrationCalled = true;
+    return Promise.resolve(true);
+  }
+
+  yield account.setSignedInUser(credentials);
+  do_check_true(deleteDeviceRegistrationCalled);
+});
+
 add_task(function* test_update_account_data() {
   _("Check updateUserAccountData does the right thing.");
   let account = MakeFxAccounts();
   let credentials = {
     email: "foo@example.com",
     uid: "1234@lcip.org",
     assertion: "foobar",
     sessionToken: "dead",
@@ -956,42 +983,40 @@ add_task(function* test_sign_out_with_de
   yield fxa.internal.setSignedInUser(credentials);
 
   const user = yield fxa.internal.getUserAccountData();
   do_check_true(user);
   Object.keys(credentials).forEach(key => do_check_eq(credentials[key], user[key]));
 
   const spy = {
     signOut: { count: 0 },
-    signOutAndDeviceDestroy: { count: 0, args: [] }
+    deleteDeviceRegistration: { count: 0, args: [] }
   };
   const client = fxa.internal.fxAccountsClient;
   client.signOut = function() {
     spy.signOut.count += 1;
     return Promise.resolve();
   };
-  client.signOutAndDestroyDevice = function() {
-    spy.signOutAndDeviceDestroy.count += 1;
-    spy.signOutAndDeviceDestroy.args.push(arguments);
+  fxa.internal.deleteDeviceRegistration = function() {
+    spy.deleteDeviceRegistration.count += 1;
+    spy.deleteDeviceRegistration.args.push(arguments);
     return Promise.resolve();
   };
 
   const promise = new Promise(resolve => {
     makeObserver(ONLOGOUT_NOTIFICATION, () => {
       log.debug("test_sign_out_with_device observed onlogout");
       // user should be undefined after sign out
       fxa.internal.getUserAccountData().then(user2 => {
         do_check_eq(user2, null);
         do_check_eq(spy.signOut.count, 0);
-        do_check_eq(spy.signOutAndDeviceDestroy.count, 1);
-        do_check_eq(spy.signOutAndDeviceDestroy.args[0].length, 3);
-        do_check_eq(spy.signOutAndDeviceDestroy.args[0][0], credentials.sessionToken);
-        do_check_eq(spy.signOutAndDeviceDestroy.args[0][1], credentials.deviceId);
-        do_check_true(spy.signOutAndDeviceDestroy.args[0][2]);
-        do_check_eq(spy.signOutAndDeviceDestroy.args[0][2].service, "sync");
+        do_check_eq(spy.deleteDeviceRegistration.count, 1);
+        do_check_eq(spy.deleteDeviceRegistration.args[0].length, 2);
+        do_check_eq(spy.deleteDeviceRegistration.args[0][0], credentials.sessionToken);
+        do_check_eq(spy.deleteDeviceRegistration.args[0][1], credentials.deviceId);
         resolve();
       });
     });
   });
 
   yield fxa.signOut();
 
   yield promise;
@@ -1003,57 +1028,56 @@ add_task(function* test_sign_out_without
   const credentials = getTestUser("alice");
   delete credentials.deviceId;
   yield fxa.internal.setSignedInUser(credentials);
 
   yield fxa.internal.getUserAccountData();
 
   const spy = {
     signOut: { count: 0, args: [] },
-    signOutAndDeviceDestroy: { count: 0 }
+    deleteDeviceRegistration: { count: 0 }
   };
   const client = fxa.internal.fxAccountsClient;
   client.signOut = function() {
     spy.signOut.count += 1;
     spy.signOut.args.push(arguments);
     return Promise.resolve();
   };
-  client.signOutAndDestroyDevice = function() {
-    spy.signOutAndDeviceDestroy.count += 1;
+  fxa.internal.deleteDeviceRegistration = function() {
+    spy.deleteDeviceRegistration.count += 1;
     return Promise.resolve();
   };
 
   const promise = new Promise(resolve => {
     makeObserver(ONLOGOUT_NOTIFICATION, () => {
       log.debug("test_sign_out_without_device observed onlogout");
       // user should be undefined after sign out
       fxa.internal.getUserAccountData().then(user2 => {
         do_check_eq(user2, null);
         do_check_eq(spy.signOut.count, 1);
         do_check_eq(spy.signOut.args[0].length, 2);
         do_check_eq(spy.signOut.args[0][0], credentials.sessionToken);
         do_check_true(spy.signOut.args[0][1]);
         do_check_eq(spy.signOut.args[0][1].service, "sync");
-        do_check_eq(spy.signOutAndDeviceDestroy.count, 0);
+        do_check_eq(spy.deleteDeviceRegistration.count, 0);
         resolve();
       });
     });
   });
 
   yield fxa.signOut();
 
   yield promise;
 });
 
 add_task(function* test_sign_out_with_remote_error() {
   let fxa = new MockFxAccounts();
-  let client = fxa.internal.fxAccountsClient;
   let remoteSignOutCalled = false;
   // Force remote sign out to trigger an error
-  client.signOutAndDestroyDevice = function() { remoteSignOutCalled = true; throw "Remote sign out error"; };
+  fxa.internal.deleteDeviceRegistration = function() { remoteSignOutCalled = true; throw "Remote sign out error"; };
   let promiseLogout = new Promise(resolve => {
     makeObserver(ONLOGOUT_NOTIFICATION, function() {
       log.debug("test_sign_out_with_remote_error observed onlogout");
       resolve();
     });
   });
 
   let jane = getTestUser("jane");
--- a/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
+++ b/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
@@ -112,16 +112,19 @@ function MockFxAccounts(device = {}) {
             getKey(type) {
               return ChromeUtils.base64URLDecode(
                 type === "auth" ? BOGUS_AUTHKEY : BOGUS_PUBLICKEY,
                 { padding: "ignore" });
             }
           });
         });
       },
+      unsubscribe() {
+        return Promise.resolve();
+      }
     },
     DEVICE_REGISTRATION_VERSION
   });
 }
 
 add_task(function* test_updateDeviceRegistration_with_new_device() {
   const deviceName = "foo";
   const deviceType = "bar";
@@ -286,16 +289,48 @@ add_task(function* test_updateDeviceRegi
 
   const state = fxa.internal.currentAccountState;
   const data = yield state.getUserAccountData();
 
   do_check_null(data.deviceId);
   do_check_eq(data.deviceRegistrationVersion, DEVICE_REGISTRATION_VERSION);
 });
 
+add_task(function* test_deleteDeviceRegistration() {
+  const credentials = getTestUser("pb");
+  const fxa = new MockFxAccounts({ name: "my device" });
+  yield fxa.internal.setSignedInUser(credentials);
+
+  const state = fxa.internal.currentAccountState;
+  let data = yield state.getUserAccountData();
+  do_check_eq(data.deviceId, credentials.deviceId);
+  do_check_eq(data.deviceRegistrationVersion, DEVICE_REGISTRATION_VERSION);
+
+  const spy = {
+    signOutAndDestroyDevice: { count: 0, args: [] }
+  };
+  const client = fxa.internal.fxAccountsClient;
+  client.signOutAndDestroyDevice = function() {
+    spy.signOutAndDestroyDevice.count += 1;
+    spy.signOutAndDestroyDevice.args.push(arguments);
+    return Promise.resolve({});
+  };
+  yield fxa.deleteDeviceRegistration(credentials.sessionToken, credentials.deviceId);
+
+  do_check_eq(spy.signOutAndDestroyDevice.count, 1);
+  do_check_eq(spy.signOutAndDestroyDevice.args[0].length, 2);
+  do_check_eq(spy.signOutAndDestroyDevice.args[0][0], credentials.sessionToken);
+  do_check_eq(spy.signOutAndDestroyDevice.args[0][1], credentials.deviceId);
+
+  data = yield state.getUserAccountData();
+
+  do_check_false(data.deviceId);
+  do_check_false(data.deviceRegistrationVersion);
+});
+
 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 });
   yield fxa.internal.setSignedInUser(credentials);
 
--- a/services/sync/tps/extensions/tps/resource/auth/fxaccounts.jsm
+++ b/services/sync/tps/extensions/tps/resource/auth/fxaccounts.jsm
@@ -106,16 +106,16 @@ var Authentication = {
       let user = Authentication.getSignedInUser();
       if (!user) {
         throw new Error("Failed to get signed in user!");
       }
       let fxc = new FxAccountsClient();
       let { sessionToken, deviceId } = user;
       if (deviceId) {
         Logger.logInfo("Destroying device " + deviceId);
-        Async.promiseSpinningly(fxc.signOutAndDestroyDevice(sessionToken, deviceId, { service: "sync" }));
+        Async.promiseSpinningly(fxAccounts.deleteDeviceRegistration(sessionToken, deviceId));
       } else {
         Logger.logError("No device found.");
         Async.promiseSpinningly(fxc.signOut(sessionToken, { service: "sync" }));
       }
     }
   }
 };