Bug 1713036 - Use Remote Settings config in DoH preferences UI. r=jaws,preferences-reviewers,necko-reviewers,valentin

Depends on D116798 Differential Revision: https://phabricator.services.mozilla.com/D117960
2021-06-23 02:29:19 +00:00 · 2021-06-23 02:29:19 +00:00 · 44665bb834
--- a/browser/components/preferences/dialogs/connection.js
+++ b/browser/components/preferences/dialogs/connection.js
@ -7,6 +7,12 @@
 /* import-globals-from ../../../../toolkit/content/preferencesBindings.js */
 /* import-globals-from ../extensionControlled.js */

+ChromeUtils.defineModuleGetter(
+  this,
+  "DoHConfigController",
+  "resource:///modules/DoHConfig.jsm"
+);
+
 document
  .getElementById("ConnectionsDialog")
  .addEventListener("dialoghelp", window.top.openPrefsHelp);
@ -33,13 +39,16 @@ Preferences.addAll([
  { id: "network.proxy.backup.ssl_port", type: "int" },
  { id: "network.trr.mode", type: "int" },
  { id: "network.trr.uri", type: "string" },
-  { id: "network.trr.resolvers", type: "string" },
  { id: "network.trr.custom_uri", type: "string" },
  { id: "doh-rollout.enabled", type: "bool" },
  { id: "doh-rollout.disable-heuristics", type: "bool" },
  { id: "doh-rollout.skipHeuristicsCheck", type: "bool" },
 ]);

+const DoHConfigObserver = () => {
+  gConnectionsDialog.initDnsOverHttpsUI();
+};
+
 window.addEventListener(
  "DOMContentLoaded",
  () => {
@ -56,9 +65,10 @@ window.addEventListener(
      gConnectionsDialog.updateDnsOverHttpsUI();
    });

-    Preferences.get("network.trr.resolvers").on("change", () => {
-      gConnectionsDialog.initDnsOverHttpsUI();
-    });
+    Services.obs.addObserver(
+      DoHConfigObserver,
+      DoHConfigController.kConfigUpdateTopic
+    );

    // XXX: We can't init the DNS-over-HTTPs UI until the onsyncfrompreference for network.trr.mode
    //      has been called. The uiReady promise will be resolved after the first call to
@ -91,6 +101,14 @@ window.addEventListener(
      .addEventListener("beforeaccept", e =>
        gConnectionsDialog.beforeAccept(e)
      );
+    document
+      .getElementById("ConnectionsDialog")
+      .addEventListener("dialogclosing", e => {
+        Services.obs.removeObserver(
+          DoHConfigObserver,
+          DoHConfigController.kConfigUpdateTopic
+        );
+      });
  },
  { once: true, capture: true }
 );
