safe_browsing_url_checker supports Safe Browsing disabled mode.

Enterprise real time URL check should work when Safe Browsing is
disabled. Currently it is not supported because
GetSafeBrowsingUrlCheckerDelegate returns nullptr when Safe Browsing
is disabled.

In this CL, add support for this scenario. Don't return nullptr when
enterprise real time URL check is enabled. Also pass in a boolean
named can_check_db to safe_browsing_url_checker. If this boolean is set
to false, bypass all database check and return safe.

Bug: 1085261
Change-Id: If1276f762dbabccb53adb7aa29814e41fbe7b1f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2236605
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777005}

chrome/browser/chrome_content_browser_client.cc

@@ -4348,33 +4348,30 @@ ChromeContentBrowserClient::CreateURLLoaderThrottles(
   bool matches_enterprise_whitelist = safe_browsing::IsURLWhitelistedByPolicy(
       request.url, *profile->GetPrefs());
   if (!matches_enterprise_whitelist) {
-    // |url_lookup_service| is used when real time url check is enabled.
-    safe_browsing::RealTimeUrlLookupServiceBase* url_lookup_service = nullptr;
-
+    bool is_enterprise_lookup_enabled =
 #if BUILDFLAG(SAFE_BROWSING_DB_LOCAL)
-    if (safe_browsing::RealTimePolicyEngine::CanPerformEnterpriseFullURLLookup(
+        safe_browsing::RealTimePolicyEngine::CanPerformEnterpriseFullURLLookup(
             safe_browsing::GetDMToken(profile).is_valid(),
-            profile->IsOffTheRecord())) {
-      url_lookup_service =
-          safe_browsing::ChromeEnterpriseRealTimeUrlLookupServiceFactory::
-              GetForProfile(profile);
-    }
+            profile->IsOffTheRecord());
+#else
+        false;
 #endif
-
-    // |safe_browsing_service_| may be unavailable in tests.
-    if (!url_lookup_service && safe_browsing_service_ &&
+    bool is_consumer_lookup_enabled =
         safe_browsing::RealTimePolicyEngine::CanPerformFullURLLookup(
             profile->GetPrefs(), profile->IsOffTheRecord(),
-            g_browser_process->variations_service())) {
-      url_lookup_service =
-          safe_browsing::RealTimeUrlLookupServiceFactory::GetForProfile(
-              profile);
-    }
+            g_browser_process->variations_service());
+
+    // |url_lookup_service| is used when real time url check is enabled.
+    safe_browsing::RealTimeUrlLookupServiceBase* url_lookup_service =
+        GetUrlLookupService(browser_context, is_enterprise_lookup_enabled,
+                            is_consumer_lookup_enabled);
     result.push_back(safe_browsing::BrowserURLLoaderThrottle::Create(
         base::BindOnce(
             &ChromeContentBrowserClient::GetSafeBrowsingUrlCheckerDelegate,
             base::Unretained(this),
-            safe_browsing::IsSafeBrowsingEnabled(*profile->GetPrefs())),
+            safe_browsing::IsSafeBrowsingEnabled(*profile->GetPrefs()),
+            // Should check for enterprise when safe browsing is disabled.
+            /*should_check_on_sb_disabled=*/is_enterprise_lookup_enabled),
         wc_getter, frame_tree_node_id,
         url_lookup_service ? url_lookup_service->GetWeakPtr() : nullptr));
   }
