Use GetUserPopulation util in Client Side Phishing Detection

This CL removes the code in the ClientSideDetectionService that
populates the ChromeUserPopulation, replacing it with the previously
defined utility.

Bug: 1178997
Change-Id: If799b887e3c15cff01c235c8a1ff2ef2f8e25360
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2716269
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Reviewed-by: Tim Volodine <timvolodine@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#857748}

chrome/browser/safe_browsing/client_side_detection_host_browsertest.cc

@@ -32,8 +32,6 @@ class FakeClientSideDetectionService : public ClientSideDetectionService {
 
   void SendClientReportPhishingRequest(
       std::unique_ptr<ClientPhishingRequest> verdict,
-      bool is_extended_reporting,
-      bool is_enhanced_protection,
       ClientReportPhishingRequestCallback callback) override {
     saved_request_ = *verdict;
     saved_callback_ = std::move(callback);

chrome/browser/safe_browsing/client_side_detection_host_unittest.cc

@@ -122,10 +122,8 @@ class MockClientSideDetectionService : public ClientSideDetectionService {
   MockClientSideDetectionService() : ClientSideDetectionService(nullptr) {}
   ~MockClientSideDetectionService() override {}
 
-  MOCK_METHOD4(SendClientReportPhishingRequest,
+  MOCK_METHOD2(SendClientReportPhishingRequest,
                void(std::unique_ptr<ClientPhishingRequest>,
-                    bool,
-                    bool,
                     ClientReportPhishingRequestCallback));
   MOCK_CONST_METHOD1(IsPrivateIPAddress, bool(const std::string&));
   MOCK_METHOD2(GetValidCachedResult, bool(const GURL&, bool*));
@@ -379,8 +377,7 @@ class ClientSideDetectionHostIncognitoTest
 TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneInvalidVerdict) {
   // Case 0: renderer sends an invalid verdict string that we're unable to
   // parse.
-  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
-      .Times(0);
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _)).Times(0);
   PhishingDetectionDone("Invalid Protocol Buffer");
   EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
 }
@@ -395,8 +392,8 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneNotPhishing) {
   verdict.set_is_phishing(true);
 
   EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 PartiallyEqualVerdict(verdict), _, _, _))
-      .WillOnce(MoveArg<3>(&cb));
+                                 PartiallyEqualVerdict(verdict), _))
+      .WillOnce(MoveArg<1>(&cb));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   ASSERT_FALSE(cb.is_null());
@@ -418,8 +415,8 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneDisabled) {
   verdict.set_is_phishing(true);
 
   EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 PartiallyEqualVerdict(verdict), _, _, _))
-      .WillOnce(MoveArg<3>(&cb));
+                                 PartiallyEqualVerdict(verdict), _))
+      .WillOnce(MoveArg<1>(&cb));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   ASSERT_FALSE(cb.is_null());
@@ -442,8 +439,8 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneShowInterstitial) {
   verdict.set_is_phishing(true);
 
   EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 PartiallyEqualVerdict(verdict), _, _, _))
-      .WillOnce(MoveArg<3>(&cb));
+                                 PartiallyEqualVerdict(verdict), _))
+      .WillOnce(MoveArg<1>(&cb));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
@@ -484,8 +481,8 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneMultiplePings) {
   verdict.set_is_phishing(true);
 
   EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 PartiallyEqualVerdict(verdict), _, _, _))
-      .WillOnce(MoveArg<3>(&cb));
+                                 PartiallyEqualVerdict(verdict), _))
+      .WillOnce(MoveArg<1>(&cb));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
@@ -504,8 +501,8 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneMultiplePings) {
   verdict.set_url(other_phishing_url.spec());
   verdict.set_client_score(0.8f);
   EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 PartiallyEqualVerdict(verdict), _, _, _))
-      .WillOnce(MoveArg<3>(&cb_other));
+                                 PartiallyEqualVerdict(verdict), _))
+      .WillOnce(MoveArg<1>(&cb_other));
   PhishingDetectionDone(verdict.SerializeAsString());
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
@@ -545,8 +542,7 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneVerdictNotPhishing) {
   verdict.set_client_score(0.1f);
   verdict.set_is_phishing(false);
 
-  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
-      .Times(0);
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _)).Times(0);
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
 }
@@ -891,8 +887,7 @@ TEST_F(ClientSideDetectionHostTest, RecordsPhishingDetectorResults) {
 
     base::HistogramTester histogram_tester;
 
-    EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
-        .Times(0);
+    EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _)).Times(0);
     PhishingDetectionDone(verdict.SerializeAsString());
     EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
 
