Make the BinaryUploadService treat APP requests as authorized

Currently the BinaryUploadService tracks authorization by checking the DM
token with the server. For Advanced Protection users, we will not have a
DM token, so we need to authorize the upload another way. This CL adds a check
for whether the user is truly enrolled in APP as the authorization.

Bug: 1020418
Change-Id: Ie9d61906aeed1fbb68fb68d455a5c24c9d5d31fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2001107
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Bettina Dea <bdea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732601}

chrome/browser/safe_browsing/advanced_protection_status_manager.cc

@@ -277,4 +277,9 @@ CoreAccountId AdvancedProtectionStatusManager::GetUnconsentedPrimaryAccountId()
                            : CoreAccountId();
 }
 
+void AdvancedProtectionStatusManager::SetAdvancedProtectionStatusForTesting(
+    bool enrolled) {
+  is_under_advanced_protection_ = enrolled;
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/advanced_protection_status_manager.h

@@ -47,6 +47,8 @@ class AdvancedProtectionStatusManager
 
   bool IsRefreshScheduled();
 
+  void SetAdvancedProtectionStatusForTesting(bool enrolled);
+
  private:
   FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest,
                            NotSignedInOnStartUp);

chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.cc

@@ -19,6 +19,8 @@
 #include "base/timer/timer.h"
 #include "chrome/browser/browser_process.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/cloud_content_scanning/binary_fcm_service.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/multipart_uploader.h"
 #include "chrome/browser/safe_browsing/dm_token_utils.h"
@@ -37,6 +39,13 @@ const int kScanningTimeoutSeconds = 5 * 60;  // 5 minutes
 const char kSbBinaryUploadUrl[] =
     "https://safebrowsing.google.com/safebrowsing/uploads/scan";
 
