Fix ResponseToDownloadCheckResult precedence

Bug: 1104289
Change-Id: Ica537738e30fdbd9e9c0c63ed48e58cdd137444c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2292787Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#787863}

chrome/browser/enterprise/connectors/common.cc

@@ -41,4 +41,51 @@ const char* ConnectorPref(ReportingConnector connector) {
   }
 }
 
+ContentAnalysisResponse::Result::TriggeredRule::Action
+GetHighestPrecedenceAction(const ContentAnalysisResponse& response) {
+  auto action =
+      ContentAnalysisResponse::Result::TriggeredRule::ACTION_UNSPECIFIED;
+
+  for (const auto& result : response.results()) {
+    if (!result.has_status() ||
+        result.status() != ContentAnalysisResponse::Result::SUCCESS) {
+      continue;
+    }
+
+    for (const auto& rule : result.triggered_rules())
+      action = GetHighestPrecedenceAction(action, rule.action());
+  }
+  return action;
+}
+
+ContentAnalysisResponse::Result::TriggeredRule::Action
+GetHighestPrecedenceAction(
+    const ContentAnalysisResponse::Result::TriggeredRule::Action& action_1,
+    const ContentAnalysisResponse::Result::TriggeredRule::Action& action_2) {
+  // Don't use the enum's int values to determine precedence since that
+  // may introduce bugs for new actions later.
+  //
+  // The current precedence is BLOCK > WARN > REPORT_ONLY > UNSPECIFIED
+  if (action_1 == ContentAnalysisResponse::Result::TriggeredRule::BLOCK ||
+      action_2 == ContentAnalysisResponse::Result::TriggeredRule::BLOCK) {
+    return ContentAnalysisResponse::Result::TriggeredRule::BLOCK;
+  }
+  if (action_1 == ContentAnalysisResponse::Result::TriggeredRule::WARN ||
+      action_2 == ContentAnalysisResponse::Result::TriggeredRule::WARN) {
+    return ContentAnalysisResponse::Result::TriggeredRule::WARN;
+  }
+  if (action_1 == ContentAnalysisResponse::Result::TriggeredRule::REPORT_ONLY ||
+      action_2 == ContentAnalysisResponse::Result::TriggeredRule::REPORT_ONLY) {
+    return ContentAnalysisResponse::Result::TriggeredRule::REPORT_ONLY;
+  }
+  if (action_1 ==
+          ContentAnalysisResponse::Result::TriggeredRule::ACTION_UNSPECIFIED ||
+      action_2 ==
+          ContentAnalysisResponse::Result::TriggeredRule::ACTION_UNSPECIFIED) {
+    return ContentAnalysisResponse::Result::TriggeredRule::ACTION_UNSPECIFIED;
+  }
+  NOTREACHED();
+  return ContentAnalysisResponse::Result::TriggeredRule::ACTION_UNSPECIFIED;
+}
+
 }  // namespace enterprise_connectors

chrome/browser/enterprise/connectors/common.h

@@ -70,6 +70,14 @@ struct ReportingSettings {
 const char* ConnectorPref(AnalysisConnector connector);
 const char* ConnectorPref(ReportingConnector connector);
 
+// Returns the highest precedence action in the given parameters.
+ContentAnalysisResponse::Result::TriggeredRule::Action
+GetHighestPrecedenceAction(const ContentAnalysisResponse& response);
+ContentAnalysisResponse::Result::TriggeredRule::Action
+GetHighestPrecedenceAction(
+    const ContentAnalysisResponse::Result::TriggeredRule::Action& action_1,
+    const ContentAnalysisResponse::Result::TriggeredRule::Action& action_2);
+
 }  // namespace enterprise_connectors
 
 #endif  // CHROME_BROWSER_ENTERPRISE_CONNECTORS_COMMON_H_

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.cc

@@ -141,28 +141,6 @@ bool DlpVerdictAllowsDataUse(
   return true;
 }
 
-enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::Action
-GetHighestPrecedenceAction(
-    const enterprise_connectors::ContentAnalysisResponse& response) {
-  auto action = enterprise_connectors::ContentAnalysisResponse::Result::
-      TriggeredRule::ACTION_UNSPECIFIED;
-
-  for (const auto& result : response.results()) {
-    if (!result.has_status() ||
-        result.status() !=
-            enterprise_connectors::ContentAnalysisResponse::Result::SUCCESS) {
-      continue;
-    }
-
-    for (const auto& rule : result.triggered_rules()) {
-      if (rule.action() > action)
-        action = rule.action();
-    }
-  }
-
-  return action;
-}
-
 bool ContentAnalysisActionAllowsDataUse(
     enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
         Action action) {
@@ -540,7 +518,7 @@ void DeepScanningDialogDelegate::ConnectorStringRequestCallback(
       access_point_, content_size, result, response);
 
   text_request_complete_ = true;
-  auto action = GetHighestPrecedenceAction(response);
+  auto action = enterprise_connectors::GetHighestPrecedenceAction(response);
   bool text_complies = ResultShouldAllowDataUse(result, data_.settings) &&
                        ContentAnalysisActionAllowsDataUse(action);
   std::fill(result_.text_results.begin(), result_.text_results.end(),

chrome/browser/safe_browsing/download_protection/deep_scanning_request.cc

@@ -54,8 +54,8 @@ void ResponseToDownloadCheckResult(
         continue;
       }
       for (const auto& rule : result.triggered_rules()) {
-        if (rule.action() > malware_action)
-          malware_action = rule.action();
+        malware_action = enterprise_connectors::GetHighestPrecedenceAction(
+            malware_action, rule.action());
       }
     }
     if (result.tag() == "dlp") {
@@ -65,41 +65,45 @@ void ResponseToDownloadCheckResult(
         continue;
       }
       for (const auto& rule : result.triggered_rules()) {
-        if (rule.action() > dlp_action)
-          dlp_action = rule.action();
+        dlp_action = enterprise_connectors::GetHighestPrecedenceAction(
+            dlp_action, rule.action());
       }
     }
   }
 
