Read ServiceProviderConfig in {Reporting|Analysis}ServiceSettings

This validates that the service provider set in a Connector policy is
recognized and includes the corresponding URL into settings.

Bug: 1069049, 1069048
Change-Id: I0486261ae2d329e6f743147eaea88906ff1a64ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220471Reviewed-by: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775108}

chrome/browser/enterprise/connectors/analysis_service_settings.cc

@@ -4,22 +4,29 @@
 
 #include "chrome/browser/enterprise/connectors/analysis_service_settings.h"
 
+#include "chrome/browser/enterprise/connectors/service_provider_config.h"
 #include "components/policy/core/browser/url_util.h"
 
 namespace enterprise_connectors {
 
 AnalysisServiceSettings::AnalysisServiceSettings(
-    const base::Value& settings_value) {
+    const base::Value& settings_value,
+    const ServiceProviderConfig& service_provider_config) {
   if (!settings_value.is_dict())
     return;
 
-  // The service provider identifier should always be there.
-  const std::string* service_provider =
+  // The service provider identifier should always be there, and it should match
+  // an existing provider.
+  const std::string* service_provider_name =
       settings_value.FindStringKey(kKeyServiceProvider);
-  if (service_provider)
-    service_provider_ = *service_provider;
-  else
+  if (service_provider_name) {
+    service_provider_ =
+        service_provider_config.GetServiceProvider(*service_provider_name);
+    if (!service_provider_)
+      return;
+  } else {
     return;
+  }
 
   // Add the patterns to the settings, which configures settings.matcher and
   // settings.*_pattern_settings. No enable patterns implies the settings are
@@ -96,6 +103,8 @@ base::Optional<AnalysisSettings> AnalysisServiceSettings::GetAnalysisSettings(
   settings.block_password_protected_files = block_password_protected_files_;
   settings.block_large_files = block_large_files_;
   settings.block_unsupported_file_types = block_unsupported_file_types_;
+  settings.analysis_url = GURL(service_provider_->analysis_url());
+  DCHECK(settings.analysis_url.is_valid());
 
   return settings;
 }
@@ -111,6 +120,7 @@ void AnalysisServiceSettings::AddUrlPatternSettings(
     bool enabled,
     url_matcher::URLMatcherConditionSet::ID* id) {
   DCHECK(id);
+  DCHECK(service_provider_);
   if (enabled)
     DCHECK(disabled_patterns_settings_.empty());
   else
@@ -121,8 +131,10 @@ void AnalysisServiceSettings::AddUrlPatternSettings(
   const base::Value* tags = url_settings_value.FindListKey(kKeyTags);
   if (tags && tags->is_list()) {
     for (const base::Value& tag : tags->GetList()) {
-      if (tag.is_string())
+      if (tag.is_string() &&
+          (service_provider_->analysis_tags().count(tag.GetString()) == 1)) {
         setting.tags.insert(tag.GetString());
+      }
     }
   } else {
     return;
@@ -177,7 +189,7 @@ std::set<std::string> AnalysisServiceSettings::GetTags(
 
 bool AnalysisServiceSettings::IsValid() const {
   // The settings are invalid if no provider was given.
-  if (service_provider_.empty())
+  if (!service_provider_)
     return false;
 
   // The settings are invalid if no enabled pattern(s) exist since that would

chrome/browser/enterprise/connectors/analysis_service_settings.h

@@ -11,6 +11,7 @@
 #include "base/optional.h"
 #include "base/values.h"
 #include "chrome/browser/enterprise/connectors/common.h"
+#include "chrome/browser/enterprise/connectors/service_provider_config.h"
 #include "components/url_matcher/url_matcher.h"
 
 namespace enterprise_connectors {
@@ -18,7 +19,9 @@ namespace enterprise_connectors {
 // The settings for an analysis service obtained from a connector policy.
 class AnalysisServiceSettings {
  public:
-  explicit AnalysisServiceSettings(const base::Value& settings_value);
+  explicit AnalysisServiceSettings(
+      const base::Value& settings_value,
+      const ServiceProviderConfig& service_provider_config);
   AnalysisServiceSettings(AnalysisServiceSettings&&);
   ~AnalysisServiceSettings();
 
@@ -64,8 +67,10 @@ class AnalysisServiceSettings {
   std::set<std::string> GetTags(
       const std::set<url_matcher::URLMatcherConditionSet::ID>& matches) const;
 
-  // The service provider's identifier. This is unique amongst providers.
-  std::string service_provider_;
+  // The service provider matching the name given in a Connector policy. nullptr
+  // implies that a corresponding service provider doesn't exist and that these
+  // settings are not valid.
+  const ServiceProviderConfig::ServiceProvider* service_provider_ = nullptr;
 
   // The URL matcher created from the patterns set in the analysis policy. The
   // condition set IDs returned after matching against a URL can be used to

chrome/browser/enterprise/connectors/analysis_service_settings_unittest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/enterprise/connectors/analysis_service_settings.h"
 #include "base/json/json_reader.h"
 #include "base/no_destructor.h"
+#include "chrome/browser/enterprise/connectors/connectors_manager.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -26,7 +27,7 @@ struct TestParam {
 };
 
 constexpr char kNormalSettings[] = R"({
-  "service_provider": "Google",
+  "service_provider": "google",
   "enable": [
     {"url_list": ["*"], "tags": ["dlp", "malware"]},
   ],
@@ -43,9 +44,9 @@ constexpr char kNormalSettings[] = R"({
 })";
 
 constexpr char kOnlyEnabledPatternsSettings[] = R"({
-  "service_provider": "Google",
+  "service_provider": "google",
   "enable": [
-    {"url_list": ["scan1.com", "scan2.com"], "tags": ["enabled"]},
+    {"url_list": ["scan1.com", "scan2.com"], "tags": ["dlp"]},
   ],
 })";
 
@@ -66,7 +67,7 @@ constexpr char kNoProviderSettings[] = R"({
 })";
 
 constexpr char kNoEnabledPatternsSettings[] = R"({
-  "service_provider": "Google",
+  "service_provider": "google",
   "disable": [
     {"url_list": ["no.dlp.com", "no.dlp.or.malware.ca"], "tags": ["dlp"]},
     {"url_list": ["no.malware.com", "no.dlp.or.malware.ca"],
@@ -88,7 +89,7 @@ constexpr char kNoDlpOrMalwareDotCa[] = "https://no.dlp.or.malware.ca";
 AnalysisSettings* OnlyEnabledSettings() {
   static base::NoDestructor<AnalysisSettings> settings([]() {
     AnalysisSettings settings;
-    settings.tags = {"enabled"};
+    settings.tags = {"dlp"};
     return settings;
   }());
   return settings.get();
@@ -133,6 +134,12 @@ class AnalysisServiceSettingsTest : public testing::TestWithParam<TestParam> {
   GURL url() const { return GURL(GetParam().url); }
   const char* settings_value() const { return GetParam().settings_value; }
   AnalysisSettings* expected_settings() const {
+    // Set the GURL field dynamically to avoid static initialization issues.
+    if (GetParam().expected_settings != NoSettings() &&
+        !GetParam().expected_settings->analysis_url.is_valid()) {
+      GetParam().expected_settings->analysis_url =
+          GURL("https://safebrowsing.google.com/safebrowsing/uploads/scan");
+    }
     return GetParam().expected_settings;
   }
 
@@ -145,7 +152,8 @@ TEST_P(AnalysisServiceSettingsTest, Test) {
                                          base::JSON_ALLOW_TRAILING_COMMAS);
   ASSERT_TRUE(settings.has_value());
 
-  AnalysisServiceSettings service_settings(settings.value());
+  ServiceProviderConfig config(kServiceProviderConfig);
+  AnalysisServiceSettings service_settings(settings.value(), config);
 
   auto analysis_settings = service_settings.GetAnalysisSettings(url());
   ASSERT_EQ((expected_settings() != nullptr), analysis_settings.has_value());
@@ -159,6 +167,8 @@ TEST_P(AnalysisServiceSettingsTest, Test) {
               expected_settings()->block_large_files);
     ASSERT_EQ(analysis_settings.value().block_unsupported_file_types,
               expected_settings()->block_unsupported_file_types);
+    ASSERT_EQ(analysis_settings.value().analysis_url,
+              expected_settings()->analysis_url);
   }
 }
 

chrome/browser/enterprise/connectors/connectors_manager.cc

@@ -206,7 +206,8 @@ void ConnectorsManager::CacheAnalysisConnectorPolicy(
       g_browser_process->local_state()->GetList(pref);
   if (policy_value && policy_value->is_list()) {
     for (const base::Value& service_settings : policy_value->GetList())
-      analysis_connector_settings_[connector].emplace_back(service_settings);
+      analysis_connector_settings_[connector].emplace_back(
+          service_settings, service_provider_config_);
   }
 }
 
@@ -222,7 +223,8 @@ void ConnectorsManager::CacheReportingConnectorPolicy(
       g_browser_process->local_state()->GetList(pref);
   if (policy_value && policy_value->is_list()) {
     for (const base::Value& service_settings : policy_value->GetList())
-      reporting_connector_settings_[connector].emplace_back(service_settings);
+      reporting_connector_settings_[connector].emplace_back(
+          service_settings, service_provider_config_);
   }
 }
 

chrome/browser/enterprise/connectors/connectors_manager_unittest.cc

@@ -79,7 +79,7 @@ constexpr char kEmptySettingsPref[] = "[]";
 
 constexpr char kNormalAnalysisSettingsPref[] = R"([
   {
-    "service_provider": "Google",
+    "service_provider": "google",
     "enable": [
       {"url_list": ["*"], "tags": ["dlp", "malware"]},
     ],
@@ -97,7 +97,7 @@ constexpr char kNormalAnalysisSettingsPref[] = R"([
 
 constexpr char kNormalReportingSettingsPref[] = R"([
   {
-    "service_provider": "Google"
+    "service_provider": "google"
   }
 ])";
 

chrome/browser/enterprise/connectors/reporting_service_settings.cc

@@ -4,20 +4,25 @@
 
 #include "chrome/browser/enterprise/connectors/reporting_service_settings.h"
 
+#include "chrome/browser/enterprise/connectors/service_provider_config.h"
 #include "components/policy/core/browser/url_util.h"
 
 namespace enterprise_connectors {
 
 ReportingServiceSettings::ReportingServiceSettings(
-    const base::Value& settings_value) {
+    const base::Value& settings_value,
+    const ServiceProviderConfig& service_provider_config) {
   if (!settings_value.is_dict())
     return;
 
-  // The service provider identifier should always be there.
-  const std::string* service_provider =
+  // The service provider identifier should always be there, and it should match
+  // an existing provider.
+  const std::string* service_provider_name =
       settings_value.FindStringKey(kKeyServiceProvider);
-  if (service_provider)
-    service_provider_ = *service_provider;
+  if (service_provider_name) {
+    service_provider_ =
+        service_provider_config.GetServiceProvider(*service_provider_name);
+  }
 }
 
 base::Optional<ReportingSettings>
@@ -27,15 +32,15 @@ ReportingServiceSettings::GetReportingSettings() const {
 
   ReportingSettings settings;
 
-  // TODO(rogerta): once service provider configs are implemented set the
-  // reporting URL field.
+  settings.reporting_url = GURL(service_provider_->reporting_url());
+  DCHECK(settings.reporting_url.is_valid());
 
   return settings;
 }
 
 bool ReportingServiceSettings::IsValid() const {
-  // The settings are valid only if provider was given.
-  return !service_provider_.empty();
+  // The settings are valid only if a provider was given.
+  return service_provider_;
 }
 
 ReportingServiceSettings::ReportingServiceSettings(ReportingServiceSettings&&) =

chrome/browser/enterprise/connectors/reporting_service_settings.h

@@ -10,13 +10,16 @@
 #include "base/optional.h"
 #include "base/values.h"
 #include "chrome/browser/enterprise/connectors/common.h"
+#include "chrome/browser/enterprise/connectors/service_provider_config.h"
 
 namespace enterprise_connectors {
 
 // The settings for a report service obtained from a connector policy.
 class ReportingServiceSettings {
  public:
-  explicit ReportingServiceSettings(const base::Value& settings_value);
+  explicit ReportingServiceSettings(
+      const base::Value& settings_value,
+      const ServiceProviderConfig& service_provider_config);
   ReportingServiceSettings(ReportingServiceSettings&&);
   ~ReportingServiceSettings();
 
@@ -29,8 +32,10 @@ class ReportingServiceSettings {
   // false, then GetAnalysisSettings will always return base::nullopt.
   bool IsValid() const;
 
-  // The service provider's identifier. This is unique amongst providers.
-  std::string service_provider_;
+  // The service provider matching the name given in a Connector policy. nullptr
+  // implies that a corresponding service provider doesn't exist and that these
+  // settings are not valid.
+  const ServiceProviderConfig::ServiceProvider* service_provider_ = nullptr;
 };
 
 }  // namespace enterprise_connectors

chrome/browser/enterprise/connectors/reporting_service_settings_unittest.cc

@@ -5,6 +5,8 @@
 #include "chrome/browser/enterprise/connectors/reporting_service_settings.h"
 #include "base/json/json_reader.h"
 #include "base/no_destructor.h"
+#include "chrome/browser/enterprise/connectors/connectors_manager.h"
+#include "chrome/browser/enterprise/connectors/service_provider_config.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -20,7 +22,7 @@ struct TestParam {
   ReportingSettings* expected_settings;
 };
 
-constexpr char kNormalSettings[] = R"({ "service_provider": "Google" })";
+constexpr char kNormalSettings[] = R"({ "service_provider": "google" })";
 
 constexpr char kNoProviderSettings[] = "{}";
 
@@ -39,6 +41,12 @@ class ReportingServiceSettingsTest : public testing::TestWithParam<TestParam> {
  public:
   const char* settings_value() const { return GetParam().settings_value; }
   ReportingSettings* expected_settings() const {
+    // Set the GURL field dynamically to avoid static initialization issues.
+    if (GetParam().expected_settings == NormalSettings() &&
+        !GetParam().expected_settings->reporting_url.is_valid()) {
+      GetParam().expected_settings->reporting_url =
+          GURL("https://chromereporting-pa.googleapis.com/v1/events");
+    }
     return GetParam().expected_settings;
   }
 
@@ -51,10 +59,16 @@ TEST_P(ReportingServiceSettingsTest, Test) {
                                          base::JSON_ALLOW_TRAILING_COMMAS);
   ASSERT_TRUE(settings.has_value());
 
-  ReportingServiceSettings service_settings(settings.value());
+  ServiceProviderConfig config(kServiceProviderConfig);
+  ReportingServiceSettings service_settings(settings.value(), config);
 
   auto reporting_settings = service_settings.GetReportingSettings();
   ASSERT_EQ((expected_settings() != nullptr), reporting_settings.has_value());
+  if (reporting_settings.has_value()) {
+    ASSERT_EQ(expected_settings(), NormalSettings());
+    ASSERT_EQ(expected_settings()->reporting_url,
+              reporting_settings.value().reporting_url);
+  }
 }
 
 INSTANTIATE_TEST_CASE_P(

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

@@ -58,7 +58,7 @@ ACTION_P(CaptureArg, wrapper) {
 
 constexpr char kConnectorsPrefValue[] = R"([
   {
-    "service_provider": "Google"
+    "service_provider": "google"
   }
 ])";
 

chrome/browser/safe_browsing/download_protection/deep_scanning_request_unittest.cc

@@ -224,6 +224,10 @@ class DeepScanningRequestTest : public testing::TestWithParam<bool> {
 
     enterprise_connectors::AnalysisSettings default_settings;
     default_settings.tags = {"malware"};
+    if (!use_legacy_policies()) {
+      default_settings.analysis_url =
+          GURL("https://safebrowsing.google.com/safebrowsing/uploads/scan");
+    }
 
     ASSERT_EQ(settings.value().tags, default_settings.tags);
     ASSERT_EQ(settings.value().block_large_files,