Add browser tests for HTTP auth and DNS stub resolver configuration.

These aren't full integration tests - they don't make sure that the
correct configuration is sent to the network service on start, or that
the configuration is updated as the prefs change. Instead, they just
check the code that maps prefs to NetworkService configuration structs.

Bug: 881168
Change-Id: I923d8536686ca3a10083c1b8ba0b76c5703e1f9d
Reviewed-on: https://chromium-review.googlesource.com/1210365Reviewed-by: Asanka Herath <asanka@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590481}

chrome/browser/net/system_network_context_manager.cc

@@ -5,10 +5,8 @@
 #include "chrome/browser/net/system_network_context_manager.h"
 
 #include <set>
-#include <string>
 #include <unordered_map>
 #include <utility>
-#include <vector>
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
@@ -97,31 +95,38 @@ void GetStubResolverConfig(
       local_state->GetList(prefs::kDnsOverHttpsServers)->GetList();
   const auto& doh_server_method_list =
       local_state->GetList(prefs::kDnsOverHttpsServerMethods)->GetList();
-  DCHECK_EQ(doh_server_list.size(), doh_server_method_list.size());
 
-  for (size_t i = 0;
-       i < doh_server_list.size() && i < doh_server_method_list.size(); ++i) {
-    if (!doh_server_list[i].is_string() ||
-        !doh_server_method_list[i].is_string()) {
-      continue;
+  // The two lists may have mismatched lengths during a pref change. Just skip
+  // the DOH server list updates then, as the configuration may be incorrect.
+  //
+  // TODO(mmenke): May need to improve safety here when / if DNS over HTTPS is
+  // exposed via settings, as it's possible for the old and new lengths to be
+  // the same, but the servers not to match. Either a PostTask or merging the
+  // prefs would work around that.
+  if (doh_server_list.size() == doh_server_method_list.size()) {
+    for (size_t i = 0; i < doh_server_list.size(); ++i) {
+      if (!doh_server_list[i].is_string() ||
+          !doh_server_method_list[i].is_string()) {
+        continue;
+      }
+
+      if (!net::IsValidDoHTemplate(doh_server_list[i].GetString(),
+                                   doh_server_method_list[i].GetString())) {
+        continue;
+      }
+
+      if (!dns_over_https_servers->has_value()) {
+        *dns_over_https_servers = base::make_optional<
+            std::vector<network::mojom::DnsOverHttpsServerPtr>>();
+      }
+
+      network::mojom::DnsOverHttpsServerPtr dns_over_https_server =
+          network::mojom::DnsOverHttpsServer::New();
+      dns_over_https_server->server_template = doh_server_list[i].GetString();
+      dns_over_https_server->use_post =
+          (doh_server_method_list[i].GetString() == "POST");
+      (*dns_over_https_servers)->emplace_back(std::move(dns_over_https_server));
     }
-
-    if (!net::IsValidDoHTemplate(doh_server_list[i].GetString(),
-                                 doh_server_method_list[i].GetString())) {
-      continue;
-    }
-
-    if (!dns_over_https_servers->has_value()) {
-      *dns_over_https_servers = base::make_optional<
-          std::vector<network::mojom::DnsOverHttpsServerPtr>>();
-    }
-
-    network::mojom::DnsOverHttpsServerPtr dns_over_https_server =
-        network::mojom::DnsOverHttpsServer::New();
-    dns_over_https_server->server_template = doh_server_list[i].GetString();
-    dns_over_https_server->use_post =
-        (doh_server_method_list[i].GetString() == "POST");
-    (*dns_over_https_servers)->emplace_back(std::move(dns_over_https_server));
   }
 
   *stub_resolver_enabled =
@@ -624,6 +629,23 @@ void SystemNetworkContextManager::FlushNetworkInterfaceForTesting() {
     url_loader_factory_.FlushForTesting();
 }
 
+void SystemNetworkContextManager::GetStubResolverConfigForTesting(
+    bool* stub_resolver_enabled,
+    base::Optional<std::vector<network::mojom::DnsOverHttpsServerPtr>>*
+        dns_over_https_servers) {
+  GetStubResolverConfig(stub_resolver_enabled, dns_over_https_servers);
+}
+
+network::mojom::HttpAuthStaticParamsPtr
+SystemNetworkContextManager::GetHttpAuthStaticParamsForTesting() {
+  return CreateHttpAuthStaticParams();
+}
+
+network::mojom::HttpAuthDynamicParamsPtr
+SystemNetworkContextManager::GetHttpAuthDynamicParamsForTesting() {
+  return CreateHttpAuthDynamicParams();
+}
+
 network::mojom::NetworkContextParamsPtr
 SystemNetworkContextManager::CreateNetworkContextParams() {
   // TODO(mmenke): Set up parameters here (in memory cookie store, etc).

chrome/browser/net/system_network_context_manager.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
@@ -124,6 +125,17 @@ class SystemNetworkContextManager {
   // use only.
   void FlushNetworkInterfaceForTesting();
 
+  // Returns configuration that would be sent to the stub DNS resolver.
+  static void GetStubResolverConfigForTesting(
+      bool* stub_resolver_enabled,
+      base::Optional<std::vector<network::mojom::DnsOverHttpsServerPtr>>*
+          dns_over_https_servers);
+
+  static network::mojom::HttpAuthStaticParamsPtr
+  GetHttpAuthStaticParamsForTesting();
+  static network::mojom::HttpAuthDynamicParamsPtr
+  GetHttpAuthDynamicParamsForTesting();
+
  private:
   class URLLoaderFactoryForSystem;
 

chrome/browser/net/system_network_context_manager_browsertest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/net/system_network_context_manager.h"
+
+#include <string>
+#include <vector>
+
+#include "base/feature_list.h"
+#include "base/optional.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/values.h"
+#include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/prefs/pref_service.h"
+#include "services/network/public/mojom/network_context.mojom.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+void GetStubResolverConfig(
+    bool* stub_resolver_enabled,
+    base::Optional<std::vector<network::mojom::DnsOverHttpsServerPtr>>*
+        dns_over_https_servers) {
+  dns_over_https_servers->reset();
+
+  SystemNetworkContextManager::GetStubResolverConfigForTesting(
+      stub_resolver_enabled, dns_over_https_servers);
+}
+
+// Checks the values returned by GetStubResolverConfigForTesting() match
+// |async_dns_feature_enabled| (With empty DNS over HTTPS prefs). Then sets
+// various DNS over HTTPS servers, and makes sure the settings are respected.
+void RunStubResolverConfigTests(bool async_dns_feature_enabled) {
+  // Check initial state.
+  bool stub_resolver_enabled = !async_dns_feature_enabled;
+  base::Optional<std::vector<network::mojom::DnsOverHttpsServerPtr>>
+      dns_over_https_servers;
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(async_dns_feature_enabled, stub_resolver_enabled);
+  EXPECT_FALSE(dns_over_https_servers.has_value());
+
+  // Check state after setting various DNS over HTTPS preferences.
+
+  // The POST template is only valid for POSTs, though the GET template is
+  // technically valid for both POSTs and GETs.
+  const char kGoodPostTemplate[] = "https://foo.test/";
+  const char kGoodGetTemplate[] = "https://bar.test/dns-query{?dns}";
+  const char kBadTemplate[] = "dns-query{?dns}";
+
+  const char kPost[] = "POST";
+  // The code actually looks for POST and not-POST, but may as well use "GET"
+  // for not-POST.
+  const char kGet[] = "GET";
+
+  PrefService* local_state = g_browser_process->local_state();
+  base::Value servers(base::Value::Type::LIST);
+  base::Value methods(base::Value::Type::LIST);
+
+  // Test cases with server and method length mismatches. This shouldn't happen
+  // at steady state, but can happen during pref changes.
+
+  servers.GetList().push_back(base::Value(kGoodGetTemplate));
+  local_state->Set(prefs::kDnsOverHttpsServers, servers);
+  local_state->Set(prefs::kDnsOverHttpsServerMethods, methods);
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(async_dns_feature_enabled, stub_resolver_enabled);
+  EXPECT_FALSE(dns_over_https_servers.has_value());
+  servers.GetList().clear();
+  methods.GetList().clear();
+
+  methods.GetList().push_back(base::Value(kPost));
+  local_state->Set(prefs::kDnsOverHttpsServers, servers);
+  local_state->Set(prefs::kDnsOverHttpsServerMethods, methods);
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(async_dns_feature_enabled, stub_resolver_enabled);
+  EXPECT_FALSE(dns_over_https_servers.has_value());
+  servers.GetList().clear();
+  methods.GetList().clear();
+
+  // Test case with incorrect server type.
+  servers.GetList().push_back(base::Value(15));
+  methods.GetList().push_back(base::Value(kPost));
+  local_state->Set(prefs::kDnsOverHttpsServers, servers);
+  local_state->Set(prefs::kDnsOverHttpsServerMethods, methods);
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(async_dns_feature_enabled, stub_resolver_enabled);
+  EXPECT_FALSE(dns_over_https_servers.has_value());
+  servers.GetList().clear();
+  methods.GetList().clear();
+
+  // Test case with incorrect method type.
+  servers.GetList().push_back(base::Value(kGoodGetTemplate));
+  methods.GetList().push_back(base::Value(3.14));
+  local_state->Set(prefs::kDnsOverHttpsServers, servers);
+  local_state->Set(prefs::kDnsOverHttpsServerMethods, methods);
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(async_dns_feature_enabled, stub_resolver_enabled);
+  EXPECT_FALSE(dns_over_https_servers.has_value());
+  servers.GetList().clear();
+  methods.GetList().clear();
+
+  // Test case with one bad template.
+  servers.GetList().push_back(base::Value(kBadTemplate));
+  methods.GetList().push_back(base::Value(kPost));
+  local_state->Set(prefs::kDnsOverHttpsServers, servers);
+  local_state->Set(prefs::kDnsOverHttpsServerMethods, methods);
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(async_dns_feature_enabled, stub_resolver_enabled);
+  EXPECT_FALSE(dns_over_https_servers.has_value());
+  servers.GetList().clear();
+  methods.GetList().clear();
+
+  // Test case with one good template.
+  servers.GetList().push_back(base::Value(kGoodPostTemplate));
+  methods.GetList().push_back(base::Value(kPost));
+  local_state->Set(prefs::kDnsOverHttpsServers, servers);
+  local_state->Set(prefs::kDnsOverHttpsServerMethods, methods);
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(true, stub_resolver_enabled);
+  ASSERT_TRUE(dns_over_https_servers.has_value());
+  ASSERT_EQ(1u, dns_over_https_servers->size());
+  EXPECT_EQ(kGoodPostTemplate, dns_over_https_servers->at(0)->server_template);
+  EXPECT_EQ(true, dns_over_https_servers->at(0)->use_post);
+  servers.GetList().clear();
+  methods.GetList().clear();
+
+  // Test case with one good template, one bad one.
+  servers.GetList().push_back(base::Value(kGoodGetTemplate));
+  methods.GetList().push_back(base::Value(kGet));
+  servers.GetList().push_back(base::Value(kBadTemplate));
+  methods.GetList().push_back(base::Value(kPost));
+  local_state->Set(prefs::kDnsOverHttpsServers, servers);
+  local_state->Set(prefs::kDnsOverHttpsServerMethods, methods);
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(true, stub_resolver_enabled);
+  ASSERT_TRUE(dns_over_https_servers.has_value());
+  ASSERT_EQ(1u, dns_over_https_servers->size());
+  EXPECT_EQ(kGoodGetTemplate, dns_over_https_servers->at(0)->server_template);
+  EXPECT_EQ(false, dns_over_https_servers->at(0)->use_post);
+  servers.GetList().clear();
+  methods.GetList().clear();
+
+  // Test case with two good templates.
+  servers.GetList().push_back(base::Value(kGoodPostTemplate));
+  methods.GetList().push_back(base::Value(kPost));
+  servers.GetList().push_back(base::Value(kGoodGetTemplate));
+  methods.GetList().push_back(base::Value(kGet));
+  local_state->Set(prefs::kDnsOverHttpsServers, servers);
+  local_state->Set(prefs::kDnsOverHttpsServerMethods, methods);
+  GetStubResolverConfig(&stub_resolver_enabled, &dns_over_https_servers);
+  EXPECT_EQ(true, stub_resolver_enabled);
+  ASSERT_TRUE(dns_over_https_servers.has_value());
+  ASSERT_EQ(2u, dns_over_https_servers->size());
+  EXPECT_EQ(kGoodPostTemplate, dns_over_https_servers->at(0)->server_template);
+  EXPECT_EQ(true, dns_over_https_servers->at(0)->use_post);
+  EXPECT_EQ(kGoodGetTemplate, dns_over_https_servers->at(1)->server_template);
+  EXPECT_EQ(false, dns_over_https_servers->at(1)->use_post);
+  servers.GetList().clear();
+  methods.GetList().clear();
+}
+
+}  // namespace
+
+using SystemNetworkContextManagerBrowsertest = InProcessBrowserTest;
+
+IN_PROC_BROWSER_TEST_F(SystemNetworkContextManagerBrowsertest,
+                       StubResolverDefaultConfig) {
+  RunStubResolverConfigTests(base::FeatureList::IsEnabled(features::kAsyncDns));
+}
+
+IN_PROC_BROWSER_TEST_F(SystemNetworkContextManagerBrowsertest,
+                       StaticAuthParams) {
+  // Test defaults.
+  network::mojom::HttpAuthStaticParamsPtr static_params =
+      SystemNetworkContextManager::GetHttpAuthStaticParamsForTesting();
+  EXPECT_THAT(static_params->supported_schemes,
+              testing::ElementsAre("basic", "digest", "ntlm", "negotiate"));
+  EXPECT_EQ("", static_params->gssapi_library_name);
+
+  // Test that prefs are reflected in params.
+
+  PrefService* local_state = g_browser_process->local_state();
+
+  local_state->SetString(prefs::kAuthSchemes, "basic");
+  static_params =
+      SystemNetworkContextManager::GetHttpAuthStaticParamsForTesting();
+  EXPECT_THAT(static_params->supported_schemes, testing::ElementsAre("basic"));
+
+#if defined(OS_POSIX) && !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
+  const char dev_null[] = "/dev/null";
+  local_state->SetString(prefs::kGSSAPILibraryName, dev_null);
+  static_params =
+      SystemNetworkContextManager::GetHttpAuthStaticParamsForTesting();
+  EXPECT_EQ(dev_null, static_params->gssapi_library_name);
+#endif
+}
+
+IN_PROC_BROWSER_TEST_F(SystemNetworkContextManagerBrowsertest, AuthParams) {
+  // Test defaults.
+  network::mojom::HttpAuthDynamicParamsPtr dynamic_params =
+      SystemNetworkContextManager::GetHttpAuthDynamicParamsForTesting();
+  EXPECT_EQ(false, dynamic_params->negotiate_disable_cname_lookup);
+  EXPECT_EQ(false, dynamic_params->enable_negotiate_port);
+  EXPECT_EQ("", dynamic_params->server_whitelist);
+  EXPECT_EQ("", dynamic_params->delegate_whitelist);
+
+  PrefService* local_state = g_browser_process->local_state();
+
+  local_state->SetBoolean(prefs::kDisableAuthNegotiateCnameLookup, true);
+  dynamic_params =
+      SystemNetworkContextManager::GetHttpAuthDynamicParamsForTesting();
+  EXPECT_EQ(true, dynamic_params->negotiate_disable_cname_lookup);
+  EXPECT_EQ(false, dynamic_params->enable_negotiate_port);
+  EXPECT_EQ("", dynamic_params->server_whitelist);
+  EXPECT_EQ("", dynamic_params->delegate_whitelist);
+
+  local_state->SetBoolean(prefs::kEnableAuthNegotiatePort, true);
+  dynamic_params =
+      SystemNetworkContextManager::GetHttpAuthDynamicParamsForTesting();
+  EXPECT_EQ(true, dynamic_params->negotiate_disable_cname_lookup);
+  EXPECT_EQ(true, dynamic_params->enable_negotiate_port);
+  EXPECT_EQ("", dynamic_params->server_whitelist);
+  EXPECT_EQ("", dynamic_params->delegate_whitelist);
+
+  const char kServerWhiteList[] = "foo";
+  local_state->SetString(prefs::kAuthServerWhitelist, kServerWhiteList);
+  dynamic_params =
+      SystemNetworkContextManager::GetHttpAuthDynamicParamsForTesting();
+  EXPECT_EQ(true, dynamic_params->negotiate_disable_cname_lookup);
+  EXPECT_EQ(true, dynamic_params->enable_negotiate_port);
+  EXPECT_EQ(kServerWhiteList, dynamic_params->server_whitelist);
+  EXPECT_EQ("", dynamic_params->delegate_whitelist);
+
+  const char kDelegateWhiteList[] = "bar, baz";
+  local_state->SetString(prefs::kAuthNegotiateDelegateWhitelist,
+                         kDelegateWhiteList);
+  dynamic_params =
+      SystemNetworkContextManager::GetHttpAuthDynamicParamsForTesting();
+  EXPECT_EQ(true, dynamic_params->negotiate_disable_cname_lookup);
+  EXPECT_EQ(true, dynamic_params->enable_negotiate_port);
+  EXPECT_EQ(kServerWhiteList, dynamic_params->server_whitelist);
+  EXPECT_EQ(kDelegateWhiteList, dynamic_params->delegate_whitelist);
+}
+
+class SystemNetworkContextManagerStubResolverBrowsertest
+    : public SystemNetworkContextManagerBrowsertest,
+      public testing::WithParamInterface<bool> {
+ public:
+  SystemNetworkContextManagerStubResolverBrowsertest() {
+    scoped_feature_list_.InitWithFeatureState(features::kAsyncDns, GetParam());
+  }
+  ~SystemNetworkContextManagerStubResolverBrowsertest() override {}
+
+  void SetUpOnMainThread() override {}
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_P(SystemNetworkContextManagerStubResolverBrowsertest,
+                       StubResolverConfig) {
+  RunStubResolverConfigTests(GetParam());
+}
+
+INSTANTIATE_TEST_CASE_P(,
+                        SystemNetworkContextManagerStubResolverBrowsertest,
+                        ::testing::Values(false, true));

chrome/test/BUILD.gn

@@ -673,6 +673,7 @@ test("browser_tests") {
       "../browser/net/profile_network_context_service_browsertest.cc",
       "../browser/net/proxy_browsertest.cc",
       "../browser/net/reporting_browsertest.cc",
+      "../browser/net/system_network_context_manager_browsertest.cc",
       "../browser/net/variations_http_headers_browsertest.cc",
       "../browser/net/websocket_browsertest.cc",
       "../browser/no_background_tasks_browsertest.cc",