Set user population for enterprise real time URL check.

Move the FillRequestProto function into the base class. Add a virtual
function named GetDMTokenString. For consumer requests,
GetDMTokenString always returns an empty string; for enterprise
requests, GetDMTokenString returns the value of the dm token.

Bug: 1085261
Change-Id: I79e1ae6cdabb7e5024ef77798c8a608ac1c1ce06
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2321725Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794351}

chrome/browser/safe_browsing/BUILD.gn

@@ -302,10 +302,13 @@ source_set("chrome_enterprise_url_lookup_service") {
   ]
 
   deps = [
+    "//components/prefs",
+    "//components/safe_browsing/core:csd_proto",
     "//components/safe_browsing/core:realtimeapi_proto",
     "//components/safe_browsing/core:verdict_cache_manager",
     "//components/safe_browsing/core/realtime:policy_engine",
     "//components/safe_browsing/core/realtime:url_lookup_service_base",
+    "//components/sync",
     "//services/network/public/cpp:cpp",
   ]
 }

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service.cc

@@ -9,11 +9,14 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/dm_token_utils.h"
 #include "components/policy/core/common/cloud/dm_token.h"
+#include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/common/thread_utils.h"
+#include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/safe_browsing/core/proto/realtimeapi.pb.h"
 #include "components/safe_browsing/core/realtime/policy_engine.h"
 #include "components/safe_browsing/core/realtime/url_lookup_service_base.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
+#include "components/sync/driver/sync_service.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
@@ -24,8 +27,20 @@ ChromeEnterpriseRealTimeUrlLookupService::
     ChromeEnterpriseRealTimeUrlLookupService(
         scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
         VerdictCacheManager* cache_manager,
-        Profile* profile)
-    : RealTimeUrlLookupServiceBase(url_loader_factory, cache_manager),
+        Profile* profile,
+        syncer::SyncService* sync_service,
+        PrefService* pref_service,
+        const ChromeUserPopulation::ProfileManagementStatus&
+            profile_management_status,
+        bool is_under_advanced_protection,
+        bool is_off_the_record)
+    : RealTimeUrlLookupServiceBase(url_loader_factory,
+                                   cache_manager,
+                                   sync_service,
+                                   pref_service,
+                                   profile_management_status,
+                                   is_under_advanced_protection,
+                                   is_off_the_record),
       profile_(profile) {}
 
 ChromeEnterpriseRealTimeUrlLookupService::
@@ -58,22 +73,16 @@ void ChromeEnterpriseRealTimeUrlLookupService::GetAccessToken(
   NOTREACHED() << "URL lookup with token is disabled for enterprise users.";
 }
 
-std::unique_ptr<RTLookupRequest>
-ChromeEnterpriseRealTimeUrlLookupService::FillRequestProto(const GURL& url) {
-  DCHECK(GetDMToken().is_valid())
-      << "Send a request only if the dm token is valid.";
-  auto request = std::make_unique<RTLookupRequest>();
-  request->set_url(SanitizeURL(url).spec());
-  request->set_lookup_type(RTLookupRequest::NAVIGATION);
-  request->set_dm_token(GetDMToken().value());
-  // TODO(crbug.com/1085261): Fill in user population.
-  return request;
-}
-
 policy::DMToken ChromeEnterpriseRealTimeUrlLookupService::GetDMToken() const {
   return ::safe_browsing::GetDMToken(profile_);
 }
 
