Bug 1604844 - add identifiers from the sync ping to the deletion-request ping. r=markh,chutten

Differential Revision: https://phabricator.services.mozilla.com/D72286

services/fxaccounts/FxAccountsTelemetry.jsm

@@ -14,13 +14,25 @@ const { XPCOMUtils } = ChromeUtils.import(
 );
 
 XPCOMUtils.defineLazyModuleGetters(this, {
-  log: "resource://gre/modules/FxAccountsCommon.js",
   // We use this observers module because we leverage its support for richer
   // "subject" data.
   Observers: "resource://services-common/observers.js",
   Services: "resource://gre/modules/Services.jsm",
+  CryptoUtils: "resource://services-crypto/utils.js",
 });
 
+const { PREF_ACCOUNT_ROOT, log } = ChromeUtils.import(
+  "resource://gre/modules/FxAccountsCommon.js"
+);
+
+const PREF_SANITIZED_UID = PREF_ACCOUNT_ROOT + "telemetry.sanitized_uid";
+XPCOMUtils.defineLazyPreferenceGetter(
+  this,
+  "pref_sanitizedUid",
+  PREF_SANITIZED_UID,
+  ""
+);
+
 class FxAccountsTelemetry {
   constructor(fxai) {
     this._fxai = fxai;
@@ -44,23 +56,36 @@ class FxAccountsTelemetry {
       .slice(1, -1);
   }
 
+  // Secret back-channel by which tokenserver client code can set the hashed UID.
+  // This value conceptually belongs to FxA, but we currently get it from tokenserver,
+  // so there's some light hackery to put it in the right place.
+  _setHashedUID(hashedUID) {
+    if (!hashedUID) {
+      Services.prefs.clearUserPref(PREF_SANITIZED_UID);
+    } else {
+      Services.prefs.setStringPref(PREF_SANITIZED_UID, hashedUID);
+    }
+  }
+
+  getSanitizedUID() {
+    // Sadly, we can only currently obtain this value if the user has enabled sync.
+    return pref_sanitizedUid || null;
+  }
+
   // Sanitize the ID of a device into something suitable for including in the
   // ping. Returns null if no transformation is possible.
   sanitizeDeviceId(deviceId) {
-    // We only know how to hash it for sync users, which kinda sucks.
-    let xps =
-      this._weaveXPCOM ||
-      Cc["@mozilla.org/weave/service;1"].getService(Ci.nsISupports)
-        .wrappedJSObject;
-    if (!xps.enabled) {
+    const uid = this.getSanitizedUID();
+    if (!uid) {
+      // Sadly, we can only currently get this if the user has enabled sync.
       return null;
     }
-    try {
-      return xps.Weave.Service.identity.hashedDeviceID(deviceId);
-    } catch {
-      // sadly this can happen in various scenarios, so don't complain.
-    }
-    return null;
+    // Combine the raw device id with the sanitized 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.
+    // The result is 64 bytes long, which in retrospect is probably excessive,
+    // but it's already shipping...
+    return CryptoUtils.sha256(deviceId + uid);
   }
 
   // Record the connection of FxA or one of its services.

services/fxaccounts/tests/xpcshell/test_telemetry.js

@@ -0,0 +1,55 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const { fxAccounts } = ChromeUtils.import(
+  "resource://gre/modules/FxAccounts.jsm"
+);
+
+const { PREF_ACCOUNT_ROOT } = ChromeUtils.import(
+  "resource://gre/modules/FxAccountsCommon.js"
+);
+
+_("Misc tests for FxAccounts.telemetry");
+
+const MOCK_HASHED_UID = "00112233445566778899aabbccddeeff";
+const MOCK_DEVICE_ID = "ffeeddccbbaa99887766554433221100";
+
+add_task(function test_sanitized_uid() {
+  Services.prefs.deleteBranch(
+    "identity.fxaccounts.account.telemetry.sanitized_uid"
+  );
+
+  // Returns `null` by default.
+  Assert.equal(fxAccounts.telemetry.getSanitizedUID(), null);
+
+  // Returns provided value if set.
+  fxAccounts.telemetry._setHashedUID(MOCK_HASHED_UID);
+  Assert.equal(fxAccounts.telemetry.getSanitizedUID(), MOCK_HASHED_UID);
+
+  // Reverts to unset for falsey values.
+  fxAccounts.telemetry._setHashedUID("");
+  Assert.equal(fxAccounts.telemetry.getSanitizedUID(), null);
+});
+
+add_task(function test_sanitize_device_id() {
+  Services.prefs.deleteBranch(
+    "identity.fxaccounts.account.telemetry.sanitized_uid"
+  );
+
+  // Returns `null` by default.
+  Assert.equal(fxAccounts.telemetry.sanitizeDeviceId(MOCK_DEVICE_ID), null);
+
+  // Hashes with the sanitized UID if set.
+  // (test value here is SHA256(MOCK_DEVICE_ID + MOCK_HASHED_UID))
+  fxAccounts.telemetry._setHashedUID(MOCK_HASHED_UID);
+  Assert.equal(
+    fxAccounts.telemetry.sanitizeDeviceId(MOCK_DEVICE_ID),
+    "dd7c845006df9baa1c6d756926519c8ce12f91230e11b6057bf8ec65f9b55c1a"
+  );
+
+  // Reverts to unset for falsey values.
+  fxAccounts.telemetry._setHashedUID("");
+  Assert.equal(fxAccounts.telemetry.sanitizeDeviceId(MOCK_DEVICE_ID), null);
+});

services/fxaccounts/tests/xpcshell/xpcshell.ini

@@ -20,6 +20,7 @@ support-files =
 [test_pairing.js]
 [test_profile_client.js]
 [test_push_service.js]
+[test_telemetry.js]
 [test_web_channel.js]
 [test_profile.js]
 [test_storage_manager.js]

services/sync/modules/browserid_identity.js

@@ -130,19 +130,20 @@ this.BrowserIDManager.prototype = {
   _userUid: null,
 
   hashedUID() {
-    if (!this._hashedUID) {
+    const id = this._fxaService.telemetry.getSanitizedUID();
+    if (!id) {
       throw new Error("hashedUID: Don't seem to have previously seen a token");
     }
-    return this._hashedUID;
+    return id;
   },
 
   // 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);
+    const id = this._fxaService.telemetry.sanitizeDeviceId(deviceID);
+    if (!id) {
+      throw new Error("hashedUID: Don't seem to have previously seen a token");
+    }
+    return id;
   },
 
   // The "node type" reported to telemetry or null if not specified.
@@ -282,7 +283,6 @@ this.BrowserIDManager.prototype = {
   resetCredentials() {
     this._syncKeyBundle = null;
     this._token = null;
-    this._hashedUID = null;
     // The cluster URL comes from the token, so resetting it to empty will
     // force Sync to not accidentally use a value from an earlier token.
     Weave.Service.clusterURL = null;
@@ -550,10 +550,11 @@ this.BrowserIDManager.prototype = {
     try {
       let token = await this._fetchTokenForUser();
       this._token = token;
-      // we store the hashed UID from the token so that if we see a transient
-      // error fetching a new token we still know the "most recent" hashed
-      // UID for telemetry.
-      this._hashedUID = token.hashed_fxa_uid;
+      // This is a little bit of a hack. The tokenserver tells us a HMACed version
+      // of the FxA uid which we can use for metrics purposes without revealing the
+      // user's true uid. It conceptually belongs to FxA but we get it from tokenserver
+      // for legacy reasons. Hand it back to the FxA client code to deal with.
+      this._fxaService.telemetry._setHashedUID(token.hashed_fxa_uid);
       return token;
     } finally {
       Services.obs.notifyObservers(null, "weave:service:login:change");

services/sync/modules/telemetry.js

@@ -6,6 +6,16 @@
 
 var EXPORTED_SYMBOLS = ["SyncTelemetry"];
 
+// Support for Sync-and-FxA-related telemetry, which is submitted in a special-purpose
+// telemetry ping called the "sync ping", documented here:
+//
+//  ../../../toolkit/components/telemetry/docs/data/sync-ping.rst
+//
+// The sync ping contains identifiers that are linked to the user's Firefox Account
+// and are separate from the main telemetry client_id, so this file is also responsible
+// for ensuring that we can delete those pings upon user request, by plumbing its
+// identifiers into the "deletion-request" ping.
+
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
@@ -50,10 +60,19 @@ const log = Log.repository.getLogger("Sync.Telemetry");
 
 const TOPICS = [
   "profile-before-change",
+
+  // For tracking change to account/device identifiers.
+  "fxaccounts:new_device_id",
+  "fxaccounts:onlogout",
+  "weave:service:ready",
+  "weave:service:login:change",
+
+  // For whole-of-sync metrics.
   "weave:service:sync:start",
   "weave:service:sync:finish",
   "weave:service:sync:error",
 
+  // For individual engine metrics.
   "weave:engine:sync:start",
   "weave:engine:sync:finish",
   "weave:engine:sync:error",
@@ -63,9 +82,9 @@ const TOPICS = [
   "weave:engine:validate:finish",
   "weave:engine:validate:error",
 
+  // For ad-hoc telemetry events.
   "weave:telemetry:event",
   "weave:telemetry:histogram",
-  // and we are now used by FxA, so a custom event for that.
   "fxa:telemetry:event",
 ];
 
@@ -372,12 +391,7 @@ class TelemetryRecord {
       this.failureReason = SyncTelemetry.transformError(error);
     }
 
-    try {
-      this.uid = Weave.Service.identity.hashedUID();
-    } catch (e) {
-      this.uid = EMPTY_UID;
-    }
-
+    this.uid = fxAccounts.telemetry.getSanitizedUID() || EMPTY_UID;
     this.syncNodeType = Weave.Service.identity.telemetryNodeType;
 
     // Check for engine statuses. -- We do this now, and not in engine.finished
@@ -566,15 +580,7 @@ class SyncTelemetryImpl {
   }
 
   sanitizeFxaDeviceId(deviceId) {
-    if (!this.syncIsEnabled()) {
-      return null;
-    }
-    try {
-      return Weave.Service.identity.hashedDeviceID(deviceId);
-    } catch {
-      // sadly this can happen in various scenarios, so don't complain.
-    }
-    return null;
+    fxAccounts.telemetry.sanitizeDeviceId(deviceId);
   }
 
   prepareFxaDevices(devices) {
@@ -708,11 +714,7 @@ class SyncTelemetryImpl {
   }
 
   submit(record) {
-    if (
-      Services.prefs.prefHasUserValue("identity.sync.tokenserver.uri") ||
-      Services.prefs.prefHasUserValue("services.sync.tokenServerURI")
-    ) {
-      log.trace(`Not sending telemetry ping for self-hosted Sync user`);
+    if (!this.isProductionSyncUser()) {
       return false;
     }
     // We still call submit() with possibly illegal payloads so that tests can
@@ -731,6 +733,17 @@ class SyncTelemetryImpl {
     return false;
   }
 
+  isProductionSyncUser() {
+    if (
+      Services.prefs.prefHasUserValue("identity.sync.tokenserver.uri") ||
+      Services.prefs.prefHasUserValue("services.sync.tokenServerURI")
+    ) {
+      log.trace(`Not sending telemetry ping for self-hosted Sync user`);
+      return false;
+    }
+    return true;
+  }
+
   onSyncStarted(data) {
     const why = data && JSON.parse(data).why;
     if (this.current) {
@@ -743,6 +756,49 @@ class SyncTelemetryImpl {
     this.current = new TelemetryRecord(this.allowedEngines, why);
   }
 
+  // We need to ensure that the telemetry `deletion-request` ping always contains the user's
+  // current sync device ID, because if the user opts out of telemetry then the deletion ping
+  // will be immediately triggered for sending, and we won't have a chance to fill it in later.
+  // This keeps the `deletion-ping` up-to-date when the user's account state changes.
+  onAccountInitOrChange() {
+    // We don't submit sync pings for self-hosters, so don't need to collect their device ids either.
+    if (!this.isProductionSyncUser()) {
+      return;
+    }
+    // Awkwardly async, but no need to await. If the user's account state changes while
+    // this promise is in flight, it will reject and we won't record any data in the ping.
+    // (And a new notification will trigger us to try again with the new state).
+    fxAccounts.device
+      .getLocalId()
+      .then(deviceId => {
+        let sanitizedDeviceId = fxAccounts.telemetry.sanitizeDeviceId(deviceId);
+        // In the past we did not persist the FxA metrics identifiers to disk,
+        // so this might be missing until we can fetch it from the server for the
+        // first time. There will be a fresh notification tirggered when it's available.
+        if (sanitizedDeviceId) {
+          // Sanitized device ids are 64 characters long, but telemetry limits scalar strings to 50.
+          // The first 32 chars are sufficient to uniquely identify the device, so just send those.
+          // It's hard to change the sync ping itself to only send 32 chars, to b/w compat reasons.
+          sanitizedDeviceId = sanitizedDeviceId.substr(0, 32);
+          Services.telemetry.scalarSet(
+            "deletion.request.sync_device_id",
+            sanitizedDeviceId
+          );
+        }
+      })
+      .catch(err => {
+        log.warn(
+          `Failed to set sync identifiers in the deletion-request ping: ${err}`
+        );
+      });
+  }
+
+  // This keeps the `deletion-request` ping up-to-date when the user signs out,
+  // clearing the now-nonexistent sync device id.
+  onAccountLogout() {
+    Services.telemetry.scalarSet("deletion.request.sync_device_id", "");
+  }
+
   _checkCurrent(topic) {
     if (!this.current) {
       log.warn(
@@ -876,6 +932,16 @@ class SyncTelemetryImpl {
         this.shutdown();
         break;
 
+      case "weave:service:ready":
+      case "weave:service:login:change":
+      case "fxaccounts:new_device_id":
+        this.onAccountInitOrChange();
+        break;
+
+      case "fxaccounts:onlogout":
+        this.onAccountLogout();
+        break;
+
       /* sync itself state changes */
       case "weave:service:sync:start":
         this.onSyncStarted(data);

services/sync/tests/unit/test_telemetry.js

@@ -11,6 +11,9 @@ const { RotaryEngine } = ChromeUtils.import(
   "resource://testing-common/services/sync/rotaryengine.js"
 );
 const { OS } = ChromeUtils.import("resource://gre/modules/osfile.jsm");
+const { fxAccounts } = ChromeUtils.import(
+  "resource://gre/modules/FxAccounts.jsm"
+);
 
 function SteamStore(engine) {
   Store.call(this, "Steam", engine);
@@ -1287,3 +1290,58 @@ add_task(async function test_node_type_change() {
   equal(pings[1].syncNodeType, "second-node-type");
   await promiseStopServer(server);
 });
+
+add_task(async function test_deletion_request_ping() {
+  async function assertRecordedSyncDeviceID(expected) {
+    // The scalar gets updated asynchronously, so wait a tick before checking.
+    await Promise.resolve();
+    const scalars =
+      Services.telemetry.getSnapshotForScalars("deletion-request").parent || {};
+    equal(scalars["deletion.request.sync_device_id"], expected);
+  }
+
+  const MOCK_HASHED_UID = "00112233445566778899aabbccddeeff";
+  const MOCK_DEVICE_ID1 = "ffeeddccbbaa99887766554433221100";
+  const MOCK_DEVICE_ID2 = "aabbccddeeff99887766554433221100";
+
+  // Calculated by hand using SHA256(DEVICE_ID + HASHED_UID)[:32]
+  const SANITIZED_DEVICE_ID1 = "dd7c845006df9baa1c6d756926519c8c";
+  const SANITIZED_DEVICE_ID2 = "0d06919a736fc029007e1786a091882c";
+
+  let currentDeviceID = null;
+  sinon.stub(fxAccounts.device, "getLocalId").callsFake(() => {
+    return Promise.resolve(currentDeviceID);
+  });
+  let telem = get_sync_test_telemetry();
+  sinon.stub(telem, "isProductionSyncUser").callsFake(() => true);
+  fxAccounts.telemetry._setHashedUID(false);
+  try {
+    // The scalar should start out undefined, since no user is actually logged in.
+    await assertRecordedSyncDeviceID(undefined);
+
+    // If we start up without knowing the hashed UID, it should stay undefined.
+    Services.obs.notifyObservers(null, "weave:service:ready");
+    await assertRecordedSyncDeviceID(undefined);
+
+    // But now let's say we've discovered the hashed UID from the server.
+    fxAccounts.telemetry._setHashedUID(MOCK_HASHED_UID);
+    currentDeviceID = MOCK_DEVICE_ID1;
+
+    // Now when we load up, we'll record the sync device id.
+    Services.obs.notifyObservers(null, "weave:service:ready");
+    await assertRecordedSyncDeviceID(SANITIZED_DEVICE_ID1);
+
+    // When the device-id changes we'll update it.
+    currentDeviceID = MOCK_DEVICE_ID2;
+    Services.obs.notifyObservers(null, "fxaccounts:new_device_id");
+    await assertRecordedSyncDeviceID(SANITIZED_DEVICE_ID2);
+
+    // When the user signs out we'll clear it.0
+    Services.obs.notifyObservers(null, "fxaccounts:onlogout");
+    await assertRecordedSyncDeviceID("");
+  } finally {
+    fxAccounts.telemetry._setHashedUID(false);
+    telem.isProductionSyncUser.restore();
+    fxAccounts.device.getLocalId.restore();
+  }
+});

toolkit/components/telemetry/Scalars.yaml

@@ -4184,6 +4184,24 @@ deletion.request:
     record_into_store:
       - 'deletion-request'
 
+  sync_device_id:
+    bug_numbers:
+      - 1604844
+    description: >
+      An identifier used by sync ping, to identify the current Firefox profile for a specific Account.
+    expires: never
+    kind: string
+    notification_emails:
+      - rfkelly@mozilla.com
+      - sync-team@mozilla.com
+    release_channel_collection: opt-out
+    products:
+      - 'firefox'
+    record_in_processes:
+      - 'main'
+    record_into_store:
+      - 'deletion-request'
+
 fxrPC:
   isFirstRun:
     bug_numbers: