Bug 1317216 - consistently use the Sync client GUID as the basis for the device ID in the sync ping. r?tcsc draft
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 10 Nov 2016 19:12:55 +1100
changeset 444050 ffe1ce492af7c4cd3bfbd7690df56487ef072d0a
parent 443977 b982373cb0e953976fd45f342910d1d1ea123fbb
child 538223 602702ff68575fc2e08ae2fdf7b86125a38113a6
push id37182
push userbmo:markh@mozilla.com
push dateFri, 25 Nov 2016 22:12:04 +0000
reviewerstcsc
bugs1317216
milestone53.0a1
Bug 1317216 - consistently use the Sync client GUID as the basis for the device ID in the sync ping. r?tcsc MozReview-Commit-ID: BDTqnM7de6n
services/sync/modules/browserid_identity.js
services/sync/modules/telemetry.js
services/sync/tests/unit/test_syncscheduler.js
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -116,16 +116,25 @@ this.BrowserIDManager.prototype = {
 
   hashedUID() {
     if (!this._hashedUID) {
       throw new Error("hashedUID: Don't seem to have previously seen a token");
     }
     return this._hashedUID;
   },
 
+  // Return a hashed version of a deviceID, suitable for telemetry.
+  hashedDeviceID(deviceID) {
+    let uid = this.hashedUID();
+    // Combine the raw device id with the metrics uid to create a stable
+    // unique identifier that can't be mapped back to the user's FxA
+    // identity without knowing the metrics HMAC key.
+    return Utils.sha256(deviceID + uid);
+  },
+
   deviceID() {
     return this._signedInUser && this._signedInUser.deviceId;
   },
 
   initialize: function() {
     for (let topic of OBSERVER_TOPICS) {
       Services.obs.addObserver(this, topic, false);
     }
--- a/services/sync/modules/telemetry.js
+++ b/services/sync/modules/telemetry.js
@@ -263,39 +263,36 @@ class TelemetryRecord {
     }
     if (error) {
       this.failureReason = transformError(error);
     }
 
     // We don't bother including the "devices" field if we can't come up with a
     // UID or device ID for *this* device -- If that's the case, any data we'd
     // put there would be likely to be full of garbage anyway.
+    // Note that we currently use the "sync device GUID" rather than the "FxA
+    // device ID" as the latter isn't stable enough for our purposes - see bug
+    // 1316535.
     let includeDeviceInfo = false;
     try {
       this.uid = Weave.Service.identity.hashedUID();
-      let deviceID = Weave.Service.identity.deviceID();
-      if (deviceID) {
-        // Combine the raw device id with the metrics uid to create a stable
-        // unique identifier that can't be mapped back to the user's FxA
-        // identity without knowing the metrics HMAC key.
-        this.deviceID = Utils.sha256(deviceID + this.uid);
-        includeDeviceInfo = true;
-      }
+      this.deviceID = Weave.Service.identity.hashedDeviceID(Weave.Service.clientsEngine.localID);
+      includeDeviceInfo = true;
     } catch (e) {
       this.uid = "0".repeat(32);
       this.deviceID = undefined;
     }
 
     if (includeDeviceInfo) {
       let remoteDevices = Weave.Service.clientsEngine.remoteClients;
       this.devices = remoteDevices.map(device => {
         return {
           os: device.os,
           version: device.version,
-          id: Utils.sha256(device.id + this.uid)
+          id: Weave.Service.identity.hashedDeviceID(device.id),
         };
       });
     }
 
     // Check for engine statuses. -- We do this now, and not in engine.finished
     // to make sure any statuses that get set "late" are recorded
     for (let engine of this.engines) {
       let status = Status.engines[engine.name];
--- a/services/sync/tests/unit/test_syncscheduler.js
+++ b/services/sync/tests/unit/test_syncscheduler.js
@@ -168,17 +168,19 @@ add_test(function test_prefAttributes() 
 add_identity_test(this, async function test_updateClientMode() {
   _("Test updateClientMode adjusts scheduling attributes based on # of clients appropriately");
   do_check_eq(scheduler.syncThreshold, SINGLE_USER_THRESHOLD);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
   do_check_false(scheduler.numClients > 1);
   do_check_false(scheduler.idle);
 
   // Trigger a change in interval & threshold by adding a client.
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  clientsEngine._store.create(
+    { id: "foo", cleartext: { os: "mobile", version: "0.01", type: "desktop" } }
+  );
   scheduler.updateClientMode();
 
   do_check_eq(scheduler.syncThreshold, MULTI_DEVICE_THRESHOLD);
   do_check_eq(scheduler.syncInterval, scheduler.activeInterval);
   do_check_true(scheduler.numClients > 1);
   do_check_false(scheduler.idle);
 
   // Resets the number of clients to 0.
@@ -433,28 +435,33 @@ add_identity_test(this, async function t
   await setUp(server);
 
   // Confirm defaults.
   do_check_eq(scheduler.syncThreshold, SINGLE_USER_THRESHOLD);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
   do_check_false(scheduler.idle);
 
   // Trigger a change in interval & threshold by adding a client.
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  clientsEngine._store.create(
+    { id: "foo", cleartext: { os: "mobile", version: "0.01", type: "desktop" } }
+  );
   do_check_false(scheduler.numClients > 1);
   scheduler.updateClientMode();
   Service.sync();
 
   do_check_eq(scheduler.syncThreshold, MULTI_DEVICE_THRESHOLD);
   do_check_eq(scheduler.syncInterval, scheduler.activeInterval);
   do_check_true(scheduler.numClients > 1);
   do_check_false(scheduler.idle);
 
   // Resets the number of clients to 0.
   clientsEngine.resetClient();
+  // Also re-init the server, or we suck our "foo" client back down.
+  await setUp(server);
+
   Service.sync();
 
   // Goes back to single user if # clients is 1.
   do_check_eq(scheduler.numClients, 1);
   do_check_eq(scheduler.syncThreshold, SINGLE_USER_THRESHOLD);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
   do_check_false(scheduler.numClients > 1);
   do_check_false(scheduler.idle);
@@ -596,17 +603,19 @@ add_identity_test(this, async function t
 
   // Single device: nothing changes.
   scheduler.observe(null, "idle", Svc.Prefs.get("scheduler.idleTime"));
   do_check_eq(scheduler.idle, true);
   do_check_eq(scheduler.syncInterval, scheduler.singleDeviceInterval);
 
   // Multiple devices: switch to idle interval.
   scheduler.idle = false;
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  clientsEngine._store.create(
+    { id: "foo", cleartext: { os: "mobile", version: "0.01", type: "desktop" } }
+  );
   scheduler.updateClientMode();
   scheduler.observe(null, "idle", Svc.Prefs.get("scheduler.idleTime"));
   do_check_eq(scheduler.idle, true);
   do_check_eq(scheduler.syncInterval, scheduler.idleInterval);
 
   await cleanUpAndGo();
 });
 
@@ -736,17 +745,19 @@ add_identity_test(this, async function t
   scheduler._syncErrors = MAX_ERROR_COUNT_BEFORE_BACKOFF;
   let server = sync_httpd_setup();
 
   let engine = Service.engineManager.get("catapult");
   engine.enabled = true;
   engine.exception = {status: 400};
 
   // Have multiple devices for an active interval.
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  clientsEngine._store.create(
+    { id: "foo", cleartext: { os: "mobile", version: "0.01", type: "desktop" } }
+  );
 
   do_check_eq(Status.sync, SYNC_SUCCEEDED);
 
   do_check_true(await setUp(server));
 
   Service.sync();
 
   do_check_eq(Status.service, SYNC_FAILED_PARTIAL);
@@ -778,17 +789,19 @@ add_identity_test(this, async function t
       response.setHeader("X-Weave-Backoff", "" + BACKOFF);
     }
     infoColl(request, response);
   }
   server.registerPathHandler(INFO_COLLECTIONS, infoCollWithBackoff);
 
   // Pretend we have two clients so that the regular sync interval is
   // sufficiently low.
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  clientsEngine._store.create(
+    { id: "foo", cleartext: { os: "mobile", version: "0.01", type: "desktop" } }
+  );
   let rec = clientsEngine._store.createRecord("foo", "clients");
   rec.encrypt(Service.collectionKeys.keyForCollection("clients"));
   rec.upload(Service.resource(clientsEngine.engineURL + rec.id));
 
   // Sync once to log in and get everything set up. Let's verify our initial
   // values.
   Service.sync();
   do_check_eq(Status.backoffInterval, 0);
@@ -835,17 +848,19 @@ add_identity_test(this, async function t
     }
     response.setHeader("Retry-After", "" + BACKOFF);
     response.setStatusLine(request.httpVersion, 503, "Service Unavailable");
   }
   server.registerPathHandler(INFO_COLLECTIONS, infoCollWithMaintenance);
 
   // Pretend we have two clients so that the regular sync interval is
   // sufficiently low.
-  clientsEngine._store.create({id: "foo", cleartext: "bar"});
+  clientsEngine._store.create(
+    { id: "foo", cleartext: { os: "mobile", version: "0.01", type: "desktop" } }
+  );
   let rec = clientsEngine._store.createRecord("foo", "clients");
   rec.encrypt(Service.collectionKeys.keyForCollection("clients"));
   rec.upload(Service.resource(clientsEngine.engineURL + rec.id));
 
   // Sync once to log in and get everything set up. Let's verify our initial
   // values.
   Service.sync();
   do_check_false(Status.enforceBackoff);