Fix missing username in reported unsafe events

The first event(s) that used GetProfileUsername() would get an empty
username since identity_manager_ is still null when events are reported.
The fix is to make the that variable in the ctor.

This also adds a check for the profile user name in EventReportValidator
to ensure correct reporting for that field.

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

chrome/browser/enterprise/connectors/content_analysis_delegate_browsertest.cc

@@ -24,6 +24,7 @@
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "components/policy/core/common/cloud/realtime_reporting_job_configuration.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "content/public/test/browser_test.h"
 
 using extensions::SafeBrowsingPrivateEventRouter;
@@ -35,6 +36,8 @@ namespace enterprise_connectors {
 
 namespace {
 
+constexpr char kUserName[] = "test@chromium.org";
+
 base::string16 text() {
   return base::UTF8ToUTF16(std::string(100, 'a'));
 }
@@ -216,6 +219,13 @@ class ContentAnalysisDelegateBrowserTest
     extensions::SafeBrowsingPrivateEventRouterFactory::GetForProfile(
         browser()->profile())
         ->SetBinaryUploadServiceForTesting(FakeBinaryUploadServiceStorage());
+    identity_test_environment_ =
+        std::make_unique<signin::IdentityTestEnvironment>();
+    identity_test_environment_->MakePrimaryAccountAvailable(kUserName);
+    extensions::SafeBrowsingPrivateEventRouterFactory::GetForProfile(
+        browser()->profile())
+        ->SetIdentityManagerForTesting(
+            identity_test_environment_->identity_manager());
   }
 
   void DestructorCalled(ContentAnalysisDialog* dialog) override {
@@ -231,6 +241,7 @@ class ContentAnalysisDelegateBrowserTest
 
  private:
   std::unique_ptr<policy::MockCloudPolicyClient> client_;
+  std::unique_ptr<signin::IdentityTestEnvironment> identity_test_environment_;
   base::ScopedTempDir temp_dir_;
 };
 
@@ -314,7 +325,8 @@ IN_PROC_BROWSER_TEST_F(ContentAnalysisDelegateBrowserTest, Files) {
       /*mimetypes*/ ExeMimeTypes(),
       /*size*/ std::string("bad file content").size(),
       /*result*/
-      safe_browsing::EventResultToString(safe_browsing::EventResult::BLOCKED));
+      safe_browsing::EventResultToString(safe_browsing::EventResult::BLOCKED),
+      /*username*/ kUserName);
 
   enterprise_connectors::ContentAnalysisResponse ok_response;
   auto* ok_result = ok_response.add_results();
@@ -406,7 +418,8 @@ IN_PROC_BROWSER_TEST_F(ContentAnalysisDelegateBrowserTest, Texts) {
       /*mimetype*/ TextMimeTypes(),
       /*size*/ 400,
       /*result*/
-      safe_browsing::EventResultToString(safe_browsing::EventResult::BLOCKED));
+      safe_browsing::EventResultToString(safe_browsing::EventResult::BLOCKED),
+      /*username*/ kUserName);
 
   bool called = false;
   base::RunLoop run_loop;
@@ -515,7 +528,8 @@ IN_PROC_BROWSER_TEST_P(ContentAnalysisDelegatePasswordProtectedFilesBrowserTest,
       expected_result() ? safe_browsing::EventResultToString(
                               safe_browsing::EventResult::ALLOWED)
                         : safe_browsing::EventResultToString(
-                              safe_browsing::EventResult::BLOCKED));
+                              safe_browsing::EventResult::BLOCKED),
+      /*username*/ kUserName);
 
   // Start test.
   ContentAnalysisDelegate::CreateForWebContents(
@@ -610,7 +624,8 @@ IN_PROC_BROWSER_TEST_P(
       expected_result() ? safe_browsing::EventResultToString(
                               safe_browsing::EventResult::ALLOWED)
                         : safe_browsing::EventResultToString(
-                              safe_browsing::EventResult::BLOCKED));
+                              safe_browsing::EventResult::BLOCKED),
+      /*username*/ kUserName);
 
   bool called = false;
   base::RunLoop run_loop;
@@ -712,7 +727,8 @@ IN_PROC_BROWSER_TEST_P(ContentAnalysisDelegateBlockLargeFileTransferBrowserTest,
       expected_result() ? safe_browsing::EventResultToString(
                               safe_browsing::EventResult::ALLOWED)
                         : safe_browsing::EventResultToString(
-                              safe_browsing::EventResult::BLOCKED));
+                              safe_browsing::EventResult::BLOCKED),
+      /*username*/ kUserName);
 
   bool called = false;
   base::RunLoop run_loop;