@@ -5119,10 +5116,13 @@ const ui::NativeTheme* ChromeContentBrowserClient::GetWebTheme() const {
 
 scoped_refptr<safe_browsing::UrlCheckerDelegate>
 ChromeContentBrowserClient::GetSafeBrowsingUrlCheckerDelegate(
-    bool safe_browsing_enabled_for_profile) {
+    bool safe_browsing_enabled_for_profile,
+    bool should_check_on_sb_disabled) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  if (!safe_browsing_enabled_for_profile)
+  // Should not bypass safe browsing check if the check is for enterprise
+  // lookup.
+  if (!safe_browsing_enabled_for_profile && !should_check_on_sb_disabled)
     return nullptr;
 
   // |safe_browsing_service_| may be unavailable in tests.
@@ -5136,6 +5136,32 @@ ChromeContentBrowserClient::GetSafeBrowsingUrlCheckerDelegate(
   return safe_browsing_url_checker_delegate_;
 }
 
+safe_browsing::RealTimeUrlLookupServiceBase*
+ChromeContentBrowserClient::GetUrlLookupService(
+    content::BrowserContext* browser_context,
+    bool is_enterprise_lookup_enabled,
+    bool is_consumer_lookup_enabled) {
+  // |safe_browsing_service_| may be unavailable in tests.
+  if (!safe_browsing_service_) {
+    return nullptr;
+  }
+
+  Profile* profile = Profile::FromBrowserContext(browser_context);
+
+#if BUILDFLAG(SAFE_BROWSING_DB_LOCAL)
+  if (is_enterprise_lookup_enabled) {
+    return safe_browsing::ChromeEnterpriseRealTimeUrlLookupServiceFactory::
+        GetForProfile(profile);
+  }
+#endif
+
+  if (is_consumer_lookup_enabled) {
+    return safe_browsing::RealTimeUrlLookupServiceFactory::GetForProfile(
+        profile);
+  }
+  return nullptr;
+}
+
 base::Optional<std::string>
 ChromeContentBrowserClient::GetOriginPolicyErrorPage(
     network::OriginPolicyState error_reason,

chrome/browser/chrome_content_browser_client.h

@@ -60,6 +60,7 @@ class PreviewsUserData;
 }  // namespace previews
 
 namespace safe_browsing {
+class RealTimeUrlLookupServiceBase;
 class SafeBrowsingService;
 class UrlCheckerDelegate;
 }  // namespace safe_browsing
@@ -718,8 +719,25 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
       bool allow);
 #endif
 
+  // Returns the existing UrlCheckerDelegate object if it is already created.
+  // Otherwise, creates a new one and returns it. It returns nullptr if
+  // |safe_browsing_enabled_for_profile| is false, because it should bypass safe
+  // browsing check when safe browsing is disabled. Set
+  // |should_check_on_sb_disabled| to true if you still want to perform safe
+  // browsing check when safe browsing is disabled(e.g. for enterprise real time
+  // URL check).
   scoped_refptr<safe_browsing::UrlCheckerDelegate>
-  GetSafeBrowsingUrlCheckerDelegate(bool safe_browsing_enabled_for_profile);
+  GetSafeBrowsingUrlCheckerDelegate(bool safe_browsing_enabled_for_profile,
+                                    bool should_check_on_sb_disabled);
+
+  // Returns a RealTimeUrlLookupServiceBase object used for real time URL check.
+  // Returns an enterprise version if |is_enterprise_lookup_enabled| is true.
+  // Returns a consumer version if |is_enterprise_lookup_enabled| is false and
+  // |is_consumer_lookup_enabled| is true. Returns nullptr if both are false.
+  safe_browsing::RealTimeUrlLookupServiceBase* GetUrlLookupService(
+      content::BrowserContext* browser_context,
+      bool is_enterprise_lookup_enabled,
+      bool is_consumer_lookup_enabled);
 
   // Vector of additional ChromeContentBrowserClientParts.
   // Parts are deleted in the reverse order they are added.

chrome/browser/chrome_content_browser_client_receiver_bindings.cc

@@ -76,7 +76,8 @@ void MaybeCreateSafeBrowsingForRenderer(
     int process_id,
     content::ResourceContext* resource_context,
     base::RepeatingCallback<scoped_refptr<safe_browsing::UrlCheckerDelegate>(
-        bool safe_browsing_enabled)> get_checker_delegate,
+        bool safe_browsing_enabled,
+        bool should_check_on_sb_disabled)> get_checker_delegate,
     mojo::PendingReceiver<safe_browsing::mojom::SafeBrowsing> receiver) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