+std::string ChromeEnterpriseRealTimeUrlLookupService::GetDMTokenString() const {
+  DCHECK(GetDMToken().is_valid())
+      << "Get a dm token string only if the dm token is valid.";
+  return GetDMToken().value();
+}
+
 net::NetworkTrafficAnnotationTag
 ChromeEnterpriseRealTimeUrlLookupService::GetTrafficAnnotationTag() const {
   // Safe Browsing Zwieback cookies are not sent for enterprise users, because

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/safe_browsing/core/realtime/url_lookup_service_base.h"
 #include "url/gurl.h"
 
@@ -23,6 +24,12 @@ namespace policy {
 class DMToken;
 }  // namespace policy
 
+namespace syncer {
+class SyncService;
+}
+
+class PrefService;
+
 class Profile;
 
 namespace safe_browsing {
@@ -36,7 +43,13 @@ class ChromeEnterpriseRealTimeUrlLookupService
   ChromeEnterpriseRealTimeUrlLookupService(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       VerdictCacheManager* cache_manager,
-      Profile* profile);
+      Profile* profile,
+      syncer::SyncService* sync_service,
+      PrefService* pref_service,
+      const ChromeUserPopulation::ProfileManagementStatus&
+          profile_management_status,
+      bool is_under_advanced_protection,
+      bool is_off_the_record);
   ~ChromeEnterpriseRealTimeUrlLookupService() override;
 
   // RealTimeUrlLookupServiceBase:
@@ -51,7 +64,7 @@ class ChromeEnterpriseRealTimeUrlLookupService
   void GetAccessToken(const GURL& url,
                       RTLookupRequestCallback request_callback,
                       RTLookupResponseCallback response_callback) override;
-  std::unique_ptr<RTLookupRequest> FillRequestProto(const GURL& url) override;
+  std::string GetDMTokenString() const override;
   std::string GetMetricSuffix() const override;
 
   policy::DMToken GetDMToken() const;

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service_factory.cc

@@ -5,11 +5,16 @@
 #include "chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service_factory.h"
 
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/advanced_protection_status_manager.h"
+#include "chrome/browser/safe_browsing/advanced_protection_status_manager_factory.h"
 #include "chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/verdict_cache_manager_factory.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/safe_browsing/core/common/utils.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
 #include "content/public/browser/browser_context.h"
 #include "services/network/public/cpp/cross_thread_pending_shared_url_loader_factory.h"
@@ -49,9 +54,17 @@ ChromeEnterpriseRealTimeUrlLookupServiceFactory::BuildServiceInstanceFor(
   auto url_loader_factory =
       std::make_unique<network::CrossThreadPendingSharedURLLoaderFactory>(
           g_browser_process->safe_browsing_service()->GetURLLoaderFactory());
+  const policy::BrowserPolicyConnector* browser_policy_connector =
+      g_browser_process->browser_policy_connector();
+  bool is_under_advanced_protection =
+      AdvancedProtectionStatusManagerFactory::GetForProfile(profile)
+          ->IsUnderAdvancedProtection();
   return new ChromeEnterpriseRealTimeUrlLookupService(
       network::SharedURLLoaderFactory::Create(std::move(url_loader_factory)),
-      VerdictCacheManagerFactory::GetForProfile(profile), profile);
+      VerdictCacheManagerFactory::GetForProfile(profile), profile,
+      ProfileSyncServiceFactory::GetForProfile(profile), profile->GetPrefs(),
+      GetProfileManagementStatus(browser_policy_connector),
+      is_under_advanced_protection, profile->IsOffTheRecord());
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service_unittest.cc

@@ -10,7 +10,9 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/policy/core/common/cloud/dm_token.h"
+#include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
+#include "components/sync/driver/test_sync_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/browser_task_environment.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -52,7 +54,10 @@ class ChromeEnterpriseRealTimeUrlLookupServiceTest : public PlatformTest {
     enterprise_rt_service_ =
         std::make_unique<ChromeEnterpriseRealTimeUrlLookupService>(
             test_shared_loader_factory_, cache_manager_.get(),
-            test_profile_.get());
+            test_profile_.get(), &test_sync_service_, &test_pref_service_,
+            ChromeUserPopulation::NOT_MANAGED,
+            /*is_under_advanced_protection=*/true,
+            /*is_off_the_record=*/false);
   }
 
   void TearDown() override {
@@ -119,6 +124,7 @@ class ChromeEnterpriseRealTimeUrlLookupServiceTest : public PlatformTest {
   content::BrowserTaskEnvironment task_environment_;
   sync_preferences::TestingPrefServiceSyncable test_pref_service_;
   std::unique_ptr<TestingProfile> test_profile_;
+  syncer::TestSyncService test_sync_service_;
 };
 
 TEST_F(ChromeEnterpriseRealTimeUrlLookupServiceTest,
@@ -158,6 +164,12 @@ TEST_F(ChromeEnterpriseRealTimeUrlLookupServiceTest,
           [](std::unique_ptr<RTLookupRequest> request, std::string token) {
             EXPECT_EQ("http://example.test/", request->url());
             EXPECT_EQ("dm_token", request->dm_token());
+            EXPECT_EQ(ChromeUserPopulation::SAFE_BROWSING,
+                      request->population().user_population());
+            EXPECT_TRUE(request->population().is_history_sync_enabled());
+            EXPECT_EQ(ChromeUserPopulation::NOT_MANAGED,
+                      request->population().profile_management_status());
+            EXPECT_TRUE(request->population().is_under_advanced_protection());
             EXPECT_EQ("", token);
           }),
       response_callback.Get());

components/safe_browsing/core/realtime/BUILD.gn

@@ -60,12 +60,14 @@ static_library("url_lookup_service_base") {
     "//base:base",
     "//components/prefs",
     "//components/safe_browsing:buildflags",
+    "//components/safe_browsing/core:csd_proto",
     "//components/safe_browsing/core:realtimeapi_proto",
     "//components/safe_browsing/core:verdict_cache_manager",
     "//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",
   ]

components/safe_browsing/core/realtime/url_lookup_service.cc

@@ -42,12 +42,16 @@ RealTimeUrlLookupService::RealTimeUrlLookupService(
     bool is_under_advanced_protection,
     bool is_off_the_record,
     variations::VariationsService* variations_service)
-    : RealTimeUrlLookupServiceBase(url_loader_factory, cache_manager),
+    : RealTimeUrlLookupServiceBase(url_loader_factory,
+                                   cache_manager,
+                                   sync_service,
+                                   pref_service,
+                                   profile_management_status,
+                                   is_under_advanced_protection,
+                                   is_off_the_record),
       identity_manager_(identity_manager),
       sync_service_(sync_service),
       pref_service_(pref_service),
-      profile_management_status_(profile_management_status),
-      is_under_advanced_protection_(is_under_advanced_protection),
       is_off_the_record_(is_off_the_record),
       variations_(variations_service) {
   token_fetcher_ =
@@ -84,15 +88,6 @@ void RealTimeUrlLookupService::OnGetAccessToken(
 
 RealTimeUrlLookupService::~RealTimeUrlLookupService() {}
 
-// TODO(bdea): Refactor this method into a util class as multiple SB classes
-// have this method.
-bool RealTimeUrlLookupService::IsHistorySyncEnabled() {
-  return sync_service_ && sync_service_->IsSyncFeatureActive() &&
-         !sync_service_->IsLocalSyncEnabled() &&
-         sync_service_->GetActiveDataTypes().Has(
-             syncer::HISTORY_DELETE_DIRECTIVES);
-}
-
 bool RealTimeUrlLookupService::CanPerformFullURLLookup() const {
   return RealTimePolicyEngine::CanPerformFullURLLookup(
       pref_service_, is_off_the_record_, variations_);
@@ -114,30 +109,6 @@ bool RealTimeUrlLookupService::CanCheckSafeBrowsingDb() const {
   return true;
 }
 
-std::unique_ptr<RTLookupRequest> RealTimeUrlLookupService::FillRequestProto(
-    const GURL& url) {
-  auto request = std::make_unique<RTLookupRequest>();
-  request->set_url(SanitizeURL(url).spec());
-  request->set_lookup_type(RTLookupRequest::NAVIGATION);
-
-  ChromeUserPopulation* user_population = request->mutable_population();
-  user_population->set_user_population(
-      IsEnhancedProtectionEnabled(*pref_service_)
-          ? ChromeUserPopulation::ENHANCED_PROTECTION
-          : IsExtendedReportingEnabled(*pref_service_)
-                ? ChromeUserPopulation::EXTENDED_REPORTING
-                : ChromeUserPopulation::SAFE_BROWSING);
-
-  user_population->set_profile_management_status(profile_management_status_);
-  user_population->set_is_history_sync_enabled(IsHistorySyncEnabled());
-#if BUILDFLAG(FULL_SAFE_BROWSING)
-  user_population->set_is_under_advanced_protection(
-      is_under_advanced_protection_);
-#endif
-  user_population->set_is_incognito(is_off_the_record_);
-  return request;
-}
-
 net::NetworkTrafficAnnotationTag
 RealTimeUrlLookupService::GetTrafficAnnotationTag() const {
   return net::DefineNetworkTrafficAnnotation(
@@ -174,6 +145,11 @@ RealTimeUrlLookupService::GetTrafficAnnotationTag() const {
         })");
 }
 
+std::string RealTimeUrlLookupService::GetDMTokenString() const {
+  // DM token should only be set for enterprise requests.
+  return "";
+}
+
 std::string RealTimeUrlLookupService::GetMetricSuffix() const {
   return ".Consumer";
 }

components/safe_browsing/core/realtime/url_lookup_service.h

@@ -79,7 +79,7 @@ class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
   void GetAccessToken(const GURL& url,
                       RTLookupRequestCallback request_callback,
                       RTLookupResponseCallback response_callback) override;
-  std::unique_ptr<RTLookupRequest> FillRequestProto(const GURL& url) override;
+  std::string GetDMTokenString() const override;
   std::string GetMetricSuffix() const override;
 
   // Called when the access token is obtained from |token_fetcher_|.
@@ -90,8 +90,6 @@ class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
       base::TimeTicks get_token_start_time,
       base::Optional<signin::AccessTokenInfo> access_token_info);
 
-  bool IsHistorySyncEnabled();
-
   // Unowned object used for getting access token when real time url check with
   // token is enabled.
   signin::IdentityManager* identity_manager_;
@@ -102,12 +100,6 @@ class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
   // Unowned object used for getting preference settings.
   PrefService* pref_service_;
 
-  const ChromeUserPopulation::ProfileManagementStatus
-      profile_management_status_;
-
-  // Whether the profile is enrolled in  advanced protection.
-  bool is_under_advanced_protection_;
-
   // The token fetcher used for getting access token.
   std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher_;
 

components/safe_browsing/core/realtime/url_lookup_service_base.cc

@@ -11,8 +11,11 @@
 #include "base/strings/string_piece.h"
 #include "base/task/post_task.h"
 #include "base/time/time.h"
+#include "components/prefs/pref_service.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/verdict_cache_manager.h"
+#include "components/sync/driver/sync_service.h"
 #include "net/base/ip_address.h"
 #include "net/base/load_flags.h"
 #include "net/base/url_util.h"
@@ -87,8 +90,20 @@ void RecordNetworkResultWithAndWithoutSuffix(const std::string& metric,
 
 RealTimeUrlLookupServiceBase::RealTimeUrlLookupServiceBase(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    VerdictCacheManager* cache_manager)
-    : url_loader_factory_(url_loader_factory), cache_manager_(cache_manager) {}
+    VerdictCacheManager* cache_manager,
+    syncer::SyncService* sync_service,
+    PrefService* pref_service,
+    const ChromeUserPopulation::ProfileManagementStatus&
+        profile_management_status,
+    bool is_under_advanced_protection,
+    bool is_off_the_record)
+    : url_loader_factory_(url_loader_factory),
+      cache_manager_(cache_manager),
+      sync_service_(sync_service),
+      pref_service_(pref_service),
+      profile_management_status_(profile_management_status),
+      is_under_advanced_protection_(is_under_advanced_protection),
+      is_off_the_record_(is_off_the_record) {}
 
 RealTimeUrlLookupServiceBase::~RealTimeUrlLookupServiceBase() = default;
 
@@ -393,6 +408,38 @@ RealTimeUrlLookupServiceBase::GetResourceRequest() {
   return resource_request;
 }
 
+std::unique_ptr<RTLookupRequest> RealTimeUrlLookupServiceBase::FillRequestProto(
+    const GURL& url) {
+  auto request = std::make_unique<RTLookupRequest>();
+  request->set_url(SanitizeURL(url).spec());
+  request->set_lookup_type(RTLookupRequest::NAVIGATION);
+  request->set_dm_token(GetDMTokenString());
+
+  ChromeUserPopulation* user_population = request->mutable_population();
+  user_population->set_user_population(
+      IsEnhancedProtectionEnabled(*pref_service_)
+          ? ChromeUserPopulation::ENHANCED_PROTECTION
+          : IsExtendedReportingEnabled(*pref_service_)
+                ? ChromeUserPopulation::EXTENDED_REPORTING
+                : ChromeUserPopulation::SAFE_BROWSING);
+
+  user_population->set_profile_management_status(profile_management_status_);
+  user_population->set_is_history_sync_enabled(IsHistorySyncEnabled());
+  user_population->set_is_under_advanced_protection(
+      is_under_advanced_protection_);
+  user_population->set_is_incognito(is_off_the_record_);
+  return request;
+}
+
+// TODO(bdea): Refactor this method into a util class as multiple SB classes
+// have this method.
+bool RealTimeUrlLookupServiceBase::IsHistorySyncEnabled() {
+  return sync_service_ && sync_service_->IsSyncFeatureActive() &&
+         !sync_service_->IsLocalSyncEnabled() &&
+         sync_service_->GetActiveDataTypes().Has(
+             syncer::HISTORY_DELETE_DIRECTIVES);
+}
+
 void RealTimeUrlLookupServiceBase::Shutdown() {
   for (auto& pending : pending_requests_) {
     // Treat all pending requests as safe.

components/safe_browsing/core/realtime/url_lookup_service_base.h

@@ -16,6 +16,7 @@
 #include "base/timer/timer.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/safe_browsing/core/db/v4_protocol_manager_util.h"
+#include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/safe_browsing/core/proto/realtimeapi.pb.h"
 #include "url/gurl.h"
 
@@ -29,6 +30,12 @@ class SimpleURLLoader;
 class SharedURLLoaderFactory;
 }  // namespace network
 
+namespace syncer {
+class SyncService;
+}
+
+class PrefService;
+
 namespace safe_browsing {
 
 using RTLookupRequestCallback =
@@ -45,7 +52,13 @@ class RealTimeUrlLookupServiceBase : public KeyedService {
  public:
   explicit RealTimeUrlLookupServiceBase(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      VerdictCacheManager* cache_manager);
+      VerdictCacheManager* cache_manager,
+      syncer::SyncService* sync_service,
+      PrefService* pref_service,
+      const ChromeUserPopulation::ProfileManagementStatus&
+          profile_management_status,
+      bool is_under_advanced_protection,
+      bool is_off_the_record);
   ~RealTimeUrlLookupServiceBase() override;
 
   // Returns true if |url|'s scheme can be checked.
@@ -123,9 +136,8 @@ class RealTimeUrlLookupServiceBase : public KeyedService {
                               RTLookupRequestCallback request_callback,
                               RTLookupResponseCallback response_callback) = 0;
 
-  // Fills in fields in |RTLookupRequest|.
-  virtual std::unique_ptr<RTLookupRequest> FillRequestProto(
-      const GURL& url) = 0;
+  // Gets a dm token string to be set in a request proto.
+  virtual std::string GetDMTokenString() const = 0;
 
   // Suffix for logging metrics.
   virtual std::string GetMetricSuffix() const = 0;
@@ -175,6 +187,11 @@ class RealTimeUrlLookupServiceBase : public KeyedService {
                            base::TimeTicks request_start_time,
                            std::unique_ptr<std::string> response_body);
 
+  // Fills in fields in |RTLookupRequest|.
+  std::unique_ptr<RTLookupRequest> FillRequestProto(const GURL& url);
+
+  bool IsHistorySyncEnabled();
+
   // Count of consecutive failures to complete URL lookup requests. When it
   // reaches |kMaxFailuresToEnforceBackoff|, we enter the backoff mode. It gets
   // reset when we complete a lookup successfully or when the backoff reset
@@ -200,6 +217,22 @@ class RealTimeUrlLookupServiceBase : public KeyedService {
   // Unowned object used for getting and storing real time url check cache.
   VerdictCacheManager* cache_manager_;
 
+  // Unowned object used for checking sync status of the profile.
+  syncer::SyncService* sync_service_;
+
+  // Unowned object used for getting preference settings.
+  PrefService* pref_service_;
+
+  const ChromeUserPopulation::ProfileManagementStatus
+      profile_management_status_;
+
+  // Whether the profile is enrolled in  advanced protection.
+  bool is_under_advanced_protection_;
+
+  // A boolean indicates whether the profile associated with this
+  // |url_lookup_service| is an off the record profile.
+  bool is_off_the_record_;
+
   // All requests that are sent but haven't received a response yet.
   PendingRTLookupRequests pending_requests_;