@ -107,7 +125,10 @@ var gConnectionsDialog = {
      if (customValue) {
        Services.prefs.setStringPref("network.trr.uri", customValue);
      } else {
-        Services.prefs.clearUserPref("network.trr.uri");
+        Services.prefs.setStringPref(
+          "network.trr.uri",
+          DoHConfigController.currentConfig.fallbackProviderURI
+        );
      }
    } else {
      Services.prefs.setStringPref(
@ -399,30 +420,14 @@ var gConnectionsDialog = {
  },

  get dnsOverHttpsResolvers() {
-    let rawValue = Preferences.get("network.trr.resolvers", "").value;
+    let providers = DoHConfigController.currentConfig.providerList;
    // if there's no default, we'll hold its position with an empty string
-    let defaultURI = Preferences.get("network.trr.uri", "").defaultValue;
-    let providers = [];
-    if (rawValue) {
-      try {
-        providers = JSON.parse(rawValue);
-      } catch (ex) {
-        Cu.reportError(
-          `Bad JSON data in pref network.trr.resolvers: ${rawValue}`
-        );
-      }
-    }
-    if (!Array.isArray(providers)) {
-      Cu.reportError(
-        `Expected a JSON array in network.trr.resolvers: ${rawValue}`
-      );
-      providers = [];
-    }
-    let defaultIndex = providers.findIndex(p => p.url == defaultURI);
+    let defaultURI = DoHConfigController.currentConfig.fallbackProviderURI;
+    let defaultIndex = providers.findIndex(p => p.uri == defaultURI);
    if (defaultIndex == -1 && defaultURI) {
      // the default value for the pref isn't included in the resolvers list
      // so we'll make a stub for it. Without an id, we'll have to use the url as the label
-      providers.unshift({ url: defaultURI });
+      providers.unshift({ uri: defaultURI });
    }
    return providers;
  },
@ -491,7 +496,7 @@ var gConnectionsDialog = {
        if (
          currentURI &&
          !customURI &&
-          !resolvers.find(r => r.url == currentURI)
+          !resolvers.find(r => r.uri == currentURI)
        ) {
          Services.prefs.setStringPref("network.trr.custom_uri", currentURI);
        }
@ -536,24 +541,24 @@ var gConnectionsDialog = {

  initDnsOverHttpsUI() {
    let resolvers = this.dnsOverHttpsResolvers;
-    let defaultURI = Preferences.get("network.trr.uri").defaultValue;
+    let defaultURI = DoHConfigController.currentConfig.fallbackProviderURI;
    let currentURI = Preferences.get("network.trr.uri").value;
    let menu = document.getElementById("networkDnsOverHttpsResolverChoices");

    // populate the DNS-Over-HTTPs resolver list
    menu.removeAllItems();
    for (let resolver of resolvers) {
-      let item = menu.appendItem(undefined, resolver.url);
-      if (resolver.url == defaultURI) {
+      let item = menu.appendItem(undefined, resolver.uri);
+      if (resolver.uri == defaultURI) {
        document.l10n.setAttributes(
          item,
          "connection-dns-over-https-url-item-default",
          {
-            name: resolver.name || resolver.url,
+            name: resolver.UIName || resolver.uri,
          }
        );
      } else {
-        item.label = resolver.name || resolver.url;
+        item.label = resolver.UIName || resolver.uri;
      }
    }
    let lastItem = menu.appendItem(undefined, "custom");
@ -564,7 +569,7 @@ var gConnectionsDialog = {

    // set initial selection in the resolver provider picker
    let selectedIndex = currentURI
-      ? resolvers.findIndex(r => r.url == currentURI)
+      ? resolvers.findIndex(r => r.uri == currentURI)
      : 0;
    if (selectedIndex == -1) {
      // select the last "Custom" item
--- a/browser/components/preferences/tests/browser_connection_dnsoverhttps.js
+++ b/browser/components/preferences/tests/browser_connection_dnsoverhttps.js
@ -5,29 +5,34 @@ ChromeUtils.defineModuleGetter(
  "DoHController",
  "resource:///modules/DoHController.jsm"
 );
+ChromeUtils.defineModuleGetter(
+  this,
+  "DoHConfigController",
+  "resource:///modules/DoHConfig.jsm"
+);
+ChromeUtils.defineModuleGetter(
+  this,
+  "DoHTestUtils",
+  "resource://testing-common/DoHTestUtils.jsm"
+);

 const SUBDIALOG_URL =
  "chrome://browser/content/preferences/dialogs/connection.xhtml";
 const TRR_MODE_PREF = "network.trr.mode";
 const TRR_URI_PREF = "network.trr.uri";
-const TRR_RESOLVERS_PREF = "network.trr.resolvers";
 const TRR_CUSTOM_URI_PREF = "network.trr.custom_uri";
 const ROLLOUT_ENABLED_PREF = "doh-rollout.enabled";
 const ROLLOUT_SELF_ENABLED_PREF = "doh-rollout.self-enabled";
 const HEURISTICS_DISABLED_PREF = "doh-rollout.disable-heuristics";
-const DEFAULT_RESOLVER_VALUE = "https://mozilla.cloudflare-dns.com/dns-query";
-const NEXTDNS_RESOLVER_VALUE = "https://firefox.dns.nextdns.io/";
+const FIRST_RESOLVER_VALUE = DoHTestUtils.providers[0].uri;
+const SECOND_RESOLVER_VALUE = DoHTestUtils.providers[1].uri;
+const DEFAULT_RESOLVER_VALUE = FIRST_RESOLVER_VALUE;

 const modeCheckboxSelector = "#networkDnsOverHttps";
 const uriTextboxSelector = "#networkCustomDnsOverHttpsInput";
 const resolverMenulistSelector = "#networkDnsOverHttpsResolverChoices";
 const defaultPrefValues = Object.freeze({
  [TRR_MODE_PREF]: 0,
-  [TRR_URI_PREF]: "https://mozilla.cloudflare-dns.com/dns-query",
-  [TRR_RESOLVERS_PREF]: JSON.stringify([
-    { name: "Cloudflare", url: DEFAULT_RESOLVER_VALUE },
-    { name: "example.org", url: "https://example.org/dns-query" },
-  ]),
  [TRR_CUSTOM_URI_PREF]: "",
 });

@ -35,7 +40,6 @@ async function resetPrefs() {
  await DoHController._uninit();
  Services.prefs.clearUserPref(TRR_MODE_PREF);
  Services.prefs.clearUserPref(TRR_URI_PREF);
-  Services.prefs.clearUserPref(TRR_RESOLVERS_PREF);
  Services.prefs.clearUserPref(TRR_CUSTOM_URI_PREF);
  Services.prefs.getChildList("doh-rollout.").forEach(pref => {
    Services.prefs.clearUserPref(pref);
@ -44,6 +48,7 @@ async function resetPrefs() {
  // confuse tests running after this one that are looking at those.
  Services.telemetry.clearEvents();
  await DoHController.init();
+  await DoHTestUtils.resetRemoteSettingsConfig();
 }
 Services.prefs.setStringPref("network.trr.confirmationNS", "skip");
 let preferencesOpen = new Promise(res => open_preferences(res));
@ -122,10 +127,6 @@ async function testWithProperties(props, startTime) {
  if (props.hasOwnProperty(TRR_URI_PREF)) {
    Services.prefs.setStringPref(TRR_URI_PREF, props[TRR_URI_PREF]);
  }
-  if (props.hasOwnProperty(TRR_RESOLVERS_PREF)) {
-    info(`Setting ${TRR_RESOLVERS_PREF} to ${props[TRR_RESOLVERS_PREF]}`);
-    Services.prefs.setStringPref(TRR_RESOLVERS_PREF, props[TRR_RESOLVERS_PREF]);
-  }

  let dialog = await openConnectionsSubDialog();
  await dialog.uiReady;
@ -263,12 +264,19 @@ async function testWithProperties(props, startTime) {
  info(Date.now() - startTime + ": testWithProperties: prefs changed");

  if (props.hasOwnProperty("expectedFinalUriPref")) {
-    let uriPref = Services.prefs.getStringPref(TRR_URI_PREF);
-    is(
-      uriPref,
-      props.expectedFinalUriPref,
-      "uri pref ended up with the expected value"
-    );
+    if (props.expectedFinalUriPref) {
+      let uriPref = Services.prefs.getStringPref(TRR_URI_PREF);
+      is(
+        uriPref,
+        props.expectedFinalUriPref,
+        "uri pref ended up with the expected value"
+      );
+    } else {
+      ok(
+        !Services.prefs.prefHasUserValue(TRR_URI_PREF),
+        "uri pref ended up with the expected value (unset)"
+      );
+    }
  }

  if (props.hasOwnProperty("expectedModePref")) {
@ -305,7 +313,7 @@ async function testWithProperties(props, startTime) {

 add_task(async function default_values() {
  let customUriPref = Services.prefs.getStringPref(TRR_CUSTOM_URI_PREF);
-  let uriPref = Services.prefs.getStringPref(TRR_URI_PREF);
+  let uriPrefHasUserValue = Services.prefs.prefHasUserValue(TRR_URI_PREF);
  let modePref = Services.prefs.getIntPref(TRR_MODE_PREF);
  is(
    modePref,
@ -313,9 +321,9 @@ add_task(async function default_values() {
    `Actual value of ${TRR_MODE_PREF} matches expected default value`
  );
  is(
-    uriPref,
-    defaultPrefValues[TRR_URI_PREF],
-    `Actual value of ${TRR_URI_PREF} matches expected default value`
+    uriPrefHasUserValue,
+    false,
+    `Actual value of ${TRR_URI_PREF} matches expected default value (unset)`
  );
  is(
    customUriPref,
@ -416,14 +424,14 @@ let testVariations = [
  {
    name: "Select NextDNS as TRR provider",
    [TRR_MODE_PREF]: 2,
-    selectResolver: NEXTDNS_RESOLVER_VALUE,
-    expectedFinalUriPref: NEXTDNS_RESOLVER_VALUE,
+    selectResolver: SECOND_RESOLVER_VALUE,
+    expectedFinalUriPref: SECOND_RESOLVER_VALUE,
  },
  {
    name: "return to default from NextDNS",
    [TRR_MODE_PREF]: 2,
-    [TRR_URI_PREF]: NEXTDNS_RESOLVER_VALUE,
-    expectedResolverListValue: NEXTDNS_RESOLVER_VALUE,
+    [TRR_URI_PREF]: SECOND_RESOLVER_VALUE,
+    expectedResolverListValue: SECOND_RESOLVER_VALUE,
    selectResolver: DEFAULT_RESOLVER_VALUE,
    expectedFinalUriPref: DEFAULT_RESOLVER_VALUE,
  },
@ -474,11 +482,9 @@ let testVariations = [
  },
  {
    name: "empty default resolver list",
-    [TRR_RESOLVERS_PREF]: "",
    [TRR_MODE_PREF]: 2,
    [TRR_URI_PREF]: "https://example.com",
    [TRR_CUSTOM_URI_PREF]: "",
-    [TRR_RESOLVERS_PREF]: "",
    expectedUriValue: "https://example.com",
    expectedResolverListValue: "custom",
    expectedFinalUriPref: "https://example.com",
--- a/modules/libpref/init/StaticPrefList.yaml
+++ b/modules/libpref/init/StaticPrefList.yaml
@ -9400,6 +9400,12 @@
  value: 1048576
  mirror: always

+# Default global TRR provider
+- name: network.trr.default_provider_uri
+  type: String
+  value: "https://mozilla.cloudflare-dns.com/dns-query"
+  mirror: never
+
 # Single TRR request timeout, in milliseconds
 - name: network.trr.request_timeout_ms
  type: RelaxedAtomicUint32
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@ -3929,11 +3929,7 @@ pref("network.connectivity-service.IPv6.url", "http://detectportal.firefox.com/s
 // DNS Trusted Recursive Resolver
 // 0 - default off, 1 - reserved/off, 2 - TRR first, 3 - TRR only, 4 - reserved/off, 5 off by choice
 pref("network.trr.mode", 0);
-// DNS-over-HTTP service to use, must be HTTPS://
-pref("network.trr.uri", "https://mozilla.cloudflare-dns.com/dns-query");
-// List of DNS-over-HTTP resolver service providers. This pref populates the
-// drop-down list in the Network Settings dialog box in about:preferences.
-pref("network.trr.resolvers", "[{ \"name\": \"Cloudflare\", \"url\": \"https://mozilla.cloudflare-dns.com/dns-query\" },{ \"name\": \"NextDNS\", \"url\": \"https://firefox.dns.nextdns.io/\" }]");
+pref("network.trr.uri", "");
 // credentials to pass to DOH end-point
 pref("network.trr.credentials", "");
 pref("network.trr.custom_uri", "");
--- a/netwerk/dns/TRRService.cpp
+++ b/netwerk/dns/TRRService.cpp
@ -372,6 +372,7 @@ nsresult TRRService::ReadPrefs(const char* name) {
    }
  }
  if (!name || !strcmp(name, TRR_PREF("uri")) ||
+      !strcmp(name, TRR_PREF("default_provider_uri")) ||
      !strcmp(name, kRolloutURIPref)) {
    OnTRRURIChange();
  }
--- a/netwerk/dns/TRRServiceBase.cpp
+++ b/netwerk/dns/TRRServiceBase.cpp
@ -13,6 +13,7 @@
 #include "nsIDNSService.h"
 // Put DNSLogging.h at the end to avoid LOG being overwritten by other headers.
 #include "DNSLogging.h"
+#include "mozilla/StaticPrefs_network.h"

 namespace mozilla {
 namespace net {
@ -85,7 +86,7 @@ void TRRServiceBase::CheckURIPrefs() {
  }

  // Otherwise just use the default value.
-  MaybeSetPrivateURI(mURIPref);
+  MaybeSetPrivateURI(mDefaultURIPref);
 }

 // static
@ -144,6 +145,7 @@ void TRRServiceBase::OnTRRURIChange() {
  mURIPrefHasUserValue = Preferences::HasUserValue("network.trr.uri");
  Preferences::GetCString("network.trr.uri", mURIPref);
  Preferences::GetCString(kRolloutURIPref, mRolloutURIPref);
+  Preferences::GetCString("network.trr.default_provider_uri", mDefaultURIPref);

  CheckURIPrefs();
 }
--- a/netwerk/dns/TRRServiceBase.h
+++ b/netwerk/dns/TRRServiceBase.h
@ -44,6 +44,7 @@ class TRRServiceBase {
  bool mURIPrefHasUserValue = false;
  nsCString mURIPref;
  nsCString mRolloutURIPref;
+  nsCString mDefaultURIPref;

  Atomic<nsIDNSService::ResolverMode, Relaxed> mMode;
  Atomic<bool, Relaxed> mURISetByDetection;
--- a/netwerk/test/unit/test_trr.js
+++ b/netwerk/test/unit/test_trr.js
@ -534,9 +534,9 @@ add_task(async function test_detected_uri() {
  dns.clearCache(true);
  Services.prefs.setIntPref("network.trr.mode", 3);
  Services.prefs.clearUserPref("network.trr.uri");
-  let defaultURI = gDefaultPref.getCharPref("network.trr.uri");
+  let defaultURI = gDefaultPref.getCharPref("network.trr.default_provider_uri");
  gDefaultPref.setCharPref(
-    "network.trr.uri",
+    "network.trr.default_provider_uri",
    `https://foo.example.com:${h2Port}/doh?responseIP=3.4.5.6`
  );
  await new TRRDNSListener("domainA.example.org.", "3.4.5.6");
@ -544,7 +544,7 @@ add_task(async function test_detected_uri() {
    `https://foo.example.com:${h2Port}/doh?responseIP=1.2.3.4`
  );
  await new TRRDNSListener("domainB.example.org.", "1.2.3.4");
-  gDefaultPref.setCharPref("network.trr.uri", defaultURI);
+  gDefaultPref.setCharPref("network.trr.default_provider_uri", defaultURI);

  // With a user-set doh uri this time.
  dns.clearCache(true);
@ -562,7 +562,7 @@ add_task(async function test_detected_uri() {
  Services.prefs.setIntPref("network.trr.mode", 3);
  Services.prefs.clearUserPref("network.trr.uri");
  gDefaultPref.setCharPref(
-    "network.trr.uri",
+    "network.trr.default_provider_uri",
    `https://foo.example.com:${h2Port}/doh?responseIP=3.4.5.6`
  );
  await new TRRDNSListener("domainA.example.org.", "3.4.5.6");
@ -584,13 +584,13 @@ add_task(async function test_detected_uri() {

  await new TRRDNSListener("domainC.example.org.", "3.4.5.6");

-  gDefaultPref.setCharPref("network.trr.uri", defaultURI);
+  gDefaultPref.setCharPref("network.trr.default_provider_uri", defaultURI);
 });

 add_task(async function test_pref_changes() {
  info("Testing pref change handling");
  Services.prefs.clearUserPref("network.trr.uri");
-  let defaultURI = gDefaultPref.getCharPref("network.trr.uri");
+  let defaultURI = gDefaultPref.getCharPref("network.trr.default_provider_uri");

  async function doThenCheckURI(closure, expectedURI, expectChange = false) {
    let uriChanged;
@ -607,7 +607,7 @@ add_task(async function test_pref_changes() {
  // setting the default value of the pref should be reflected in the URI
  await doThenCheckURI(() => {
    gDefaultPref.setCharPref(
-      "network.trr.uri",
+      "network.trr.default_provider_uri",
      `https://foo.example.com:${h2Port}/doh?default`
    );
  }, `https://foo.example.com:${h2Port}/doh?default`);
@ -690,7 +690,7 @@ add_task(async function test_pref_changes() {
  );

  // Restore the pref
-  gDefaultPref.setCharPref("network.trr.uri", defaultURI);
+  gDefaultPref.setCharPref("network.trr.default_provider_uri", defaultURI);
 });

 add_task(async function test_dohrollout_mode() {