-  switch (malware_action) {
-    case enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
-        BLOCK:
-      *download_result = DownloadCheckResult::DANGEROUS;
-      return;
-    case enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
-        WARN:
-      *download_result = DownloadCheckResult::POTENTIALLY_UNWANTED;
-      return;
-    case enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
-        REPORT_ONLY:
-    case enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
-        ACTION_UNSPECIFIED:
-      break;
-  }
-  switch (dlp_action) {
-    case enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
-        BLOCK:
-      *download_result = DownloadCheckResult::SENSITIVE_CONTENT_BLOCK;
-      return;
-    case enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
-        WARN:
-      *download_result = DownloadCheckResult::SENSITIVE_CONTENT_WARNING;
-      return;
-    case enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
-        REPORT_ONLY:
-    case enterprise_connectors::ContentAnalysisResponse::Result::TriggeredRule::
-        ACTION_UNSPECIFIED:
-      break;
+  if (malware_action == enterprise_connectors::GetHighestPrecedenceAction(
+                            malware_action, dlp_action)) {
+    switch (malware_action) {
+      case enterprise_connectors::ContentAnalysisResponse::Result::
+          TriggeredRule::BLOCK:
+        *download_result = DownloadCheckResult::DANGEROUS;
+        return;
+      case enterprise_connectors::ContentAnalysisResponse::Result::
+          TriggeredRule::WARN:
+        *download_result = DownloadCheckResult::POTENTIALLY_UNWANTED;
+        return;
+      case enterprise_connectors::ContentAnalysisResponse::Result::
+          TriggeredRule::REPORT_ONLY:
+      case enterprise_connectors::ContentAnalysisResponse::Result::
+          TriggeredRule::ACTION_UNSPECIFIED:
+        break;
+    }
+  } else {
+    switch (dlp_action) {
+      case enterprise_connectors::ContentAnalysisResponse::Result::
+          TriggeredRule::BLOCK:
+        *download_result = DownloadCheckResult::SENSITIVE_CONTENT_BLOCK;
+        return;
+      case enterprise_connectors::ContentAnalysisResponse::Result::
+          TriggeredRule::WARN:
+        *download_result = DownloadCheckResult::SENSITIVE_CONTENT_WARNING;
+        return;
+      case enterprise_connectors::ContentAnalysisResponse::Result::
+          TriggeredRule::REPORT_ONLY:
+      case enterprise_connectors::ContentAnalysisResponse::Result::
+          TriggeredRule::ACTION_UNSPECIFIED:
+        break;
+    }
   }
 
   if (dlp_scan_failure || malware_scan_failure) {
@@ -112,18 +116,11 @@ void ResponseToDownloadCheckResult(
 
 void ResponseToDownloadCheckResult(const DeepScanningClientResponse& response,
                                    DownloadCheckResult* download_result) {
-  if (response.has_malware_scan_verdict()) {
-    if (response.malware_scan_verdict().verdict() ==
-        MalwareDeepScanningVerdict::MALWARE) {
-      *download_result = DownloadCheckResult::DANGEROUS;
-      return;
-    }
-
-    if (response.malware_scan_verdict().verdict() ==
-        MalwareDeepScanningVerdict::UWS) {
-      *download_result = DownloadCheckResult::POTENTIALLY_UNWANTED;
-      return;
-    }
+  if (response.has_malware_scan_verdict() &&
+      response.malware_scan_verdict().verdict() ==
+          MalwareDeepScanningVerdict::MALWARE) {
+    *download_result = DownloadCheckResult::DANGEROUS;
+    return;
   }
 
   if (response.has_dlp_scan_verdict() &&
@@ -138,7 +135,17 @@ void ResponseToDownloadCheckResult(const DeepScanningClientResponse& response,
       *download_result = DownloadCheckResult::SENSITIVE_CONTENT_BLOCK;
       return;
     }
+  }
 
+  if (response.has_malware_scan_verdict() &&
+      response.malware_scan_verdict().verdict() ==
+          MalwareDeepScanningVerdict::UWS) {
+    *download_result = DownloadCheckResult::POTENTIALLY_UNWANTED;
+    return;
+  }
+
+  if (response.has_dlp_scan_verdict() &&
+      response.dlp_scan_verdict().status() == DlpDeepScanningVerdict::SUCCESS) {
     bool should_dlp_warn = std::any_of(
         response.dlp_scan_verdict().triggered_rules().begin(),
         response.dlp_scan_verdict().triggered_rules().end(),

chrome/browser/safe_browsing/download_protection/deep_scanning_request_unittest.cc

@@ -692,7 +692,7 @@ TEST_P(DeepScanningReportingTest, ProcessesResponseCorrectly) {
       response.mutable_dlp_scan_verdict()->set_status(
           DlpDeepScanningVerdict::SUCCESS);
       response.mutable_dlp_scan_verdict()->add_triggered_rules()->set_action(
-          DlpDeepScanningVerdict::TriggeredRule::BLOCK);
+          DlpDeepScanningVerdict::TriggeredRule::WARN);
       download_protection_service_.GetFakeBinaryUploadService()->SetResponse(
           BinaryUploadService::Result::SUCCESS, response);
       dlp_verdict = SensitiveDataVerdictToResult(response.dlp_scan_verdict());
@@ -714,8 +714,7 @@ TEST_P(DeepScanningReportingTest, ProcessesResponseCorrectly) {
           enterprise_connectors::ContentAnalysisResponse::Result::SUCCESS);
       auto* dlp_rule = dlp_result->add_triggered_rules();
       dlp_rule->set_action(enterprise_connectors::ContentAnalysisResponse::
-                               Result::TriggeredRule::BLOCK);
-      dlp_rule->set_rule_name("dlp_rule");
+                               Result::TriggeredRule::WARN);
       dlp_rule->set_rule_name("dlp_rule");
       dlp_rule->set_rule_id("0");