Check SafeSearch API in PolicyBlacklistNavigationThrottle

When the SafeSitesFilterBehavior policy is enabled, filter top-level
HTTP[S] navigations using Google's SafeSearch API (porn classifier).

The SafeSites check is skipped if the URL is blacklisted or whitelisted
by policy.

This check is added to the existing PolicyBlacklistNavigationThrottle
instead of creating a standalone navigation throttle because it depends
on the blacklist/whitelist check that's already there, and conceptually
serves a similar purpose (determining if a URL should be blocked by
policy).

Bug: 819405
Change-Id: Ica243fdbf44b06a2fc4148fb68f47a11d9b79a6b
Reviewed-on: https://chromium-review.googlesource.com/1119102
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584681}

chrome/browser/chrome_content_browser_client.cc

@@ -188,6 +188,7 @@
 #include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
 #include "components/payments/content/payment_request_display_manager.h"
 #include "components/policy/content/policy_blacklist_navigation_throttle.h"
+#include "components/policy/content/policy_blacklist_service.h"
 #include "components/policy/core/common/cloud/policy_header_service.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -950,7 +951,7 @@ void LaunchURL(
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   PolicyBlacklistService* service =
-      PolicyBlacklistFactory::GetForProfile(profile);
+      PolicyBlacklistFactory::GetForBrowserContext(profile);
   if (service) {
     const policy::URLBlacklist::URLBlacklistState url_state =
         service->GetURLBlacklistState(url);

components/BUILD.gn

@@ -231,6 +231,7 @@ test("components_unittests") {
       "//components/password_manager/content/browser:unit_tests",
       "//components/payments/content:unit_tests",
       "//components/payments/content/utility:unit_tests",
+      "//components/policy/content:unit_tests",
       "//components/policy/core/browser:unit_tests",
       "//components/policy/core/common:unit_tests",
       "//components/previews/content:unit_tests",

components/policy/content/BUILD.gn

@@ -10,6 +10,8 @@ source_set("content") {
   sources = [
     "policy_blacklist_navigation_throttle.cc",
     "policy_blacklist_navigation_throttle.h",
+    "policy_blacklist_service.cc",
+    "policy_blacklist_service.h",
   ]
 
   deps = [
@@ -17,8 +19,30 @@ source_set("content") {
     "//components/keyed_service/content:content",
     "//components/policy/core/browser",
     "//components/prefs",
+    "//components/safe_search_api",
     "//components/user_prefs:user_prefs",
     "//content/public/browser",
     "//net",
   ]
 }
+
+source_set("unit_tests") {
+  testonly = true
+  sources = [
+    "policy_blacklist_navigation_throttle_unittest.cc",
+  ]
+  deps = [
+    ":content",
+    "//base",
+    "//components/keyed_service/content",
+    "//components/policy/core/browser",
+    "//components/safe_search_api",
+    "//components/safe_search_api:test_support",
+    "//components/sync_preferences:test_support",
+    "//components/user_prefs:user_prefs",
+    "//content/public/browser",
+    "//content/test:test_support",
+    "//testing/gtest",
+    "//url",
+  ]
+}

components/policy/content/DEPS

 include_rules = [
-  "+content/public",
+  "+content/public/browser",
   "+components/policy",
   "+components/keyed_service",
   "+components/prefs",
+  "+components/safe_search_api",
   "+components/user_prefs",
   "+net/base",
 ]
+
+specific_include_rules = {
+  "policy_blacklist_navigation_throttle_unittest\.cc": [
+    "+components/sync_preferences/testing_pref_service_syncable.h",
+    "+content/public/test",
+  ],
+}

components/policy/content/policy_blacklist_navigation_throttle.cc

@@ -4,75 +4,80 @@
 
 #include "components/policy/content/policy_blacklist_navigation_throttle.h"
 
-#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "base/bind.h"
+#include "base/logging.h"
+#include "components/policy/content/policy_blacklist_service.h"
+#include "components/policy/core/browser/url_blacklist_manager.h"
+#include "components/policy/core/browser/url_blacklist_policy_handler.h"
+#include "components/policy/core/browser/url_util.h"
+#include "components/policy/core/common/policy_pref_names.h"
+#include "components/prefs/pref_service.h"
 #include "components/user_prefs/user_prefs.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_handle.h"
+#include "url/gurl.h"
 
-PolicyBlacklistService::PolicyBlacklistService(
-    std::unique_ptr<policy::URLBlacklistManager> url_blacklist_manager)
-    : url_blacklist_manager_(std::move(url_blacklist_manager)) {}
-
-PolicyBlacklistService::~PolicyBlacklistService() {}
-
-bool PolicyBlacklistService::IsURLBlocked(const GURL& url) const {
-  return url_blacklist_manager_->IsURLBlocked(url);
-}
-
-policy::URLBlacklist::URLBlacklistState
-PolicyBlacklistService::GetURLBlacklistState(const GURL& url) const {
-  return url_blacklist_manager_->GetURLBlacklistState(url);
-}
-
-// static
-PolicyBlacklistFactory* PolicyBlacklistFactory::GetInstance() {
-  return base::Singleton<PolicyBlacklistFactory>::get();
-}
-
-// static
-PolicyBlacklistService* PolicyBlacklistFactory::GetForProfile(
-    content::BrowserContext* context) {
-  return static_cast<PolicyBlacklistService*>(
-      GetInstance()->GetServiceForBrowserContext(context, true));
-}
-
-PolicyBlacklistFactory::PolicyBlacklistFactory()
-    : BrowserContextKeyedServiceFactory(
-          "PolicyBlacklist",
-          BrowserContextDependencyManager::GetInstance()) {}
-
-PolicyBlacklistFactory::~PolicyBlacklistFactory() {}
-
-KeyedService* PolicyBlacklistFactory::BuildServiceInstanceFor(
-    content::BrowserContext* context) const {
-  PrefService* pref_service = user_prefs::UserPrefs::Get(context);
-  auto url_blacklist_manager =
-      std::make_unique<policy::URLBlacklistManager>(pref_service);
-  return new PolicyBlacklistService(std::move(url_blacklist_manager));
-}
-
-content::BrowserContext* PolicyBlacklistFactory::GetBrowserContextToUse(
-    content::BrowserContext* context) const {
-  // TODO(crbug.com/701326): This DCHECK should be moved to GetContextToUse().
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return context;
-}
+using URLBlacklistState = policy::URLBlacklist::URLBlacklistState;
+using SafeSitesFilterBehavior = policy::SafeSitesFilterBehavior;
 
 PolicyBlacklistNavigationThrottle::PolicyBlacklistNavigationThrottle(
     content::NavigationHandle* navigation_handle,
     content::BrowserContext* context)
-    : NavigationThrottle(navigation_handle) {
-  blacklist_service_ = PolicyBlacklistFactory::GetForProfile(context);
+    : NavigationThrottle(navigation_handle), weak_ptr_factory_(this) {
+  blacklist_service_ = PolicyBlacklistFactory::GetForBrowserContext(context);
+  prefs_ = user_prefs::UserPrefs::Get(context);
+  DCHECK(prefs_);
 }
 
 PolicyBlacklistNavigationThrottle::~PolicyBlacklistNavigationThrottle() {}
 
 content::NavigationThrottle::ThrottleCheckResult
 PolicyBlacklistNavigationThrottle::WillStartRequest() {
-  if (blacklist_service_->IsURLBlocked(navigation_handle()->GetURL())) {
+  GURL url = navigation_handle()->GetURL();
+
+  // Ignore blob scheme because PlzNavigate may use it to deliver navigation
+  // responses to the renderer process.
+  if (url.SchemeIs(url::kBlobScheme))
+    return PROCEED;
+
+  URLBlacklistState blacklist_state =
+      blacklist_service_->GetURLBlacklistState(url);
+  if (blacklist_state == URLBlacklistState::URL_IN_BLACKLIST) {
     return ThrottleCheckResult(BLOCK_REQUEST,
                                net::ERR_BLOCKED_BY_ADMINISTRATOR);
   }
+
+  if (blacklist_state == URLBlacklistState::URL_IN_WHITELIST)
+    return PROCEED;
+
+  // Safe Sites filter applies to top-level HTTP[S] requests.
+  if (!url.SchemeIsHTTPOrHTTPS())
+    return PROCEED;
+
+  SafeSitesFilterBehavior filter_behavior =
+      static_cast<SafeSitesFilterBehavior>(
+          prefs_->GetInteger(policy::policy_prefs::kSafeSitesFilterBehavior));
+  if (filter_behavior == SafeSitesFilterBehavior::kSafeSitesFilterDisabled)
+    return PROCEED;
+
+  DCHECK_EQ(filter_behavior, SafeSitesFilterBehavior::kSafeSitesFilterEnabled);
+
+  GURL effective_url = policy::url_util::GetEmbeddedURL(url);
+  if (!effective_url.is_valid())
+    effective_url = url;
+
+  bool synchronous = blacklist_service_->CheckSafeSearchURL(
+      effective_url,
+      base::BindOnce(
+          &PolicyBlacklistNavigationThrottle::CheckSafeSearchCallback,
+          weak_ptr_factory_.GetWeakPtr()));
+  if (!synchronous) {
+    deferred_ = true;
+    return DEFER;
+  }
+
+  if (should_cancel_)
+    return ThrottleCheckResult(CANCEL, net::ERR_BLOCKED_BY_ADMINISTRATOR);
   return PROCEED;
 }
 
@@ -84,3 +89,18 @@ PolicyBlacklistNavigationThrottle::WillRedirectRequest() {
 const char* PolicyBlacklistNavigationThrottle::GetNameForLogging() {
   return "PolicyBlacklistNavigationThrottle";
 }
+
+void PolicyBlacklistNavigationThrottle::CheckSafeSearchCallback(bool is_safe) {
+  if (!deferred_) {
+    should_cancel_ = !is_safe;
+    return;
+  }
+
+  deferred_ = false;
+  if (is_safe) {
+    Resume();
+  } else {
+    CancelDeferredNavigation(
+        ThrottleCheckResult(CANCEL, net::ERR_BLOCKED_BY_ADMINISTRATOR));
+  }
+}

components/policy/content/policy_blacklist_navigation_throttle.h

@@ -5,57 +5,24 @@
 #ifndef COMPONENTS_POLICY_CONTENT_POLICY_BLACKLIST_NAVIGATION_THROTTLE_H_
 #define COMPONENTS_POLICY_CONTENT_POLICY_BLACKLIST_NAVIGATION_THROTTLE_H_
 
-#include "base/memory/singleton.h"
-#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
-#include "components/keyed_service/core/keyed_service.h"
-#include "components/policy/core/browser/url_blacklist_manager.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/navigation_throttle.h"
 
-// PolicyBlacklistService and PolicyBlacklistFactory provide a way for
-// us to access URLBlacklistManager, a policy block list service based on
-// the Preference Service. The URLBlacklistManager responses to permission
-// changes and is per Profile.  From the PolicyBlacklistNavigationThrottle,
-// we access this service to provide information about what we should block.
-class PolicyBlacklistService : public KeyedService {
- public:
-  explicit PolicyBlacklistService(
-      std::unique_ptr<policy::URLBlacklistManager> url_blacklist_manager);
-  ~PolicyBlacklistService() override;
-
-  bool IsURLBlocked(const GURL& url) const;
-  policy::URLBlacklist::URLBlacklistState GetURLBlacklistState(
-      const GURL& url) const;
-
- private:
-  std::unique_ptr<policy::URLBlacklistManager> url_blacklist_manager_;
-
-  DISALLOW_COPY_AND_ASSIGN(PolicyBlacklistService);
-};
-
-class PolicyBlacklistFactory : public BrowserContextKeyedServiceFactory {
- public:
-  static PolicyBlacklistFactory* GetInstance();
-  static PolicyBlacklistService* GetForProfile(
-      content::BrowserContext* context);
-
- private:
-  PolicyBlacklistFactory();
-  ~PolicyBlacklistFactory() override;
-  friend struct base::DefaultSingletonTraits<PolicyBlacklistFactory>;
-
-  // BrowserContextKeyedServiceFactory implementation
-  KeyedService* BuildServiceInstanceFor(
-      content::BrowserContext* context) const override;
+class PolicyBlacklistService;
+class PrefService;
 
-  // Finds which browser context (if any) to use.
-  content::BrowserContext* GetBrowserContextToUse(
-      content::BrowserContext* context) const override;
-
-  DISALLOW_COPY_AND_ASSIGN(PolicyBlacklistFactory);
-};
+namespace content {
+class BrowserContext;
+class NavigationHandle;
+}  // namespace content
 
 // PolicyBlacklistNavigationThrottle provides a simple way to block a navigation
-// based on the URLBlacklistManager.
+// based on the URLBlacklistManager and Safe Search API. If the URL is
+// blacklisted or whitelisted, the throttle will immediately block or allow the
+// navigation. Otherwise, the URL will be checked against the Safe Search API if
+// the SafeSitesFilterBehavior policy is enabled. This final check may be
+// asynchronous if the result hasn't been cached yet.
 class PolicyBlacklistNavigationThrottle : public content::NavigationThrottle {
  public:
   PolicyBlacklistNavigationThrottle(
@@ -70,7 +37,22 @@ class PolicyBlacklistNavigationThrottle : public content::NavigationThrottle {
   const char* GetNameForLogging() override;
 
  private:
+  // Callback from PolicyBlacklistService.
+  void CheckSafeSearchCallback(bool is_safe);
+
   PolicyBlacklistService* blacklist_service_;
+
+  PrefService* prefs_;
+
+  // Whether the request was deferred in order to check the Safe Search API.
+  bool deferred_ = false;
+
+  // Whether the Safe Search API callback determined the in-progress navigation
+  // should be canceled.
+  bool should_cancel_ = false;
+
+  base::WeakPtrFactory<PolicyBlacklistNavigationThrottle> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(PolicyBlacklistNavigationThrottle);
 };
 

components/policy/content/policy_blacklist_service.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 "components/policy/content/policy_blacklist_service.h"
+
+#include <utility>
+
+#include "base/sequence_checker.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/policy/core/browser/url_util.h"
+#include "components/safe_search_api/url_checker.h"
+#include "components/user_prefs/user_prefs.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/storage_partition.h"
+#include "net/base/net_errors.h"
+
+namespace {
+
+// Calls the PolicyBlacklistService callback with the result of the Safe Search
+// API check.
+void OnCheckURLDone(PolicyBlacklistService::CheckSafeSearchCallback callback,
+                    const GURL& /* url */,
+                    safe_search_api::Classification classification,
+                    bool /* uncertain */) {
+  std::move(callback).Run(classification ==
+                          safe_search_api::Classification::SAFE);
+}
+
+}  // namespace
+
+PolicyBlacklistService::PolicyBlacklistService(
+    content::BrowserContext* browser_context,
+    std::unique_ptr<policy::URLBlacklistManager> url_blacklist_manager)
+    : browser_context_(browser_context),
+      url_blacklist_manager_(std::move(url_blacklist_manager)) {}
+
+PolicyBlacklistService::~PolicyBlacklistService() = default;
+
+policy::URLBlacklist::URLBlacklistState
+PolicyBlacklistService::GetURLBlacklistState(const GURL& url) const {
+  return url_blacklist_manager_->GetURLBlacklistState(url);
+}
+
+bool PolicyBlacklistService::CheckSafeSearchURL(
+    const GURL& url,
+    CheckSafeSearchCallback callback) {
+  if (!safe_search_url_checker_) {
+    net::NetworkTrafficAnnotationTag traffic_annotation =
+        net::DefineNetworkTrafficAnnotation("policy_blacklist_service", R"(
+          semantics {
+            sender: "Cloud Policy"
+            description:
+              "Checks whether a given URL (or set of URLs) is considered safe "
+              "by Google SafeSearch."
+            trigger:
+              "If the policy for safe sites is enabled, this is sent for every "
+              "top-level navigation if the result isn't already cached."
+            data: "URL to be checked."
+            destination: GOOGLE_OWNED_SERVICE
+          }
+          policy {
+            cookies_allowed: NO
+            setting:
+              "This feature is off by default and cannot be controlled in "
+              "settings."
+            chrome_policy {
+              SafeSitesFilterBehavior {
+                SafeSitesFilterBehavior: 0
+              }
+            }
+          })");
+
+    // TODO(michaelpg): Find the country code.
+    safe_search_url_checker_ = std::make_unique<safe_search_api::URLChecker>(
+        content::BrowserContext::GetDefaultStoragePartition(browser_context_)
+            ->GetURLLoaderFactoryForBrowserProcess(),
+        traffic_annotation, std::string());
+  }
+
+  return safe_search_url_checker_->CheckURL(
+      policy::url_util::Normalize(url),
+      base::BindOnce(&OnCheckURLDone, std::move(callback)));
+}
+
+void PolicyBlacklistService::SetSafeSearchURLCheckerForTest(
+    std::unique_ptr<safe_search_api::URLChecker> safe_search_url_checker) {
+  safe_search_url_checker_ = std::move(safe_search_url_checker);
+}
+
+// static
+PolicyBlacklistFactory* PolicyBlacklistFactory::GetInstance() {
+  return base::Singleton<PolicyBlacklistFactory>::get();
+}
+
+// static
+PolicyBlacklistService* PolicyBlacklistFactory::GetForBrowserContext(
+    content::BrowserContext* context) {
+  return static_cast<PolicyBlacklistService*>(
+      GetInstance()->GetServiceForBrowserContext(context, true));
+}
+
+PolicyBlacklistFactory::PolicyBlacklistFactory()
+    : BrowserContextKeyedServiceFactory(
+          "PolicyBlacklist",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+PolicyBlacklistFactory::~PolicyBlacklistFactory() = default;
+
+KeyedService* PolicyBlacklistFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  PrefService* pref_service = user_prefs::UserPrefs::Get(context);
+  auto url_blacklist_manager =
+      std::make_unique<policy::URLBlacklistManager>(pref_service);
+  return new PolicyBlacklistService(context, std::move(url_blacklist_manager));
+}
+
+content::BrowserContext* PolicyBlacklistFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  // TODO(crbug.com/701326): This DCHECK should be moved to GetContextToUse().
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return context;
+}

components/policy/content/policy_blacklist_service.h

+// 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.
+
+#ifndef COMPONENTS_POLICY_CONTENT_POLICY_BLACKLIST_SERVICE_H_
+#define COMPONENTS_POLICY_CONTENT_POLICY_BLACKLIST_SERVICE_H_
+
+#include <memory>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/memory/singleton.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "components/policy/core/browser/url_blacklist_manager.h"
+
+namespace safe_search_api {
+class URLChecker;
+}  // namespace safe_search_api
+
+// PolicyBlacklistService and PolicyBlacklistFactory provide a way for
+// us to access URLBlacklistManager, a policy block list service based on
+// the Preference Service. The URLBlacklistManager responds to permission
+// changes and is per-Profile. The PolicyBlacklistNavigationThrottle accesses
+// this service to determine what we should block.
+class PolicyBlacklistService : public KeyedService {
+ public:
+  using CheckSafeSearchCallback = base::OnceCallback<void(bool is_safe)>;
+
+  PolicyBlacklistService(
+      content::BrowserContext* browser_context,
+      std::unique_ptr<policy::URLBlacklistManager> url_blacklist_manager);
+  ~PolicyBlacklistService() override;
+
+  policy::URLBlacklist::URLBlacklistState GetURLBlacklistState(
+      const GURL& url) const;
+
+  // Starts a call to the Safe Search API for the given URL to determine whether
+  // the URL is "safe" (not porn). Returns whether |callback| was run
+  // synchronously.
+  bool CheckSafeSearchURL(const GURL& url, CheckSafeSearchCallback callback);
+
+  // Creates a SafeSearch URLChecker using a given URLLoaderFactory for testing.
+  void SetSafeSearchURLCheckerForTest(
+      std::unique_ptr<safe_search_api::URLChecker> safe_search_url_checker);
+
+ private:
+  content::BrowserContext* const browser_context_;
+  std::unique_ptr<policy::URLBlacklistManager> url_blacklist_manager_;
+  std::unique_ptr<safe_search_api::URLChecker> safe_search_url_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(PolicyBlacklistService);
+};
+
+class PolicyBlacklistFactory : public BrowserContextKeyedServiceFactory {
+ public:
+  static PolicyBlacklistFactory* GetInstance();
+  static PolicyBlacklistService* GetForBrowserContext(
+      content::BrowserContext* context);
+
+ private:
+  PolicyBlacklistFactory();
+  ~PolicyBlacklistFactory() override;
+  friend struct base::DefaultSingletonTraits<PolicyBlacklistFactory>;
+
+  // BrowserContextKeyedServiceFactory:
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const override;
+
+  // Finds which browser context (if any) to use.
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
+
+  DISALLOW_COPY_AND_ASSIGN(PolicyBlacklistFactory);
+};
+
+#endif  // COMPONENTS_POLICY_CONTENT_POLICY_BLACKLIST_SERVICE_H_

components/policy/core/browser/url_blacklist_manager.cc

@@ -27,6 +27,7 @@
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/values.h"
+#include "components/policy/core/browser/url_blacklist_policy_handler.h"
 #include "components/policy/core/common/policy_pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
@@ -513,6 +514,9 @@ void URLBlacklistManager::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterListPref(policy_prefs::kUrlBlacklist);
   registry->RegisterListPref(policy_prefs::kUrlWhitelist);
+  registry->RegisterIntegerPref(
+      policy_prefs::kSafeSitesFilterBehavior,
+      static_cast<int>(SafeSitesFilterBehavior::kSafeSitesFilterDisabled));
 }
 
 }  // namespace policy

components/policy/core/browser/url_blacklist_policy_handler.h

@@ -12,6 +12,13 @@
 
 namespace policy {
 
+// Possible values for kSafeSitesFilterBehavior pref from policy. Values must
+// coincide with SafeSitesFilterBehavior from policy_templates.json.
+enum class SafeSitesFilterBehavior {
+  kSafeSitesFilterDisabled = 0,
+  kSafeSitesFilterEnabled = 1,
+};
+
 // Handles URLBlacklist policies.
 class POLICY_EXPORT URLBlacklistPolicyHandler
     : public ConfigurationPolicyHandler {

components/safe_search_api/BUILD.gn

@@ -18,10 +18,11 @@ source_set("safe_search_api") {
   ]
 }
 
-source_set("unit_tests") {
+source_set("test_support") {
   testonly = true
   sources = [
-    "url_checker_unittest.cc",
+    "stub_url_checker.cc",
+    "stub_url_checker.h",
   ]
   deps = [
     ":safe_search_api",
@@ -31,6 +32,21 @@ source_set("unit_tests") {
     "//net/traffic_annotation:test_support",
     "//services/network:test_support",
     "//services/network/public/cpp",
+    "//url",
+  ]
+}
+
+source_set("unit_tests") {
+  testonly = true
+  sources = [
+    "url_checker_unittest.cc",
+  ]
+  deps = [
+    ":safe_search_api",
+    ":test_support",
+    "//base",
+    "//base/test:test_support",
+    "//net",
     "//testing/gmock",
     "//testing/gtest",
     "//url",

components/safe_search_api/stub_url_checker.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 "components/safe_search_api/stub_url_checker.h"
+
+#include "base/json/json_writer.h"
+#include "base/values.h"
+#include "components/safe_search_api/url_checker.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "url/gurl.h"
+
+namespace safe_search_api {
+
+namespace {
+
+constexpr char kSafeSearchApiUrl[] =
+    "https://safesearch.googleapis.com/v1:classify";
+
+std::string BuildResponse(bool is_porn) {
+  base::DictionaryValue dict;
+  auto classification_dict = std::make_unique<base::DictionaryValue>();
+  if (is_porn)
+    classification_dict->SetBoolean("pornography", is_porn);
+  auto classifications_list = std::make_unique<base::ListValue>();
+  classifications_list->Append(std::move(classification_dict));
+  dict.SetWithoutPathExpansion("classifications",
+                               std::move(classifications_list));
+  std::string result;
+  base::JSONWriter::Write(dict, &result);
+  return result;
+}
+
+}  // namespace
+
+StubURLChecker::StubURLChecker()
+    : test_shared_loader_factory_(
+          base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+              &test_url_loader_factory_)) {}
+
+StubURLChecker::~StubURLChecker() = default;
+
+std::unique_ptr<URLChecker> StubURLChecker::BuildURLChecker(size_t cache_size) {
+  return std::make_unique<URLChecker>(test_shared_loader_factory_,
+                                      TRAFFIC_ANNOTATION_FOR_TESTS,
+                                      std::string(), cache_size);
+}
+
+void StubURLChecker::SetUpValidResponse(bool is_porn) {
+  SetUpResponse(net::OK, BuildResponse(is_porn));
+}
+
+void StubURLChecker::SetUpFailedResponse() {
+  SetUpResponse(net::ERR_ABORTED, std::string());
+}
+
+void StubURLChecker::ClearResponses() {
+  test_url_loader_factory_.ClearResponses();
+}
+
+void StubURLChecker::SetUpResponse(net::Error error,
+                                   const std::string& response) {
+  network::URLLoaderCompletionStatus status(error);
+  status.decoded_body_length = response.size();
+  test_url_loader_factory_.AddResponse(GURL(kSafeSearchApiUrl),
+                                       network::ResourceResponseHead(),
+                                       response, status);
+}
+
+}  // namespace safe_search_api

components/safe_search_api/stub_url_checker.h

+// 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.
+
+#ifndef COMPONENTS_SAFE_SEARCH_API_STUB_URL_CHECKER_H_
+#define COMPONENTS_SAFE_SEARCH_API_STUB_URL_CHECKER_H_
+
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
+#include "net/base/net_errors.h"
+#include "services/network/test/test_url_loader_factory.h"
+
+namespace network {
+class SharedURLLoaderFactory;
+}  // namespace network
+
+namespace safe_search_api {
+
+class URLChecker;
+
+// Helper class to stub out a URLLoaderFactory for use with URLChecker. This
+// lets tests control the response the URLChecker will receive from the Safe
+// Search API. Used to test both URLChecker itself and classes that use it.
+// This class builds a real URLChecker but maintains control over it to set up
+// fake responses.
+class StubURLChecker {
+ public:
+  StubURLChecker();
+  ~StubURLChecker();
+
+  // Returns a URLChecker that will use the stubbed-out responses. Can be called
+  // before or after setting up the responses.
+  std::unique_ptr<URLChecker> BuildURLChecker(size_t cache_size);
+
+  // Sets the stub to return a successful response to all Safe Search API calls
+  // from now on.
+  void SetUpValidResponse(bool is_porn);
+
+  // Sets the stub to respond to all Safe Search API calls with a failure from
+  // now on.
+  void SetUpFailedResponse();
+
+  // Clears the stub so it won't return any response from now on.
+  void ClearResponses();
+
+ private:
+  void SetUpResponse(net::Error error, const std::string& response);
+
+  network::TestURLLoaderFactory test_url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(StubURLChecker);
+};
+
+}  // namespace safe_search_api
+
+#endif  // COMPONENTS_SAFE_SEARCH_API_STUB_URL_CHECKER_H_

components/safe_search_api/url_checker_unittest.cc

@@ -11,18 +11,13 @@
 #include <utility>
 
 #include "base/callback.h"
-#include "base/json/json_writer.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "base/values.h"
+#include "components/safe_search_api/stub_url_checker.h"
 #include "net/base/net_errors.h"
-#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
-#include "services/network/public/cpp/shared_url_loader_factory.h"
-#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
-#include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -33,7 +28,7 @@ namespace safe_search_api {
 
 namespace {
 
-const size_t kCacheSize = 2;
+constexpr size_t kCacheSize = 2;
 
 const char* kURLs[] = {
     "http://www.randomsite1.com", "http://www.randomsite2.com",
@@ -43,40 +38,12 @@ const char* kURLs[] = {
     "http://www.randomsite9.com",
 };
 
-const char kSafeSearchApiUrl[] =
-    "https://safesearch.googleapis.com/v1:classify";
-
-std::string BuildResponse(bool is_porn) {
-  base::DictionaryValue dict;
-  auto classification_dict = std::make_unique<base::DictionaryValue>();
-  if (is_porn)
-    classification_dict->SetBoolean("pornography", is_porn);
-  auto classifications_list = std::make_unique<base::ListValue>();
-  classifications_list->Append(std::move(classification_dict));
-  dict.SetWithoutPathExpansion("classifications",
-                               std::move(classifications_list));
-  std::string result;
-  base::JSONWriter::Write(dict, &result);
-  return result;
-}
-
-std::string BuildPornResponse() {
-  return BuildResponse(true);
-}
-
 }  // namespace
 
 class SafeSearchURLCheckerTest : public testing::Test {
  public:
   SafeSearchURLCheckerTest()
-      : next_url_(0),
-        test_shared_loader_factory_(
-            base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
-                &test_url_loader_factory_)),
-        checker_(test_shared_loader_factory_,
-                 TRAFFIC_ANNOTATION_FOR_TESTS,
-                 "us",
-                 kCacheSize) {}
+      : next_url_(0), checker_(stub_url_checker_.BuildURLChecker(kCacheSize)) {}
 
   MOCK_METHOD3(OnCheckDone,
                void(const GURL& url,
@@ -89,19 +56,9 @@ class SafeSearchURLCheckerTest : public testing::Test {
     return GURL(kURLs[next_url_++]);
   }
 
-  void SetupResponse(const GURL& url,
-                     net::Error error,
-                     const std::string& response) {
-    network::URLLoaderCompletionStatus status(error);
-    status.decoded_body_length = response.size();
-    test_url_loader_factory_.AddResponse(GURL(kSafeSearchApiUrl),
-                                         network::ResourceResponseHead(),
-                                         response, status);
-  }
-
   // Returns true if the result was returned synchronously (cache hit).
   bool CheckURL(const GURL& url) {
-    bool cached = checker_.CheckURL(
+    bool cached = checker_->CheckURL(
         url, base::BindOnce(&SafeSearchURLCheckerTest::OnCheckDone,
                             base::Unretained(this)));
     return cached;
@@ -110,14 +67,14 @@ class SafeSearchURLCheckerTest : public testing::Test {
   void WaitForResponse() { base::RunLoop().RunUntilIdle(); }
 
   bool SendValidResponse(const GURL& url, bool is_porn) {
-    SetupResponse(url, net::OK, BuildResponse(is_porn));
+    stub_url_checker_.SetUpValidResponse(is_porn);
     bool result = CheckURL(url);
     WaitForResponse();
     return result;
   }
 
   bool SendFailedResponse(const GURL& url) {
-    SetupResponse(url, net::ERR_ABORTED, std::string());
+    stub_url_checker_.SetUpFailedResponse();
     bool result = CheckURL(url);
     WaitForResponse();
     return result;
@@ -125,9 +82,8 @@ class SafeSearchURLCheckerTest : public testing::Test {
 
   size_t next_url_;
   base::MessageLoop message_loop_;
-  network::TestURLLoaderFactory test_url_loader_factory_;
-  scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
-  URLChecker checker_;
+  StubURLChecker stub_url_checker_;
+  std::unique_ptr<URLChecker> checker_;
 };
 
 TEST_F(SafeSearchURLCheckerTest, Simple) {
@@ -162,7 +118,7 @@ TEST_F(SafeSearchURLCheckerTest, Cache) {
   ASSERT_FALSE(SendValidResponse(url2, false));
 
   // Now we should get results synchronously, without a network request.
-  test_url_loader_factory_.ClearResponses();
+  stub_url_checker_.ClearResponses();
   EXPECT_CALL(*this, OnCheckDone(url2, Classification::SAFE, false));
   ASSERT_TRUE(CheckURL(url2));
   EXPECT_CALL(*this, OnCheckDone(url1, Classification::SAFE, false));
@@ -179,7 +135,7 @@ TEST_F(SafeSearchURLCheckerTest, Cache) {
 TEST_F(SafeSearchURLCheckerTest, CoalesceRequestsToSameURL) {
   GURL url(GetNewURL());
   // Start two checks for the same URL.
-  SetupResponse(url, net::OK, BuildResponse(false));
+  stub_url_checker_.SetUpValidResponse(false);
   ASSERT_FALSE(CheckURL(url));
   ASSERT_FALSE(CheckURL(url));
   // A single response should answer both of those checks
@@ -190,7 +146,7 @@ TEST_F(SafeSearchURLCheckerTest, CoalesceRequestsToSameURL) {
 TEST_F(SafeSearchURLCheckerTest, CacheTimeout) {
   GURL url(GetNewURL());
 
-  checker_.SetCacheTimeoutForTesting(base::TimeDelta::FromSeconds(0));
+  checker_->SetCacheTimeoutForTesting(base::TimeDelta::FromSeconds(0));
 
   EXPECT_CALL(*this, OnCheckDone(url, Classification::SAFE, false));
   ASSERT_FALSE(SendValidResponse(url, false));
@@ -226,7 +182,7 @@ TEST_F(SafeSearchURLCheckerTest, NoAllowAllGoogleURLs) {
   {
     GURL url("https://sites.google.com/porn");
     EXPECT_CALL(*this, OnCheckDone(url, Classification::UNSAFE, _));
-    SetupResponse(url, net::OK, BuildPornResponse());
+    stub_url_checker_.SetUpValidResponse(true);
     bool cache_hit = CheckURL(url);
     ASSERT_FALSE(cache_hit);
     WaitForResponse();
@@ -234,7 +190,7 @@ TEST_F(SafeSearchURLCheckerTest, NoAllowAllGoogleURLs) {
   {
     GURL url("https://youtube.com/porn");
     EXPECT_CALL(*this, OnCheckDone(url, Classification::UNSAFE, _));
-    SetupResponse(url, net::OK, BuildPornResponse());
+    stub_url_checker_.SetUpValidResponse(true);
     bool cache_hit = CheckURL(url);
     ASSERT_FALSE(cache_hit);
     WaitForResponse();

tools/traffic_annotation/auditor/safe_list.txt

@@ -4,4 +4,5 @@ all,tools/*,*test*,*fuzzer*,*mock*,*fake*
 missing,net/url_request/url_fetcher.cc
 missing,net/url_request/url_request_context.cc
 direct_assignment,download::ProtoConversions::EntryFromProto@components/download/internal/background_service/proto_conversions.cc
-test_annotation,net/quic/quic_chromium_client_session_peer.cc
+test_annotation,components/safe_search_api/stub_url_checker.cc
\ No newline at end of file
+test_annotation,net/quic/quic_chromium_client_session_peer.cc

tools/traffic_annotation/summary/annotations.xml

@@ -181,6 +181,7 @@ Refer to README.md for content description and update process.
  <item id="permission_request_creator" hash_code="43206794" type="0" content_hash_code="73571699" os_list="linux,windows" file_path="chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc"/>
  <item id="persist_blob_to_indexed_db" hash_code="32030464" type="0" deprecated="2018-08-13" content_hash_code="35410079" file_path=""/>
  <item id="plugins_resource_service" hash_code="49601082" type="0" content_hash_code="6877335" os_list="linux,windows" file_path="chrome/browser/plugins/plugins_resource_service.cc"/>
+ <item id="policy_blacklist_service" hash_code="49799644" type="0" content_hash_code="57843386" os_list="linux,mac,windows" file_path="components/policy/content/policy_blacklist_service.cc"/>
  <item id="popular_sites_fetch" hash_code="50755044" type="0" content_hash_code="6910083" os_list="linux,windows" file_path="components/ntp_tiles/popular_sites_impl.cc"/>
  <item id="port_forwarding_controller_socket" hash_code="95075845" type="0" content_hash_code="122163428" os_list="linux,windows" file_path="chrome/browser/devtools/device/port_forwarding_controller.cc"/>
  <item id="ppapi_download_request" hash_code="135967426" type="0" content_hash_code="110461402" os_list="linux,windows" file_path="chrome/browser/safe_browsing/download_protection/ppapi_download_request.cc"/>