Revert "Remove UnsafeEventReportingEnabled policy"

This reverts commit a9fc4ecc5d.

Reason for revert: b/169019687

Original change's description:
> Remove UnsafeEventReportingEnabled policy
>
> This should not change deep scanning behaviour with Connectors since
> OnSecurityEventEnterpriseConnector replaces this policy. Removing this
> policy completely is safe since it was marked as future and it has only
> been delivered as a cloud policy.
>
> Bug: 1103390
> Change-Id: Ie886239acd53e28c5a9104812738b7627d082a47
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2414550
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Reviewed-by: Owen Min <zmin@chromium.org>
> Reviewed-by: Daniel Rubery <drubery@chromium.org>
> Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#808361}

TBR=rogerta@chromium.org,pastarmovj@chromium.org,zmin@chromium.org,drubery@chromium.org,domfc@chromium.org

BYPASS_POLICY_COMPATIBILITY_CHECK='future was already in use on the policy'

Bug: 1103390
Change-Id: I59f8bb3b5925acdbd6e66fdb80a592a898dde4eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422165
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Reviewed-by: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808977}

chrome/browser/enterprise/connectors/connectors_manager.cc

@@ -8,7 +8,6 @@
 
 #include "base/feature_list.h"
 #include "base/no_destructor.h"