@@ -904,8 +899,7 @@ TEST_F(ClientSideDetectionHostTest, RecordsPhishingDetectorResults) {
   {
     base::HistogramTester histogram_tester;
 
-    EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
-        .Times(0);
+    EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _)).Times(0);
     PhishingDetectionError(mojom::PhishingDetectorResult::CLASSIFIER_NOT_READY);
     EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
 
@@ -917,8 +911,7 @@ TEST_F(ClientSideDetectionHostTest, RecordsPhishingDetectorResults) {
   {
     base::HistogramTester histogram_tester;
 
-    EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
-        .Times(0);
+    EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _)).Times(0);
     PhishingDetectionError(
         mojom::PhishingDetectorResult::FORWARD_BACK_TRANSITION);
     EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
@@ -992,7 +985,7 @@ TEST_F(ClientSideDetectionHostTest, ClearsScreenshotData) {
 
   ClientPhishingRequest request;
 
-  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _))
       .WillOnce(testing::SaveArgPointee<0>(&request));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
@@ -1015,7 +1008,7 @@ TEST_F(ClientSideDetectionHostTest, AllowsScreenshotDataForSBER) {
 
   ClientPhishingRequest request;
 
-  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _))
       .WillOnce(testing::SaveArgPointee<0>(&request));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));

chrome/browser/safe_browsing/client_side_detection_service_delegate.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/safe_browsing/user_population.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/common/utils.h"
 
@@ -43,10 +44,8 @@ ClientSideDetectionServiceDelegate::GetSafeBrowsingURLLoaderFactory() {
   return nullptr;
 }
 
