From 4730d315959e58b94e355db9a516f46bfe4f2e0c Mon Sep 17 00:00:00 2001 From: Dave Townsend Date: Fri, 23 Aug 2024 12:45:33 +0000 Subject: [PATCH] Bug 1913765: profileGroupId should default to the clientId for existing clients and should reset at the same time. r=chutten This also changes `removeClientId` to `resetIdentifiers` and actually generates the new identifiers. Everywhere that currently calls `removeClientId` already immediately re-creates the client ID anyway. Differential Revision: https://phabricator.services.mozilla.com/D219594 --- browser/modules/test/unit/test_discovery.js | 2 +- .../components/telemetry/app/ClientID.sys.mjs | 82 +++++++++++-------- .../app/TelemetryControllerParent.sys.mjs | 78 +++++++++++++----- .../telemetry/app/TelemetryUtils.sys.mjs | 7 ++ .../tests/integration/tests/conftest.py | 2 + .../tests/test_deletion_request_ping.py | 13 ++- .../harness/telemetry_harness/testcase.py | 4 + .../client/test_deletion_request_ping.py | 5 +- .../unit/test_TelemetryClientID_reset.js | 52 +++++++++--- .../tests/unit/test_TelemetryController.js | 18 ++-- .../tests/unit/test_TelemetrySession.js | 1 + .../telemetry/tests/unit/test_client_id.js | 67 +++++++++++++-- 12 files changed, 245 insertions(+), 86 deletions(-) diff --git a/browser/modules/test/unit/test_discovery.js b/browser/modules/test/unit/test_discovery.js index 08f67273a667..25dec93f1cec 100644 --- a/browser/modules/test/unit/test_discovery.js +++ b/browser/modules/test/unit/test_discovery.js @@ -131,7 +131,7 @@ add_task(async function test_discovery() { equal(cookie.host, uri.host, "cookie exists for host"); return true; }); - await ClientID.removeClientID(); + await ClientID.resetIdentifiers(); await ClientID.getClientID(); await changed; diff --git a/toolkit/components/telemetry/app/ClientID.sys.mjs b/toolkit/components/telemetry/app/ClientID.sys.mjs index a92ddf5b70a8..ed8d376240fa 100644 --- a/toolkit/components/telemetry/app/ClientID.sys.mjs +++ b/toolkit/components/telemetry/app/ClientID.sys.mjs @@ -9,6 +9,9 @@ const LOGGER_NAME = "Toolkit.Telemetry"; const LOGGER_PREFIX = "ClientID::"; // Must match ID in TelemetryUtils const CANARY_CLIENT_ID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0"; +const CANARY_PROFILE_GROUP_ID = "decafdec-afde-cafd-ecaf-decafdecafde"; + +const DRS_STATE_VERSION = 2; const lazy = {}; @@ -109,29 +112,26 @@ export var ClientID = Object.freeze({ }, /** - * Sets the client ID to the canary (known) client ID, + * Sets the client ID and profile group ID to the canary (known) identifiers, * writing it to disk and updating the cached version. * - * Use `removeClientID` followed by `getClientID` to clear the - * existing ID and generate a new, random one if required. + * Use `resetIdentifiers` to clear the existing identifiers and generate new, + * random ones if required. * * @return {Promise} */ - setCanaryClientID() { - return ClientIDImpl.setCanaryClientID(); + setCanaryIdentifiers() { + return ClientIDImpl.setCanaryIdentifiers(); }, /** - * Clears the client ID asynchronously, removing it - * from disk. Use `getClientID()` to generate - * a fresh ID after calling this method. - * - * Should only be used if a reset is explicitly requested by the user. + * Assigns new random values to client ID and profile group ID. Should only be + * used if a reset is explicitly requested by the user. * * @return {Promise} */ - removeClientID() { - return ClientIDImpl.removeClientID(); + resetIdentifiers() { + return ClientIDImpl.resetIdentifiers(); }, /** @@ -150,7 +150,7 @@ var ClientIDImpl = { _profileGroupID: null, _loadClientIdTask: null, _saveDataReportingStateTask: null, - _removeClientIdTask: null, + _resetIdentifiersTask: null, _logger: null, _loadDataReportingState() { @@ -171,7 +171,7 @@ var ClientIDImpl = { async _doLoadDataReportingState() { this._log.trace(`_doLoadDataReportingState`); // If there's a removal in progress, let's wait for it - await this._removeClientIdTask; + await this._resetIdentifiersTask; // Try to load the client id from the DRS state file. let hasCurrentClientID = false; @@ -179,10 +179,26 @@ var ClientIDImpl = { try { let state = await IOUtils.readJSON(lazy.gStateFilePath); if (state) { + if (!("version" in state)) { + // Old version, clear out any previously generated profile group ID. + delete state.profileGroupID; + } + hasCurrentClientID = this.updateClientID(state.clientID); hasCurrentProfileGroupID = this.updateProfileGroupID( state.profileGroupID ); + + if (!hasCurrentProfileGroupID && hasCurrentClientID) { + // A pre-existing profile should be assigned the existing client ID. + hasCurrentProfileGroupID = this.updateProfileGroupID(this._clientID); + + if (hasCurrentProfileGroupID) { + this._saveDataReportingStateTask = this._saveDataReportingState(); + await this._saveDataReportingStateTask; + } + } + if (hasCurrentClientID && hasCurrentProfileGroupID) { this._log.trace( `_doLoadDataReportingState: Client and Group IDs loaded from state.` @@ -250,6 +266,7 @@ var ClientIDImpl = { try { this._log.trace(`_saveDataReportingState`); let obj = { + version: DRS_STATE_VERSION, clientID: this._clientID, profileGroupID: this._profileGroupID, }; @@ -438,47 +455,44 @@ var ClientIDImpl = { this._profileGroupID = null; }, - async setCanaryClientID() { - this._log.trace("setCanaryClientID"); + async setCanaryIdentifiers() { + this._log.trace("setCanaryIdentifiers"); this.updateClientID(CANARY_CLIENT_ID); + this.updateProfileGroupID(CANARY_PROFILE_GROUP_ID); this._saveDataReportingStateTask = this._saveDataReportingState(); await this._saveDataReportingStateTask; return this._clientID; }, - async _doRemoveClientID() { - this._log.trace("_doRemoveClientID"); + async _doResetIdentifiers() { + this._log.trace("_doResetIdentifiers"); // Reset the cached client ID. - this._clientID = null; + this.updateClientID(lazy.CommonUtils.generateUUID()); this._clientIDHash = null; - // Clear the client id from the preference cache. - Services.prefs.clearUserPref(PREF_CACHED_CLIENTID); + // Reset the cached profile group ID. + this.updateProfileGroupID(lazy.CommonUtils.generateUUID()); // If there is a save in progress, wait for it to complete. await this._saveDataReportingStateTask; - // Remove the client-id-containing state file from disk - await IOUtils.remove(lazy.gStateFilePath); + // Save the new identifiers to disk. + this._saveDataReportingStateTask = this._saveDataReportingState(); + await this._saveDataReportingStateTask; }, - async removeClientID() { - this._log.trace("removeClientID"); - - if (AppConstants.platform != "android") { - // We can't clear the client_id in Glean, but we can make it the canary. - Glean.legacyTelemetry.clientId.set(CANARY_CLIENT_ID); - } + async resetIdentifiers() { + this._log.trace("resetIdentifiers"); // Wait for the removal. // Asynchronous calls to getClientID will also be blocked on this. - this._removeClientIdTask = this._doRemoveClientID(); - let clear = () => (this._removeClientIdTask = null); - this._removeClientIdTask.then(clear, clear); + this._resetIdentifiersTask = this._doResetIdentifiers(); + let clear = () => (this._resetIdentifiersTask = null); + this._resetIdentifiersTask.then(clear, clear); - await this._removeClientIdTask; + await this._resetIdentifiersTask; }, /** diff --git a/toolkit/components/telemetry/app/TelemetryControllerParent.sys.mjs b/toolkit/components/telemetry/app/TelemetryControllerParent.sys.mjs index 745026a11593..6b016c76a1a4 100644 --- a/toolkit/components/telemetry/app/TelemetryControllerParent.sys.mjs +++ b/toolkit/components/telemetry/app/TelemetryControllerParent.sys.mjs @@ -157,6 +157,8 @@ export var TelemetryController = Object.freeze({ * @param {Boolean} [aOptions.usePingSender=false] if true, send the ping using the PingSender. * @param {String} [aOptions.overrideClientId=undefined] if set, override the * client id to the provided value. Implies aOptions.addClientId=true. + * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the + * profile group id to the provided value. Implies aOptions.addClientId=true. * @returns {Promise} Test-only - a promise that resolves with the ping id once the ping is stored or sent. */ submitExternalPing(aType, aPayload, aOptions = {}) { @@ -192,6 +194,8 @@ export var TelemetryController = Object.freeze({ * @param {Object} [aOptions.overrideEnvironment=null] set to override the environment data. * @param {String} [aOptions.overrideClientId=undefined] if set, override the * client id to the provided value. Implies aOptions.addClientId=true. + * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the + * profile group id to the provided value. Implies aOptions.addClientId=true. * * @returns {Promise} A promise that resolves with the ping id when the ping is saved to * disk. @@ -374,6 +378,8 @@ var Impl = { * @param {Object} [aOptions.overrideEnvironment=null] set to override the environment data. * @param {String} [aOptions.overrideClientId=undefined] if set, override the * client id to the provided value. Implies aOptions.addClientId=true. + * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the + * profile group id to the provided value. Implies aOptions.addClientId=true. * @param {Boolean} [aOptions.useEncryption=false] if true, encrypt data client-side before sending. * @param {Object} [aOptions.publicKey=null] the public key to use if encryption is enabled (JSON Web Key). * @param {String} [aOptions.encryptionKeyId=null] the public key ID to use if encryption is enabled. @@ -406,9 +412,14 @@ var Impl = { payload, }; - if (aOptions.addClientId || aOptions.overrideClientId) { - pingData.clientId = aOptions.overrideClientId || this._clientID; - pingData.profileGroupId = this._profileGroupID; + if ( + aOptions.addClientId || + aOptions.overrideClientId || + aOptions.overrideProfileGroupId + ) { + pingData.clientId = aOptions.overrideClientId ?? this._clientID; + pingData.profileGroupId = + aOptions.overrideProfileGroupId ?? this._profileGroupID; } if (aOptions.addEnvironment) { @@ -460,16 +471,23 @@ var Impl = { * pioneer id to the provided value. Only works if aOptions.addPioneerId=true. * @param {String} [aOptions.overrideClientId=undefined] if set, override the * client id to the provided value. Implies aOptions.addClientId=true. + * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the + * profile group id to the provided value. Implies aOptions.addClientId=true. * @returns {Promise} Test-only - a promise that is resolved with the ping id once the ping is stored or sent. */ async _submitPingLogic(aType, aPayload, aOptions) { // Make sure to have a clientId if we need one. This cover the case of submitting // a ping early during startup, before Telemetry is initialized, if no client id was // cached. - if ( - aOptions.addClientId && - (!this._profileGroupID || (!this._clientID && !aOptions.overrideClientId)) - ) { + let needsIdentifiers = + aOptions.addClientId || + aOptions.overrideClientId || + aOptions.overrideProfileGroupId; + let hasClientId = aOptions.overrideClientId ?? this._clientID; + let hasProfileGroupId = + aOptions.overrideProfileGroupId ?? this._profileGroupID; + + if (needsIdentifiers && !(hasClientId && hasProfileGroupId)) { this._log.trace( "_submitPingLogic - Waiting on client id or profile group id" ); @@ -582,6 +600,8 @@ var Impl = { * pioneer id to the provided value. Only works if aOptions.addPioneerId=true. * @param {String} [aOptions.overrideClientId=undefined] if set, override the * client id to the provided value. Implies aOptions.addClientId=true. + * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the + * profile group id to the provided value. Implies aOptions.addClientId=true. * @returns {Promise} Test-only - a promise that is resolved with the ping id once the ping is stored or sent. */ submitExternalPing: function send(aType, aPayload, aOptions) { @@ -646,6 +666,8 @@ var Impl = { * @param {Object} [aOptions.overrideEnvironment=null] set to override the environment data. * @param {String} [aOptions.overrideClientId=undefined] if set, override the * client id to the provided value. Implies aOptions.addClientId=true. + * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the + * profile group id to the provided value. Implies aOptions.addClientId=true. * * @returns {Promise} A promise that resolves with the ping id when the ping is saved to * disk. @@ -834,18 +856,28 @@ var Impl = { TelemetryUtils.Preferences.FhrUploadEnabled, false ); - if (uploadEnabled && this._clientID == Utils.knownClientID) { + if ( + uploadEnabled && + (this._clientID == Utils.knownClientID || + this._profileGroupID == Utils.knownProfileGroupID) + ) { this._log.trace( - "Upload enabled, but got canary client ID. Resetting." + "Upload enabled, but got canary identifiers. Resetting." ); - await lazy.ClientID.removeClientID(); + await lazy.ClientID.resetIdentifiers(); this._clientID = await lazy.ClientID.getClientID(); - } else if (!uploadEnabled && this._clientID != Utils.knownClientID) { + this._profileGroupID = await lazy.ClientID.getProfileGroupID(); + } else if ( + !uploadEnabled && + (this._clientID != Utils.knownClientID || + this._profileGroupID != Utils.knownProfileGroupID) + ) { this._log.trace( "Upload disabled, but got a valid client ID. Setting canary client ID." ); - await lazy.ClientID.setCanaryClientID(); + await lazy.ClientID.setCanaryIdentifiers(); this._clientID = await lazy.ClientID.getClientID(); + this._profileGroupID = await lazy.ClientID.getProfileGroupID(); } await lazy.TelemetrySend.setup(this._testMode); @@ -1073,17 +1105,18 @@ var Impl = { ); if (uploadEnabled) { this._log.trace( - "_onUploadPrefChange - upload was enabled again. Resetting client ID" + "_onUploadPrefChange - upload was enabled again. Resetting identifiers" ); - // Delete cached client ID immediately, so other usage is forced to refetch it. + // Delete cached identifiers immediately, so other usage is forced to refetch it. this._clientID = null; + this._profileGroupID = null; // Generate a new client ID and make sure this module uses the new version let p = (async () => { - await lazy.ClientID.removeClientID(); - let id = await lazy.ClientID.getClientID(); - this._clientID = id; + await lazy.ClientID.resetIdentifiers(); + this._clientID = await lazy.ClientID.getClientID(); + this._profileGroupID = await lazy.ClientID.getProfileGroupID(); Services.telemetry.scalarSet("telemetry.data_upload_optin", true); await this.saveUninstallPing().catch(e => @@ -1125,17 +1158,22 @@ var Impl = { /* clear */ true ); - // 6. Set ClientID to a known value + // 6. Set identifiers to a known values let oldClientId = await lazy.ClientID.getClientID(); - await lazy.ClientID.setCanaryClientID(); + let oldProfileGroupId = await lazy.ClientID.getProfileGroupID(); + await lazy.ClientID.setCanaryIdentifiers(); this._clientID = await lazy.ClientID.getClientID(); + this._profileGroupID = await lazy.ClientID.getProfileGroupID(); // 7. Send the deletion-request ping. this._log.trace("_onUploadPrefChange - Sending deletion-request ping."); this.submitExternalPing( PING_TYPE_DELETION_REQUEST, { scalars }, - { overrideClientId: oldClientId } + { + overrideClientId: oldClientId, + overrideProfileGroupId: oldProfileGroupId, + } ); this._deletionRequestPingSubmittedPromise = null; } diff --git a/toolkit/components/telemetry/app/TelemetryUtils.sys.mjs b/toolkit/components/telemetry/app/TelemetryUtils.sys.mjs index d0387353b9fb..803d52a1ac08 100644 --- a/toolkit/components/telemetry/app/TelemetryUtils.sys.mjs +++ b/toolkit/components/telemetry/app/TelemetryUtils.sys.mjs @@ -96,6 +96,13 @@ export var TelemetryUtils = { return "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0"; }, + /** + * A fixed valid profile group ID used when Telemetry upload is disabled. + */ + get knownProfileGroupID() { + return "decafdec-afde-cafd-ecaf-decafdecafde"; + }, + /** * True if this is a content process. */ diff --git a/toolkit/components/telemetry/tests/integration/tests/conftest.py b/toolkit/components/telemetry/tests/integration/tests/conftest.py index 009c8f75b40a..57067bedb73f 100644 --- a/toolkit/components/telemetry/tests/integration/tests/conftest.py +++ b/toolkit/components/telemetry/tests/integration/tests/conftest.py @@ -19,6 +19,7 @@ from six import reraise from telemetry_harness.ping_server import PingServer CANARY_CLIENT_ID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0" +CANARY_PROFILE_GROUP_ID = "decafdec-afde-cafd-ecaf-decafdecafde" SERVER_ROOT = "toolkit/components/telemetry/tests/marionette/harness/www" UUID_PATTERN = re.compile( r"^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" @@ -295,6 +296,7 @@ class Helpers(object): assert value is not None assert value != "" assert value != CANARY_CLIENT_ID + assert value != CANARY_PROFILE_GROUP_ID assert re.match(UUID_PATTERN, value) is not None def wait_for_ping(self, action_func, ping_filter): diff --git a/toolkit/components/telemetry/tests/integration/tests/test_deletion_request_ping.py b/toolkit/components/telemetry/tests/integration/tests/test_deletion_request_ping.py index 2eb74efe385e..77f6568e2d43 100644 --- a/toolkit/components/telemetry/tests/integration/tests/test_deletion_request_ping.py +++ b/toolkit/components/telemetry/tests/integration/tests/test_deletion_request_ping.py @@ -12,15 +12,19 @@ from telemetry_harness.ping_filters import ( def test_deletion_request_ping(browser, helpers): """Test the "deletion-request" ping behaviour across sessions""" # Get the client_id after installing an addon - client_id = helpers.wait_for_ping(browser.install_addon, ANY_PING)["clientId"] + ping = helpers.wait_for_ping(browser.install_addon, ANY_PING) + client_id = ping["clientId"] + profile_group_id = ping["profileGroupId"] # Make sure it's a valid UUID helpers.assert_is_valid_uuid(client_id) + helpers.assert_is_valid_uuid(profile_group_id) # Trigger a "deletion-request" ping. ping = helpers.wait_for_ping(browser.disable_telemetry, DELETION_REQUEST_PING) assert "clientId" in ping + assert "profileGroupId" in ping assert "payload" in ping assert "environment" not in ping["payload"] @@ -48,6 +52,11 @@ def test_deletion_request_ping(browser, helpers): helpers.assert_is_valid_uuid(new_client_id) assert new_client_id != client_id + assert "profileGroupId" in main_ping + new_profile_group_id = main_ping["profileGroupId"] + helpers.assert_is_valid_uuid(new_profile_group_id) + assert new_profile_group_id != profile_group_id + # Ensure we note in the ping that the user opted in. parent_scalars = main_ping["payload"]["processes"]["parent"]["scalars"] @@ -58,6 +67,8 @@ def test_deletion_request_ping(browser, helpers): for ping in browser.ping_server.pings: if "clientId" in ping: helpers.assert_is_valid_uuid(ping["clientId"]) + if "profileGroupId" in ping: + helpers.assert_is_valid_uuid(ping["profileGroupId"]) if __name__ == "__main__": diff --git a/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py b/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py index b78f01b7ba13..163123574c20 100644 --- a/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py +++ b/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py @@ -16,6 +16,7 @@ from marionette_harness.runner.mixins.window_manager import WindowManagerMixin from telemetry_harness.ping_server import PingServer CANARY_CLIENT_ID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0" +CANARY_PROFILE_GROUP_ID = "decafdec-afde-cafd-ecaf-decafdecafde" UUID_PATTERN = re.compile( r"^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" ) @@ -85,6 +86,9 @@ class TelemetryTestCase(WindowManagerMixin, MarionetteTestCase): # Check for client ID that is used when Telemetry upload is disabled self.assertNotEqual(value, CANARY_CLIENT_ID, msg="UUID is CANARY CLIENT ID") + self.assertNotEqual( + value, CANARY_PROFILE_GROUP_ID, msg="UUID is CANARY PROFILE GROUP ID" + ) self.assertIsNotNone( re.match(UUID_PATTERN, value), diff --git a/toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_request_ping.py b/toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_request_ping.py index b7f50e0ae0d9..1ace8065d6f5 100644 --- a/toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_request_ping.py +++ b/toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_request_ping.py @@ -75,7 +75,8 @@ class TestDeletionRequestPing(TelemetryTestCase): self.assertIn("clientId", main_ping) self.assertIsValidUUID(main_ping["clientId"]) self.assertNotEqual(main_ping["clientId"], client_id) - self.assertEqual(main_ping["profileGroupId"], profile_group_id) + self.assertIsValidUUID(main_ping["profileGroupId"]) + self.assertNotEqual(main_ping["profileGroupId"], profile_group_id) # Ensure we note in the ping that the user opted in. parent_scalars = main_ping["payload"]["processes"]["parent"]["scalars"] @@ -87,3 +88,5 @@ class TestDeletionRequestPing(TelemetryTestCase): for ping in self.ping_server.pings: if "clientId" in ping: self.assertIsValidUUID(ping["clientId"]) + if "profileGroupId" in ping: + self.assertIsValidUUID(ping["profileGroupId"]) diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js b/toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js index dae540aca00c..a72069d14a06 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js @@ -91,10 +91,15 @@ add_task(async function test_clientid_reset_after_reenabling() { let clientId = await ClientID.getClientID(); Assert.equal(TelemetryUtils.knownClientID, clientId); let profileGroupId = await ClientID.getProfileGroupID(); - Assert.equal( + Assert.notEqual( firstProfileGroupId, profileGroupId, - "The profile group ID should not have been reset." + "The profile group ID should have been reset." + ); + Assert.notEqual( + profileGroupId, + clientId, + "The profile group ID should not match the new client ID." ); // Now shutdown the instance @@ -119,10 +124,20 @@ add_task(async function test_clientid_reset_after_reenabling() { "Client ID should be newly generated" ); let newProfileGroupId = await ClientID.getProfileGroupID(); - Assert.equal( + Assert.notEqual( + TelemetryUtils.knownProfileGroupID, + newProfileGroupId, + "The profile group ID should be valid and random" + ); + Assert.notEqual( firstProfileGroupId, newProfileGroupId, - "The profile group ID should not have been reset." + "The profile group ID should have been reset." + ); + Assert.notEqual( + newProfileGroupId, + newClientId, + "The profile group ID should not match the client ID." ); }); @@ -155,6 +170,16 @@ add_task(async function test_clientid_canary_after_disabling() { firstClientId, "Client ID should be valid and random" ); + Assert.notEqual( + TelemetryUtils.knownProfileGroupID, + firstProfileGroupId, + "Profile Group ID should be valid and random" + ); + Assert.notEqual( + firstClientId, + firstProfileGroupId, + "Profile Group ID should be valid and not match the client ID" + ); // Disable FHR upload: this should trigger a deletion-request ping. Services.prefs.setBoolPref( @@ -173,11 +198,7 @@ add_task(async function test_clientid_canary_after_disabling() { let clientId = await ClientID.getClientID(); Assert.equal(TelemetryUtils.knownClientID, clientId); let profileGroupId = await ClientID.getProfileGroupID(); - Assert.equal( - firstProfileGroupId, - profileGroupId, - "Profile group ID should not have been reset" - ); + Assert.equal(TelemetryUtils.knownProfileGroupID, profileGroupId); Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true); await sendPing(); @@ -188,10 +209,15 @@ add_task(async function test_clientid_canary_after_disabling() { ping.clientId, "Client ID should be newly generated" ); - Assert.equal( + Assert.notEqual( firstProfileGroupId, ping.profileGroupId, - "Profile group ID should not have been reset" + "Profile group ID should be newly generated" + ); + Assert.notEqual( + ping.profileGroupId, + ping.clientId, + "Profile group ID should not match the client ID" ); // Now shutdown the instance @@ -215,9 +241,9 @@ add_task(async function test_clientid_canary_after_disabling() { ); let newProfileGroupId = await ClientID.getProfileGroupID(); Assert.equal( - firstProfileGroupId, + TelemetryUtils.knownProfileGroupID, newProfileGroupId, - "Profile group ID should not have been reset" + "Profile group ID should be a canary when upload disabled" ); }); diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryController.js b/toolkit/components/telemetry/tests/unit/test_TelemetryController.js index a4ff6cba7350..1d596a99234c 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryController.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetryController.js @@ -208,7 +208,11 @@ add_task(async function test_disableDataUpload() { firstClientId, "Client ID should be valid and random" ); - Assert.ok(firstProfileGroupId, "Test ping needs a profile group ID"); + Assert.notEqual( + TelemetryUtils.knownProfileGroupID, + firstProfileGroupId, + "Profile group ID should be valid and random" + ); // The next step should trigger an event, watch for it. let disableObserved = TestUtils.topicObserved( @@ -262,10 +266,10 @@ add_task(async function test_disableDataUpload() { ); let secondProfileGroupId = TelemetryController.getCurrentPingData().profileGroupId; - Assert.equal( + Assert.notEqual( firstProfileGroupId, secondProfileGroupId, - "The profile group id must not have changed" + "The profile group id must have changed" ); // Simulate a failure in sending the deletion-request ping by disabling the HTTP server. await PingServer.stop(); @@ -332,10 +336,10 @@ add_task(async function test_disableDataUpload() { ping.clientId, "Client ID should be different from the previous value" ); - Assert.equal( + Assert.notEqual( firstProfileGroupId, ping.profileGroupId, - "The profile group ID should not change" + "The profile group ID should change" ); // The "deletion-request" ping should come next, as it was pending. @@ -347,9 +351,9 @@ add_task(async function test_disableDataUpload() { "Deletion must be requested for correct client id" ); Assert.equal( - firstProfileGroupId, + secondProfileGroupId, ping.profileGroupId, - "The profile group ID should not change" + "Deletion must be requested for correct profile group id" ); // Wait on ping activity to settle before moving on to the next test. If we were diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js index 84eacf913e91..83efee129235 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js @@ -1288,6 +1288,7 @@ add_task(async function test_sendShutdownPing() { await TelemetryController.testShutdown(); // After re-enabling FHR, wait for the new client ID gClientID = await ClientID.getClientID(); + gProfileGroupID = await ClientID.getProfileGroupID(); // Check that the "shutdown" ping was correctly saved to disk. await checkPendingShutdownPing(); diff --git a/toolkit/components/telemetry/tests/unit/test_client_id.js b/toolkit/components/telemetry/tests/unit/test_client_id.js index e06d39f24063..855ac76bcbef 100644 --- a/toolkit/components/telemetry/tests/unit/test_client_id.js +++ b/toolkit/components/telemetry/tests/unit/test_client_id.js @@ -131,11 +131,15 @@ add_task(async function test_profile_group_id() { // If there is no DRS file, and no cached id, we should get a new profile group ID. await ClientID._reset(); + Services.prefs.clearUserPref(PREF_CACHED_CLIENTID); Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID); await IOUtils.remove(drsPath, { ignoreAbsent: true }); + let clientID = await ClientID.getClientID(); let profileGroupID = await ClientID.getProfileGroupID(); Assert.equal(typeof profileGroupID, "string"); Assert.ok(uuidRegex.test(profileGroupID)); + // A new client should have distinct client ID and profile group ID. + Assert.notEqual(profileGroupID, clientID); if (AppConstants.platform != "android") { Assert.equal( profileGroupID, @@ -145,6 +149,7 @@ add_task(async function test_profile_group_id() { // We should be guarded against invalid DRS json. await ClientID._reset(); + Services.prefs.clearUserPref(PREF_CACHED_CLIENTID); Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID); await IOUtils.writeUTF8(drsPath, "abcd", { tmpPath: drsPath + ".tmp", @@ -174,17 +179,42 @@ add_task(async function test_profile_group_id() { } } - // Test that valid DRS actually works. const validProfileGroupID = "5afebd62-a33c-416c-b519-5c60fb988e8e"; const validClientID = "d06361a2-67d8-4d41-b804-6fab6ddf5461"; + + // An older version of DRS should reset the profile group ID to the client ID. await ClientID._reset(); + Services.prefs.clearUserPref(PREF_CACHED_CLIENTID); + Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID); await IOUtils.writeJSON(drsPath, { clientID: validClientID, profileGroupID: validProfileGroupID, }); + clientID = await ClientID.getClientID(); + profileGroupID = await ClientID.getProfileGroupID(); + Assert.equal(typeof profileGroupID, "string"); + Assert.ok(uuidRegex.test(profileGroupID)); + Assert.equal(clientID, validClientID); + Assert.equal(profileGroupID, clientID); + if (AppConstants.platform != "android") { + Assert.equal( + profileGroupID, + Glean.legacyTelemetry.profileGroupId.testGetValue() + ); + } + + // Test that valid DRS actually works. + await ClientID._reset(); + Services.prefs.clearUserPref(PREF_CACHED_CLIENTID); + Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID); + await IOUtils.writeJSON(drsPath, { + version: 2, + clientID: validClientID, + profileGroupID: validProfileGroupID, + }); profileGroupID = await ClientID.getProfileGroupID(); Assert.equal(profileGroupID, validProfileGroupID); - let clientID = await ClientID.getClientID(); + clientID = await ClientID.getClientID(); Assert.equal(clientID, validClientID); if (AppConstants.platform != "android") { Assert.equal( @@ -197,9 +227,12 @@ add_task(async function test_profile_group_id() { // Test that valid DRS actually works when the client ID is missing. await ClientID._reset(); await IOUtils.writeJSON(drsPath, { + version: 2, profileGroupID: validProfileGroupID, }); + clientID = await ClientID.getClientID(); profileGroupID = await ClientID.getProfileGroupID(); + Assert.equal(clientID, validClientID); Assert.equal(profileGroupID, validProfileGroupID); if (AppConstants.platform != "android") { Assert.equal( @@ -211,6 +244,7 @@ add_task(async function test_profile_group_id() { // Test that reloading a valid DRS works. await ClientID._reset(); + Services.prefs.clearUserPref(PREF_CACHED_CLIENTID); Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID); profileGroupID = await ClientID.getProfileGroupID(); Assert.equal(profileGroupID, validProfileGroupID); @@ -367,33 +401,48 @@ add_task(async function test_set_profile_group_id() { } }); -add_task(async function test_setCanaryClientID() { - const KNOWN_UUID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0"; +add_task(async function test_setCanaryIdentifiers() { + const KNOWN_CLIENT_UUID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0"; + const KNOWN_PROFILE_GROUP_UUID = "decafdec-afde-cafd-ecaf-decafdecafde"; await ClientID._reset(); // We should be able to set a valid UUID - await ClientID.setCanaryClientID(); + await ClientID.setCanaryIdentifiers(); let clientID = await ClientID.getClientID(); - Assert.equal(KNOWN_UUID, clientID); + Assert.equal(KNOWN_CLIENT_UUID, clientID); + let profileGroupID = await ClientID.getProfileGroupID(); + Assert.equal(KNOWN_PROFILE_GROUP_UUID, profileGroupID); if (AppConstants.platform != "android") { Assert.equal(clientID, Glean.legacyTelemetry.clientId.testGetValue()); + Assert.equal( + profileGroupID, + Glean.legacyTelemetry.profileGroupId.testGetValue() + ); } }); add_task(async function test_removeParallelGet() { // We should get a valid UUID after reset - await ClientID.removeClientID(); + await ClientID.resetIdentifiers(); let firstClientID = await ClientID.getClientID(); + let firstProfileGroupID = await ClientID.getProfileGroupID(); if (AppConstants.platform != "android") { Assert.equal(firstClientID, Glean.legacyTelemetry.clientId.testGetValue()); + Assert.equal( + firstProfileGroupID, + Glean.legacyTelemetry.profileGroupId.testGetValue() + ); } + // The IDs should differ after being reset. + Assert.notEqual(firstClientID, firstProfileGroupID); + // We should get the same ID twice when requesting it in parallel to a reset. - let promiseRemoveClientID = ClientID.removeClientID(); + let promiseResetIdentifiers = ClientID.resetIdentifiers(); let p = ClientID.getClientID(); let newClientID = await ClientID.getClientID(); - await promiseRemoveClientID; + await promiseResetIdentifiers; let otherClientID = await p; Assert.notEqual(