Replace GPOs with CBCM for local content analysis.
Bug: b:257248802
Change-Id: I97c89d925a372c50f67df4b83362a9279afe5410
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4067361
Commit-Queue: Roger Tawa <[email protected]>
Reviewed-by: Dominique Fauteux-Chapleau <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1109556}
diff --git a/chrome/browser/enterprise/connectors/analysis/analysis_service_settings.cc b/chrome/browser/enterprise/connectors/analysis/analysis_service_settings.cc
index 6b64393..0f6aa39 100644
--- a/chrome/browser/enterprise/connectors/analysis/analysis_service_settings.cc
+++ b/chrome/browser/enterprise/connectors/analysis/analysis_service_settings.cc
@@ -190,14 +190,14 @@
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_;
- if (analysis_config_->url) {
+ if (is_cloud_analysis()) {
CloudAnalysisSettings cloud_settings;
cloud_settings.analysis_url = GURL(analysis_config_->url);
DCHECK(cloud_settings.analysis_url.is_valid());
settings.cloud_or_local_settings =
CloudOrLocalAnalysisSettings(std::move(cloud_settings));
} else {
- DCHECK(analysis_config_->local_path);
+ DCHECK(is_local_analysis());
LocalAnalysisSettings local_settings;
local_settings.local_path = analysis_config_->local_path;
local_settings.user_specific = analysis_config_->user_specific;
@@ -286,6 +286,14 @@
return tags_.find(tag) != tags_.end() && tags_.at(tag).requires_justification;
}
+bool AnalysisServiceSettings::is_cloud_analysis() const {
+ return analysis_config_ && analysis_config_->url != nullptr;
+}
+
+bool AnalysisServiceSettings::is_local_analysis() const {
+ return analysis_config_ && analysis_config_->local_path != nullptr;
+}
+
void AnalysisServiceSettings::AddUrlPatternSettings(
const base::Value::Dict& url_settings_dict,
bool enabled,
diff --git a/chrome/browser/enterprise/connectors/analysis/analysis_service_settings.h b/chrome/browser/enterprise/connectors/analysis/analysis_service_settings.h
index 6c64a18f..86641b8a 100644
--- a/chrome/browser/enterprise/connectors/analysis/analysis_service_settings.h
+++ b/chrome/browser/enterprise/connectors/analysis/analysis_service_settings.h
@@ -56,6 +56,10 @@
std::string service_provider_name() const { return service_provider_name_; }
+ // Helpers for convenient check of the underlying variant.
+ bool is_cloud_analysis() const;
+ bool is_local_analysis() const;
+
const AnalysisConfig* GetAnalysisConfig() const { return analysis_config_; }
private:
diff --git a/chrome/browser/enterprise/connectors/connectors_manager.cc b/chrome/browser/enterprise/connectors/connectors_manager.cc
index 6cb3695..6670568 100644
--- a/chrome/browser/enterprise/connectors/connectors_manager.cc
+++ b/chrome/browser/enterprise/connectors/connectors_manager.cc
@@ -7,8 +7,10 @@
#include <memory>
#include "base/check.h"
+#include "base/feature_list.h"
#include "base/values.h"
#include "build/chromeos_buildflags.h"
+#include "chrome/browser/enterprise/connectors/analysis/content_analysis_features.h"
#include "chrome/browser/enterprise/connectors/reporting/browser_crash_event_router.h"
#include "chrome/browser/enterprise/connectors/reporting/extension_install_event_router.h"
#include "components/prefs/pref_service.h"
@@ -36,11 +38,21 @@
ConnectorsManager::~ConnectorsManager() = default;
bool ConnectorsManager::IsConnectorEnabled(AnalysisConnector connector) const {
- if (analysis_connector_settings_.count(connector) == 1)
- return true;
+ if (analysis_connector_settings_.count(connector) == 0 &&
+ prefs()->HasPrefPath(ConnectorPref(connector))) {
+ CacheAnalysisConnectorPolicy(connector);
+ }
- const char* pref = ConnectorPref(connector);
- return pref && pref_change_registrar_.prefs()->HasPrefPath(pref);
+ if (analysis_connector_settings_.count(connector) != 1) {
+ return false;
+ }
+
+ // If the connector is for local content analysis, make sure it is also
+ // enabled by flags. For now, only one connector is supported at a time.
+ const auto& settings = analysis_connector_settings_.at(connector)[0];
+
+ return settings.is_cloud_analysis() ||
+ base::FeatureList::IsEnabled(kLocalContentAnalysisEnabled);
}
bool ConnectorsManager::IsConnectorEnabled(ReportingConnector connector) const {
@@ -48,7 +60,7 @@
return true;
const char* pref = ConnectorPref(connector);
- return pref && pref_change_registrar_.prefs()->HasPrefPath(pref);
+ return pref && prefs()->HasPrefPath(pref);
}
absl::optional<ReportingSettings> ConnectorsManager::GetReportingSettings(
@@ -130,15 +142,14 @@
}
void ConnectorsManager::CacheAnalysisConnectorPolicy(
- AnalysisConnector connector) {
+ AnalysisConnector connector) const {
analysis_connector_settings_.erase(connector);
// Connectors with non-existing policies should not reach this code.
const char* pref = ConnectorPref(connector);
DCHECK(pref);
- const base::Value::List& policy_value =
- pref_change_registrar_.prefs()->GetList(pref);
+ const base::Value::List& policy_value = prefs()->GetList(pref);
for (const base::Value& service_settings : policy_value)
analysis_connector_settings_[connector].emplace_back(
service_settings, *service_provider_config_);
@@ -152,8 +163,7 @@
const char* pref = ConnectorPref(connector);
DCHECK(pref);
- const base::Value::List& policy_value =
- pref_change_registrar_.prefs()->GetList(pref);
+ const base::Value::List& policy_value = prefs()->GetList(pref);
for (const base::Value& service_settings : policy_value)
reporting_connector_settings_[connector].emplace_back(
service_settings, *service_provider_config_);
diff --git a/chrome/browser/enterprise/connectors/connectors_manager.h b/chrome/browser/enterprise/connectors/connectors_manager.h
index 91bba9f..5072a5287 100644
--- a/chrome/browser/enterprise/connectors/connectors_manager.h
+++ b/chrome/browser/enterprise/connectors/connectors_manager.h
@@ -97,7 +97,7 @@
AnalysisConnector connector);
// Read and cache the policy corresponding to |connector|.
- void CacheAnalysisConnectorPolicy(AnalysisConnector connector);
+ void CacheAnalysisConnectorPolicy(AnalysisConnector connector) const;
void CacheReportingConnectorPolicy(ReportingConnector connector);
// Sets up |pref_change_registrar_|. Used by the constructor and
@@ -112,13 +112,19 @@
absl::optional<ReportingSettings> GetReportingSettingsFromConnectorPolicy(
ReportingConnector connector);
+ PrefService* prefs() { return pref_change_registrar_.prefs(); }
+
+ const PrefService* prefs() const { return pref_change_registrar_.prefs(); }
+
// Cached values of available service providers. This information validates
// the Connector policies have a valid provider.
raw_ptr<const ServiceProviderConfig> service_provider_config_;
// Cached values of the connector policies. Updated when a connector is first
- // used or when a policy is updated.
- AnalysisConnectorsSettings analysis_connector_settings_;
+ // used or when a policy is updated. Analysis connectors settings are
+ // mutable because they maybe updated by a call to IsConnectorEnabled(),
+ // which is a const method.
+ mutable AnalysisConnectorsSettings analysis_connector_settings_;
ReportingConnectorsSettings reporting_connector_settings_;
// Used to track changes of connector policies and propagate them in
diff --git a/chrome/browser/enterprise/connectors/connectors_manager_unittest.cc b/chrome/browser/enterprise/connectors/connectors_manager_unittest.cc
index 1083a7c..c20bf06 100644
--- a/chrome/browser/enterprise/connectors/connectors_manager_unittest.cc
+++ b/chrome/browser/enterprise/connectors/connectors_manager_unittest.cc
@@ -14,6 +14,7 @@
#include "base/test/task_environment.h"
#include "base/values.h"
#include "chrome/browser/browser_process.h"
+#include "chrome/browser/enterprise/connectors/analysis/content_analysis_features.h"
#include "chrome/browser/enterprise/connectors/common.h"
#include "chrome/browser/enterprise/connectors/connectors_prefs.h"
#include "chrome/browser/enterprise/connectors/connectors_service.h"
@@ -165,6 +166,45 @@
std::set<std::string> expected_mime_types_;
};
+// Platform policies should only act as a kill switch.
+class ConnectorsManagerLocalAnalysisPolicyTest
+ : public ConnectorsManagerTest,
+ public testing::WithParamInterface<
+ std::tuple<AnalysisConnector, bool, bool>> {
+ protected:
+ AnalysisConnector connector() const { return std::get<0>(GetParam()); }
+ bool enable_feature() const { return std::get<1>(GetParam()); }
+ bool set_policy() const { return std::get<2>(GetParam()); }
+};
+
+TEST_P(ConnectorsManagerLocalAnalysisPolicyTest, Test) {
+ if (enable_feature()) {
+ scoped_feature_list_.InitWithFeatures({kLocalContentAnalysisEnabled}, {});
+ } else {
+ scoped_feature_list_.InitWithFeatures({}, {kLocalContentAnalysisEnabled});
+ }
+
+ std::unique_ptr<ScopedConnectorPref> scoped_pref =
+ set_policy() ? std::make_unique<ScopedConnectorPref>(
+ pref_service(), ConnectorPref(connector()),
+ kNormalLocalAnalysisSettingsPref)
+ : nullptr;
+
+ ConnectorsManager manager(
+ std::make_unique<BrowserCrashEventRouter>(profile_),
+ std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+ GetServiceProviderConfig());
+ EXPECT_EQ(enable_feature() && set_policy(),
+ manager.IsConnectorEnabled(connector()));
+}
+
+INSTANTIATE_TEST_SUITE_P(
+ ConnectorsManagerLocalAnalysisPolicyTest,
+ ConnectorsManagerLocalAnalysisPolicyTest,
+ testing::Combine(testing::ValuesIn(kAllAnalysisConnectors),
+ testing::Bool(),
+ testing::Bool()));
+
class ConnectorsManagerConnectorPoliciesTest
: public ConnectorsManagerTest,
public testing::WithParamInterface<
diff --git a/chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler.cc b/chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler.cc
index 4821d35c..fbb317f 100644
--- a/chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler.cc
+++ b/chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler.cc
@@ -4,9 +4,7 @@
#include "chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler.h"
-#include "base/feature_list.h"
#include "base/values.h"
-#include "chrome/browser/enterprise/connectors/analysis/content_analysis_features.h"
#include "chrome/browser/enterprise/connectors/connectors_prefs.h"
#include "chrome/browser/enterprise/connectors/service_provider_config.h"
#include "components/policy/core/browser/policy_error_map.h"
@@ -18,71 +16,6 @@
namespace enterprise_connectors {
-namespace {
-
-bool IsContentAnalysisPref(const char* pref) {
- return pref == kOnFileAttachedPref || pref == kOnFileDownloadedPref ||
- pref == kOnBulkDataEntryPref || pref == kOnPrintPref;
-}
-
-bool CanUseNonCloudPolicySource(const char* pref,
- const policy::PolicyMap::Entry* policy) {
- DCHECK(policy);
-
- // The condition below is a quick fix to avoid accessing feature state before
- // FeatureList initialization, because that results in a crash.
- //
- // TODO(crbug.com/1381113): Instead of this quick fix, move code that depends
- // on feature state after FeatureList initialization.
- if (base::FeatureList::GetInstance()) {
- if (!base::FeatureList::IsEnabled(kLocalContentAnalysisEnabled))
- return false;
- } else {
- if (kLocalContentAnalysisEnabled.default_state ==
- base::FEATURE_DISABLED_BY_DEFAULT) {
- return false;
- }
- }
-
- // Only content analysis policies with a LCA provider are exempt from using
- // cloud policies.
- if (!IsContentAnalysisPref(pref))
- return false;
-
- const base::Value* value = policy->value_unsafe();
- if (!value)
- return false;
-
- // Content analysis policies have the following format:
- // [
- // {
- // "service_provider": "foo",
- // "other_param_1": { ... },
- // "other_param_2": { ... },
- // },
- // ]
- //
- // So we simply need to validate that the service provider is valid for LCA.
- if (!value->is_list() || value->GetList().empty())
- return false;
-
- const base::Value::List& configs = value->GetList();
- if (!configs[0].is_dict() ||
- !configs[0].GetDict().FindString("service_provider")) {
- return false;
- }
-
- const std::string* service_provider =
- configs[0].GetDict().FindString("service_provider");
- const ServiceProviderConfig* service_providers = GetServiceProviderConfig();
-
- return service_providers->count(*service_provider) &&
- service_providers->at(*service_provider).analysis &&
- service_providers->at(*service_provider).analysis->local_path;
-}
-
-} // namespace
-
EnterpriseConnectorsPolicyHandler::EnterpriseConnectorsPolicyHandler(
const char* policy_name,
const char* pref_path,
@@ -115,8 +48,7 @@
if (!policy)
return true;
- if (!CanUseNonCloudPolicySource(pref_path_, policy) &&
- policy->source != policy::POLICY_SOURCE_CLOUD &&
+ if (policy->source != policy::POLICY_SOURCE_CLOUD &&
policy->source != policy::POLICY_SOURCE_CLOUD_FROM_ASH) {
errors->AddError(policy_name(), IDS_POLICY_CLOUD_SOURCE_ONLY_ERROR);
return false;
diff --git a/chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler_unittest.cc b/chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler_unittest.cc
index c7cd65c..2d6f474 100644
--- a/chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler_unittest.cc
+++ b/chrome/browser/enterprise/connectors/enterprise_connectors_policy_handler_unittest.cc
@@ -8,9 +8,7 @@
#include <tuple>
#include "base/json/json_reader.h"
-#include "base/test/scoped_feature_list.h"
#include "base/values.h"
-#include "chrome/browser/enterprise/connectors/analysis/content_analysis_features.h"
#include "chrome/browser/enterprise/connectors/connectors_prefs.h"
#include "components/policy/core/browser/policy_error_map.h"
#include "components/policy/core/common/policy_map.h"
@@ -188,40 +186,19 @@
class EnterpriseConnectorsPolicyHandlerLocalTest
: public EnterpriseConnectorsPolicyHandlerTestBase,
- public testing::TestWithParam<
- std::tuple<const char*, const char*, bool>> {
+ public testing::TestWithParam<std::tuple<const char*, const char*>> {
public:
- EnterpriseConnectorsPolicyHandlerLocalTest() {
- if (enable_feature())
- scoped_feature_list_.InitAndEnableFeature(kLocalContentAnalysisEnabled);
- }
+ EnterpriseConnectorsPolicyHandlerLocalTest() = default;
const char* policy() const override { return std::get<0>(GetParam()); }
const char* policy_pref() const { return std::get<1>(GetParam()); }
- bool enable_feature() const { return std::get<2>(GetParam()); }
bool policy_is_valid() const {
if (policy() == kEmptyPolicy)
return true;
- if (!enable_feature())
- return false;
-
- if (policy_pref() != kOnFileAttachedPref ||
- policy_pref() != kOnFileDownloadedPref ||
- policy_pref() != kOnBulkDataEntryPref ||
-#if BUILDFLAG(IS_CHROMEOS)
- policy_pref() != kOnFileTransferPref ||
-#endif
- policy_pref() != kOnPrintPref) {
- return false;
- }
-
- return policy() == kValidLocalContentAnalysisPolicy;
+ return false;
}
-
- private:
- base::test::ScopedFeatureList scoped_feature_list_;
};
TEST_P(EnterpriseConnectorsPolicyHandlerLocalTest, Test) {
@@ -248,7 +225,9 @@
INSTANTIATE_TEST_SUITE_P(
EnterpriseConnectorsPolicyHandlerLocalTest,
EnterpriseConnectorsPolicyHandlerLocalTest,
- testing::Combine(testing::Values(kValidLocalContentAnalysisPolicy,
+ testing::Combine(testing::Values(kValidPolicy,
+ kInvalidPolicy,
+ kValidLocalContentAnalysisPolicy,
kInvalidProviderLocalContentAnalysisPolicy,
kFakeProviderLocalContentAnalysisPolicy,
kEmptyPolicy),
@@ -259,7 +238,6 @@
#if BUILDFLAG(IS_CHROMEOS)
kOnFileTransferPref,
#endif
- kOnSecurityEventPref),
- testing::Bool()));
+ kOnSecurityEventPref)));
} // namespace enterprise_connectors