+bool IsAdvancedProtectionRequest(const BinaryUploadService::Request& request) {
+  return !request.deep_scanning_request().has_dlp_scan_request() &&
+         request.deep_scanning_request().has_malware_scan_request() &&
+         request.deep_scanning_request().malware_scan_request().population() ==
+             MalwareDeepScanningClientRequest::POPULATION_TITANIUM;
+}
+
 }  // namespace
 
 BinaryUploadService::BinaryUploadService(
@@ -49,10 +58,11 @@ BinaryUploadService::BinaryUploadService(
 
 BinaryUploadService::BinaryUploadService(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+    Profile* profile,
     std::unique_ptr<BinaryFCMService> binary_fcm_service)
     : url_loader_factory_(url_loader_factory),
       binary_fcm_service_(std::move(binary_fcm_service)),
-      profile_(nullptr),
+      profile_(profile),
       weakptr_factory_(this) {}
 
 BinaryUploadService::~BinaryUploadService() {}
@@ -60,7 +70,16 @@ BinaryUploadService::~BinaryUploadService() {}
 void BinaryUploadService::MaybeUploadForDeepScanning(
     std::unique_ptr<BinaryUploadService::Request> request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (!can_upload_data_.has_value()) {
+  if (IsAdvancedProtectionRequest(*request)) {
+    MaybeUploadForDeepScanningCallback(
+        std::move(request),
+        /*authorized=*/safe_browsing::AdvancedProtectionStatusManagerFactory::
+            GetForProfile(profile_)
+                ->is_under_advanced_protection());
+    return;
+  }
+
+  if (!can_upload_enterprise_data_.has_value()) {
     IsAuthorized(
         base::BindOnce(&BinaryUploadService::MaybeUploadForDeepScanningCallback,
                        weakptr_factory_.GetWeakPtr(), std::move(request)));
@@ -68,7 +87,7 @@ void BinaryUploadService::MaybeUploadForDeepScanning(
   }
 
   MaybeUploadForDeepScanningCallback(std::move(request),
-                                     can_upload_data_.value());
+                                     can_upload_enterprise_data_.value());
 }
 
 void BinaryUploadService::MaybeUploadForDeepScanningCallback(
@@ -389,7 +408,7 @@ void BinaryUploadService::IsAuthorized(AuthorizationCallback callback) {
                  &BinaryUploadService::ResetAuthorizationData);
   }
 
-  if (!can_upload_data_.has_value()) {
+  if (!can_upload_enterprise_data_.has_value()) {
     // Send a request to check if the browser can upload data.
     if (!pending_validate_data_upload_request_) {
       auto dm_token = GetDMToken(profile_);
@@ -408,36 +427,36 @@ void BinaryUploadService::IsAuthorized(AuthorizationCallback callback) {
     authorization_callbacks_.push_back(std::move(callback));
     return;
   }
-  std::move(callback).Run(can_upload_data_.value());
+  std::move(callback).Run(can_upload_enterprise_data_.value());
 }
 
 void BinaryUploadService::ValidateDataUploadRequestCallback(
     BinaryUploadService::Result result,
     DeepScanningClientResponse response) {
   pending_validate_data_upload_request_ = false;
-  can_upload_data_ = result == BinaryUploadService::Result::SUCCESS;
+  can_upload_enterprise_data_ = result == BinaryUploadService::Result::SUCCESS;
   RunAuthorizationCallbacks();
 }
 
 void BinaryUploadService::RunAuthorizationCallbacks() {
-  DCHECK(can_upload_data_.has_value());
+  DCHECK(can_upload_enterprise_data_.has_value());
   for (auto& callback : authorization_callbacks_) {
-    std::move(callback).Run(can_upload_data_.value());
+    std::move(callback).Run(can_upload_enterprise_data_.value());
   }
   authorization_callbacks_.clear();
 }
 
 void BinaryUploadService::ResetAuthorizationData() {
-  // Setting |can_upload_data_| to base::nullopt will make the next call to
-  // IsAuthorized send out a request to validate data uploads.
-  can_upload_data_ = base::nullopt;
+  // Setting |can_upload_enterprise_data_| to base::nullopt will make the next
+  // call to IsAuthorized send out a request to validate data uploads.
+  can_upload_enterprise_data_ = base::nullopt;
 
-  // Call IsAuthorized  to update |can_upload_data_| right away.
+  // Call IsAuthorized  to update |can_upload_enterprise_data_| right away.
   IsAuthorized(base::DoNothing());
 }
 
 void BinaryUploadService::SetAuthForTesting(bool authorized) {
-  can_upload_data_ = authorized;
+  can_upload_enterprise_data_ = authorized;
 }
 
 // static

chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.h

@@ -41,6 +41,7 @@ class BinaryUploadService {
   // service's |binary_fcm_service_|.
   BinaryUploadService(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+      Profile* profile,
       std::unique_ptr<BinaryFCMService> binary_fcm_service);
   virtual ~BinaryUploadService();
 
@@ -213,12 +214,14 @@ class BinaryUploadService {
   base::flat_map<Request*, std::unique_ptr<DlpDeepScanningVerdict>>
       received_dlp_verdicts_;
 
-  // Indicates whether this browser can upload data.
+  // Indicates whether this browser can upload data for enterprise requests.
+  // Advanced Protection scans are validated using the user's Advanced
+  // Protection enrollment status.
   // base::nullopt means the response from the backend has not been received
   // yet.
   // true means the response indicates data can be uploaded.
   // false means the response indicates data cannot be uploaded.
-  base::Optional<bool> can_upload_data_ = base::nullopt;
+  base::Optional<bool> can_upload_enterprise_data_ = base::nullopt;
 
   // Callbacks waiting on IsAuthorized request.
   std::list<base::OnceCallback<void(bool)>> authorization_callbacks_;

chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service_unittest.cc

@@ -10,8 +10,11 @@
 #include "base/bind.h"
 #include "base/callback_forward.h"
 #include "base/time/time.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/cloud_content_scanning/binary_fcm_service.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/multipart_uploader.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/safe_browsing/core/proto/webprotect.pb.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/test_utils.h"
@@ -102,8 +105,8 @@ class BinaryUploadServiceTest : public testing::Test {
 
     // Since we have mocked the MultipartUploadRequest, we don't need a
     // URLLoaderFactory, so pass nullptr here.
-    service_ =
-        std::make_unique<BinaryUploadService>(nullptr, std::move(fcm_service));
+    service_ = std::make_unique<BinaryUploadService>(nullptr, &profile_,
+                                                     std::move(fcm_service));
   }
   ~BinaryUploadServiceTest() override = default;
 
@@ -122,8 +125,8 @@ class BinaryUploadServiceTest : public testing::Test {
 
   void UploadForDeepScanning(
       std::unique_ptr<BinaryUploadService::Request> request,
-      bool authorized = true) {
-    service_->SetAuthForTesting(authorized);
+      bool authorized_for_enterprise = true) {
+    service_->SetAuthForTesting(authorized_for_enterprise);
     service_->MaybeUploadForDeepScanning(std::move(request));
   }
 
@@ -140,7 +143,7 @@ class BinaryUploadServiceTest : public testing::Test {
 
   void ServiceWithNoFCMConnection() {
     service_ = std::make_unique<BinaryUploadService>(
-        nullptr, std::unique_ptr<BinaryFCMService>(nullptr));
+        nullptr, &profile_, std::unique_ptr<BinaryFCMService>(nullptr));
   }
 
   std::unique_ptr<MockRequest> MakeRequest(
@@ -180,6 +183,7 @@ class BinaryUploadServiceTest : public testing::Test {
 
  protected:
   content::BrowserTaskEnvironment task_environment_;
+  TestingProfile profile_;
   std::unique_ptr<BinaryUploadService> service_;
   MockBinaryFCMService* fcm_service_;
   FakeMultipartUploadRequestFactory fake_factory_;
@@ -283,7 +287,7 @@ TEST_F(BinaryUploadServiceTest, TimesOut) {
   ExpectNetworkResponse(true, DeepScanningClientResponse());
   UploadForDeepScanning(std::move(request));
   content::RunAllTasksUntilIdle();
-  task_environment_.FastForwardUntilNoTasksRemain();
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(300));
 
   EXPECT_EQ(scanning_result, BinaryUploadService::Result::TIMEOUT);
 }
@@ -308,7 +312,7 @@ TEST_F(BinaryUploadServiceTest, OnInstanceIDAfterTimeout) {
   ExpectNetworkResponse(true, DeepScanningClientResponse());
   UploadForDeepScanning(std::move(request));
   content::RunAllTasksUntilIdle();
-  task_environment_.FastForwardUntilNoTasksRemain();
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(300));
 
   EXPECT_EQ(scanning_result, BinaryUploadService::Result::TIMEOUT);
 
@@ -332,7 +336,7 @@ TEST_F(BinaryUploadServiceTest, OnUploadCompleteAfterTimeout) {
   MockRequest* raw_request = request.get();
   UploadForDeepScanning(std::move(request));
   content::RunAllTasksUntilIdle();
-  task_environment_.FastForwardUntilNoTasksRemain();
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(300));
   EXPECT_EQ(scanning_result, BinaryUploadService::Result::TIMEOUT);
 
   // Expect nothing to change if the upload finishes after the timeout.
@@ -355,7 +359,7 @@ TEST_F(BinaryUploadServiceTest, OnGetResponseAfterTimeout) {
   MockRequest* raw_request = request.get();
   UploadForDeepScanning(std::move(request));
   content::RunAllTasksUntilIdle();
-  task_environment_.FastForwardUntilNoTasksRemain();
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(300));
   EXPECT_EQ(scanning_result, BinaryUploadService::Result::TIMEOUT);
 
   // Expect nothing to change if we get a message after the timeout.
@@ -381,7 +385,8 @@ TEST_F(BinaryUploadServiceTest, OnUnauthorized) {
 
   EXPECT_EQ(scanning_result, BinaryUploadService::Result::UNKNOWN);
 
-  UploadForDeepScanning(std::move(request), /*authorized=*/false);
+  UploadForDeepScanning(std::move(request),
+                        /*authorized_for_enterprise=*/false);
 
   // The result is set synchronously on unauthorized requests, so it is
   // UNAUTHORIZED before and after waiting.
@@ -441,4 +446,74 @@ TEST_F(BinaryUploadServiceTest, IsAuthorizedValidTimer) {
   ValidateAuthorizationTimerStarted();
 }
 
+TEST_F(BinaryUploadServiceTest, AdvancedProtectionMalwareRequestAuthorized) {
+  AdvancedProtectionStatusManagerFactory::GetForProfile(&profile_)
+      ->SetAdvancedProtectionStatusForTesting(/*enrolled=*/true);
+
+  BinaryUploadService::Result scanning_result =
+      BinaryUploadService::Result::UNKNOWN;
+  DeepScanningClientResponse scanning_response;
+  std::unique_ptr<MockRequest> request =
+      MakeRequest(&scanning_result, &scanning_response);
+
+  MalwareDeepScanningClientRequest malware_request;
+  malware_request.set_population(
+      MalwareDeepScanningClientRequest::POPULATION_TITANIUM);
+  request->set_request_malware_scan(malware_request);
+
+  ExpectInstanceID("valid id");
+
+  DeepScanningClientResponse simulated_response;
+  simulated_response.mutable_dlp_scan_verdict();
+  simulated_response.mutable_malware_scan_verdict();
+  ExpectNetworkResponse(true, simulated_response);
+
+  EXPECT_EQ(scanning_result, BinaryUploadService::Result::UNKNOWN);
+
+  UploadForDeepScanning(std::move(request),
+                        /*authorized_for_enterprise=*/false);
+
+  content::RunAllTasksUntilIdle();
+
+  EXPECT_EQ(scanning_result, BinaryUploadService::Result::SUCCESS);
+}
+
+TEST_F(BinaryUploadServiceTest, AdvancedProtectionDlpRequestUnauthorized) {
+  AdvancedProtectionStatusManagerFactory::GetForProfile(&profile_)
+      ->SetAdvancedProtectionStatusForTesting(/*enrolled=*/true);
+
+  BinaryUploadService::Result scanning_result =
+      BinaryUploadService::Result::UNKNOWN;
+  DeepScanningClientResponse scanning_response;
+  std::unique_ptr<MockRequest> request =
+      MakeRequest(&scanning_result, &scanning_response);
+
+  request->set_request_dlp_scan(DlpDeepScanningClientRequest());
+
+  MalwareDeepScanningClientRequest malware_request;
+  malware_request.set_population(
+      MalwareDeepScanningClientRequest::POPULATION_TITANIUM);
+  request->set_request_malware_scan(malware_request);
+
+  ExpectInstanceID("valid id");
+
+  DeepScanningClientResponse simulated_response;
+  simulated_response.mutable_dlp_scan_verdict();
+  simulated_response.mutable_malware_scan_verdict();
+  ExpectNetworkResponse(true, simulated_response);
+
+  EXPECT_EQ(scanning_result, BinaryUploadService::Result::UNKNOWN);
+
+  UploadForDeepScanning(std::move(request),
+                        /*authorized_for_enterprise=*/false);
+
+  // The result is set synchronously on unauthorized requests, so it is
+  // UNAUTHORIZED before and after waiting.
+  EXPECT_EQ(scanning_result, BinaryUploadService::Result::UNAUTHORIZED);
+
+  content::RunAllTasksUntilIdle();
+
+  EXPECT_EQ(scanning_result, BinaryUploadService::Result::UNAUTHORIZED);
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/cloud_content_scanning/test_binary_upload_service.cc

@@ -12,8 +12,9 @@
 namespace safe_browsing {
 
 TestBinaryUploadService::TestBinaryUploadService()
-    : BinaryUploadService(nullptr, std::unique_ptr<BinaryFCMService>(nullptr)) {
-}
+    : BinaryUploadService(/*url_loader_factory=*/nullptr,
+                          /*profile=*/nullptr,
+                          /*binary_fcm_service=*/nullptr) {}
 
 void TestBinaryUploadService::MaybeUploadForDeepScanning(
     std::unique_ptr<Request> request) {

chrome/browser/safe_browsing/download_protection/deep_scanning_request_unittest.cc

@@ -37,8 +37,9 @@ using ::testing::ReturnRef;
 class FakeBinaryUploadService : public BinaryUploadService {
  public:
   FakeBinaryUploadService()
-      : BinaryUploadService(nullptr,
-                            std::unique_ptr<BinaryFCMService>(nullptr)),
+      : BinaryUploadService(/*url_loader_factory=*/nullptr,
+                            /*profile=*/nullptr,
+                            /*binary_fcm_service=*/nullptr),
         saved_result_(BinaryUploadService::Result::UNKNOWN),
         saved_response_(DeepScanningClientResponse()) {}