Add profile-awareness to realtime URL lookups

The BrowserURLLoaderThrottle now checks the current BrowserContext to
determine whether real-time checks are enabled at all, then passes that
information to the SafeBrowsingUrlCheckerImpl.

Bug: 991394, 963165, 1001566
Change-Id: I5dbff848a4ea5a6ae3ac5c76e5d5624596ee3616
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1825482Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702956}

chrome/browser/policy/policy_browsertest.cc

@@ -5709,17 +5709,21 @@ IN_PROC_BROWSER_TEST_F(SignedExchangePolicyTest, SignedExchangeEnabled) {
 
 IN_PROC_BROWSER_TEST_F(PolicyTest, CheckURLsInRealTime) {
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      safe_browsing::kRealTimeUrlLookupFetchAllowlist);
+  scoped_feature_list.InitWithFeatures(
+      {safe_browsing::kRealTimeUrlLookupEnabled,
+       safe_browsing::kRealTimeUrlLookupFetchAllowlist},
+      {});
 
-  EXPECT_FALSE(safe_browsing::RealTimePolicyEngine::CanPerformFullURLLookup());
+  EXPECT_FALSE(safe_browsing::RealTimePolicyEngine::CanPerformFullURLLookup(
+      browser()->profile()));
 
   PolicyMap policies;
   SetPolicy(&policies, key::kSafeBrowsingRealTimeLookupEnabled,
             std::make_unique<base::Value>(true));
   UpdateProviderPolicy(policies);
 
-  EXPECT_TRUE(safe_browsing::RealTimePolicyEngine::CanPerformFullURLLookup());
+  EXPECT_TRUE(safe_browsing::RealTimePolicyEngine::CanPerformFullURLLookup(
+      browser()->profile()));
 }
 
 class HSTSPolicyTest : public PolicyTest {

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -409,7 +409,6 @@ void SafeBrowsingService::RefreshState() {
   // Check if any profile requires the service to be active.
   enabled_by_prefs_ = false;
   estimated_extended_reporting_by_prefs_ = SBER_LEVEL_OFF;
-  bool is_real_time_lookup_enabled = false;
   for (const auto& pref : prefs_map_) {
     if (pref.first->GetBoolean(prefs::kSafeBrowsingEnabled)) {
       enabled_by_prefs_ = true;
@@ -419,17 +418,9 @@ void SafeBrowsingService::RefreshState() {
       if (erl != SBER_LEVEL_OFF) {
         estimated_extended_reporting_by_prefs_ = erl;
       }
-
-      if (pref.first->GetBoolean(prefs::kSafeBrowsingRealTimeLookupEnabled)) {
-        is_real_time_lookup_enabled = true;
-      }
     }
   }
 
-  // TODO(crbug.com/991394): This enables real-time URL lookup if it is enabled
-  // for any of the active profiles. This should be fixed.
-  RealTimePolicyEngine::SetEnabled(is_real_time_lookup_enabled);
-
   if (enabled_by_prefs_)
     Start();
   else

components/safe_browsing/DEPS

@@ -2,11 +2,13 @@ include_rules = [
   "+components/content_settings/core/browser",
   "+components/history/core/browser",
   "+components/password_manager/core/browser/password_manager_metrics_util.h",
+  "+components/prefs",
   "+components/security_interstitials/content",
   "+components/security_interstitials/core",
   "+components/sync/protocol",
   "+components/sync_preferences/testing_pref_service_syncable.h",
-  "+components/prefs",
+  "+components/unified_consent",
+  "+components/user_prefs/user_prefs.h",
   "+content/public/browser",
   "+content/public/common",
   "+content/public/test",

components/safe_browsing/browser/browser_url_loader_throttle.cc

@@ -11,7 +11,9 @@
 #include "components/safe_browsing/browser/url_checker_delegate.h"
 #include "components/safe_browsing/common/safebrowsing_constants.h"
 #include "components/safe_browsing/common/utils.h"
+#include "components/safe_browsing/realtime/policy_engine.h"
 #include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/web_contents.h"
 #include "net/log/net_log_event_type.h"
 #include "net/url_request/redirect_info.h"
 #include "services/network/public/cpp/resource_request.h"
@@ -29,12 +31,14 @@ class BrowserURLLoaderThrottle::CheckerOnIO
       content::ResourceContext* resource_context,
       int frame_tree_node_id,
       base::RepeatingCallback<content::WebContents*()> web_contents_getter,
-      base::WeakPtr<BrowserURLLoaderThrottle> throttle)
+      base::WeakPtr<BrowserURLLoaderThrottle> throttle,
+      bool real_time_lookup_enabled)
       : delegate_getter_(std::move(delegate_getter)),
         resource_context_(resource_context),
         frame_tree_node_id_(frame_tree_node_id),
         web_contents_getter_(web_contents_getter),
-        throttle_(std::move(throttle)) {}
+        throttle_(std::move(throttle)),
+        real_time_lookup_enabled_(real_time_lookup_enabled) {}
 
   // Starts the initial safe browsing check. This check and future checks may be
   // skipped after checking with the UrlCheckerDelegate.
@@ -63,7 +67,7 @@ class BrowserURLLoaderThrottle::CheckerOnIO
 
     url_checker_ = std::make_unique<SafeBrowsingUrlCheckerImpl>(
         headers, load_flags, resource_type, has_user_gesture,
-        url_checker_delegate, web_contents_getter_);
+        url_checker_delegate, web_contents_getter_, real_time_lookup_enabled_);
 
     CheckUrl(url, method);
   }