-#include "base/optional.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/enterprise/connectors/connectors_prefs.h"
@@ -163,20 +162,11 @@ bool ConnectorsManager::IsConnectorEnabled(ReportingConnector connector) const {
 
 base::Optional<ReportingSettings> ConnectorsManager::GetReportingSettings(
     ReportingConnector connector) {
-  if (!IsConnectorEnabled(connector))
-    return base::nullopt;
+  // Prioritize new Connector policies over legacy ones.
+  if (IsConnectorEnabled(connector))
+    return GetReportingSettingsFromConnectorPolicy(connector);
 
-  if (reporting_connector_settings_.count(connector) == 0)
-    CacheReportingConnectorPolicy(connector);
-
-  // If the connector is still not in memory, it means the pref is set to an
-  // empty list or that it is not a list.
-  if (reporting_connector_settings_.count(connector) == 0)
-    return base::nullopt;
-
-  // While multiple services can be set by the connector policies, only the
-  // first one is considered for now.
-  return reporting_connector_settings_[connector][0].GetReportingSettings();
+  return GetReportingSettingsFromLegacyPolicies(connector);
 }
 
 base::Optional<AnalysisSettings> ConnectorsManager::GetAnalysisSettings(
@@ -386,6 +376,35 @@ std::set<std::string> ConnectorsManager::MatchURLAgainstLegacyPolicies(
   return tags;
 }
 
+base::Optional<ReportingSettings>
+ConnectorsManager::GetReportingSettingsFromConnectorPolicy(
+    ReportingConnector connector) {
+  if (reporting_connector_settings_.count(connector) == 0)
+    CacheReportingConnectorPolicy(connector);
+
+  // If the connector is still not in memory, it means the pref is set to an
+  // empty list or that it is not a list.
+  if (reporting_connector_settings_.count(connector) == 0)
+    return base::nullopt;
+
+  // While multiple services can be set by the connector policies, only the
+  // first one is considered for now.
+  return reporting_connector_settings_[connector][0].GetReportingSettings();
+}
+
+base::Optional<ReportingSettings>
+ConnectorsManager::GetReportingSettingsFromLegacyPolicies(
+    ReportingConnector connector) const {
+  if (!g_browser_process || !g_browser_process->local_state() ||
+      !g_browser_process->local_state()->GetBoolean(
+          prefs::kUnsafeEventsReportingEnabled)) {
+    return base::nullopt;
+  }
+
+  return ReportingSettings(
+      GURL("https://chromereporting-pa.googleapis.com/v1/events"));
+}
+
 void ConnectorsManager::StartObservingPrefs() {
   pref_change_registrar_.Init(g_browser_process->local_state());
   if (base::FeatureList::IsEnabled(kEnterpriseConnectorsEnabled)) {

chrome/browser/enterprise/connectors/connectors_manager.h

@@ -49,8 +49,8 @@ class ConnectorsManager {
   static ConnectorsManager* GetInstance();
 
   // Validates which settings should be applied to a reporting event
-  // against cached policies. Cache the policy value the first time this is
-  // called for every different connector.
+  // against cached policies. This function will prioritize new connector
+  // policies over legacy ones if they are set.
   base::Optional<ReportingSettings> GetReportingSettings(
       ReportingConnector connector);
 
@@ -124,6 +124,16 @@ class ConnectorsManager {
   std::set<std::string> MatchURLAgainstLegacyPolicies(const GURL& url,
                                                       bool upload) const;
 
+  // Validates which settings should be applied to an analysis connector event
+  // against connector policies. Cache the policy value the first time this is
+  // called for every different connector.
+  base::Optional<ReportingSettings> GetReportingSettingsFromConnectorPolicy(
+      ReportingConnector connector);
+
+  // Returns reporting settings based on legacy policies.
+  base::Optional<ReportingSettings> GetReportingSettingsFromLegacyPolicies(
+      ReportingConnector connector) const;
+
   // Cached values of available service providers. This information validates
   // the Connector policies have a valid provider.
   ServiceProviderConfig service_provider_config_ =

chrome/browser/enterprise/connectors/connectors_manager_unittest.cc

@@ -678,17 +678,17 @@ INSTANTIATE_TEST_CASE_P(ConnectorsManagerReportingDynamicTest,
                         ConnectorsManagerReportingDynamicTest,
                         testing::ValuesIn(kAllReportingConnectors));
 
-// Tests to make sure getting reporting settings work with both the feature flag
-// and the OnSecurityEventEnterpriseConnector policy. The parameter for these
-// tests is a tuple of:
+// Tests to make sure getting reporting settings works with both new and legacy
+// feature flags and policies.  The parameter for these tests is a tuple of:
 //
 //   enum class ReportingConnector[]: array of all reporting connectors.
 //   bool: enable feature flag.
-//   int: policy value.  0: don't set, 1: set to normal, 2: set to empty.
+//   int: legacy policy value.  0: don't set, 1: set to true, 2: set to false.
+//   int: new policy value.  0: don't set, 1: set to normal, 2: set to empty.
 class ConnectorsManagerReportingFeatureTest
     : public ConnectorsManagerTest,
       public testing::WithParamInterface<
-          std::tuple<ReportingConnector, bool, int>> {
+          std::tuple<ReportingConnector, bool, int, int>> {
  public:
   ConnectorsManagerReportingFeatureTest() {
     if (enable_feature_flag()) {
@@ -700,7 +700,8 @@ class ConnectorsManagerReportingFeatureTest
 
   ReportingConnector connector() const { return std::get<0>(GetParam()); }
   bool enable_feature_flag() const { return std::get<1>(GetParam()); }
-  int policy_value() const { return std::get<2>(GetParam()); }
+  int legacy_policy_value() const { return std::get<2>(GetParam()); }
+  int policy_value() const { return std::get<3>(GetParam()); }
 
   const char* pref() const { return ConnectorPref(connector()); }
   const char* pref_value() const {
@@ -713,9 +714,22 @@ class ConnectorsManagerReportingFeatureTest
     NOTREACHED();
     return nullptr;
   }
+  bool legacy_pref_value() const {
+    switch (legacy_policy_value()) {
+      case 1:
+        return true;
+      case 2:
+        return false;
+    }
+    NOTREACHED();
+    return false;
+  }
 
   bool reporting_enabled() const {
-    return enable_feature_flag() && policy_value() == 1;
+    return (enable_feature_flag() &&
+            (policy_value() == 1 ||
+             (policy_value() == 0 && legacy_policy_value() == 1))) ||
+           (!enable_feature_flag() && legacy_policy_value() == 1);
   }
 };
 
@@ -724,6 +738,14 @@ TEST_P(ConnectorsManagerReportingFeatureTest, Test) {
   if (policy_value() != 0)
     scoped_pref = std::make_unique<ScopedConnectorPref>(pref(), pref_value());
 
+  if (legacy_policy_value() != 0) {
+    TestingBrowserProcess::GetGlobal()->local_state()->SetBoolean(
+        prefs::kUnsafeEventsReportingEnabled, legacy_pref_value());
+  } else {
+    TestingBrowserProcess::GetGlobal()->local_state()->ClearPref(
+        prefs::kUnsafeEventsReportingEnabled);
+  }
+
   auto settings =
       ConnectorsManager::GetInstance()->GetReportingSettings(connector());
   EXPECT_EQ(reporting_enabled(), settings.has_value());
@@ -741,6 +763,7 @@ INSTANTIATE_TEST_CASE_P(
     ConnectorsManagerReportingFeatureTest,
     testing::Combine(testing::ValuesIn(kAllReportingConnectors),
                      testing::Bool(),
+                     testing::ValuesIn({0, 1, 2}),
                      testing::ValuesIn({0, 1, 2})));
 
 }  // namespace enterprise_connectors

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

@@ -18,7 +18,6 @@
 #include "chrome/browser/enterprise/connectors/common.h"
 #include "chrome/browser/enterprise/connectors/connectors_manager.h"
 #include "chrome/browser/extensions/api/safe_browsing_private/safe_browsing_private_event_router_factory.h"
-#include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_test_utils.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_utils.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/api/safe_browsing_private.h"
@@ -222,7 +221,8 @@ class SafeBrowsingPrivateEventRouterTest : public testing::Test {
   }
 
   void SetReportingPolicy(bool enabled) {
-    safe_browsing::SetOnSecurityEventReporting(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.
@@ -937,23 +937,38 @@ TEST_F(SafeBrowsingPrivateEventRouterTest, TestProfileUsername) {
 class SafeBrowsingIsRealtimeReportingEnabledTest
     : public SafeBrowsingPrivateEventRouterTest,
       public testing::WithParamInterface<
-          testing::tuple<bool, bool, bool, bool>> {
+          testing::tuple<bool, bool, bool, bool, bool>> {
  public:
   SafeBrowsingIsRealtimeReportingEnabledTest()
       : is_feature_flag_enabled_(testing::get<0>(GetParam())),
         is_manageable_(testing::get<1>(GetParam())),
         is_policy_enabled_(testing::get<2>(GetParam())),
-        is_authorized_(testing::get<3>(GetParam())) {
-    if (is_feature_flag_enabled_) {
-      scoped_feature_list_.InitWithFeatures(
-          {enterprise_connectors::kEnterpriseConnectorsEnabled},
-          {extensions::SafeBrowsingPrivateEventRouter::
-               kRealtimeReportingFeature});
+        is_authorized_(testing::get<3>(GetParam())),
+        use_new_connectors_(testing::get<4>(GetParam())) {
+    if (use_new_connectors_) {
+      if (is_feature_flag_enabled_) {
+        scoped_feature_list_.InitWithFeatures(
+            {enterprise_connectors::kEnterpriseConnectorsEnabled},
+            {extensions::SafeBrowsingPrivateEventRouter::
+                 kRealtimeReportingFeature});
+      } else {
+        scoped_feature_list_.InitWithFeatures(
+            {}, {enterprise_connectors::kEnterpriseConnectorsEnabled,
+                 extensions::SafeBrowsingPrivateEventRouter::
+                     kRealtimeReportingFeature});
+      }
     } else {
-      scoped_feature_list_.InitWithFeatures(
-          {}, {enterprise_connectors::kEnterpriseConnectorsEnabled,
-               extensions::SafeBrowsingPrivateEventRouter::
-                   kRealtimeReportingFeature});
+      if (is_feature_flag_enabled_) {
+        scoped_feature_list_.InitWithFeatures(
+            {extensions::SafeBrowsingPrivateEventRouter::
+                 kRealtimeReportingFeature},
+            {enterprise_connectors::kEnterpriseConnectorsEnabled});
+      } else {
+        scoped_feature_list_.InitWithFeatures(
+            {}, {enterprise_connectors::kEnterpriseConnectorsEnabled,
+                 extensions::SafeBrowsingPrivateEventRouter::
+                     kRealtimeReportingFeature});
+      }
     }
 
     // In chrome branded desktop builds, the browser is always manageable.
@@ -964,11 +979,16 @@ class SafeBrowsingIsRealtimeReportingEnabledTest
     }
 #endif
 
-    if (is_policy_enabled_) {
-      scoped_connector_pref_ = std::make_unique<ScopedConnectorPref>(
-          ConnectorPref(
-              enterprise_connectors::ReportingConnector::SECURITY_EVENT),
-          kConnectorsPrefValue);
+    if (use_new_connectors_) {
+      if (is_policy_enabled_) {
+        scoped_connector_pref_ = std::make_unique<ScopedConnectorPref>(
+            ConnectorPref(
+                enterprise_connectors::ReportingConnector::SECURITY_EVENT),
+            kConnectorsPrefValue);
+      }
+    } else {
+      TestingBrowserProcess::GetGlobal()->local_state()->SetBoolean(
+          prefs::kUnsafeEventsReportingEnabled, is_policy_enabled_);
     }
 
 #if defined(OS_CHROMEOS)
@@ -1033,6 +1053,7 @@ class SafeBrowsingIsRealtimeReportingEnabledTest
   const bool is_manageable_;
   const bool is_policy_enabled_;
   const bool is_authorized_;
+  const bool use_new_connectors_;
 
 #if defined(OS_CHROMEOS)
  private:
@@ -1088,6 +1109,7 @@ TEST_P(SafeBrowsingIsRealtimeReportingEnabledTest, CheckRealtimeReport) {
 INSTANTIATE_TEST_SUITE_P(All,
                          SafeBrowsingIsRealtimeReportingEnabledTest,
                          testing::Combine(testing::Bool(),
+                                          testing::Bool(),
                                           testing::Bool(),
                                           testing::Bool(),
                                           testing::Bool()));

chrome/browser/policy/configuration_policy_handler_list_factory.cc

@@ -676,6 +676,9 @@ const PolicyToPreferenceMapEntry kSimplePolicyMap[] = {
   { key::kBrowserGuestModeEnforced,
     prefs::kBrowserGuestModeEnforced,
     base::Value::Type::BOOLEAN },
+  { key::kUnsafeEventsReportingEnabled,
+    prefs::kUnsafeEventsReportingEnabled,
+    base::Value::Type::BOOLEAN },
   { key::kDelayDeliveryUntilVerdict,
     prefs::kDelayDeliveryUntilVerdict,
     base::Value::Type::INTEGER },

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_browsertest_base.cc

@@ -96,11 +96,16 @@ void DeepScanningBrowserTestBase::TearDownOnMainThread() {
       BlockUnsupportedFiletypesValues::BLOCK_UNSUPPORTED_FILETYPES_NONE);
   SetBlockLargeFileTransferPolicyForConnectors(
       BlockLargeFileTransferValues::BLOCK_NONE);
-  SetOnSecurityEventReporting(false);
+  SetUnsafeEventsReportingPolicy(false);
   ClearUrlsToCheckComplianceOfDownloadsForConnectors();
   ClearUrlsToCheckForMalwareOfUploadsForConnectors();
 }
 
+void DeepScanningBrowserTestBase::SetUnsafeEventsReportingPolicy(bool report) {
+  g_browser_process->local_state()->SetBoolean(
+      prefs::kUnsafeEventsReportingEnabled, report);
+}
+
 void DeepScanningBrowserTestBase::SetUpDelegate() {
   SetDMTokenForTesting(policy::DMToken::CreateValidTokenForTesting(kDmToken));
   DeepScanningDialogDelegate::SetFactoryForTesting(base::BindRepeating(

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_browsertest_base.h

@@ -21,6 +21,9 @@ class DeepScanningBrowserTestBase : public InProcessBrowserTest {
   void SetUpOnMainThread() override;
   void TearDownOnMainThread() override;
 
+  // Setters for deep scanning policies.
+  void SetUnsafeEventsReportingPolicy(bool report);
+
   // Sets up a FakeDeepScanningDialogDelegate to use this class's StatusCallback
   // and EncryptionStatusCallback. Also sets up a test DM token.
   void SetUpDelegate();

chrome/browser/safe_browsing/download_protection/deep_scanning_browsertest.cc

@@ -115,7 +115,7 @@ class DownloadDeepScanningBrowserTest
   }
 
   void SetUpReporting() {
-    SetOnSecurityEventReporting(true);
+    SetUnsafeEventsReportingPolicy(true);
     client_ = std::make_unique<policy::MockCloudPolicyClient>();
     extensions::SafeBrowsingPrivateEventRouterFactory::GetForProfile(
         browser()->profile())

chrome/browser/safe_browsing/download_protection/deep_scanning_request_unittest.cc

@@ -410,7 +410,8 @@ class DeepScanningReportingTest : public DeepScanningRequestTest {
     download_protection_service_.GetFakeBinaryUploadService()
         ->SetAuthForTesting(true);
 
-    SetOnSecurityEventReporting(true);
+    TestingBrowserProcess::GetGlobal()->local_state()->SetBoolean(
+        prefs::kUnsafeEventsReportingEnabled, true);
     EnableAllFeatures();
   }
 

chrome/test/data/policy/policy_test_cases.json

@@ -7604,6 +7604,18 @@
     ]
   },
 
+  "UnsafeEventsReportingEnabled": {
+    "os": ["win", "linux", "mac", "chromeos"],
+    "policy_pref_mapping_test": [
+      {
+        "policies": {
+          "UnsafeEventsReportingEnabled": false
+        },
+        "prefs": { "safebrowsing.unsafe_events_reporting": { "local_state": true } }
+      }
+    ]
+  },
+
   "BlockLargeFileTransfer": {
     "os": ["win", "linux", "mac", "chromeos"],
     "policy_pref_mapping_test": [

components/policy/resources/policy_templates.json

@@ -20799,6 +20799,29 @@
       If this policy is set to True or not set, then media controls will be displayed on the lock screen if the user locks the device when media is playing.
       '''
     },
+    {
+      'name': 'UnsafeEventsReportingEnabled',
+      'owners': ['rogerta@chromium.org', 'mad@chromium.org'],
+      'type': 'main',
+      'schema': {
+        'type': 'boolean',
+      },
+      'example_value': True,
+      'features': {
+        'dynamic_refresh': True,
+        'per_profile': False,
+        'cloud_only': True,
+      },
+      'id': 585,
+      'supported_on': ['chrome.*:78-', 'chrome_os:81-'],
+      'future': True,
+      'caption': '''Allows Chrome to report unsafe events to Google''',
+      'tags': [],
+      'desc': '''When this policy is set to enabled, <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> reports unsafe events such as password reuse, malware downloads, interstitials, sensitive data protection rule violations etc. to Google for analysis.  Some unsafe events such as sensitive data protection rule violations depend on other policies that enable such checks.
+
+      If this policy is set to disabled or not set, <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will not report unsafe events.
+      '''
+    },
     {
       'name': 'BlockLargeFileTransfer',
       'owners': ['rogerta@chromium.org', 'mad@chromium.org'],
@@ -24013,7 +24036,7 @@ The recommended way to configure policy on Windows is via GPO, although provisio
     },
   ],
   'placeholders': [],
-  'deleted_policy_ids': [114, 115, 412, 476, 544, 546, 562, 569, 578, 583, 585, 589],
+  'deleted_policy_ids': [114, 115, 412, 476, 544, 546, 562, 569, 578, 583, 589],
   'highest_id_currently_used': 785,
   'highest_atomic_group_id_currently_used': 40
 }

components/safe_browsing/core/common/safe_browsing_prefs.cc

@@ -98,6 +98,8 @@ const char kAdvancedProtectionLastRefreshInUs[] =
     "safebrowsing.advanced_protection_last_refresh";
 const char kSafeBrowsingSendFilesForMalwareCheck[] =
     "safebrowsing.send_files_for_malware_check";
+const char kUnsafeEventsReportingEnabled[] =
+    "safebrowsing.unsafe_events_reporting";
 const char kBlockLargeFileTransfer[] =
     "safebrowsing.block_large_file_transfers";
 const char kDelayDeliveryUntilVerdict[] =
@@ -232,6 +234,7 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry) {
 
 void RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kSafeBrowsingTriggerEventTimestamps);
+  registry->RegisterBooleanPref(prefs::kUnsafeEventsReportingEnabled, false);
   registry->RegisterIntegerPref(prefs::kBlockLargeFileTransfer, 0);
   registry->RegisterIntegerPref(prefs::kDelayDeliveryUntilVerdict, DELAY_NONE);
   registry->RegisterIntegerPref(

components/safe_browsing/core/common/safe_browsing_prefs.h

@@ -96,6 +96,9 @@ extern const char kAdvancedProtectionLastRefreshInUs[];
 // is configured by enterprise policy.
 extern const char kSafeBrowsingSendFilesForMalwareCheck[];
 
+// Boolean that indidicates if Chrome reports unsafe events to Google.
+extern const char kUnsafeEventsReportingEnabled[];
+
 // Integer that specifies if large files are blocked form either uploads or
 // downloads or both.
 extern const char kBlockLargeFileTransfer[];

tools/metrics/histograms/enums.xml

@@ -21580,6 +21580,7 @@ Called by update_document_policy_enum.py.-->
   <int value="581" label="LockScreenMediaPlaybackEnabled"/>
   <int value="582" label="DnsOverHttpsMode"/>
   <int value="584" label="PolicyAtomicGroupsEnabled"/>
+  <int value="585" label="UnsafeEventsReportingEnabled"/>
   <int value="586" label="BlockLargeFileTransfer"/>
   <int value="587" label="DelayDeliveryUntilVerdict"/>
   <int value="588" label="AllowPasswordProtectedFiles"/>