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

--HG--
extra : rebase_source : 169b3ad54dd19b5ef319d1e09684c0fd474e695b

services/sync/modules/browserid_identity.js

@@ -121,6 +121,15 @@ this.BrowserIDManager.prototype = {
     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;
   },

services/sync/modules/telemetry.js

@@ -268,17 +268,14 @@ class TelemetryRecord {
     // 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;
@@ -290,7 +287,7 @@ class TelemetryRecord {
         return {
           os: device.os,
           version: device.version,
-          id: Utils.sha256(device.id + this.uid)
+          id: Weave.Service.identity.hashedDeviceID(device.id),
         };
       });
     }

services/sync/tests/unit/test_syncscheduler.js

@@ -173,7 +173,9 @@ add_identity_test(this, async function test_updateClientMode() {
   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);
@@ -438,7 +440,9 @@ add_identity_test(this, async function test_client_sync_finish_updateClientMode(
   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();
@@ -450,6 +454,9 @@ add_identity_test(this, async function test_client_sync_finish_updateClientMode(
 
   // 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.
@@ -601,7 +608,9 @@ add_identity_test(this, async function test_idle_adjustSyncInterval() {
 
   // 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);
@@ -741,7 +750,9 @@ add_identity_test(this, async function test_sync_failed_partial_400s() {
   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);
 
@@ -783,7 +794,9 @@ add_identity_test(this, async function test_sync_X_Weave_Backoff() {
 
   // 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));
@@ -840,7 +853,9 @@ add_identity_test(this, async function test_sync_503_Retry_After() {
 
   // 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));