@@ -130,6 +134,7 @@ class BrowserURLLoaderThrottle::CheckerOnIO
   base::RepeatingCallback<content::WebContents*()> web_contents_getter_;
   bool skip_checks_ = false;
   base::WeakPtr<BrowserURLLoaderThrottle> throttle_;
+  bool real_time_lookup_enabled_ = false;
 };
 
 // static
@@ -150,9 +155,18 @@ BrowserURLLoaderThrottle::BrowserURLLoaderThrottle(
     int frame_tree_node_id,
     content::ResourceContext* resource_context) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Decide whether to do real time URL lookups or not.
+  content::WebContents* web_contents = web_contents_getter.Run();
+  bool real_time_lookup_enabled =
+      web_contents ? RealTimePolicyEngine::CanPerformFullURLLookup(
+                         web_contents->GetBrowserContext())
+                   : false;
+
   io_checker_ = std::make_unique<CheckerOnIO>(
       std::move(delegate_getter), resource_context, frame_tree_node_id,
-      web_contents_getter, weak_factory_.GetWeakPtr());
+      web_contents_getter, weak_factory_.GetWeakPtr(),
+      real_time_lookup_enabled);
 }
 
 BrowserURLLoaderThrottle::~BrowserURLLoaderThrottle() {

components/safe_browsing/browser/mojo_safe_browsing_impl.cc

@@ -147,11 +147,16 @@ void MojoSafeBrowsingImpl::CreateCheckerAndCheck(
     return;
   }
 
+  // This is not called for frame resources, and real time URL checks currently
+  // only support frame resources. If we extend real time URL checks to support
+  // non-main frames, we will need to provide the user preferences regarding
+  // real time lookup here.
   auto checker_impl = std::make_unique<SafeBrowsingUrlCheckerImpl>(
       headers, static_cast<int>(load_flags), resource_type, has_user_gesture,
       delegate_,
       base::Bind(&GetWebContentsFromID, render_process_id_,
-                 static_cast<int>(render_frame_id)));
+                 static_cast<int>(render_frame_id)),
+      /*real_time_lookup_enabled=*/false);
 
   checker_impl->CheckUrl(
       url, method,

components/safe_browsing/browser/safe_browsing_url_checker_impl.cc

@@ -99,14 +99,16 @@ SafeBrowsingUrlCheckerImpl::SafeBrowsingUrlCheckerImpl(
     content::ResourceType resource_type,
     bool has_user_gesture,
     scoped_refptr<UrlCheckerDelegate> url_checker_delegate,
-    const base::Callback<content::WebContents*()>& web_contents_getter)
+    const base::Callback<content::WebContents*()>& web_contents_getter,
+    bool real_time_lookup_enabled)
     : headers_(headers),
       load_flags_(load_flags),
       resource_type_(resource_type),
       has_user_gesture_(has_user_gesture),
       web_contents_getter_(web_contents_getter),
       url_checker_delegate_(std::move(url_checker_delegate)),
