UnsafeEventsReportingEnabled policy does not control all reporting.

Bug: 1012865
Change-Id: I83e03782d6975b1b5a7b53c994476f933f451536
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849984
Commit-Queue: Roger Tawa <rogerta@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704492}

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

@@ -22,6 +22,8 @@
 #include "components/policy/core/common/cloud/cloud_policy_client.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "components/policy/core/common/cloud/realtime_reporting_job_configuration.h"
+#include "components/prefs/pref_service.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/proto/webprotect.pb.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "content/public/browser/browser_context.h"
@@ -70,7 +72,18 @@ SafeBrowsingPrivateEventRouter::SafeBrowsingPrivateEventRouter(
     content::BrowserContext* context)
     : context_(context) {
   event_router_ = EventRouter::Get(context_);
-  InitRealtimeReportingClient();
+
+  // g_browser_process and/or g_browser_process->local_state() may be null
+  // in tests.
+  if (g_browser_process && g_browser_process->local_state()) {
+    RealtimeReportingPrefChanged(std::string());
+    registrar_.Init(g_browser_process->local_state());
+    registrar_.Add(
+        prefs::kUnsafeEventsReportingEnabled,
+        base::BindRepeating(
+            &SafeBrowsingPrivateEventRouter::RealtimeReportingPrefChanged,
+            base::Unretained(this)));
+  }
 }
 
 SafeBrowsingPrivateEventRouter::~SafeBrowsingPrivateEventRouter() {}
@@ -98,7 +111,7 @@ void SafeBrowsingPrivateEventRouter::OnPolicySpecifiedPasswordReuseDetected(
     event_router_->BroadcastEvent(std::move(extension_event));
   }
 
