Factor out IsGoogleDomain to google_util::IsGoogleRelevantUrl

AutofillDownloadManager calls variations::ShouldAppendVariationsHeader()
to know if the specified URL is trusted by Google. But this call has a
side effect to record histograms, and results in double counting for
those uses.

This patch factors out Google trusted URL checks into google_util, and
change variations:: and AutofillDownloadManager to call
google_util::IsGoogleRelevantUrl for each slightly different purpose.

Bug: None
Change-Id: I66dc666011e4cc57327e06889adaeb2828ac6232
Tbr: mahmadi@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1451504
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629922}

components/autofill/core/browser/BUILD.gn

@@ -304,6 +304,7 @@ jumbo_static_library("browser") {
     "//base",
     "//base:i18n",
     "//components/data_use_measurement/core",
+    "//components/google/core/common",
     "//components/history/core/browser",
     "//components/infobars/core",
     "//components/keyed_service/core",

components/autofill/core/browser/DEPS

 include_rules = [
   "+components/data_use_measurement/core",
+  "+components/google/core/common/google_util.h",
   "+components/grit/components_scaled_resources.h",
   "+components/history/core/browser",
   "+components/infobars/core",

components/autofill/core/browser/autofill_download_manager.cc

@@ -34,6 +34,7 @@
 #include "components/autofill/core/common/autofill_switches.h"
 #include "components/autofill/core/common/submission_source.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
+#include "components/google/core/common/google_util.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -649,17 +650,10 @@ bool AutofillDownloadManager::StartRequest(FormRequestData request_data) {
     resource_request->headers.SetHeader(kGoogEncodeResponseIfExecutable,
                                         "base64");
 
-  // Put API key in request's header if there is.
-  // Note: variations::AppendVariationsHeaderUnknownsignedIn() returned true
-  // when the vadiations header was added, but variations header is not set if
-  // variations::Incognito::kYes is passed. On the other hand, the API key is
-  // needed even for variations::Incognito::kYes. So, we need to call
-  // variations::ShouldAppendVariationsHeader() below to check the condition
-  // without variations::Incognito value. Probably we want to factor out some
-  // code to know if the URL needs the API key.
-  if (!api_key_.empty() &&
-      variations::ShouldAppendVariationsHeader(request_url)) {
-    // Make sure that we only send the API key to endpoints trusted by Chrome.
+  // Put API key in request's header if a key exists, and the endpoint is
+  // trusted by Google.
+  if (!api_key_.empty() && request_url.SchemeIs(url::kHttpsScheme) &&
+      google_util::IsGoogleAssociatedDomainUrl(request_url)) {
     resource_request->headers.SetHeader(kGoogApiKey, api_key_);
   }
 

components/google/core/common/google_util.cc

@@ -288,6 +288,49 @@ bool IsYoutubeDomainUrl(const GURL& url,
                          nullptr);
 }
 
+bool IsGoogleAssociatedDomainUrl(const GURL& url) {
+  if (IsGoogleDomainUrl(url, ALLOW_SUBDOMAIN, ALLOW_NON_STANDARD_PORTS))
+    return true;
+
+  if (IsYoutubeDomainUrl(url, ALLOW_SUBDOMAIN, ALLOW_NON_STANDARD_PORTS))
+    return true;
+
+  // Some domains don't have international TLD extensions, so testing for them
+  // is very straightforward.
+  static const char* kSuffixesToSetHeadersFor[] = {
+      ".android.com",
+      ".doubleclick.com",
+      ".doubleclick.net",
+      ".ggpht.com",
+      ".googleadservices.com",
+      ".googleapis.com",
+      ".googlesyndication.com",
+      ".googleusercontent.com",
+      ".googlevideo.com",
+      ".gstatic.com",
+      ".litepages.googlezip.net",
+      ".ytimg.com",
+  };
+  const std::string host = url.host();
+  for (size_t i = 0; i < base::size(kSuffixesToSetHeadersFor); ++i) {
+    if (base::EndsWith(host, kSuffixesToSetHeadersFor[i],
+                       base::CompareCase::INSENSITIVE_ASCII)) {
+      return true;
+    }
+  }
+
+  // Exact hostnames in lowercase to set headers for.
+  static const char* kHostsToSetHeadersFor[] = {
+      "googleweblight.com",
+  };
+  for (size_t i = 0; i < base::size(kHostsToSetHeadersFor); ++i) {
+    if (base::LowerCaseEqualsASCII(host, kHostsToSetHeadersFor[i]))
+      return true;
+  }
+
+  return false;
+}
+
 const std::vector<std::string>& GetGoogleRegistrableDomains() {
   static base::NoDestructor<std::vector<std::string>>
       kGoogleRegisterableDomains([]() {

components/google/core/common/google_util.h

@@ -86,9 +86,8 @@ bool IsGoogleHostname(base::StringPiece host,
 // port for its scheme (80 for HTTP, 443 for HTTPS).
 //
 // Note that this only checks for google.<TLD> domains, but not other Google
-// properties. There is code in variations_http_header_provider.cc that checks
-// for additional Google properties, which can be moved here if more callers
-// are interested in this in the future.
+// properties. If you want to check domains including other Google properties,
+// you can use IsGoogleAssociatedDomainUrl() below.
 bool IsGoogleDomainUrl(const GURL& url,
                        SubdomainPermission subdomain_permission,
                        PortPermission port_permission);
@@ -106,6 +105,9 @@ bool IsYoutubeDomainUrl(const GURL& url,
                         SubdomainPermission subdomain_permission,
                         PortPermission port_permission);
 
+// True if |url| is hosted by Google.
+bool IsGoogleAssociatedDomainUrl(const GURL& url);
+
 // Returns the list of all Google's registerable domains, i.e. domains named
 // google.<eTLD> owned by Google.
 // TODO(msramek): This is currently only used to ensure the deletion of Google

components/variations/net/variations_http_headers.cc

@@ -33,26 +33,6 @@ namespace {
 // Note that prior to M33 this header was named X-Chrome-Variations.
 const char kClientDataHeader[] = "X-Client-Data";
 
-const char* kSuffixesToSetHeadersFor[] = {
-    ".android.com",
-    ".doubleclick.com",
-    ".doubleclick.net",
-    ".ggpht.com",
-    ".googleadservices.com",
-    ".googleapis.com",
-    ".googlesyndication.com",
-    ".googleusercontent.com",
-    ".googlevideo.com",
-    ".gstatic.com",
-    ".litepages.googlezip.net",
-    ".ytimg.com",
-};
-
-// Exact hostnames in lowercase to set headers for.
-const char* kHostsToSetHeadersFor[] = {
-    "googleweblight.com",
-};
-
 // The result of checking if a URL should have variations headers appended.
 // This enum is used to record UMA histogram values, and should not be
 // reordered.
@@ -66,37 +46,32 @@ enum URLValidationResult {
   URL_VALIDATION_RESULT_SIZE,
 };
 
-// Checks whether headers should be appended to the |url|, based on the domain
-// of |url|. |url| is assumed to be valid, and to have an http/https scheme.
-bool IsGoogleDomain(const GURL& url) {
-  if (google_util::IsGoogleDomainUrl(url, google_util::ALLOW_SUBDOMAIN,
-                                     google_util::ALLOW_NON_STANDARD_PORTS)) {
-    return true;
+void LogUrlValidationHistogram(URLValidationResult result) {
+  UMA_HISTOGRAM_ENUMERATION("Variations.Headers.URLValidationResult", result,
+                            URL_VALIDATION_RESULT_SIZE);
+}
+
+bool ShouldAppendVariationsHeader(const GURL& url) {
+  if (!url.is_valid()) {
+    LogUrlValidationHistogram(INVALID_URL);
+    return false;
   }
-  if (google_util::IsYoutubeDomainUrl(url, google_util::ALLOW_SUBDOMAIN,
-                                      google_util::ALLOW_NON_STANDARD_PORTS)) {
-    return true;
+  if (!url.SchemeIsHTTPOrHTTPS()) {
+    LogUrlValidationHistogram(NEITHER_HTTP_HTTPS);
+    return false;
   }
-
-  // Some domains don't have international TLD extensions, so testing for them
-  // is very straight forward.
-  const std::string host = url.host();
-  for (size_t i = 0; i < base::size(kSuffixesToSetHeadersFor); ++i) {
-    if (base::EndsWith(host, kSuffixesToSetHeadersFor[i],
-                       base::CompareCase::INSENSITIVE_ASCII))
-      return true;
+  if (!google_util::IsGoogleAssociatedDomainUrl(url)) {
+    LogUrlValidationHistogram(NOT_GOOGLE_DOMAIN);
+    return false;
   }
-  for (size_t i = 0; i < base::size(kHostsToSetHeadersFor); ++i) {
-    if (base::LowerCaseEqualsASCII(host, kHostsToSetHeadersFor[i]))
-      return true;
+  // We check https here, rather than before the IsGoogleDomain() check, to know
+  // how many Google domains are being rejected by the change to https only.
+  if (!url.SchemeIs(url::kHttpsScheme)) {
+    LogUrlValidationHistogram(IS_GOOGLE_NOT_HTTPS);
+    return false;
   }
-
-  return false;
-}
-
-void LogUrlValidationHistogram(URLValidationResult result) {
-  UMA_HISTOGRAM_ENUMERATION("Variations.Headers.URLValidationResult", result,
-                            URL_VALIDATION_RESULT_SIZE);
+  LogUrlValidationHistogram(SHOULD_APPEND);
+  return true;
 }
 
 constexpr network::ResourceRequest* null_resource_request = nullptr;
@@ -257,27 +232,8 @@ bool HasVariationsHeader(const network::ResourceRequest& request) {
   return !request.client_data_header.empty();
 }
 
-bool ShouldAppendVariationsHeader(const GURL& url) {
-  if (!url.is_valid()) {
-    LogUrlValidationHistogram(INVALID_URL);
-    return false;
-  }
-  if (!url.SchemeIsHTTPOrHTTPS()) {
-    LogUrlValidationHistogram(NEITHER_HTTP_HTTPS);
-    return false;
-  }
-  if (!IsGoogleDomain(url)) {
-    LogUrlValidationHistogram(NOT_GOOGLE_DOMAIN);
-    return false;
-  }
-  // We check https here, rather than before the IsGoogleDomain() check, to know
-  // how many Google domains are being rejected by the change to https only.
-  if (!url.SchemeIs("https")) {
-    LogUrlValidationHistogram(IS_GOOGLE_NOT_HTTPS);
-    return false;
-  }
-  LogUrlValidationHistogram(SHOULD_APPEND);
-  return true;
+bool ShouldAppendVariationsHeaderForTesting(const GURL& url) {
+  return ShouldAppendVariationsHeader(url);
 }
 
 }  // namespace variations

components/variations/net/variations_http_headers.h

@@ -109,9 +109,8 @@ bool IsVariationsHeader(const std::string& header_name);
 // Checks if |request| contains the variations header.
 bool HasVariationsHeader(const network::ResourceRequest& request);
 
-// Checks whether variation headers should be appended to requests to the
-// specified |url|. Returns true for Google's hosts served over secure schemes.
-bool ShouldAppendVariationsHeader(const GURL& url);
+// Calls the internal ShouldAppendVariationsHeader() for testing.
+bool ShouldAppendVariationsHeaderForTesting(const GURL& url);
 
 }  // namespace variations
 

components/variations/net/variations_http_headers_unittest.cc

@@ -154,7 +154,8 @@ TEST(VariationsHttpHeadersTest, ShouldAppendVariationsHeader) {
 
   for (size_t i = 0; i < base::size(cases); ++i) {
     const GURL url(cases[i].url);
-    EXPECT_EQ(cases[i].should_append_headers, ShouldAppendVariationsHeader(url))
+    EXPECT_EQ(cases[i].should_append_headers,
+              ShouldAppendVariationsHeaderForTesting(url))
         << url;
   }
 }