-      database_manager_(url_checker_delegate_->GetDatabaseManager()) {}
+      database_manager_(url_checker_delegate_->GetDatabaseManager()),
+      real_time_lookup_enabled_(real_time_lookup_enabled) {}
 
 SafeBrowsingUrlCheckerImpl::~SafeBrowsingUrlCheckerImpl() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
@@ -293,7 +295,8 @@ void SafeBrowsingUrlCheckerImpl::ProcessUrls() {
 
     bool safe_synchronously;
     auto* rt_lookup_service = database_manager_->GetRealTimeUrlLookupService();
-    if (RealTimePolicyEngine::CanPerformFullURLLookupForResourceType(
+    if (real_time_lookup_enabled_ &&
+        RealTimePolicyEngine::CanPerformFullURLLookupForResourceType(
             resource_type_) &&
         rt_lookup_service && rt_lookup_service->CanCheckUrl(url) &&
         !rt_lookup_service->IsInBackoffMode()) {

components/safe_browsing/browser/safe_browsing_url_checker_impl.h

@@ -58,13 +58,18 @@ class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker,
                               bool /* proceed */,
                               bool /* showed_interstitial */)>;
 
+  // Constructor for SafeBrowsingUrlCheckerImpl. |real_time_lookup_enabled|
+  // indicates whether or not the profile has enabled real time URL lookups, as
+  // computed by the RealTimePolicyEngine. This must be computed in advance,
+  // since this class only exists on the IO thread.
   SafeBrowsingUrlCheckerImpl(
       const net::HttpRequestHeaders& headers,
       int load_flags,
       content::ResourceType resource_type,
       bool has_user_gesture,
       scoped_refptr<UrlCheckerDelegate> url_checker_delegate,
-      const base::Callback<content::WebContents*()>& web_contents_getter);
+      const base::Callback<content::WebContents*()>& web_contents_getter,
+      bool real_time_lookup_enabled);
 
   ~SafeBrowsingUrlCheckerImpl() override;
 
@@ -181,6 +186,9 @@ class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker,
   // Timer to abort the SafeBrowsing check if it takes too long.
   base::OneShotTimer timer_;
 
+  // Whether real time lookup is enabled for this request.
+  bool real_time_lookup_enabled_;
+
   base::WeakPtrFactory<SafeBrowsingUrlCheckerImpl> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingUrlCheckerImpl);

components/safe_browsing/realtime/BUILD.gn

@@ -14,6 +14,9 @@ static_library("policy_engine") {
     "//components/safe_browsing:features",
     "//components/safe_browsing:realtimeapi_proto",
     "//components/safe_browsing/common:safe_browsing_prefs",
+    "//components/unified_consent",
+    "//components/user_prefs",
+    "//content/public/browser",
     "//content/public/common:resource_type_header",
   ]
 }