@@ -93,7 +94,11 @@ void MaybeCreateSafeBrowsingForRenderer(
       base::BindOnce(
           &safe_browsing::MojoSafeBrowsingImpl::MaybeCreate, process_id,
           resource_context,
-          base::BindRepeating(get_checker_delegate, safe_browsing_enabled),
+          base::BindRepeating(get_checker_delegate, safe_browsing_enabled,
+                              // Navigation initiated from renderer should never
+                              // check when safe browsing is disabled, because
+                              // enterprise check only supports mainframe URL.
+                              /*should_check_on_sb_disabled=*/false),
           std::move(receiver)));
 }
 

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service.cc

@@ -45,6 +45,10 @@ bool ChromeEnterpriseRealTimeUrlLookupService::CanCheckSubresourceURL() const {
   return false;
 }
 
+bool ChromeEnterpriseRealTimeUrlLookupService::CanCheckSafeBrowsingDb() const {
+  return safe_browsing::IsSafeBrowsingEnabled(*profile_->GetPrefs());
+}
+
 policy::DMToken ChromeEnterpriseRealTimeUrlLookupService::GetDMToken() const {
   return ::safe_browsing::GetDMToken(profile_);
 }

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service.h

@@ -40,6 +40,8 @@ class ChromeEnterpriseRealTimeUrlLookupService
 
   bool CanCheckSubresourceURL() const override;
 
+  bool CanCheckSafeBrowsingDb() const override;
+
   void StartLookup(const GURL& url,
                    RTLookupRequestCallback request_callback,
                    RTLookupResponseCallback response_callback) override;

components/safe_browsing/content/browser/browser_url_loader_throttle.cc

@@ -35,13 +35,15 @@ class BrowserURLLoaderThrottle::CheckerOnIO
       base::WeakPtr<BrowserURLLoaderThrottle> throttle,
       bool real_time_lookup_enabled,
       bool can_rt_check_subresource_url,
+      bool can_check_db,
       base::WeakPtr<RealTimeUrlLookupServiceBase> url_lookup_service)
       : delegate_getter_(std::move(delegate_getter)),
         frame_tree_node_id_(frame_tree_node_id),
         web_contents_getter_(web_contents_getter),
         throttle_(std::move(throttle)),
         real_time_lookup_enabled_(real_time_lookup_enabled),
         can_rt_check_subresource_url_(can_rt_check_subresource_url),
+        can_check_db_(can_check_db),
         url_lookup_service_(url_lookup_service) {}
 
   // Starts the initial safe browsing check. This check and future checks may be
@@ -72,7 +74,7 @@ class BrowserURLLoaderThrottle::CheckerOnIO
     url_checker_ = std::make_unique<SafeBrowsingUrlCheckerImpl>(
         headers, load_flags, resource_type, has_user_gesture,
         url_checker_delegate, web_contents_getter_, real_time_lookup_enabled_,
-        can_rt_check_subresource_url_, url_lookup_service_);
+        can_rt_check_subresource_url_, can_check_db_, url_lookup_service_);
 
     CheckUrl(url, method);
   }
@@ -140,6 +142,7 @@ class BrowserURLLoaderThrottle::CheckerOnIO
   base::WeakPtr<BrowserURLLoaderThrottle> throttle_;
   bool real_time_lookup_enabled_ = false;
   bool can_rt_check_subresource_url_ = false;
+  bool can_check_db_ = true;
   base::WeakPtr<RealTimeUrlLookupServiceBase> url_lookup_service_;
 };
 
