Implement a way for users to provide justification to bypass download…

… warnings

Bug: b/225351697
Change-Id: Id003b1b49bb96e5fe0e709445ebb80e7998f57fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3614746
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Dominique Fauteux-Chapleau <domfc@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: Sneha Nagpaul <snehanagpaul@google.com>
Cr-Commit-Position: refs/heads/main@{#1002310}

chrome/browser/enterprise/connectors/analysis/analysis_service_settings.cc

@@ -186,6 +186,12 @@ absl::optional<GURL> AnalysisServiceSettings::GetLearnMoreUrl(
   return element->second.learn_more_url;
 }
 
+absl::optional<bool> AnalysisServiceSettings::GetBypassJustificationRequired(
+    const std::string& tag) {
+  return tags_requiring_justification_.find(tag) !=
+         tags_requiring_justification_.end();
+}
+
 void AnalysisServiceSettings::AddUrlPatternSettings(
     const base::Value& url_settings_value,
     bool enabled,

chrome/browser/enterprise/connectors/analysis/analysis_service_settings.h

@@ -37,6 +37,7 @@ class AnalysisServiceSettings {
   // settings are invalid or if the message/URL are empty.
   absl::optional<std::u16string> GetCustomMessage(const std::string& tag);
   absl::optional<GURL> GetLearnMoreUrl(const std::string& tag);
+  absl::optional<bool> GetBypassJustificationRequired(const std::string& tag);
 
   std::string service_provider_name() const { return service_provider_name_; }
 

chrome/browser/enterprise/connectors/analysis/content_analysis_dialog.cc

@@ -863,9 +863,10 @@ void ContentAnalysisDialog::AddJustificationTextLengthToDialog() {
 
   // Set the color to red initially because a 0 length message is invalid, but
   // the label doesn't have a Color Provider yet when it's created.
-  bypass_justification_text_length_->SetEnabledColor(
-      bypass_justification_text_length_->GetColorProvider()->GetColor(
-          ui::kColorAlertHighSeverity));
+  // TODO(b/232104687): Re-enable once the bug is fixed
+  // bypass_justification_text_length_->SetEnabledColor(
+  //     bypass_justification_text_length_->GetColorProvider()->GetColor(
+  //         ui::kColorAlertHighSeverity));
 }
 
 const gfx::ImageSkia* ContentAnalysisDialog::GetTopImage() const {

chrome/browser/enterprise/connectors/analysis/content_analysis_dialog_browsertest.cc

@@ -6,6 +6,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
@@ -19,6 +20,7 @@
 #include "chrome/browser/ui/test/test_browser_dialog.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
+#include "components/download/public/common/mock_download_item.h"
 #include "components/enterprise/common/proto/connectors.pb.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "content/public/test/browser_test.h"
@@ -766,6 +768,9 @@ INSTANTIATE_TEST_SUITE_P(
 
 class ContentAnalysisDialogPlainTests : public InProcessBrowserTest {
  public:
+  ContentAnalysisDialogPlainTests() {
+    scoped_feature_list_.InitAndEnableFeature(kBypassJustificationEnabled);
+  }
   void OpenCallback() { ++times_open_called_; }
 
   void DiscardCallback() { ++times_discard_called_; }
@@ -861,6 +866,7 @@ class ContentAnalysisDialogPlainTests : public InProcessBrowserTest {
 
  private:
   raw_ptr<ContentAnalysisDialog> dialog_;
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_F(ContentAnalysisDialogPlainTests, TestCustomMessage) {
@@ -969,32 +975,44 @@ IN_PROC_BROWSER_TEST_F(ContentAnalysisDialogPlainTests,
 
 IN_PROC_BROWSER_TEST_F(ContentAnalysisDialogPlainTests,
                        TestWithDownloadsDelegateBypassWarning) {
+  download::MockDownloadItem mock_download_item;
   ContentAnalysisDialog* dialog = CreateContentAnalysisDialog(
       std::make_unique<ContentAnalysisDownloadsDelegate>(
-          u"", u"", GURL(),
+          u"", u"", GURL(), true,
           base::BindOnce(&ContentAnalysisDialogPlainTests::OpenCallback,
                          base::Unretained(this)),
           base::BindOnce(&ContentAnalysisDialogPlainTests::DiscardCallback,
-                         base::Unretained(this))),
+                         base::Unretained(this)),
+          &mock_download_item),
       ContentAnalysisDelegateBase::FinalResult::WARNING);
 
   EXPECT_EQ(0, times_open_called_);
   EXPECT_EQ(0, times_discard_called_);
 
+  std::u16string test_user_justification = u"user's justification for bypass";
+  dialog->GetBypassJustificationTextareaForTesting()->InsertOrReplaceText(
+      test_user_justification);
   dialog->AcceptDialog();
   EXPECT_EQ(1, times_open_called_);
   EXPECT_EQ(0, times_discard_called_);
+  enterprise_connectors::ScanResult* stored_result =
+      static_cast<enterprise_connectors::ScanResult*>(
+          mock_download_item.GetUserData(
+              enterprise_connectors::ScanResult::kKey));
+  ASSERT_TRUE(stored_result);
+  EXPECT_EQ(stored_result->user_justification, test_user_justification);
 }
 
 IN_PROC_BROWSER_TEST_F(ContentAnalysisDialogPlainTests,
                        TestWithDownloadsDelegateDiscardWarning) {
   ContentAnalysisDialog* dialog = CreateContentAnalysisDialog(
       std::make_unique<ContentAnalysisDownloadsDelegate>(
-          u"", u"", GURL(),
+          u"", u"", GURL(), false,
           base::BindOnce(&ContentAnalysisDialogPlainTests::OpenCallback,
                          base::Unretained(this)),
           base::BindOnce(&ContentAnalysisDialogPlainTests::DiscardCallback,
-                         base::Unretained(this))),
+                         base::Unretained(this)),
+          nullptr),
       ContentAnalysisDelegateBase::FinalResult::WARNING);
 
   EXPECT_EQ(0, times_open_called_);
@@ -1009,11 +1027,12 @@ IN_PROC_BROWSER_TEST_F(ContentAnalysisDialogPlainTests,
                        TestWithDownloadsDelegateDiscardBlock) {
   ContentAnalysisDialog* dialog = CreateContentAnalysisDialog(
       std::make_unique<ContentAnalysisDownloadsDelegate>(
-          u"", u"", GURL(),
+          u"", u"", GURL(), false,
           base::BindOnce(&ContentAnalysisDialogPlainTests::OpenCallback,
                          base::Unretained(this)),
           base::BindOnce(&ContentAnalysisDialogPlainTests::DiscardCallback,
-                         base::Unretained(this))),
+                         base::Unretained(this)),
+          nullptr),
       ContentAnalysisDelegateBase::FinalResult::FAILURE);
 
   EXPECT_EQ(0, times_open_called_);
@@ -1024,33 +1043,55 @@ IN_PROC_BROWSER_TEST_F(ContentAnalysisDialogPlainTests,
   EXPECT_EQ(1, times_discard_called_);
 }
 
-class ContentAnalysysDialogUiTest : public DialogBrowserTest {
+class ContentAnalysysDialogUiTest
+    : public DialogBrowserTest,
+      public testing::WithParamInterface<std::tuple<bool, bool, bool>> {
  public:
   ContentAnalysysDialogUiTest() = default;
   ContentAnalysysDialogUiTest(const ContentAnalysysDialogUiTest&) = delete;
   ContentAnalysysDialogUiTest& operator=(const ContentAnalysysDialogUiTest&) =
       delete;
   ~ContentAnalysysDialogUiTest() override = default;
 
+  bool custom_message_provided() const { return std::get<0>(GetParam()); }
+  bool custom_url_provided() const { return std::get<1>(GetParam()); }
+  bool bypass_justification_enabled() const { return std::get<2>(GetParam()); }
+
+  std::u16string get_custom_message() {
+    return custom_message_provided() ? u"Admin comment" : u"";
+  }
+
+  GURL get_custom_url() {
+    return custom_url_provided() ? GURL("http://learn-more-url.com/") : GURL();
+  }
+
   // DialogBrowserTest:
   void ShowUi(const std::string& name) override {
     auto delegate = std::make_unique<ContentAnalysisDownloadsDelegate>(
-        u"File Name", u"Admin comment", GURL("http://learn-more-url.com/"),
-        base::DoNothing(), base::DoNothing());
+        u"File Name", get_custom_message(), get_custom_url(),
+        bypass_justification_enabled(), base::DoNothing(), base::DoNothing(),
+        nullptr);
 
     // This ctor ends up calling into constrained_window to show itself, in a
     // way that relinquishes its ownership. Because of this, new it here and
     // let it be deleted by the constrained_window code.
     new ContentAnalysisDialog(
         std::move(delegate),
         browser()->tab_strip_model()->GetActiveWebContents(),
-        safe_browsing::DeepScanAccessPoint::DOWNLOAD, 0,
+        safe_browsing::DeepScanAccessPoint::DOWNLOAD, 1,
         ContentAnalysisDelegateBase::FinalResult::WARNING);
   }
 };
 
-IN_PROC_BROWSER_TEST_F(ContentAnalysysDialogUiTest, InvokeUi_default) {
+IN_PROC_BROWSER_TEST_P(ContentAnalysysDialogUiTest, InvokeUi_default) {
   ShowAndVerifyUi();
 }
 
+INSTANTIATE_TEST_SUITE_P(,
+                         ContentAnalysysDialogUiTest,
+                         testing::Combine(
+                             /*custom_message_exists*/ testing::Bool(),
+                             /*custom_url_exists*/ testing::Bool(),
+                             /*bypass_justification_enabled*/ testing::Bool()));
+
 }  // namespace enterprise_connectors

chrome/browser/enterprise/connectors/analysis/content_analysis_downloads_delegate.cc

@@ -4,7 +4,9 @@
 
 #include "chrome/browser/enterprise/connectors/analysis/content_analysis_downloads_delegate.h"
 
+#include "chrome/browser/enterprise/connectors/common.h"
 #include "chrome/browser/enterprise/connectors/connectors_service.h"
+#include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_utils.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/enterprise/common/proto/connectors.pb.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -15,18 +17,39 @@ ContentAnalysisDownloadsDelegate::ContentAnalysisDownloadsDelegate(
     const std::u16string& filename,
     const std::u16string& custom_message,
     GURL custom_learn_more_url,
+    bool bypass_justification_required,
     base::OnceCallback<void()> open_file_callback,
-    base::OnceCallback<void()> discard_file_callback)
+    base::OnceCallback<void()> discard_file_callback,
+    download::DownloadItem* download_item)
     : filename_(filename),
       custom_message_(custom_message),
       custom_learn_more_url_(custom_learn_more_url),
+      bypass_justification_required_(bypass_justification_required),
       open_file_callback_(std::move(open_file_callback)),
-      discard_file_callback_(std::move(discard_file_callback)) {}
+      discard_file_callback_(std::move(discard_file_callback)),
+      download_item_(download_item) {}
 
 ContentAnalysisDownloadsDelegate::~ContentAnalysisDownloadsDelegate() = default;
 
 void ContentAnalysisDownloadsDelegate::BypassWarnings(
     absl::optional<std::u16string> user_justification) {
+  if (download_item_) {
+    enterprise_connectors::ScanResult* stored_result =
+        static_cast<enterprise_connectors::ScanResult*>(
+            download_item_->GetUserData(
+                enterprise_connectors::ScanResult::kKey));
+
+    if (stored_result) {
+      stored_result->user_justification = user_justification;
+    } else {
+      auto stored_result =
+          std::make_unique<enterprise_connectors::ScanResult>();
+      stored_result->user_justification = user_justification;
+      download_item_->SetUserData(enterprise_connectors::ScanResult::kKey,
+                                  std::move(stored_result));
+    }
+  }
+
   if (open_file_callback_)
     std::move(open_file_callback_).Run();
   ResetCallbacks();
@@ -60,7 +83,10 @@ absl::optional<GURL> ContentAnalysisDownloadsDelegate::GetCustomLearnMoreUrl()
 }
 
 bool ContentAnalysisDownloadsDelegate::BypassRequiresJustification() const {
-  return false;
+  if (!base::FeatureList::IsEnabled(kBypassJustificationEnabled))
+    return false;
+
+  return bypass_justification_required_;
 }
 
 std::u16string ContentAnalysisDownloadsDelegate::GetBypassJustificationLabel()

chrome/browser/enterprise/connectors/analysis/content_analysis_downloads_delegate.h

@@ -8,6 +8,10 @@
 #include "base/callback.h"
 #include "chrome/browser/enterprise/connectors/analysis/content_analysis_delegate_base.h"
 
+namespace download {
+class DownloadItem;
+}
+
 namespace enterprise_connectors {
 
 // A ContentAnalysisDelegateBase implementation meant to be used to display the
@@ -19,8 +23,10 @@ class ContentAnalysisDownloadsDelegate : public ContentAnalysisDelegateBase {
       const std::u16string& filename,
       const std::u16string& custom_message,
       GURL custom_learn_more_url,
+      bool bypass_justification_required,
       base::OnceCallback<void()> open_file_callback,
-      base::OnceCallback<void()> discard_file_callback);
+      base::OnceCallback<void()> discard_file_callback,
+      download::DownloadItem* download_item);
   ~ContentAnalysisDownloadsDelegate() override;
 
   // Called when the user opts to keep the download and open it. Should not be
@@ -50,8 +56,10 @@ class ContentAnalysisDownloadsDelegate : public ContentAnalysisDelegateBase {
   std::u16string filename_;
   std::u16string custom_message_;
   GURL custom_learn_more_url_;
+  bool bypass_justification_required_;
   base::OnceCallback<void()> open_file_callback_;
   base::OnceCallback<void()> discard_file_callback_;
+  download::DownloadItem* download_item_;
 };
 
 }  // namespace enterprise_connectors

chrome/browser/enterprise/connectors/analysis/content_analysis_downloads_delegate_unittest.cc

@@ -6,6 +6,8 @@
 
 #include <gtest/gtest.h>
 
+#include "components/download/public/common/mock_download_item.h"
+
 namespace enterprise_connectors {
 
 class ContentAnalysisDownloadsDelegateTest : public testing::Test {
@@ -16,22 +18,24 @@ class ContentAnalysisDownloadsDelegateTest : public testing::Test {
 
   int times_open_called_ = 0;
   int times_discard_called_ = 0;
+  download::MockDownloadItem mock_download_item;
 };
 
 TEST_F(ContentAnalysisDownloadsDelegateTest, TestOpenFile) {
   ContentAnalysisDownloadsDelegate delegate(
-      u"", u"", GURL(),
+      u"", u"", GURL(), true,
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::OpenCallback,
                      base::Unretained(this)),
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::DiscardCallback,
-                     base::Unretained(this)));
+                     base::Unretained(this)),
+      &mock_download_item);
 
-  delegate.BypassWarnings(absl::nullopt);
+  delegate.BypassWarnings(u"User's justification");
   EXPECT_EQ(1, times_open_called_);
   EXPECT_EQ(0, times_discard_called_);
 
   // Attempting any action after one has been performed is a no-op.
-  delegate.BypassWarnings(absl::nullopt);
+  delegate.BypassWarnings(u"User's justification");
   EXPECT_EQ(1, times_open_called_);
   EXPECT_EQ(0, times_discard_called_);
 
@@ -46,11 +50,12 @@ TEST_F(ContentAnalysisDownloadsDelegateTest, TestOpenFile) {
 
 TEST_F(ContentAnalysisDownloadsDelegateTest, TestDiscardFileWarning) {
   ContentAnalysisDownloadsDelegate delegate(
-      u"", u"", GURL(),
+      u"", u"", GURL(), true,
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::OpenCallback,
                      base::Unretained(this)),
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::DiscardCallback,
-                     base::Unretained(this)));
+                     base::Unretained(this)),
+      &mock_download_item);
 
   delegate.Cancel(true);
   EXPECT_EQ(0, times_open_called_);