@@ -45,6 +48,10 @@ source_set("unit_tests") {
     ":url_lookup_service",
     "//base/test:test_support",
     "//components/safe_browsing:features",
+    "//components/safe_browsing/common:safe_browsing_prefs",
+    "//components/sync_preferences:test_support",
+    "//components/unified_consent",
+    "//components/user_prefs",
     "//content/test:test_support",
     "//services/network:test_support",
     "//services/network/public/cpp:cpp",

components/safe_browsing/realtime/policy_engine.cc

@@ -9,11 +9,12 @@
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/features.h"
+#include "components/unified_consent/pref_names.h"
+#include "components/user_prefs/user_prefs.h"
+#include "content/public/browser/browser_context.h"
 
 namespace safe_browsing {
 
-bool RealTimePolicyEngine::is_enabled_by_pref_ = false;
-
 // static
 bool RealTimePolicyEngine::IsFetchAllowlistEnabled() {
   return base::FeatureList::IsEnabled(kRealTimeUrlLookupFetchAllowlist);
@@ -25,39 +26,38 @@ bool RealTimePolicyEngine::IsUrlLookupEnabled() {
 }
 
 // static
-bool RealTimePolicyEngine::IsUserOptedIn() {
-  // TODO(crbug.com/991394): Implement this soon. For now, disabled.
-  return false;
+bool RealTimePolicyEngine::IsUserOptedIn(
+    content::BrowserContext* browser_context) {
+  PrefService* pref_service = user_prefs::UserPrefs::Get(browser_context);
+  return pref_service->GetBoolean(
+      unified_consent::prefs::kUrlKeyedAnonymizedDataCollectionEnabled);
 }
 
 // static
-bool RealTimePolicyEngine::CanPerformFullURLLookup() {
-  // TODO(crbug.com/991394): This should take into account whether the user is
-  // eligible for this service (see "Target Users" in the design doc).
+bool RealTimePolicyEngine::IsEnabledByPolicy(
+    content::BrowserContext* browser_context) {
+  PrefService* pref_service = user_prefs::UserPrefs::Get(browser_context);
+  return pref_service->GetBoolean(prefs::kSafeBrowsingRealTimeLookupEnabled);
+}
 
+// static
+bool RealTimePolicyEngine::CanPerformFullURLLookup(
+    content::BrowserContext* browser_context) {
   if (!IsFetchAllowlistEnabled())
     return false;
 
-  if (is_enabled_by_pref())
+  if (IsEnabledByPolicy(browser_context))
     return true;
 
-  return IsUrlLookupEnabled() && IsUserOptedIn();
+  return IsUrlLookupEnabled() && IsUserOptedIn(browser_context);
 }
 
 // static
 bool RealTimePolicyEngine::CanPerformFullURLLookupForResourceType(
     content::ResourceType resource_type) {
-  if (!CanPerformFullURLLookup())
-    return false;
-
   UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.RT.ResourceTypes.Requested",
                             resource_type);
   return resource_type == content::ResourceType::kMainFrame;
 }
 
-// static
-void RealTimePolicyEngine::SetEnabled(bool is_enabled) {
-  is_enabled_by_pref_ = is_enabled;
-}
-
 }  // namespace safe_browsing

components/safe_browsing/realtime/policy_engine.h

@@ -7,6 +7,10 @@
 
 #include "content/public/common/resource_type.h"
 
+namespace content {
+class BrowserContext;
+}
+
 namespace safe_browsing {
 
 // This class implements the logic to decide whether the real time lookup
@@ -19,16 +23,15 @@ class RealTimePolicyEngine {
   // Is the feature to sync high confidence allowlist enabled?
   static bool IsFetchAllowlistEnabled();
 
-  // Return true if the feature to enable full URL lookups is enabled and the
-  // allowlist fetch is enabled, |resource_type| is kMainFrame.
+  // Return true if full URL lookups are enabled for |resource_type|.
   static bool CanPerformFullURLLookupForResourceType(
       content::ResourceType resource_type);
 
   // Return true if the feature to enable full URL lookups is enabled and the
-  // allowlist fetch is enabled.
-  static bool CanPerformFullURLLookup();
+  // allowlist fetch is enabled for the profile represented by
+  // |browser_context|.
+  static bool CanPerformFullURLLookup(content::BrowserContext* browser_context);
 
-  static bool is_enabled_by_pref() { return is_enabled_by_pref_; }
   friend class SafeBrowsingService;
 
  private:
@@ -36,13 +39,10 @@ class RealTimePolicyEngine {
   static bool IsUrlLookupEnabled();
 
   // Is user opted-in to the feature?
-  static bool IsUserOptedIn();
+  static bool IsUserOptedIn(content::BrowserContext* browser_context);
 
-  // TODO(crbug.com/991394): This is a temporary way of checking whether the
-  // real-time lookup is enabled for any active profile. This must be fixed
-  // before full launch.
-  static bool is_enabled_by_pref_;
-  static void SetEnabled(bool is_enabled);
+  // Is the feature enabled due to enterprise policy?
+  static bool IsEnabledByPolicy(content::BrowserContext* browser_context);
 
   friend class RealTimePolicyEngineTest;
 };  // class RealTimePolicyEngine

components/safe_browsing/realtime/policy_engine_unittest.cc

@@ -5,54 +5,78 @@
 #include "components/safe_browsing/realtime/policy_engine.h"
 
 #include "base/test/scoped_feature_list.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/features.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "components/unified_consent/pref_names.h"
+#include "components/unified_consent/unified_consent_service.h"
+#include "components/user_prefs/user_prefs.h"
+#include "content/public/test/browser_task_environment.h"
+#include "content/public/test/test_browser_context.h"
 #include "testing/platform_test.h"
 
 namespace safe_browsing {
 
 class RealTimePolicyEngineTest : public PlatformTest {
  public:
-  void ForceSetRealTimeLookupPolicy(bool is_enabled) {
-    RealTimePolicyEngine::SetEnabled(is_enabled);
+  void SetUp() override {
+    user_prefs::UserPrefs::Set(&test_context_, &pref_service_);
+    RegisterProfilePrefs(pref_service_.registry());
+    unified_consent::UnifiedConsentService::RegisterPrefs(
+        pref_service_.registry());
   }
 
-  bool IsUserOptedIn() { return RealTimePolicyEngine::IsUserOptedIn(); }
+  bool IsUserOptedIn() {
+    return RealTimePolicyEngine::IsUserOptedIn(&test_context_);
+  }
+
+  bool CanPerformFullURLLookup() {
+    return RealTimePolicyEngine::CanPerformFullURLLookup(&test_context_);
+  }
+
+  content::BrowserTaskEnvironment task_environment_;
+  content::TestBrowserContext test_context_;
+  sync_preferences::TestingPrefServiceSyncable pref_service_;
 };
 
 TEST_F(RealTimePolicyEngineTest,
        TestCanPerformFullURLLookup_DisabledFetchAllowlist) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndDisableFeature(kRealTimeUrlLookupFetchAllowlist);
-  EXPECT_FALSE(RealTimePolicyEngine::CanPerformFullURLLookup());
+  EXPECT_FALSE(CanPerformFullURLLookup());
 }
 
 TEST_F(RealTimePolicyEngineTest, TestCanPerformFullURLLookup_EnabledByPolicy) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(kRealTimeUrlLookupFetchAllowlist);
-  ForceSetRealTimeLookupPolicy(/* is_enabled= */ true);
-  ASSERT_TRUE(RealTimePolicyEngine::is_enabled_by_pref());
-  EXPECT_TRUE(RealTimePolicyEngine::CanPerformFullURLLookup());
+  pref_service_.SetUserPref(prefs::kSafeBrowsingRealTimeLookupEnabled,
+                            std::make_unique<base::Value>(true));
+  EXPECT_TRUE(CanPerformFullURLLookup());
 }
 
 TEST_F(RealTimePolicyEngineTest,
        TestCanPerformFullURLLookup_DisabledUrlLookup) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndDisableFeature(kRealTimeUrlLookupEnabled);
-  EXPECT_FALSE(RealTimePolicyEngine::CanPerformFullURLLookup());
+  EXPECT_FALSE(CanPerformFullURLLookup());
 }
 
 TEST_F(RealTimePolicyEngineTest,
        TestCanPerformFullURLLookup_DisabledUserOptin) {
-  // This is hardcoded as false for now so ensure that.
   ASSERT_FALSE(IsUserOptedIn());
 }
 
+TEST_F(RealTimePolicyEngineTest, TestCanPerformFullURLLookup_EnabledUserOptin) {
+  pref_service_.SetUserPref(
+      unified_consent::prefs::kUrlKeyedAnonymizedDataCollectionEnabled,
+      std::make_unique<base::Value>(true));
+  ASSERT_TRUE(IsUserOptedIn());
+}
+
 TEST_F(RealTimePolicyEngineTest,
        TestCanPerformFullURLLookup_EnabledMainFrameOnly) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(kRealTimeUrlLookupFetchAllowlist);
-  ForceSetRealTimeLookupPolicy(/* is_enabled= */ true);
-  ASSERT_TRUE(RealTimePolicyEngine::is_enabled_by_pref());
 
   for (int i = 0; i <= static_cast<int>(content::ResourceType::kMaxValue);
        i++) {