@@ -170,10 +173,15 @@ BrowserURLLoaderThrottle::BrowserURLLoaderThrottle(
   bool can_rt_check_subresource_url =
       url_lookup_service && url_lookup_service->CanCheckSubresourceURL();
 
+  // Decide whether safe browsing database can be checked.
+  // If url_lookup_service is null, safe browsing database should be checked by
+  // default.
+  bool can_check_db =
+      url_lookup_service ? url_lookup_service->CanCheckSafeBrowsingDb() : true;
   io_checker_ = std::make_unique<CheckerOnIO>(
       std::move(delegate_getter), frame_tree_node_id, web_contents_getter,
       weak_factory_.GetWeakPtr(), real_time_lookup_enabled,
-      can_rt_check_subresource_url, url_lookup_service);
+      can_rt_check_subresource_url, can_check_db, url_lookup_service);
 }
 
 BrowserURLLoaderThrottle::~BrowserURLLoaderThrottle() {

components/safe_browsing/content/browser/mojo_safe_browsing_impl.cc

@@ -158,6 +158,7 @@ void MojoSafeBrowsingImpl::CreateCheckerAndCheck(
                           static_cast<int>(render_frame_id)),
       /*real_time_lookup_enabled=*/false,
       /*can_rt_check_subresource_url=*/false,
+      /*can_check_db=*/true,
       /*url_lookup_service=*/nullptr);
 
   checker_impl->CheckUrl(

components/safe_browsing/core/browser/safe_browsing_url_checker_impl.cc

@@ -103,6 +103,7 @@ SafeBrowsingUrlCheckerImpl::SafeBrowsingUrlCheckerImpl(
     const base::RepeatingCallback<content::WebContents*()>& web_contents_getter,
     bool real_time_lookup_enabled,
     bool can_rt_check_subresource_url,
+    bool can_check_db,
     base::WeakPtr<RealTimeUrlLookupServiceBase> url_lookup_service_on_ui)
     : headers_(headers),
       load_flags_(load_flags),
@@ -113,9 +114,11 @@ SafeBrowsingUrlCheckerImpl::SafeBrowsingUrlCheckerImpl(
       database_manager_(url_checker_delegate_->GetDatabaseManager()),
       real_time_lookup_enabled_(real_time_lookup_enabled),
       can_rt_check_subresource_url_(can_rt_check_subresource_url),
+      can_check_db_(can_check_db),
       url_lookup_service_on_ui_(url_lookup_service_on_ui) {
   DCHECK(!web_contents_getter_.is_null());
   DCHECK(!can_rt_check_subresource_url_ || real_time_lookup_enabled_);
+  DCHECK(real_time_lookup_enabled_ || can_check_db_);
 }
 
 SafeBrowsingUrlCheckerImpl::SafeBrowsingUrlCheckerImpl(
@@ -128,15 +131,19 @@ SafeBrowsingUrlCheckerImpl::SafeBrowsingUrlCheckerImpl(
       web_state_getter_(web_state_getter),
       url_checker_delegate_(url_checker_delegate),
       database_manager_(url_checker_delegate_->GetDatabaseManager()),
-      real_time_lookup_enabled_(false) {
+      real_time_lookup_enabled_(false),
+      can_rt_check_subresource_url_(false),
+      can_check_db_(true) {
   DCHECK(!web_state_getter_.is_null());
 }
 
 SafeBrowsingUrlCheckerImpl::~SafeBrowsingUrlCheckerImpl() {
   DCHECK(CurrentlyOnThread(ThreadID::IO));
 
   if (state_ == STATE_CHECKING_URL) {
-    database_manager_->CancelCheck(this);
+    if (can_check_db_) {
+      database_manager_->CancelCheck(this);
+    }
     const GURL& url = urls_[next_index_].url;
     TRACE_EVENT_ASYNC_END1("safe_browsing", "CheckUrl", this, "url",
                            url.spec());
@@ -270,7 +277,9 @@ void SafeBrowsingUrlCheckerImpl::OnUrlResult(const GURL& url,
 void SafeBrowsingUrlCheckerImpl::OnTimeout() {
   RecordCheckUrlTimeout(/*timed_out=*/true);
 
-  database_manager_->CancelCheck(this);
+  if (can_check_db_) {
+    database_manager_->CancelCheck(this);
+  }
 
   // Any pending callbacks on this URL check should be skipped.
   weak_factory_.InvalidateWeakPtrs();
@@ -363,7 +372,9 @@ void SafeBrowsingUrlCheckerImpl::ProcessUrls() {
                                 resource_type_);
       safe_synchronously = false;
       AsyncMatch match =
-          database_manager_->CheckUrlForHighConfidenceAllowlist(url, this);
+          can_check_db_
+              ? database_manager_->CheckUrlForHighConfidenceAllowlist(url, this)
+              : AsyncMatch::NO_MATCH;
       UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.RT.LocalMatch.Result", match);
       switch (match) {
         case AsyncMatch::ASYNC:
@@ -382,8 +393,8 @@ void SafeBrowsingUrlCheckerImpl::ProcessUrls() {
                              /*did_match_allowlist=*/true));
           break;
         case AsyncMatch::NO_MATCH:
-          // No match found locally. Queue the call to
-          // |OnCheckUrlForHighConfidenceAllowlist| to perform the full URL
+          // No match found locally or |can_check_db_| is false. Queue the call
+          // to |OnCheckUrlForHighConfidenceAllowlist| to perform the full URL
           // lookup.
           base::PostTask(
               FROM_HERE, CreateTaskTraits(ThreadID::IO),
@@ -394,8 +405,13 @@ void SafeBrowsingUrlCheckerImpl::ProcessUrls() {
           break;
       }
     } else {
-      safe_synchronously = database_manager_->CheckBrowseUrl(
-          url, url_checker_delegate_->GetThreatTypes(), this);
+      // TODO(crbug.com/1085261): Add a metric to track how often
+      // |can_check_db_| is false.
+      safe_synchronously =
+          can_check_db_
+              ? database_manager_->CheckBrowseUrl(
+                    url, url_checker_delegate_->GetThreatTypes(), this)
+              : true;
     }
 
     if (safe_synchronously) {
@@ -541,7 +557,8 @@ void SafeBrowsingUrlCheckerImpl::StartLookupOnUIThread(
 
 void SafeBrowsingUrlCheckerImpl::PerformHashBasedCheck(const GURL& url) {
   DCHECK(CurrentlyOnThread(ThreadID::IO));
-  if (database_manager_->CheckBrowseUrl(
+  if (!can_check_db_ ||
+      database_manager_->CheckBrowseUrl(
           url, url_checker_delegate_->GetThreatTypes(), this)) {
     // No match found in the local database. Safe to call |OnUrlResult| here
     // directly.

components/safe_browsing/core/browser/safe_browsing_url_checker_impl.h

@@ -86,6 +86,7 @@ class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker,
           web_contents_getter,
       bool real_time_lookup_enabled,
       bool can_rt_check_subresource_url,
+      bool can_check_db,
       base::WeakPtr<RealTimeUrlLookupServiceBase> url_lookup_service_on_ui);
 
   // Constructor that takes only a ResourceType and a UrlCheckerDelegate,
@@ -258,6 +259,11 @@ class SafeBrowsingUrlCheckerImpl : public mojom::SafeBrowsingUrlChecker,
   // Whether non mainframe url can be checked for this profile.
   bool can_rt_check_subresource_url_;
 
+  // Whether safe browsing database can be checked. It is set to false when
+  // enterprise real time URL lookup is enabled and safe browsing is disabled
+  // for this profile.
+  bool can_check_db_;
+
   // This object is used to perform real time url check. Can only be accessed in
   // UI thread.
   base::WeakPtr<RealTimeUrlLookupServiceBase> url_lookup_service_on_ui_;