[Safe Browsing] Abstract signin dep from RealTimeUrlLookupService

This CL continues with the abstraction of dependencies on signin and sync from //components/safe_browsing's access token fetching flow in order to enable reuse by WebLayer. The concrete step taken here is to abstract the dependencies on //components/safe_browsing/core/browser/sync from RealTimeUrlLookupService, passing these dependencies in via the constructor. There are no behavioral changes in this CL: - //chrome and //ios/chrome's factories glue the relevant parameters from //components/safe_browsing/core/browser/sync - //weblayer's factory passes in values for the parameters such that access token fetching for URL lookups continues to be disabled in WebLayer Followup work will bring up support for access token fetching in WebLayer. Bug: 1080748 Change-Id: I69fca4b2d04118088bb48596eb192beab7bafb4f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2621301 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Reviewed-by: Xinghui Lu <xinghuilu@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#842942}
blueboxd · Jan 13, 2021 · 85753b0 · 85753b0
1 parent 4b932f7
commit 85753b0
Show file tree

Hide file tree

Showing 13 changed files with 117 additions and 62 deletions.
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -2187,6 +2187,7 @@ static_library("browser") {
     "//components/safe_browsing/core:file_type_policies",
     "//components/safe_browsing/core:public",
     "//components/safe_browsing/core/browser",
+    "//components/safe_browsing/core/browser/sync",
     "//components/safe_browsing/core/common",
     "//components/safe_browsing/core/common:safe_browsing_policy_handler",
     "//components/safe_browsing/core/db:database_manager",

diff --git a/chrome/browser/safe_browsing/url_lookup_service_factory.cc b/chrome/browser/safe_browsing/url_lookup_service_factory.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/safe_browsing/url_lookup_service_factory.h"
 
+#include "base/bind.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
@@ -15,6 +16,8 @@
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/safe_browsing/buildflags.h"
+#include "components/safe_browsing/core/browser/sync/safe_browsing_primary_account_token_fetcher.h"
+#include "components/safe_browsing/core/browser/sync/sync_utils.h"
 #include "components/safe_browsing/core/common/utils.h"
 #include "components/safe_browsing/core/realtime/url_lookup_service.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
@@ -69,8 +72,13 @@ KeyedService* RealTimeUrlLookupServiceFactory::BuildServiceInstanceFor(
   return new RealTimeUrlLookupService(
       network::SharedURLLoaderFactory::Create(std::move(url_loader_factory)),
       VerdictCacheManagerFactory::GetForProfile(profile),
-      IdentityManagerFactory::GetForProfile(profile),
       ProfileSyncServiceFactory::GetForProfile(profile), profile->GetPrefs(),
+      std::make_unique<SafeBrowsingPrimaryAccountTokenFetcher>(
+          IdentityManagerFactory::GetForProfile(profile)),
+      base::BindRepeating(&safe_browsing::SyncUtils::
+                              AreSigninAndSyncSetUpForSafeBrowsingTokenFetches,
+                          ProfileSyncServiceFactory::GetForProfile(profile),
+                          IdentityManagerFactory::GetForProfile(profile)),
       GetProfileManagementStatus(browser_policy_connector),
       is_under_advanced_protection, profile->IsOffTheRecord(),
       g_browser_process->variations_service());

diff --git a/components/safe_browsing/core/browser/BUILD.gn b/components/safe_browsing/core/browser/BUILD.gn
@@ -58,6 +58,7 @@ source_set("safe_browsing_url_checker_unittest") {
     ":browser",
     "//base/test:test_support",
     "//components/safe_browsing/core:csd_proto",
+    "//components/safe_browsing/core/browser:token_fetcher",
     "//components/safe_browsing/core/common:test_support",
     "//components/safe_browsing/core/common:thread_utils",
     "//components/safe_browsing/core/db:test_database_manager",

diff --git a/components/safe_browsing/core/browser/safe_browsing_url_checker_impl_unittest.cc b/components/safe_browsing/core/browser/safe_browsing_url_checker_impl_unittest.cc
@@ -5,11 +5,13 @@
 #include "components/safe_browsing/core/browser/safe_browsing_url_checker_impl.h"
 #include <memory>
 
+#include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
+#include "components/safe_browsing/core/browser/safe_browsing_token_fetcher.h"
 #include "components/safe_browsing/core/browser/url_checker_delegate.h"
 #include "components/safe_browsing/core/common/test_task_environment.h"
 #include "components/safe_browsing/core/common/thread_utils.h"
@@ -166,15 +168,19 @@ class MockUrlCheckerDelegate : public UrlCheckerDelegate {
 class MockRealTimeUrlLookupService : public RealTimeUrlLookupService {
  public:
   MockRealTimeUrlLookupService()
-      : RealTimeUrlLookupService(/*url_loader_factory=*/nullptr,
-                                 /*cache_manager=*/nullptr,
-                                 /*identity_manager=*/nullptr,
-                                 /*sync_service=*/nullptr,
-                                 /*pref_service=*/nullptr,
-                                 ChromeUserPopulation::NOT_MANAGED,
-                                 /*is_under_advanced_protection=*/false,
-                                 /*is_off_the_record=*/false,
-                                 /*variations_service=*/nullptr) {}
+      : RealTimeUrlLookupService(
+            /*url_loader_factory=*/nullptr,
+            /*cache_manager=*/nullptr,
+            /*sync_service=*/nullptr,
+            /*pref_service=*/nullptr,
+            /*token_fetcher=*/nullptr,
+            /*client_token_config_callback=*/base::BindRepeating([](bool) {
+              return false;
+            }),
+            ChromeUserPopulation::NOT_MANAGED,
+            /*is_under_advanced_protection=*/false,
+            /*is_off_the_record=*/false,
+            /*variations_service=*/nullptr) {}
   // Returns the threat type previously set by |SetThreatTypeForUrl|. It crashes
   // if the threat type for the |gurl| is not set in advance.
   void StartLookup(const GURL& gurl,

diff --git a/components/safe_browsing/core/realtime/BUILD.gn b/components/safe_browsing/core/realtime/BUILD.gn
@@ -36,11 +36,10 @@ static_library("url_lookup_service") {
     "//components/safe_browsing/core:csd_proto",
     "//components/safe_browsing/core:realtimeapi_proto",
     "//components/safe_browsing/core:verdict_cache_manager",
-    "//components/safe_browsing/core/browser/sync",
+    "//components/safe_browsing/core/browser:token_fetcher",
     "//components/safe_browsing/core/common:safe_browsing_prefs",
     "//components/safe_browsing/core/common:thread_utils",
     "//components/safe_browsing/core/db:v4_protocol_manager_util",
-    "//components/signin/public/identity_manager",
     "//components/sync",
     "//components/variations/service:service",
     "//services/network/public/cpp:cpp",
@@ -57,6 +56,7 @@ static_library("url_lookup_service_base") {
   deps = [
     "//base:base",
     "//build:chromeos_buildflags",
+    "//components/keyed_service/core",
     "//components/prefs",
     "//components/safe_browsing:buildflags",
     "//components/safe_browsing/core:csd_proto",
@@ -67,7 +67,6 @@ static_library("url_lookup_service_base") {
     "//components/safe_browsing/core/common:safe_browsing_prefs",
     "//components/safe_browsing/core/common:thread_utils",
     "//components/safe_browsing/core/db:v4_protocol_manager_util",
-    "//components/signin/public/identity_manager",
     "//components/sync",
     "//services/network/public/cpp:cpp",
     "//url:url",
@@ -88,10 +87,10 @@ source_set("unit_tests") {
     "//components/safe_browsing:buildflags",
     "//components/safe_browsing/core:features",
     "//components/safe_browsing/core:verdict_cache_manager",
+    "//components/safe_browsing/core/browser:token_fetcher",
     "//components/safe_browsing/core/common",
     "//components/safe_browsing/core/common:safe_browsing_prefs",
     "//components/safe_browsing/core/common:test_support",
-    "//components/signin/public/identity_manager:test_support",
     "//components/sync:test_support",
     "//components/sync_preferences:test_support",
     "//components/unified_consent",

diff --git a/components/safe_browsing/core/realtime/policy_engine.h b/components/safe_browsing/core/realtime/policy_engine.h
@@ -30,7 +30,7 @@ class RealTimePolicyEngine {
   RealTimePolicyEngine() = delete;
   ~RealTimePolicyEngine() = delete;
 
-  // A callback via which the client of this component indicates whether they
+  // A callback via which the client of this class indicates whether they
   // are configured to support token fetches. Used as part of
   // CanPerformFullURLLookupWithToken().
   using ClientConfiguredForTokenFetchesCallback =

diff --git a/components/safe_browsing/core/realtime/url_lookup_service.cc b/components/safe_browsing/core/realtime/url_lookup_service.cc
@@ -13,14 +13,11 @@
 #include "base/time/time.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/buildflags.h"
-#include "components/safe_browsing/core/browser/sync/safe_browsing_primary_account_token_fetcher.h"
-#include "components/safe_browsing/core/browser/sync/sync_utils.h"
+#include "components/safe_browsing/core/browser/safe_browsing_token_fetcher.h"
 #include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/core/common/thread_utils.h"
 #include "components/safe_browsing/core/db/v4_protocol_manager_util.h"
 #include "components/safe_browsing/core/realtime/policy_engine.h"
-#include "components/signin/public/identity_manager/consent_level.h"
-#include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/sync/driver/sync_service.h"
 #include "net/base/ip_address.h"
 #include "net/base/load_flags.h"
@@ -35,9 +32,10 @@ namespace safe_browsing {
 RealTimeUrlLookupService::RealTimeUrlLookupService(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     VerdictCacheManager* cache_manager,
-    signin::IdentityManager* identity_manager,
     syncer::SyncService* sync_service,
     PrefService* pref_service,
+    std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher,
+    const ClientConfiguredForTokenFetchesCallback& client_token_config_callback,
     const ChromeUserPopulation::ProfileManagementStatus&
         profile_management_status,
     bool is_under_advanced_protection,
@@ -50,14 +48,12 @@ RealTimeUrlLookupService::RealTimeUrlLookupService(
                                    profile_management_status,
                                    is_under_advanced_protection,
                                    is_off_the_record),
-      identity_manager_(identity_manager),
       sync_service_(sync_service),
       pref_service_(pref_service),
+      token_fetcher_(std::move(token_fetcher)),
+      client_token_config_callback_(client_token_config_callback),
       is_off_the_record_(is_off_the_record),
-      variations_(variations_service) {
-  token_fetcher_ = std::make_unique<SafeBrowsingPrimaryAccountTokenFetcher>(
-      identity_manager_);
-}
+      variations_(variations_service) {}
 
 void RealTimeUrlLookupService::GetAccessToken(
     const GURL& url,
@@ -93,10 +89,7 @@ bool RealTimeUrlLookupService::CanPerformFullURLLookup() const {
 
 bool RealTimeUrlLookupService::CanPerformFullURLLookupWithToken() const {
   return RealTimePolicyEngine::CanPerformFullURLLookupWithToken(
-      pref_service_, is_off_the_record_,
-      base::BindOnce(
-          &SyncUtils::AreSigninAndSyncSetUpForSafeBrowsingTokenFetches,
-          sync_service_, identity_manager_),
+      pref_service_, is_off_the_record_, client_token_config_callback_,
       variations_);
 }
 

diff --git a/components/safe_browsing/core/realtime/url_lookup_service.h b/components/safe_browsing/core/realtime/url_lookup_service.h
@@ -18,7 +18,6 @@
 #include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/safe_browsing/core/proto/realtimeapi.pb.h"
 #include "components/safe_browsing/core/realtime/url_lookup_service_base.h"
-#include "components/signin/public/identity_manager/access_token_info.h"
 #include "url/gurl.h"
 
 namespace net {
@@ -29,10 +28,6 @@ namespace network {
 class SharedURLLoaderFactory;
 }  // namespace network
 
-namespace signin {
-class IdentityManager;
-}
-
 namespace syncer {
 class SyncService;
 }
@@ -52,14 +47,23 @@ class SafeBrowsingTokenFetcher;
 // users.(See: go/chrome-protego-enterprise-dd)
 class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
  public:
-  // |cache_manager|, |identity_manager|, |sync_service| and |pref_service| may
-  // be null in tests.
+  // A callback via which the client of this component indicates whether they
+  // are configured to support token fetches.
+  using ClientConfiguredForTokenFetchesCallback =
+      base::RepeatingCallback<bool(bool user_has_enabled_enhanced_protection)>;
+
+  // |cache_manager|, |sync_service|, and |pref_service| may be null in tests.
+  // |token_fetcher| may also be null, but in that case the passed-in
+  // |client_token_config_callback| should return false to ensure that access
+  // token fetches are not actually invoked.
   RealTimeUrlLookupService(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       VerdictCacheManager* cache_manager,
-      signin::IdentityManager* identity_manager,
       syncer::SyncService* sync_service,
       PrefService* pref_service,
+      std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher,
+      const ClientConfiguredForTokenFetchesCallback&
+          client_token_config_callback,
       const ChromeUserPopulation::ProfileManagementStatus&
           profile_management_status,
       bool is_under_advanced_protection,
@@ -91,10 +95,6 @@ class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
                         base::TimeTicks get_token_start_time,
                         const std::string& access_token);
 
-  // Unowned object used for getting access token when real time url check with
-  // token is enabled.
-  signin::IdentityManager* identity_manager_;
-
   // Unowned object used for checking sync status of the profile.
   syncer::SyncService* sync_service_;
 
@@ -104,6 +104,10 @@ class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
   // The token fetcher used for getting access token.
   std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher_;
 
+  // The callback via which the client of this component indicates whether they
+  // are configured to support token fetches.
+  ClientConfiguredForTokenFetchesCallback client_token_config_callback_;
+
   // A boolean indicates whether the profile associated with this
   // |url_lookup_service| is an off the record profile.
   bool is_off_the_record_;