-  if (client_) {
+  if (IsRealtimeReportingEnabled()) {
     // Convert |params| to a real-time event dictionary and report it.
     base::Value event(base::Value::Type::DICTIONARY);
     event.SetStringKey(kKeyUrl, params.url);
@@ -123,7 +136,7 @@ void SafeBrowsingPrivateEventRouter::OnPolicySpecifiedPasswordChanged(
     event_router_->BroadcastEvent(std::move(extension_event));
   }
 
-  if (client_) {
+  if (IsRealtimeReportingEnabled()) {
     // Convert |params| to a real-time event dictionary and report it.
     base::Value event(base::Value::Type::DICTIONARY);
     event.SetStringKey(kKeyUserName, user_name);
@@ -154,7 +167,7 @@ void SafeBrowsingPrivateEventRouter::OnDangerousDownloadOpened(
     event_router_->BroadcastEvent(std::move(extension_event));
   }
 
-  if (client_) {
+  if (IsRealtimeReportingEnabled()) {
     // Convert |params| to a real-time event dictionary and report it.
     base::Value event(base::Value::Type::DICTIONARY);
     event.SetStringKey(kKeyUrl, params.url);
@@ -190,7 +203,7 @@ void SafeBrowsingPrivateEventRouter::OnSecurityInterstitialShown(
     event_router_->BroadcastEvent(std::move(extension_event));
   }
 
-  if (client_) {
+  if (IsRealtimeReportingEnabled()) {
     // Convert |params| to a real-time event dictionary and report it.
     base::Value event(base::Value::Type::DICTIONARY);
     event.SetStringKey(kKeyUrl, params.url);
@@ -227,7 +240,7 @@ void SafeBrowsingPrivateEventRouter::OnSecurityInterstitialProceeded(
     event_router_->BroadcastEvent(std::move(extension_event));
   }
 
-  if (client_) {
+  if (IsRealtimeReportingEnabled()) {
     // Convert |params| to a real-time event dictionary and report it.
     base::Value event(base::Value::Type::DICTIONARY);
     event.SetStringKey(kKeyUrl, params.url);
@@ -244,7 +257,7 @@ void SafeBrowsingPrivateEventRouter::OnDangerousDeepScanningResult(
     const std::string& file_name,
     const std::string& download_digest_sha256,
     const std::string& threat_type) {
-  if (client_) {
+  if (IsRealtimeReportingEnabled()) {
     // Create a real-time event dictionary from the arguments and report it.
     base::Value event(base::Value::Type::DICTIONARY);
     event.SetStringKey(kKeyUrl, url.spec());
@@ -261,7 +274,7 @@ void SafeBrowsingPrivateEventRouter::OnSensitiveDataEvent(
     const GURL& url,
     const std::string& file_name,
     const std::string& download_digest_sha256) {
-  if (client_) {
+  if (IsRealtimeReportingEnabled()) {
     // Create a real-time event dictionary from the arguments and report it.
     base::Value event(base::Value::Type::DICTIONARY);
     event.SetStringKey(kKeyUrl, url.spec());
@@ -282,7 +295,7 @@ void SafeBrowsingPrivateEventRouter::OnLargeUnscannedFileEvent(
     const GURL& url,
     const std::string& file_name,
     const std::string& download_digest_sha256) {
-  if (client_) {
+  if (IsRealtimeReportingEnabled()) {
     // Create a real-time event dictionary from the arguments and report it.
     base::Value event(base::Value::Type::DICTIONARY);
     event.SetStringKey(kKeyUrl, url.spec());
@@ -298,7 +311,7 @@ void SafeBrowsingPrivateEventRouter::OnDangerousDownloadWarning(
     const std::string& file_name,
     const std::string& download_digest_sha256,
     const std::string& threat_type) {
-  if (!client_)
+  if (!IsRealtimeReportingEnabled())
     return;
 
   // Create a real-time event dictionary and report it.
@@ -317,7 +330,7 @@ void SafeBrowsingPrivateEventRouter::OnDangerousDownloadWarningBypassed(
     const std::string& file_name,
     const std::string& download_digest_sha256,
     const std::string& threat_type) {
-  if (!client_)
+  if (!IsRealtimeReportingEnabled())
     return;
 
   // Create a real-time event dictionary and report it.
@@ -339,6 +352,10 @@ void SafeBrowsingPrivateEventRouter::SetCloudPolicyClientForTesting(
 
 void SafeBrowsingPrivateEventRouter::InitRealtimeReportingClient() {
 #if !defined(OS_CHROMEOS)
+  // If already initialized, do nothing.
+  if (client_)
+    return;
+
   // This method is not compiled on Chrome OS because
   // MachineLevelUserCloudPolicyController does not exist. Also,
   // policy::BrowserDMTokenStorage::Get()->RetrieveDMToken() doesn't return a
@@ -405,6 +422,21 @@ void SafeBrowsingPrivateEventRouter::InitRealtimeReportingClient() {
 #endif
 }
 
+bool SafeBrowsingPrivateEventRouter::IsRealtimeReportingEnabled() {
+  // g_browser_process and/or g_browser_process->local_state() may be null
+  // in tests.
+  return g_browser_process && g_browser_process->local_state() &&
+         g_browser_process->local_state()->GetBoolean(
+             prefs::kUnsafeEventsReportingEnabled);
+}
+
+void SafeBrowsingPrivateEventRouter::RealtimeReportingPrefChanged(
+    const std::string& pref) {
+  // If the reporting policy has been turned on, try to initialized now.
+  if (IsRealtimeReportingEnabled())
+    InitRealtimeReportingClient();
+}
+
 void SafeBrowsingPrivateEventRouter::ReportRealtimeEvent(const char* name,
                                                          base::Value event) {
   // Format the current time (UTC) in RFC3339 format.

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

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/values.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/prefs/pref_change_registrar.h"
 
 namespace content {
 class BrowserContext;
@@ -142,6 +143,12 @@ class SafeBrowsingPrivateEventRouter : public KeyedService {
   // with CBCM and the appropriate policies are enabled.
   void InitRealtimeReportingClient();
 
+  // Determines if the real-time reporting feature is enabled.
+  bool IsRealtimeReportingEnabled();
+
+  // Called whenever the real-time reporting policy changes.
+  void RealtimeReportingPrefChanged(const std::string& pref);
+
   // Report safe browsing event through real-time reporting channel, if enabled.
   void ReportRealtimeEvent(const char* name, base::Value event);
 
@@ -153,6 +160,7 @@ class SafeBrowsingPrivateEventRouter : public KeyedService {
   signin::IdentityManager* identity_manager_ = nullptr;
   EventRouter* event_router_ = nullptr;
   std::unique_ptr<policy::CloudPolicyClient> client_;
+  PrefChangeRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingPrivateEventRouter);
 };

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

@@ -14,6 +14,7 @@
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "components/policy/core/common/cloud/realtime_reporting_job_configuration.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "content/public/test/browser_task_environment.h"
 #include "extensions/browser/test_event_router.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -108,10 +109,14 @@ class SafeBrowsingPrivateEventRouterTest : public testing::Test {
                                           "PHISHING", -201);
   }
 
-  void SetUpRouters() {
-    event_router_ = extensions::CreateAndUseTestEventRouter(profile_);
-    SafeBrowsingPrivateEventRouterFactory::GetInstance()->SetTestingFactory(
-        profile_, base::BindRepeating(&BuildSafeBrowsingPrivateEventRouter));
+  void SetReportingPolicy(bool enabled) {
+    TestingBrowserProcess::GetGlobal()->local_state()->SetBoolean(
+        prefs::kUnsafeEventsReportingEnabled, enabled);
+
+    // If we are not enabling reporting, or if the client has already been
+    // set for testing, just return.
+    if (!enabled || client_)
+      return;
 
     // Set a mock cloud policy client in the router.  The router will own the
     // client, but a pointer to the client is maintained in the test class to
@@ -122,12 +127,20 @@ class SafeBrowsingPrivateEventRouterTest : public testing::Test {
         ->SetCloudPolicyClientForTesting(std::move(client));
   }
 
+  void SetUpRouters(bool realtime_reporting_enable = true) {
+    event_router_ = extensions::CreateAndUseTestEventRouter(profile_);
+    SafeBrowsingPrivateEventRouterFactory::GetInstance()->SetTestingFactory(
+        profile_, base::BindRepeating(&BuildSafeBrowsingPrivateEventRouter));
+
+    SetReportingPolicy(realtime_reporting_enable);
+  }
+
  protected:
   content::BrowserTaskEnvironment task_environment_;
   TestingProfileManager profile_manager_;
   TestingProfile* profile_;
   extensions::TestEventRouter* event_router_ = nullptr;
-  policy::MockCloudPolicyClient* client_;
+  policy::MockCloudPolicyClient* client_ = nullptr;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingPrivateEventRouterTest);
@@ -323,4 +336,45 @@ TEST_F(SafeBrowsingPrivateEventRouterTest, TestOnSecurityInterstitialShown) {
   EXPECT_FALSE(
       *event->FindBoolKey(SafeBrowsingPrivateEventRouter::kKeyClickedThrough));
 }
+
+TEST_F(SafeBrowsingPrivateEventRouterTest, PolicyControlOnToOffIsDynamic) {
+  SetUpRouters();
+  SafeBrowsingEventObserver event_observer(
+      api::safe_browsing_private::OnSecurityInterstitialShown::kEventName);
+  event_router_->AddEventObserver(&event_observer);
+
+  EXPECT_CALL(*client_, UploadRealtimeReport(_, _)).Times(1);
+  TriggerOnSecurityInterstitialShownEvent();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, event_observer.PassEventArgs().GetList().size());
+  Mock::VerifyAndClearExpectations(client_);
+
+  // Now turn off policy.  This time no report should be generated.
+  SetReportingPolicy(false);
+  EXPECT_CALL(*client_, UploadRealtimeReport(_, _)).Times(0);
+  TriggerOnSecurityInterstitialShownEvent();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, event_observer.PassEventArgs().GetList().size());
+  Mock::VerifyAndClearExpectations(client_);
+}
+
+TEST_F(SafeBrowsingPrivateEventRouterTest, PolicyControlOffToOnIsDynamic) {
+  SetUpRouters(/*realtime_reporting_enable=*/false);
+  SafeBrowsingEventObserver event_observer(
+      api::safe_browsing_private::OnSecurityInterstitialShown::kEventName);
+  event_router_->AddEventObserver(&event_observer);
+
+  TriggerOnSecurityInterstitialShownEvent();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, event_observer.PassEventArgs().GetList().size());
+
+  // Now turn on policy.
+  SetReportingPolicy(true);
+  EXPECT_CALL(*client_, UploadRealtimeReport(_, _)).Times(1);
+  TriggerOnSecurityInterstitialShownEvent();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, event_observer.PassEventArgs().GetList().size());
+  Mock::VerifyAndClearExpectations(client_);
+}
+
 }  // namespace extensions

chrome/browser/safe_browsing/download_protection/check_client_download_request.cc

@@ -103,10 +103,6 @@ void MaybeReportDownloadDeepScanningVerdict(
   if (result != BinaryUploadService::Result::SUCCESS)
     return;
 
-  if (!g_browser_process->local_state()->GetBoolean(
-          prefs::kUnsafeEventsReportingEnabled))
-    return;
-
   if (response.malware_scan_verdict().verdict() ==
           MalwareDeepScanningVerdict::UWS ||
       response.malware_scan_verdict().verdict() ==