-ChromeUserPopulation::ProfileManagementStatus
-ClientSideDetectionServiceDelegate::GetManagementStatus() {
-  return GetProfileManagementStatus(
-      g_browser_process->browser_policy_connector());
+ChromeUserPopulation ClientSideDetectionServiceDelegate::GetUserPopulation() {
+  return ::safe_browsing::GetUserPopulation(profile_);
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/client_side_detection_service_delegate.h

@@ -24,7 +24,7 @@ class ClientSideDetectionServiceDelegate
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
   scoped_refptr<network::SharedURLLoaderFactory>
   GetSafeBrowsingURLLoaderFactory() override;
-  ChromeUserPopulation::ProfileManagementStatus GetManagementStatus() override;
+  ChromeUserPopulation GetUserPopulation() override;
 
  private:
   Profile* profile_;

chrome/browser/safe_browsing/client_side_detection_service_unittest.cc

@@ -83,10 +83,7 @@ class ClientSideDetectionServiceTest : public testing::Test {
     csd_service_.reset();
   }
 
-  bool SendClientReportPhishingRequest(const GURL& phishing_url,
-                                       float score,
-                                       bool is_extended_reporting,
-                                       bool is_enhanced_reporting) {
+  bool SendClientReportPhishingRequest(const GURL& phishing_url, float score) {
     std::unique_ptr<ClientPhishingRequest> request =
         std::make_unique<ClientPhishingRequest>(ClientPhishingRequest());
     request->set_url(phishing_url.spec());
@@ -95,7 +92,7 @@ class ClientSideDetectionServiceTest : public testing::Test {
 
     base::RunLoop run_loop;
     csd_service_->SendClientReportPhishingRequest(
-        std::move(request), is_extended_reporting, is_enhanced_reporting,
+        std::move(request),
         base::BindOnce(&ClientSideDetectionServiceTest::SendRequestDone,
                        base::Unretained(this), run_loop.QuitWhenIdleClosure()));
     phishing_url_ = phishing_url;
@@ -246,30 +243,29 @@ TEST_F(ClientSideDetectionServiceTest, SendClientReportPhishingRequest) {
 
   // Safe browsing is not enabled.
   profile_->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled, false);
-  EXPECT_FALSE(SendClientReportPhishingRequest(url, score, false, true));
+  EXPECT_FALSE(SendClientReportPhishingRequest(url, score));
 
   profile_->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled, true);
   base::Time before = base::Time::Now();
 
   // Invalid response body from the server.
   SetClientReportPhishingResponse("invalid proto response", net::OK);
-  EXPECT_FALSE(SendClientReportPhishingRequest(url, score, false, false));
+  EXPECT_FALSE(SendClientReportPhishingRequest(url, score));
 
   // Normal behavior.
   ClientPhishingResponse response;
   response.set_phishy(true);
   SetClientReportPhishingResponse(response.SerializeAsString(), net::OK);
-  EXPECT_TRUE(SendClientReportPhishingRequest(url, score, false, true));
-  EXPECT_TRUE(SendClientReportPhishingRequest(url, score, true, false));
-  EXPECT_TRUE(SendClientReportPhishingRequest(url, score, false, false));
+  EXPECT_TRUE(SendClientReportPhishingRequest(url, score));
+  EXPECT_TRUE(SendClientReportPhishingRequest(url, score));
+  EXPECT_TRUE(SendClientReportPhishingRequest(url, score));
 
   // This request will fail
   GURL second_url("http://b.com/");
   response.set_phishy(false);
   SetClientReportPhishingResponse(response.SerializeAsString(),
                                   net::ERR_FAILED);
-  EXPECT_FALSE(
-      SendClientReportPhishingRequest(second_url, score, false, false));
+  EXPECT_FALSE(SendClientReportPhishingRequest(second_url, score));
 
   base::Time after = base::Time::Now();
 

components/safe_browsing/content/browser/client_side_detection_host.cc

@@ -451,11 +451,8 @@ void ClientSideDetectionHost::PhishingDetectionDone(
           base::BindOnce(&ClientSideDetectionHost::MaybeShowPhishingWarning,
                          weak_factory_.GetWeakPtr(),
                          /*is_from_cache=*/false);
-      csd_service_->SendClientReportPhishingRequest(
-          std::move(verdict),
-          IsExtendedReportingEnabled(*delegate_->GetPrefs()),
-          IsEnhancedProtectionEnabled(*delegate_->GetPrefs()),
-          std::move(callback));
+      csd_service_->SendClientReportPhishingRequest(std::move(verdict),
+                                                    std::move(callback));
     }
   }
 }

components/safe_browsing/content/browser/client_side_detection_service.cc

@@ -146,16 +146,13 @@ void ClientSideDetectionService::OnPrefsUpdated() {
 
 void ClientSideDetectionService::SendClientReportPhishingRequest(
     std::unique_ptr<ClientPhishingRequest> verdict,
-    bool is_extended_reporting,
-    bool is_enhanced_reporting,
     ClientReportPhishingRequestCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(
           &ClientSideDetectionService::StartClientReportPhishingRequest,
-          weak_factory_.GetWeakPtr(), std::move(verdict), is_extended_reporting,
-          is_enhanced_reporting, std::move(callback)));
+          weak_factory_.GetWeakPtr(), std::move(verdict), std::move(callback)));
 }
 
 bool ClientSideDetectionService::IsPrivateIPAddress(
@@ -205,8 +202,6 @@ void ClientSideDetectionService::SendModelToRenderers() {
 
 void ClientSideDetectionService::StartClientReportPhishingRequest(
     std::unique_ptr<ClientPhishingRequest> request,
-    bool is_extended_reporting,
-    bool is_enhanced_reporting,
     ClientReportPhishingRequestCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -218,20 +213,7 @@ void ClientSideDetectionService::StartClientReportPhishingRequest(
 
   // Fill in metadata about which model we used.
   request->set_model_filename(model_loader_->name());
-  if (is_extended_reporting || is_enhanced_reporting) {
-    if (is_enhanced_reporting) {
-      request->mutable_population()->set_user_population(
-          ChromeUserPopulation::ENHANCED_PROTECTION);
-    } else {
-      request->mutable_population()->set_user_population(
-          ChromeUserPopulation::EXTENDED_REPORTING);
-    }
-  } else {
-    request->mutable_population()->set_user_population(
-        ChromeUserPopulation::SAFE_BROWSING);
-  }
-  request->mutable_population()->set_profile_management_status(
-      delegate_->GetManagementStatus());
+  *request->mutable_population() = delegate_->GetUserPopulation();
 
   std::string request_data;
   request->SerializeToString(&request_data);

components/safe_browsing/content/browser/client_side_detection_service.h

@@ -67,8 +67,7 @@ class ClientSideDetectionService : public KeyedService {
     virtual scoped_refptr<network::SharedURLLoaderFactory>
     GetSafeBrowsingURLLoaderFactory() = 0;
     // Returns the management status for current profile.
-    virtual ChromeUserPopulation::ProfileManagementStatus
-    GetManagementStatus() = 0;
+    virtual ChromeUserPopulation GetUserPopulation() = 0;
   };
 
   explicit ClientSideDetectionService(std::unique_ptr<Delegate> delegate);
@@ -91,17 +90,13 @@ class ClientSideDetectionService : public KeyedService {
   // The URL scheme of the |url()| in the request should be HTTP.  This method
   // takes ownership of the |verdict| as well as the |callback| and calls the
   // the callback once the result has come back from the server or if an error
-  // occurs during the fetch.  |is_extended_reporting| and
-  // |is_enhanced_protection| should be set based on the active profile setting.
-  // If the service is disabled or an error occurs the phishing verdict will
-  // always be false.  The callback is always called after
+  // occurs during the fetch.  If the service is disabled or an error occurs the
+  // phishing verdict will always be false.  The callback is always called after
   // SendClientReportPhishingRequest() returns and on the same thread as
   // SendClientReportPhishingRequest() was called.  You may set |callback| to
   // NULL if you don't care about the server verdict.
   virtual void SendClientReportPhishingRequest(
       std::unique_ptr<ClientPhishingRequest> verdict,
-      bool is_extended_reporting,
-      bool is_enhanced_protection,
       ClientReportPhishingRequestCallback callback);
 
   // Returns true if the given IP address string falls within a private
@@ -183,8 +178,6 @@ class ClientSideDetectionService : public KeyedService {
   // This method takes ownership of both pointers.
   void StartClientReportPhishingRequest(
       std::unique_ptr<ClientPhishingRequest> request,
-      bool is_extended_reporting,
-      bool is_enhanced_protection,
       ClientReportPhishingRequestCallback callback);
 
   // Called by OnURLFetchComplete to handle the server response from

weblayer/browser/safe_browsing/client_side_detection_service_delegate.cc

@@ -4,7 +4,10 @@
 
 #include "weblayer/browser/safe_browsing/client_side_detection_service_delegate.h"
 
+#include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/common/utils.h"
+#include "components/safe_browsing/core/proto/csd.pb.h"
+#include "components/unified_consent/pref_names.h"
 #include "content/public/browser/storage_partition.h"
 #include "weblayer/browser/browser_context_impl.h"
 #include "weblayer/browser/browser_process.h"
@@ -37,10 +40,28 @@ ClientSideDetectionServiceDelegate::GetSafeBrowsingURLLoaderFactory() {
   return sb_service ? sb_service->GetURLLoaderFactory() : nullptr;
 }
 
-safe_browsing::ChromeUserPopulation::ProfileManagementStatus
-ClientSideDetectionServiceDelegate::GetManagementStatus() {
-  // corresponds to unmanaged "unavailable" status on android
-  return safe_browsing::GetProfileManagementStatus(nullptr);
+safe_browsing::ChromeUserPopulation
+ClientSideDetectionServiceDelegate::GetUserPopulation() {
+  safe_browsing::ChromeUserPopulation population;
+  if (safe_browsing::IsEnhancedProtectionEnabled(*GetPrefs())) {
+    population.set_user_population(
+        safe_browsing::ChromeUserPopulation::ENHANCED_PROTECTION);
+  } else if (safe_browsing::IsExtendedReportingEnabled(*GetPrefs())) {
+    population.set_user_population(
+        safe_browsing::ChromeUserPopulation::EXTENDED_REPORTING);
+  } else if (safe_browsing::IsSafeBrowsingEnabled(*GetPrefs())) {
+    population.set_user_population(
+        safe_browsing::ChromeUserPopulation::SAFE_BROWSING);
+  }
+
+  population.set_profile_management_status(
+      safe_browsing::ChromeUserPopulation::UNAVAILABLE);
+  population.set_is_mbb_enabled(GetPrefs()->GetBoolean(
+      unified_consent::prefs::kUrlKeyedAnonymizedDataCollectionEnabled));
+  population.set_is_incognito(browser_context_->IsOffTheRecord());
+  population.set_is_history_sync_enabled(false);
+
+  return population;
 }
 
 }  // namespace weblayer

weblayer/browser/safe_browsing/client_side_detection_service_delegate.h

@@ -22,8 +22,7 @@ class ClientSideDetectionServiceDelegate
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
   scoped_refptr<network::SharedURLLoaderFactory>
   GetSafeBrowsingURLLoaderFactory() override;
-  safe_browsing::ChromeUserPopulation::ProfileManagementStatus
-  GetManagementStatus() override;
+  safe_browsing::ChromeUserPopulation GetUserPopulation() override;
 
  private:
   BrowserContextImpl* browser_context_;