Bug 1714486 - [DoH] Allow pref values to override Remote Settings. r=necko-reviewers,jaws,dragana

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

browser/app/profile/firefox.js

@@ -2039,17 +2039,6 @@ pref("extensions.screenshots.disabled", false);
 // Preference that determines whether Screenshots is opened as a dedicated browser component
 pref("screenshots.browser.component.enabled", false);
 
-// DoH Rollout: whether to enable automatic performance-based TRR-selection.
-// This pref is controlled by a Normandy rollout so we don't overload providers.
-pref("doh-rollout.trr-selection.enabled", false);
-
-// DoH Rollout: whether to enable automatic steering to provider endpoints.
-// This pref is also controlled by a Normandy rollout.
-pref("doh-rollout.provider-steering.enabled", true);
-
-// DoH Rollout: provider details for automatic steering.
-pref("doh-rollout.provider-steering.provider-list", "[{ \"name\": \"comcast\", \"canonicalName\": \"doh-discovery.xfinity.com\", \"uri\": \"https://doh.xfinity.com/dns-query\" }]");
-
 // DoH Rollout: whether to clear the mode value at shutdown.
 pref("doh-rollout.clearModeOnShutdown", false);
 

browser/components/doh/DoHConfig.jsm

@@ -26,36 +26,39 @@ XPCOMUtils.defineLazyModuleGetters(this, {
 const kGlobalPrefBranch = "doh-rollout";
 var kRegionPrefBranch;
 
-const kEnabledPref = "enabled";
-
-const kProvidersPref = "provider-list";
-
-const kTRRSelectionEnabledPref = "trr-selection.enabled";
-const kTRRSelectionProvidersPref = "trr-selection.provider-list";
-const kTRRSelectionCommitResultPref = "trr-selection.commit-result";
-
-const kProviderSteeringEnabledPref = "provider-steering.enabled";
-const kProviderSteeringListPref = "provider-steering.provider-list";
+const kConfigPrefs = {
+  kEnabledPref: "enabled",
+  kProvidersPref: "provider-list",
+  kTRRSelectionEnabledPref: "trr-selection.enabled",
+  kTRRSelectionProvidersPref: "trr-selection.provider-list",
+  kTRRSelectionCommitResultPref: "trr-selection.commit-result",
+  kProviderSteeringEnabledPref: "provider-steering.enabled",
+  kProviderSteeringListPref: "provider-steering.provider-list",
+};
 
 const kPrefChangedTopic = "nsPref:changed";
 
 const gProvidersCollection = RemoteSettings("doh-providers");
 const gConfigCollection = RemoteSettings("doh-config");
 
-function getPrefValueRegionFirst(prefName, defaultValue) {
-  return (
-    Preferences.get(`${kRegionPrefBranch}.${prefName}`) ||
-    Preferences.get(`${kGlobalPrefBranch}.${prefName}`, defaultValue)
-  );
+function getPrefValueRegionFirst(prefName) {
+  let regionalPrefName = `${kRegionPrefBranch}.${prefName}`;
+  if (Services.prefs.prefHasUserValue(regionalPrefName)) {
+    return Preferences.get(regionalPrefName);
+  }
+  return Preferences.get(`${kGlobalPrefBranch}.${prefName}`);
 }
 
 function getProviderListFromPref(prefName) {
-  try {
-    return JSON.parse(getPrefValueRegionFirst(prefName, "[]"));
-  } catch (e) {
-    Cu.reportError(`DoH provider list not a valid JSON array: ${prefName}`);
+  let prefVal = getPrefValueRegionFirst(prefName);
+  if (prefVal) {
+    try {
+      return JSON.parse(prefVal);
+    } catch (e) {
+      Cu.reportError(`DoH provider list not a valid JSON array: ${prefName}`);
+    }
   }
-  return [];
+  return undefined;
 }
 
 // Generate a base config object with getters that return pref values. When
@@ -65,43 +68,94 @@ function getProviderListFromPref(prefName) {
 // from it, we lose the ability to override getters because they are defined
 // as non-configureable properties on class instances. So just use a function.
 function makeBaseConfigObject() {
-  return {
-    get enabled() {
-      return getPrefValueRegionFirst(kEnabledPref, false);
-    },
+  function makeConfigProperty({
+    obj,
+    propName,
+    defaultVal,
+    prefName,
+    isProviderList,
+  }) {
+    let prefFn = isProviderList
+      ? getProviderListFromPref
+      : getPrefValueRegionFirst;
 
-    get providerList() {
-      return getProviderListFromPref(kProvidersPref);
-    },
+    let overridePropName = "_" + propName;
 
+    Object.defineProperty(obj, propName, {
+      get() {
+        // If a pref value exists, it gets top priority. Otherwise, if it has an
+        // explicitly set value (from Remote Settings), we return that.
+        let prefVal = prefFn(prefName);
+        if (prefVal !== undefined) {
+          return prefVal;
+        }
+        if (this[overridePropName] !== undefined) {
+          return this[overridePropName];
+        }
+        return defaultVal;
+      },
+      set(val) {
+        this[overridePropName] = val;
+      },
+    });
+  }
+  let newConfig = {
     get fallbackProviderURI() {
       return this.providerList[0]?.uri;
     },
-
-    trrSelection: {
-      get enabled() {
-        return getPrefValueRegionFirst(kTRRSelectionEnabledPref, false);
-      },
-
-      get commitResult() {
-        return getPrefValueRegionFirst(kTRRSelectionCommitResultPref, false);
-      },
-
-      get providerList() {
-        return getProviderListFromPref(kTRRSelectionProvidersPref);
-      },
-    },
-
-    providerSteering: {
-      get enabled() {
-        return getPrefValueRegionFirst(kProviderSteeringEnabledPref, false);
-      },
-
-      get providerList() {
-        return getProviderListFromPref(kProviderSteeringListPref);
-      },
-    },
+    trrSelection: {},
+    providerSteering: {},
   };
+  makeConfigProperty({
+    obj: newConfig,
+    propName: "enabled",
+    defaultVal: false,
+    prefName: kConfigPrefs.kEnabledPref,
+    isProviderList: false,
+  });
+  makeConfigProperty({
+    obj: newConfig,
+    propName: "providerList",
+    defaultVal: [],
+    prefName: kConfigPrefs.kProvidersPref,
+    isProviderList: true,
+  });
+  makeConfigProperty({
+    obj: newConfig.trrSelection,
+    propName: "enabled",
+    defaultVal: false,
+    prefName: kConfigPrefs.kTRRSelectionEnabledPref,
+    isProviderList: false,
+  });
+  makeConfigProperty({
+    obj: newConfig.trrSelection,
+    propName: "commitResult",
+    defaultVal: false,
+    prefName: kConfigPrefs.kTRRSelectionCommitResultPref,
+    isProviderList: false,
+  });
+  makeConfigProperty({
+    obj: newConfig.trrSelection,
+    propName: "providerList",
+    defaultVal: [],
+    prefName: kConfigPrefs.kTRRSelectionProvidersPref,
+    isProviderList: true,
+  });
+  makeConfigProperty({
+    obj: newConfig.providerSteering,
+    propName: "enabled",
+    defaultVal: false,
+    prefName: kConfigPrefs.kProviderSteeringEnabledPref,
+    isProviderList: false,
+  });
+  makeConfigProperty({
+    obj: newConfig.providerSteering,
+    propName: "providerList",
+    defaultVal: [],
+    prefName: kConfigPrefs.kProviderSteeringListPref,
+    isProviderList: true,
+  });
+  return newConfig;
 }
 
 const DoHConfigController = {
@@ -172,10 +226,16 @@ const DoHConfigController = {
   observe(subject, topic, data) {
     switch (topic) {
       case kPrefChangedTopic:
+        let allowedPrefs = Object.getOwnPropertyNames(kConfigPrefs).map(
+          k => kConfigPrefs[k]
+        );
         if (
-          !data.startsWith(kRegionPrefBranch) &&
-          data != `${kGlobalPrefBranch}.${kEnabledPref}` &&
-          data != `${kGlobalPrefBranch}.${kProvidersPref}`
+          !allowedPrefs.some(pref =>
+            [
+              `${kRegionPrefBranch}.${pref}`,
+              `${kGlobalPrefBranch}.${pref}`,
+            ].includes(data)
+          )
         ) {
           break;
         }
@@ -220,7 +280,6 @@ const DoHConfigController = {
     }
 
     if (localConfig.rolloutEnabled) {
-      delete newConfig.enabled;
       newConfig.enabled = true;
     }
 
@@ -242,7 +301,6 @@ const DoHConfigController = {
 
     let regionalProviders = parseProviderList(localConfig.providers);
     if (regionalProviders?.length) {
-      delete newConfig.providerList;
       newConfig.providerList = regionalProviders;
     }
 
@@ -252,10 +310,7 @@ const DoHConfigController = {
         p => p.canonicalName?.length
       );
       if (steeringProviders?.length) {
-        delete newConfig.providerSteering.providerList;
         newConfig.providerSteering.providerList = steeringProviders;
-
-        delete newConfig.providerSteering.enabled;
         newConfig.providerSteering.enabled = true;
       }
     }
@@ -265,10 +320,7 @@ const DoHConfigController = {
         localConfig.autoDefaultProviders
       );
       if (defaultProviders?.length) {
-        delete newConfig.trrSelection.providerList;
         newConfig.trrSelection.providerList = defaultProviders;
-
-        delete newConfig.trrSelection.enabled;
         newConfig.trrSelection.enabled = true;
       }
     }

browser/components/doh/test/browser/browser_providerSteering.js

@@ -29,10 +29,13 @@ add_task(async function testProviderSteering() {
       uri: "https://bar.provider2.com/query",
     },
   ];
+  let configFlushPromise = DoHTestUtils.waitForConfigFlush();
   Preferences.set(
     prefs.PROVIDER_STEERING_LIST_PREF,
     JSON.stringify(providerTestcases)
   );
+  await configFlushPromise;
+  await checkHeuristicsTelemetry("enable_doh", "startup");
 
   let testNetChangeResult = async (
     expectedURI,

browser/components/doh/test/browser/browser_remoteSettings_newProfile.js

@@ -7,6 +7,12 @@
 add_task(setup);
 add_task(setupRegion);
 
+async function setPrefAndWaitForConfigFlush(pref, value) {
+  let configFlushedPromise = DoHTestUtils.waitForConfigFlush();
+  Preferences.set(pref, value);
+  await configFlushedPromise;
+}
+
 add_task(async function testNewProfile() {
   is(
     DoHConfigController.currentConfig.enabled,
@@ -52,6 +58,7 @@ add_task(async function testNewProfile() {
     "Rollout should be enabled"
   );
   await ensureTRRMode(2);
+  await checkHeuristicsTelemetry("enable_doh", "startup");
   Assert.deepEqual(
     DoHConfigController.currentConfig.providerList,
     [provider1, provider3],
@@ -83,6 +90,38 @@ add_task(async function testNewProfile() {
     "Fallback provider URI should be that of the first one"
   );
 
+  // Test that overriding with prefs works.
+  await setPrefAndWaitForConfigFlush(prefs.PROVIDER_STEERING_PREF, false);
+  is(
+    DoHConfigController.currentConfig.providerSteering.enabled,
+    false,
+    "Provider steering should be disabled"
+  );
+  await ensureTRRMode(2);
+  await checkHeuristicsTelemetry("enable_doh", "startup");
+
+  await setPrefAndWaitForConfigFlush(prefs.TRR_SELECT_ENABLED_PREF, false);
+  is(
+    DoHConfigController.currentConfig.trrSelection.enabled,
+    false,
+    "TRR selection should be disabled"
+  );
+  await ensureTRRMode(2);
+  await checkHeuristicsTelemetry("enable_doh", "startup");
+
+  // Try a regional pref this time
+  await setPrefAndWaitForConfigFlush(
+    `${kRegionalPrefNamespace}.enabled`,
+    false
+  );
+  is(
+    DoHConfigController.currentConfig.enabled,
+    false,
+    "Rollout should be disabled"
+  );
+  await ensureTRRMode(undefined);
+  await ensureNoHeuristicsTelemetry();
+
   await DoHTestUtils.resetRemoteSettingsConfig();
 
   is(

browser/components/doh/test/browser/browser_trrSelect.js

@@ -4,6 +4,22 @@
 
 "use strict";
 
+async function waitForStartup() {
+  await ensureTRRMode(2);
+  await checkHeuristicsTelemetry("enable_doh", "startup");
+}
+
+async function setPrefAndWaitForConfigFlush(pref, value) {
+  let configFlushed = DoHTestUtils.waitForConfigFlush();
+  if (value) {
+    Preferences.set(pref, value);
+  } else {
+    Preferences.reset(pref);
+  }
+  await configFlushed;
+  await waitForStartup();
+}
+
 add_task(setup);
 
 add_task(async function testTRRSelect() {
@@ -26,25 +42,22 @@ add_task(async function testTRRSelect() {
   // Reset and restart the controller for good measure.
   Preferences.reset(prefs.TRR_SELECT_DRY_RUN_RESULT_PREF);
   Preferences.reset(prefs.TRR_SELECT_URI_PREF);
-
-  prefPromise = TestUtils.waitForPrefChange(prefs.TRR_SELECT_URI_PREF);
   await restartDoHController();
-  await prefPromise;
+  await waitForStartup();
+
   is(
     Preferences.get(prefs.TRR_SELECT_URI_PREF),
     "https://example.com/dns-query",
     "TRR selection complete."
   );
 
-  // Wait for heuristics to complete.
-  await ensureTRRMode(2);
-  await checkHeuristicsTelemetry("enable_doh", "startup");
-
-  // Disable committing and reset. The committed URI should be reset to the
+  // Disable committing. The committed URI should be reset to the
   // default provider and the dry-run-result should persist.
-  Preferences.set(prefs.TRR_SELECT_COMMIT_PREF, false);
-  prefPromise = TestUtils.waitForPrefChange(prefs.TRR_SELECT_URI_PREF);
-  await restartDoHController();
+  prefPromise = TestUtils.waitForPrefChange(
+    prefs.TRR_SELECT_URI_PREF,
+    newVal => newVal == "https://example.com/1"
+  );
+  await setPrefAndWaitForConfigFlush(prefs.TRR_SELECT_COMMIT_PREF, false);
   await prefPromise;
   is(
     Preferences.get(prefs.TRR_SELECT_URI_PREF),
@@ -65,15 +78,13 @@ add_task(async function testTRRSelect() {
     "dry-run result has the correct value."
   );
 
-  // Wait for heuristics to complete.
-  await ensureTRRMode(2);
-  await checkHeuristicsTelemetry("enable_doh", "startup");
-
-  // Reset and restart again, dry-run-result should be recorded but not
+  // Reset again, dry-run-result should be recorded but not
   // be committed. Committing is still disabled from above.
   Preferences.reset(prefs.TRR_SELECT_DRY_RUN_RESULT_PREF);
   Preferences.reset(prefs.TRR_SELECT_URI_PREF);
   await restartDoHController();
+  await waitForStartup();
+
   try {
     await BrowserTestUtils.waitForCondition(() => {
       return (
@@ -95,11 +106,6 @@ add_task(async function testTRRSelect() {
     "https://example.com/dns-query",
     "TRR selection complete, dry-run result recorded."
   );
-  Preferences.set(prefs.TRR_SELECT_COMMIT_PREF, true);
-
-  // Wait for heuristics to complete.
-  await ensureTRRMode(2);
-  await checkHeuristicsTelemetry("enable_doh", "startup");
 
   // Reset doh-rollout.uri, and change the dry-run-result to another one on the
   // default list. After init, the existing dry-run-result should be committed.
@@ -112,7 +118,7 @@ add_task(async function testTRRSelect() {
     prefs.TRR_SELECT_URI_PREF,
     newVal => newVal == "https://example.com/2"
   );
-  await restartDoHController();
+  await setPrefAndWaitForConfigFlush(prefs.TRR_SELECT_COMMIT_PREF, true);
   await prefPromise;
   is(
     Preferences.get(prefs.TRR_SELECT_URI_PREF),
@@ -120,18 +126,17 @@ add_task(async function testTRRSelect() {
     "TRR selection complete, existing dry-run-result committed."
   );
 
-  // Wait for heuristics to complete.
-  await ensureTRRMode(2);
-  await checkHeuristicsTelemetry("enable_doh", "startup");
-
   // Reset doh-rollout.uri, and change the dry-run-result to another one NOT on
   // default list. After init, a new TRR should be selected and committed.
+  prefPromise = TestUtils.waitForPrefChange(
+    prefs.TRR_SELECT_URI_PREF,
+    newVal => newVal == "https://example.com/dns-query"
+  );
   Preferences.reset(prefs.TRR_SELECT_URI_PREF);
   Preferences.set(
     prefs.TRR_SELECT_DRY_RUN_RESULT_PREF,
     "https://example.com/4"
   );
-  prefPromise = TestUtils.waitForPrefChange(prefs.TRR_SELECT_URI_PREF);
   await restartDoHController();
   await prefPromise;
   is(
@@ -139,8 +144,4 @@ add_task(async function testTRRSelect() {
     "https://example.com/dns-query",
     "TRR selection complete, existing dry-run-result discarded and refreshed."
   );
-
-  // Wait for heuristics to complete.
-  await ensureTRRMode(2);
-  await checkHeuristicsTelemetry("enable_doh", "startup");
 });

browser/components/doh/test/browser/browser_trrSelection_disable.js

@@ -7,8 +7,12 @@
 add_task(setup);
 
 add_task(async function testTrrSelectionDisable() {
-  // Set up a passing environment and enable DoH.
+  // Turn off TRR Selection.
+  let configFlushed = DoHTestUtils.waitForConfigFlush();
   Preferences.set(prefs.TRR_SELECT_ENABLED_PREF, false);
+  await configFlushed;
+
+  // Set up a passing environment and enable DoH.
   setPassingHeuristics();
   let promise = waitForDoorhanger();
   Preferences.set(prefs.ENABLED_PREF, true);

browser/components/doh/test/browser/head.js

@@ -86,9 +86,10 @@ async function setup() {
   // Avoid non-local connections to the TRR endpoint.
   Preferences.set(prefs.CONFIRMATION_NS_PREF, "skip");
 
-  // Enable trr selection for tests. This is off by default so it can be
-  // controlled via Normandy.
+  // Enable trr selection and provider steeringfor tests. This is off
+  // by default so it can be controlled via Normandy.
   Preferences.set(prefs.TRR_SELECT_ENABLED_PREF, true);
+  Preferences.set(prefs.PROVIDER_STEERING_PREF, true);
 
   // Enable committing the TRR selection. This pref ships false by default so
   // it can be controlled e.g. via Normandy, but for testing let's set enable.
@@ -210,11 +211,11 @@ async function checkHeuristicsTelemetry(
     events = Services.telemetry.snapshotEvents(
       Ci.nsITelemetry.DATASET_PRERELEASE_CHANNELS
     ).parent;
-    return events && events.length;
+    events = events?.filter(
+      e => e[1] == "doh" && e[2] == "evaluate_v2" && e[3] == "heuristics"
+    );
+    return events?.length;
   });
-  events = events.filter(
-    e => e[1] == "doh" && e[2] == "evaluate_v2" && e[3] == "heuristics"
-  );
   is(events.length, 1, "Found the expected heuristics event.");
   is(events[0][4], decision, "The event records the expected decision");
   if (evaluateReason) {