@@ -835,7 +851,8 @@ IN_PROC_BROWSER_TEST_P(ContentAnalysisDelegateDelayDeliveryUntilVerdictTest,
       /*result*/
       safe_browsing::EventResultToString(
           expected_result() ? safe_browsing::EventResult::ALLOWED
-                            : safe_browsing::EventResult::BLOCKED));
+                            : safe_browsing::EventResult::BLOCKED),
+      /*username*/ kUserName);
 
   bool called = false;
   base::RunLoop run_loop;

chrome/browser/extensions/api/safe_browsing_private/safe_browsing_private_event_router.cc

@@ -155,6 +155,8 @@ SafeBrowsingPrivateEventRouter::SafeBrowsingPrivateEventRouter(
     content::BrowserContext* context)
     : context_(context) {
   event_router_ = EventRouter::Get(context_);
+  identity_manager_ = IdentityManagerFactory::GetForProfile(
+      Profile::FromBrowserContext(context_));
 }
 
 SafeBrowsingPrivateEventRouter::~SafeBrowsingPrivateEventRouter() {
@@ -757,8 +759,6 @@ void SafeBrowsingPrivateEventRouter::InitRealtimeReportingClient() {
   // |identity_manager_| may be null in tests. If there is no identity
   // manager don't enable the real-time reporting API since the router won't
   // be able to fill in all the info needed for the reports.
-  identity_manager_ = IdentityManagerFactory::GetForProfile(
-      Profile::FromBrowserContext(context_));
   if (!identity_manager_) {
     DVLOG(2) << "Safe browsing real-time event requires an identity manager.";
     return;

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_test_utils.cc

@@ -141,7 +141,8 @@ void EventReportValidator::ExpectUnscannedFileEvent(
     const std::string& expected_reason,
     const std::set<std::string>* expected_mimetypes,
     int expected_content_size,
-    const std::string& expected_result) {
+    const std::string& expected_result,
+    const std::string& expected_username) {
   event_key_ = SafeBrowsingPrivateEventRouter::kKeyUnscannedFileEvent;
   url_ = expected_url;
   filename_ = expected_filename;
@@ -151,6 +152,7 @@ void EventReportValidator::ExpectUnscannedFileEvent(
   unscanned_reason_ = expected_reason;
   content_size_ = expected_content_size;
   result_ = expected_result;
+  username_ = expected_username;
   EXPECT_CALL(*client_, UploadRealtimeReport_(_, _))
       .WillOnce([this](base::Value& report,
                        base::OnceCallback<void(bool)>& callback) {
@@ -168,7 +170,8 @@ void EventReportValidator::ExpectDangerousDeepScanningResult(
     const std::string& expected_trigger,
     const std::set<std::string>* expected_mimetypes,
     int expected_content_size,
-    const std::string& expected_result) {
+    const std::string& expected_result,
+    const std::string& expected_username) {
   event_key_ = SafeBrowsingPrivateEventRouter::kKeyDangerousDownloadEvent;
   url_ = expected_url;
   filename_ = expected_filename;
@@ -178,6 +181,7 @@ void EventReportValidator::ExpectDangerousDeepScanningResult(
   trigger_ = expected_trigger;
   content_size_ = expected_content_size;
   result_ = expected_result;
+  username_ = expected_username;
   EXPECT_CALL(*client_, UploadRealtimeReport_(_, _))
       .WillOnce([this](base::Value& report,
                        base::OnceCallback<void(bool)>& callback) {
@@ -196,7 +200,8 @@ void EventReportValidator::ExpectSensitiveDataEvent(
         expected_dlp_verdict,
     const std::set<std::string>* expected_mimetypes,
     int expected_content_size,
-    const std::string& expected_result) {
+    const std::string& expected_result,
+    const std::string& expected_username) {
   event_key_ = SafeBrowsingPrivateEventRouter::kKeySensitiveDataEvent;
   url_ = expected_url;
   dlp_verdict_ = expected_dlp_verdict;
@@ -206,6 +211,7 @@ void EventReportValidator::ExpectSensitiveDataEvent(
   trigger_ = expected_trigger;
   content_size_ = expected_content_size;
   result_ = expected_result;
+  username_ = expected_username;
   EXPECT_CALL(*client_, UploadRealtimeReport_(_, _))
       .WillOnce([this](base::Value& report,
                        base::OnceCallback<void(bool)>& callback) {
@@ -226,7 +232,8 @@ void EventReportValidator::
             expected_dlp_verdict,
         const std::set<std::string>* expected_mimetypes,
         int expected_content_size,
-        const std::string& expected_result) {
+        const std::string& expected_result,
+        const std::string& expected_username) {
   event_key_ = SafeBrowsingPrivateEventRouter::kKeyDangerousDownloadEvent;
   url_ = expected_url;
   filename_ = expected_filename;
@@ -236,6 +243,7 @@ void EventReportValidator::
   mimetypes_ = expected_mimetypes;
   content_size_ = expected_content_size;
   result_ = expected_result;
+  username_ = expected_username;
   EXPECT_CALL(*client_, UploadRealtimeReport_(_, _))
       .WillOnce([this](base::Value& report,
                        base::OnceCallback<void(bool)>& callback) {
@@ -264,7 +272,8 @@ void EventReportValidator::
             expected_dlp_verdict,
         const std::set<std::string>* expected_mimetypes,
         int expected_content_size,
-        const std::string& expected_result) {
+        const std::string& expected_result,
+        const std::string& expected_username) {
   event_key_ = SafeBrowsingPrivateEventRouter::kKeySensitiveDataEvent;
   url_ = expected_url;
   filename_ = expected_filename;
@@ -274,6 +283,7 @@ void EventReportValidator::
   content_size_ = expected_content_size;
   result_ = expected_result;
   dlp_verdict_ = expected_dlp_verdict;
+  username_ = expected_username;
   EXPECT_CALL(*client_, UploadRealtimeReport_(_, _))
       .WillOnce([this](base::Value& report,
                        base::OnceCallback<void(bool)>& callback) {
@@ -299,7 +309,8 @@ void EventReportValidator::ExpectDangerousDownloadEvent(
     const std::string& expected_trigger,
     const std::set<std::string>* expected_mimetypes,
     int expected_content_size,
-    const std::string& expected_result) {
+    const std::string& expected_result,
+    const std::string& expected_username) {
   event_key_ = SafeBrowsingPrivateEventRouter::kKeyDangerousDownloadEvent;
   url_ = expected_url;
   filename_ = expected_filename;
@@ -309,6 +320,7 @@ void EventReportValidator::ExpectDangerousDownloadEvent(
   trigger_ = expected_trigger;
   content_size_ = expected_content_size;
   result_ = expected_result;
+  username_ = expected_username;
   EXPECT_CALL(*client_, UploadRealtimeReport_(_, _))
       .WillOnce([this](base::Value& report,
                        base::OnceCallback<void(bool)>& callback) {
@@ -350,6 +362,8 @@ void EventReportValidator::ValidateReport(base::Value* report) {
                 threat_type_);
   ValidateField(event, SafeBrowsingPrivateEventRouter::kKeyUnscannedReason,
                 unscanned_reason_);
+  ValidateField(event, SafeBrowsingPrivateEventRouter::kKeyProfileUserName,
+                username_);
   ValidateMimeType(event);
   ValidateDlpVerdict(event);
 }

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_test_utils.h

@@ -42,7 +42,8 @@ class EventReportValidator {
       const std::string& expected_trigger,
       const std::set<std::string>* expected_mimetypes,
       int expected_content_size,
-      const std::string& expected_result);
+      const std::string& expected_result,
+      const std::string& expected_username);
 
   void ExpectSensitiveDataEvent(
       const std::string& expected_url,
@@ -53,7 +54,8 @@ class EventReportValidator {
           expected_dlp_verdict,
       const std::set<std::string>* expected_mimetypes,
       int expected_content_size,
-      const std::string& expected_result);
+      const std::string& expected_result,
+      const std::string& expected_username);
 
   void ExpectDangerousDeepScanningResultAndSensitiveDataEvent(
       const std::string& expected_url,
@@ -65,7 +67,8 @@ class EventReportValidator {
           expected_dlp_verdict,
       const std::set<std::string>* expected_mimetypes,
       int expected_content_size,
-      const std::string& expected_result);
+      const std::string& expected_result,
+      const std::string& expected_username);
 
   void ExpectSensitiveDataEventAndDangerousDeepScanningResult(
       const std::string& expected_url,
@@ -77,7 +80,8 @@ class EventReportValidator {
           expected_dlp_verdict,
       const std::set<std::string>* expected_mimetypes,
       int expected_content_size,
-      const std::string& expected_result);
+      const std::string& expected_result,
+      const std::string& expected_username);
 
   void ExpectUnscannedFileEvent(const std::string& expected_url,
                                 const std::string& expected_filename,
@@ -86,7 +90,8 @@ class EventReportValidator {
                                 const std::string& expected_reason,
                                 const std::set<std::string>* expected_mimetypes,
                                 int expected_content_size,
-                                const std::string& expected_result);
+                                const std::string& expected_result,
+                                const std::string& expected_username);
 
   void ExpectDangerousDownloadEvent(
       const std::string& expected_url,
@@ -96,7 +101,8 @@ class EventReportValidator {
       const std::string& expected_trigger,
       const std::set<std::string>* expected_mimetypes,
       int expected_content_size,
-      const std::string& expected_result);
+      const std::string& expected_result,
+      const std::string& expected_username);
 
   void ExpectNoReport();
 
@@ -134,6 +140,7 @@ class EventReportValidator {
   base::Optional<int> content_size_ = base::nullopt;
   const std::set<std::string>* mimetypes_ = nullptr;
   base::Optional<std::string> result_ = base::nullopt;
+  std::string username_;
 
   base::RepeatingClosure done_closure_;
 };

chrome/browser/safe_browsing/download_protection/deep_scanning_browsertest.cc

@@ -36,6 +36,7 @@
 #include "components/safe_browsing/core/features.h"
 #include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/safe_browsing/core/proto/webprotect.pb.h"
+#include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/test/browser_test.h"
@@ -47,6 +48,8 @@ namespace safe_browsing {
 
 namespace {
 
+constexpr char kUserName[] = "test@chromium.org";
+
 // Extract the metadata proto from the raw request string. Returns true on
 // success.
 bool GetUploadMetadata(
@@ -119,6 +122,13 @@ class DownloadDeepScanningBrowserTest
     extensions::SafeBrowsingPrivateEventRouterFactory::GetForProfile(
         browser()->profile())
         ->SetCloudPolicyClientForTesting(client_.get());
+    identity_test_environment_ =
+        std::make_unique<signin::IdentityTestEnvironment>();
+    identity_test_environment_->MakePrimaryAccountAvailable(kUserName);
+    extensions::SafeBrowsingPrivateEventRouterFactory::GetForProfile(
+        browser()->profile())
+        ->SetIdentityManagerForTesting(
+            identity_test_environment_->identity_manager());
   }
 
   policy::MockCloudPolicyClient* client() { return client_.get(); }
@@ -357,6 +367,7 @@ class DownloadDeepScanningBrowserTest
   base::flat_set<download::DownloadItem*> download_items_;
 
   std::unique_ptr<policy::MockCloudPolicyClient> client_;
+  std::unique_ptr<signin::IdentityTestEnvironment> identity_test_environment_;
 };
 
 IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest,
@@ -650,7 +661,8 @@ IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest, MultipleFCMResponses) {
       extensions::SafeBrowsingPrivateEventRouter::kTriggerFileDownload,
       /*mimetypes*/ &zip_types,
       /*size*/ 276,
-      /*result*/ EventResultToString(EventResult::WARNED));
+      /*result*/ EventResultToString(EventResult::WARNED),
+      /*username*/ kUserName);
 
   // The DLP scan finishes asynchronously, and finds nothing. The malware result
   // is attached to the response again.
@@ -736,7 +748,8 @@ IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest,
       /*dlp_verdict*/ *result,
       /*mimetypes*/ &zip_types,
       /*size*/ 276,
-      /*result*/ EventResultToString(EventResult::WARNED));
+      /*result*/ EventResultToString(EventResult::WARNED),
+      /*username*/ kUserName);
   WaitForDownloadToFinish();
 
   // The file should be blocked.
@@ -804,7 +817,8 @@ IN_PROC_BROWSER_TEST_F(DownloadRestrictionsDeepScanningBrowserTest,
       extensions::SafeBrowsingPrivateEventRouter::kTriggerFileDownload,
       /*mimetypes*/ &zip_types,
       /*size*/ 276,
-      /*result*/ EventResultToString(EventResult::BLOCKED));
+      /*result*/ EventResultToString(EventResult::BLOCKED),
+      /*username*/ kUserName);
 
   WaitForDownloadToFinish();
 
@@ -1044,7 +1058,8 @@ IN_PROC_BROWSER_TEST_P(MetadataCheckAndDeepScanningBrowserTest, Test) {
         extensions::SafeBrowsingPrivateEventRouter::kTriggerFileDownload,
         /*mimetypes*/ &zip_types,
         /*size*/ 276,
-        /*result*/ EventResultToString(EventResult::WARNED));
+        /*result*/ EventResultToString(EventResult::WARNED),
+        /*username*/ kUserName);
   }
 
   // The deep scanning malware verdict is returned asynchronously. It is not

chrome/browser/safe_browsing/download_protection/deep_scanning_request_unittest.cc

@@ -36,6 +36,7 @@
 #include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/core/features.h"
 #include "components/safe_browsing/core/proto/webprotect.pb.h"
+#include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/download_item_utils.h"
 #include "content/public/test/browser_task_environment.h"
@@ -50,6 +51,8 @@ using ::testing::ReturnRef;
 
 namespace {
 
+constexpr char kUserName[] = "test@chromium.org";
+
 const std::set<std::string>* ExeMimeTypes() {
   static std::set<std::string> set = {"application/x-msdownload",
                                       "application/x-ms-dos-executable",
@@ -409,6 +412,10 @@ class DeepScanningReportingTest : public DeepScanningRequestTest {
     extensions::SafeBrowsingPrivateEventRouterFactory::GetForProfile(profile_)
         ->SetBinaryUploadServiceForTesting(
             download_protection_service_.GetFakeBinaryUploadService());
+    identity_test_environment_.MakePrimaryAccountAvailable(kUserName);
+    extensions::SafeBrowsingPrivateEventRouterFactory::GetForProfile(profile_)
+        ->SetIdentityManagerForTesting(
+            identity_test_environment_.identity_manager());
     download_protection_service_.GetFakeBinaryUploadService()
         ->SetAuthForTesting(true);
 
@@ -424,6 +431,7 @@ class DeepScanningReportingTest : public DeepScanningRequestTest {
 
  protected:
   std::unique_ptr<policy::MockCloudPolicyClient> client_;
+  signin::IdentityTestEnvironment identity_test_environment_;
 };
 
 TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
@@ -477,7 +485,8 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
         /*dlp_verdict*/ *dlp_result,
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
-        /*result*/ EventResultToString(EventResult::WARNED));
+        /*result*/ EventResultToString(EventResult::WARNED),
+        /*username*/ kUserName);
 
     request.Start();
 
@@ -527,7 +536,8 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
         /*dlp_verdict*/ *dlp_result,
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
-        /*result*/ EventResultToString(EventResult::WARNED));
+        /*result*/ EventResultToString(EventResult::WARNED),
+        /*username*/ kUserName);
 
     request.Start();
 
@@ -567,7 +577,8 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
         /*dlp_verdict*/ *dlp_result,
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
-        EventResultToString(EventResult::BLOCKED));
+        EventResultToString(EventResult::BLOCKED),
+        /*username*/ kUserName);
 
     request.Start();
 
@@ -607,7 +618,8 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
         /*dlp_verdict*/ *dlp_result,
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
-        EventResultToString(EventResult::WARNED));
+        EventResultToString(EventResult::WARNED),
+        /*username*/ kUserName);
 
     request.Start();
 
@@ -651,7 +663,8 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
         /*dlp_verdict*/ *dlp_result,
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
-        EventResultToString(EventResult::BLOCKED));
+        EventResultToString(EventResult::BLOCKED),
+        /*username*/ kUserName);
 
     request.Start();
 
@@ -688,7 +701,8 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
         /*result*/
-        EventResultToString(EventResult::ALLOWED));
+        EventResultToString(EventResult::ALLOWED),
+        /*username*/ kUserName);
 
     request.Start();
 
@@ -725,7 +739,8 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
         /*result*/
-        EventResultToString(EventResult::ALLOWED));
+        EventResultToString(EventResult::ALLOWED),
+        /*username*/ kUserName);
 
     request.Start();
 
@@ -804,7 +819,8 @@ TEST_P(DeepScanningDownloadRestrictionsTest, GeneratesCorrectReport) {
         extensions::SafeBrowsingPrivateEventRouter::kTriggerFileDownload,
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
-        /*result*/ EventResultToString(expected_event_result_for_malware()));
+        /*result*/ EventResultToString(expected_event_result_for_malware()),
+        /*username*/ kUserName);
 
     request.Start();
 
@@ -843,7 +859,8 @@ TEST_P(DeepScanningDownloadRestrictionsTest, GeneratesCorrectReport) {
         extensions::SafeBrowsingPrivateEventRouter::kTriggerFileDownload,
         /*mimetypes*/ ExeMimeTypes(),
         /*size*/ std::string("download contents").size(),
-        /*result*/ EventResultToString(EventResult::WARNED));
+        /*result*/ EventResultToString(EventResult::WARNED),
+        /*username*/ kUserName);
 
     request.Start();