Fix DlpTriggeredRulesOK when policy is off

Every call to DlpTriggeredRulesOK implicitly assumes that DLP should
be checked. By returning true on non-UPLOADS policy values we avoid
getting false-negatives on DLP checks. Also update tests since they all
run with the assumption that every response has both a malware and a
dlp verdict.

Change-Id: I2fd329ced7932ce98d9af8d376ceaf984614f3c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1929851Reviewed-by: Roger Tawa <rogerta@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718818}

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.cc

@@ -142,6 +142,11 @@ void StringSourceRequest::GetRequestData(DataCallback callback) {
 
 bool DlpTriggeredRulesOK(
     const ::safe_browsing::DlpDeepScanningVerdict& verdict) {
+  // No status returns true since this function is called even when the server
+  // doesn't return a DLP scan verdict.
+  if (!verdict.has_status())
+    return true;
+
   if (verdict.status() != DlpDeepScanningVerdict::SUCCESS)
     return false;
 

chrome/browser/safe_browsing/cloud_content_scanning/fake_deep_scanning_dialog_delegate.cc

@@ -43,13 +43,18 @@ FakeDeepScanningDialogDelegate::Create(base::RepeatingClosure delete_closure,
 }
 
 // static
-DeepScanningClientResponse
-FakeDeepScanningDialogDelegate::SuccessfulResponse() {
+DeepScanningClientResponse FakeDeepScanningDialogDelegate::SuccessfulResponse(
+    bool include_dlp,
+    bool include_malware) {
   DeepScanningClientResponse response;
-  response.mutable_dlp_scan_verdict()->set_status(
-      DlpDeepScanningVerdict::SUCCESS);
-  response.mutable_malware_scan_verdict()->set_verdict(
-      MalwareDeepScanningVerdict::CLEAN);
+  if (include_dlp) {
+    response.mutable_dlp_scan_verdict()->set_status(
+        DlpDeepScanningVerdict::SUCCESS);
+  }
+  if (include_malware) {
+    response.mutable_malware_scan_verdict()->set_verdict(
+        MalwareDeepScanningVerdict::CLEAN);
+  }
   return response;
 }
 
@@ -57,8 +62,6 @@ FakeDeepScanningDialogDelegate::SuccessfulResponse() {
 DeepScanningClientResponse FakeDeepScanningDialogDelegate::MalwareResponse(
     MalwareDeepScanningVerdict::Verdict verdict) {
   DeepScanningClientResponse response;
-  response.mutable_dlp_scan_verdict()->set_status(
-      DlpDeepScanningVerdict::SUCCESS);
   response.mutable_malware_scan_verdict()->set_verdict(verdict);
   return response;
 }
@@ -70,8 +73,6 @@ DeepScanningClientResponse FakeDeepScanningDialogDelegate::DlpResponse(
     DlpDeepScanningVerdict::TriggeredRule::Action action) {
   DeepScanningClientResponse response;
   response.mutable_dlp_scan_verdict()->set_status(status);
-  response.mutable_malware_scan_verdict()->set_verdict(
-      MalwareDeepScanningVerdict::CLEAN);
 
   if (!rule_name.empty()) {
     DlpDeepScanningVerdict::TriggeredRule* rule =
@@ -83,18 +84,33 @@ DeepScanningClientResponse FakeDeepScanningDialogDelegate::DlpResponse(
   return response;
 }
 
+// static
+DeepScanningClientResponse
+FakeDeepScanningDialogDelegate::MalwareAndDlpResponse(
+    MalwareDeepScanningVerdict::Verdict verdict,
+    DlpDeepScanningVerdict::Status status,
+    const std::string& rule_name,
+    DlpDeepScanningVerdict::TriggeredRule::Action action) {
+  DeepScanningClientResponse response;
+  *response.mutable_malware_scan_verdict() =
+      FakeDeepScanningDialogDelegate::MalwareResponse(verdict)
+          .malware_scan_verdict();
+  *response.mutable_dlp_scan_verdict() =
+      FakeDeepScanningDialogDelegate::DlpResponse(status, rule_name, action)
+          .dlp_scan_verdict();
+  return response;
+}
+
 void FakeDeepScanningDialogDelegate::Response(
     base::FilePath path,
     std::unique_ptr<BinaryUploadService::Request> request) {
   DeepScanningClientResponse response = status_callback_.is_null()
                                             ? DeepScanningClientResponse()
                                             : status_callback_.Run(path);
-
-  if (path.empty()) {
+  if (path.empty())
     StringRequestCallback(BinaryUploadService::Result::SUCCESS, response);
-  } else {
+  else
     FileRequestCallback(path, BinaryUploadService::Result::SUCCESS, response);
-  }
 }
 
 void FakeDeepScanningDialogDelegate::UploadTextForDeepScanning(
@@ -115,9 +131,6 @@ void FakeDeepScanningDialogDelegate::UploadFileForDeepScanning(
     const base::FilePath& path,
     std::unique_ptr<BinaryUploadService::Request> request) {
   DCHECK(!path.empty());
-  DCHECK_EQ(
-      DlpDeepScanningClientRequest::FILE_UPLOAD,
-      request->deep_scanning_request().dlp_scan_request().content_source());
   DCHECK_EQ(dm_token_, request->deep_scanning_request().dm_token());
 
   // Simulate a response.

chrome/browser/safe_browsing/cloud_content_scanning/fake_deep_scanning_dialog_delegate.h

@@ -51,18 +51,27 @@ class FakeDeepScanningDialogDelegate : public DeepScanningDialogDelegate {
       CompletionCallback callback);
 
   // Returns a deep scanning response that represents a successful scan.
-  static DeepScanningClientResponse SuccessfulResponse();
+  static DeepScanningClientResponse SuccessfulResponse(
+      bool include_dlp = true,
+      bool include_malware = true);
 
-  // Returns a deep scanning response with a specific malware verdict,
+  // Returns a deep scanning response with a specific malware verdict.
   static DeepScanningClientResponse MalwareResponse(
       MalwareDeepScanningVerdict::Verdict verdict);
 
-  // Returns a deep scanning response with a specific DLP verdict,
+  // Returns a deep scanning response with a specific DLP verdict.
   static DeepScanningClientResponse DlpResponse(
       DlpDeepScanningVerdict::Status status,
       const std::string& rule_name,
       DlpDeepScanningVerdict::TriggeredRule::Action action);
 
+  // Returns a deep scanning response with specific malware and DLP verdicts.
+  static DeepScanningClientResponse MalwareAndDlpResponse(
+      MalwareDeepScanningVerdict::Verdict verdict,
+      DlpDeepScanningVerdict::Status status,
+      const std::string& rule_name,
+      DlpDeepScanningVerdict::TriggeredRule::Action action);
+
  private:
   // Simulates a response from the binary upload service.  the |path| argument
   // is used to call |status_callback_| to determine if the path should succeed