@@ -72,11 +77,12 @@ TEST_F(ContentAnalysisDownloadsDelegateTest, TestDiscardFileWarning) {
 
 TEST_F(ContentAnalysisDownloadsDelegateTest, TestDiscardFileBlock) {
   ContentAnalysisDownloadsDelegate delegate(
-      u"", u"", GURL(),
+      u"", u"", GURL(), true,
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::OpenCallback,
                      base::Unretained(this)),
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::DiscardCallback,
-                     base::Unretained(this)));
+                     base::Unretained(this)),
+      &mock_download_item);
 
   delegate.Cancel(false);
   EXPECT_EQ(0, times_open_called_);
@@ -98,23 +104,25 @@ TEST_F(ContentAnalysisDownloadsDelegateTest, TestDiscardFileBlock) {
 
 TEST_F(ContentAnalysisDownloadsDelegateTest, TestNoMessageOrUrlReturnsNullOpt) {
   ContentAnalysisDownloadsDelegate delegate(
-      u"", u"", GURL(),
+      u"", u"", GURL(), true,
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::OpenCallback,
                      base::Unretained(this)),
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::DiscardCallback,
-                     base::Unretained(this)));
+                     base::Unretained(this)),
+      &mock_download_item);
 
   EXPECT_FALSE(delegate.GetCustomMessage());
   EXPECT_FALSE(delegate.GetCustomLearnMoreUrl());
 }
 
 TEST_F(ContentAnalysisDownloadsDelegateTest, TestGetMessageAndUrl) {
   ContentAnalysisDownloadsDelegate delegate(
-      u"foo.txt", u"Message", GURL("http://www.example.com"),
+      u"foo.txt", u"Message", GURL("http://www.example.com"), true,
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::OpenCallback,
                      base::Unretained(this)),
       base::BindOnce(&ContentAnalysisDownloadsDelegateTest::DiscardCallback,
-                     base::Unretained(this)));
+                     base::Unretained(this)),
+      nullptr);
 
   EXPECT_TRUE(delegate.GetCustomMessage());
   EXPECT_TRUE(delegate.GetCustomLearnMoreUrl());

chrome/browser/enterprise/connectors/common.cc

@@ -158,6 +158,7 @@ FileMetadata& FileMetadata::operator=(const FileMetadata&) = default;
 FileMetadata::~FileMetadata() = default;
 
 const char ScanResult::kKey[] = "enterprise_connectors.scan_result_key";
+ScanResult::ScanResult() = default;
 ScanResult::ScanResult(FileMetadata metadata) {
   file_metadata.push_back(std::move(metadata));
 }