From 632c46235a33321e452a7944aefd0ccf30b45478 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Thu, 10 Nov 2016 19:12:55 +1100 Subject: [PATCH] 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 | 9 +++++++ services/sync/modules/telemetry.js | 15 +++++------ .../sync/tests/unit/test_syncscheduler.js | 27 ++++++++++++++----- 3 files changed, 36 insertions(+), 15 deletions(-) diff --git a/services/sync/modules/browserid_identity.js b/services/sync/modules/browserid_identity.js index 26d3deeee96f..ea91197eca5d 100644 --- a/services/sync/modules/browserid_identity.js +++ b/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; }, diff --git a/services/sync/modules/telemetry.js b/services/sync/modules/telemetry.js index c311387f7996..705bff81046c 100644 --- a/services/sync/modules/telemetry.js +++ b/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), }; }); } diff --git a/services/sync/tests/unit/test_syncscheduler.js b/services/sync/tests/unit/test_syncscheduler.js index de3cc7841631..2e81a5f2455d 100644 --- a/services/sync/tests/unit/test_syncscheduler.js +++ b/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));