Refactor policy handlers with policy names to derive from same base class.

By making TypeCheckingPolicyHandler, SchemaValidatingPolicyHandler, and
SimpleJsonStringSchemaValidatingPolicyHandler all derive from the same
class we can allow SimpleDeprecatingPolicyHandler to work with all of them.

BUG=chromium:1112810

Change-Id: I67a75965c123989e227efc7a9fad47bd4b7f405e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2337118
Commit-Queue: Brian Malcolm <bmalcolm@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794702}

components/policy/core/browser/configuration_policy_handler.cc

@@ -47,19 +47,25 @@ void ConfigurationPolicyHandler::ApplyPolicySettingsWithParameters(
   ApplyPolicySettings(policies, prefs);
 }
 
+// NamedPolicyHandler implementation -------------------------------------------
+NamedPolicyHandler::NamedPolicyHandler(const char* policy_name)
+    : policy_name_(policy_name) {}
+
+NamedPolicyHandler::~NamedPolicyHandler() = default;
+
+const char* NamedPolicyHandler::policy_name() const {
+  return policy_name_;
+}
+
 // TypeCheckingPolicyHandler implementation ------------------------------------
 
 TypeCheckingPolicyHandler::TypeCheckingPolicyHandler(
     const char* policy_name,
     base::Value::Type value_type)
-    : policy_name_(policy_name), value_type_(value_type) {}
+    : NamedPolicyHandler(policy_name), value_type_(value_type) {}
 
 TypeCheckingPolicyHandler::~TypeCheckingPolicyHandler() {}
 
-const char* TypeCheckingPolicyHandler::policy_name() const {
-  return policy_name_;
-}
-
 bool TypeCheckingPolicyHandler::CheckPolicySettings(const PolicyMap& policies,
                                                     PolicyErrorMap* errors) {
   const base::Value* value = nullptr;
@@ -69,9 +75,9 @@ bool TypeCheckingPolicyHandler::CheckPolicySettings(const PolicyMap& policies,
 bool TypeCheckingPolicyHandler::CheckAndGetValue(const PolicyMap& policies,
                                                  PolicyErrorMap* errors,
                                                  const base::Value** value) {
-  *value = policies.GetValue(policy_name_);
+  *value = policies.GetValue(policy_name());
   if (*value && (*value)->type() != value_type_) {
-    errors->AddError(policy_name_, IDS_POLICY_TYPE_ERROR,
+    errors->AddError(policy_name(), IDS_POLICY_TYPE_ERROR,
                      base::Value::GetTypeName(value_type_));
     return false;
   }
@@ -352,16 +358,12 @@ SchemaValidatingPolicyHandler::SchemaValidatingPolicyHandler(
     const char* policy_name,
     Schema schema,
     SchemaOnErrorStrategy strategy)
-    : policy_name_(policy_name), schema_(schema), strategy_(strategy) {
+    : NamedPolicyHandler(policy_name), schema_(schema), strategy_(strategy) {
   DCHECK(schema_.valid());
 }
 
 SchemaValidatingPolicyHandler::~SchemaValidatingPolicyHandler() {}
 
-const char* SchemaValidatingPolicyHandler::policy_name() const {
-  return policy_name_;
-}
-
 bool SchemaValidatingPolicyHandler::CheckPolicySettings(
     const PolicyMap& policies,
     PolicyErrorMap* errors) {
@@ -376,7 +378,7 @@ bool SchemaValidatingPolicyHandler::CheckPolicySettings(
   if (errors && !error.empty()) {
     if (error_path.empty())
       error_path = "(ROOT)";
-    errors->AddError(policy_name_, error_path, error);
+    errors->AddError(policy_name(), error_path, error);
   }
 
   return result;
@@ -399,7 +401,7 @@ bool SchemaValidatingPolicyHandler::CheckAndGetValue(
   if (errors && !error.empty()) {
     if (error_path.empty())
       error_path = "(ROOT)";
-    errors->AddError(policy_name_, error_path, error);
+    errors->AddError(policy_name(), error_path, error);
   }
 
   return result;
@@ -462,8 +464,8 @@ SimpleJsonStringSchemaValidatingPolicyHandler::
             recommended_permission,
         SimpleSchemaValidatingPolicyHandler::MandatoryPermission
             mandatory_permission)
-    : policy_name_(policy_name),
-      schema_(schema.GetKnownProperty(policy_name_)),
+    : NamedPolicyHandler(policy_name),
+      schema_(schema.GetKnownProperty(policy_name)),
       pref_path_(pref_path),
       allow_recommended_(
           recommended_permission ==
@@ -478,17 +480,17 @@ SimpleJsonStringSchemaValidatingPolicyHandler::
 bool SimpleJsonStringSchemaValidatingPolicyHandler::CheckPolicySettings(
     const PolicyMap& policies,
     PolicyErrorMap* errors) {
-  const base::Value* root_value = policies.GetValue(policy_name_);
+  const base::Value* root_value = policies.GetValue(policy_name());
   if (!root_value)
     return true;
 
-  const PolicyMap::Entry* policy_entry = policies.Get(policy_name_);
+  const PolicyMap::Entry* policy_entry = policies.Get(policy_name());
   if ((policy_entry->level == policy::POLICY_LEVEL_MANDATORY &&
        !allow_mandatory_) ||
       (policy_entry->level == policy::POLICY_LEVEL_RECOMMENDED &&
        !allow_recommended_)) {
     if (errors)
-      errors->AddError(policy_name_, IDS_POLICY_LEVEL_ERROR);
+      errors->AddError(policy_name(), IDS_POLICY_LEVEL_ERROR);
     return false;
   }
 
@@ -504,7 +506,7 @@ bool SimpleJsonStringSchemaValidatingPolicyHandler::CheckSingleJsonString(
   // First validate the root value is a string.
   if (!root_value->is_string()) {
     if (errors) {
-      errors->AddError(policy_name_, "(ROOT)", IDS_POLICY_TYPE_ERROR,
+      errors->AddError(policy_name(), "(ROOT)", IDS_POLICY_TYPE_ERROR,
                        base::Value::GetTypeName(base::Value::Type::STRING));
     }
     return false;
@@ -525,7 +527,7 @@ bool SimpleJsonStringSchemaValidatingPolicyHandler::CheckListOfJsonStrings(
   // First validate the root value is a list.
   if (!root_value->is_list()) {
     if (errors) {
-      errors->AddError(policy_name_, "(ROOT)", IDS_POLICY_TYPE_ERROR,
+      errors->AddError(policy_name(), "(ROOT)", IDS_POLICY_TYPE_ERROR,
                        base::Value::GetTypeName(base::Value::Type::LIST));
     }
     return false;
@@ -540,7 +542,7 @@ bool SimpleJsonStringSchemaValidatingPolicyHandler::CheckListOfJsonStrings(
     const base::Value& entry = list[index];
     if (!entry.is_string()) {
       if (errors) {
-        errors->AddError(policy_name_, index, IDS_POLICY_TYPE_ERROR,
+        errors->AddError(policy_name(), index, IDS_POLICY_TYPE_ERROR,
                          base::Value::GetTypeName(base::Value::Type::STRING));
       }
       continue;
@@ -567,7 +569,7 @@ bool SimpleJsonStringSchemaValidatingPolicyHandler::ValidateJsonString(
           json_string, base::JSONParserOptions::JSON_ALLOW_TRAILING_COMMAS);
   if (!value_with_error.value) {
     if (errors) {
-      errors->AddError(policy_name_, ErrorPath(index, ""),
+      errors->AddError(policy_name(), ErrorPath(index, ""),
                        IDS_POLICY_INVALID_JSON_ERROR,
                        value_with_error.error_message);
     }
@@ -585,7 +587,7 @@ bool SimpleJsonStringSchemaValidatingPolicyHandler::ValidateJsonString(
                                                SCHEMA_ALLOW_UNKNOWN,
                                                &error_path, &schema_error);
   if (errors && !schema_error.empty())
-    errors->AddError(policy_name_, ErrorPath(index, error_path), schema_error);
+    errors->AddError(policy_name(), ErrorPath(index, error_path), schema_error);
   if (!validated)
     return false;
 
@@ -609,13 +611,13 @@ void SimpleJsonStringSchemaValidatingPolicyHandler::ApplyPolicySettings(
     PrefValueMap* prefs) {
   if (!pref_path_)
     return;
-  const base::Value* value = policies.GetValue(policy_name_);
+  const base::Value* value = policies.GetValue(policy_name());
   if (value)
     prefs->SetValue(pref_path_, value->Clone());
 }
 
 void SimpleJsonStringSchemaValidatingPolicyHandler::RecordJsonError() {
-  const PolicyDetails* details = GetChromePolicyDetails(policy_name_);
+  const PolicyDetails* details = GetChromePolicyDetails(policy_name());
   if (details) {
     base::UmaHistogramSparse("EnterpriseCheck.InvalidJsonPolicies",
                              details->id);
@@ -680,8 +682,8 @@ void LegacyPoliciesDeprecatingPolicyHandler::ApplyPolicySettings(
 // SimpleDeprecatingPolicyHandler implementation -----------------------
 
 SimpleDeprecatingPolicyHandler::SimpleDeprecatingPolicyHandler(
-    std::unique_ptr<TypeCheckingPolicyHandler> legacy_policy_handler,
-    std::unique_ptr<TypeCheckingPolicyHandler> new_policy_handler)
+    std::unique_ptr<NamedPolicyHandler> legacy_policy_handler,
+    std::unique_ptr<NamedPolicyHandler> new_policy_handler)
     : legacy_policy_handler_(std::move(legacy_policy_handler)),
       new_policy_handler_(std::move(new_policy_handler)) {}
 

components/policy/core/browser/configuration_policy_handler.h

@@ -71,10 +71,25 @@ class POLICY_EXPORT ConfigurationPolicyHandler {
   DISALLOW_COPY_AND_ASSIGN(ConfigurationPolicyHandler);
 };
 
+// Abstract class derived from ConfigurationPolicyHandler that should be
+// subclassed to handle policies that have a name.
+class POLICY_EXPORT NamedPolicyHandler : public ConfigurationPolicyHandler {
+ public:
+  explicit NamedPolicyHandler(const char* policy_name);
+  ~NamedPolicyHandler() override;
+  NamedPolicyHandler(const NamedPolicyHandler&) = delete;
+  NamedPolicyHandler& operator=(const NamedPolicyHandler&) = delete;
+
+  const char* policy_name() const;
+
+ private:
+  // The name of the policy.
+  const char* policy_name_;
+};
+
 // Abstract class derived from ConfigurationPolicyHandler that should be
 // subclassed to handle a single policy (not a combination of policies).
-class POLICY_EXPORT TypeCheckingPolicyHandler
-    : public ConfigurationPolicyHandler {
+class POLICY_EXPORT TypeCheckingPolicyHandler : public NamedPolicyHandler {
  public:
   TypeCheckingPolicyHandler(const char* policy_name,
                             base::Value::Type value_type);
@@ -84,8 +99,6 @@ class POLICY_EXPORT TypeCheckingPolicyHandler
   bool CheckPolicySettings(const PolicyMap& policies,
                            PolicyErrorMap* errors) override;
 
-  const char* policy_name() const;
-
  protected:
   // Runs policy checks and returns the policy value if successful.
   bool CheckAndGetValue(const PolicyMap& policies,
@@ -93,9 +106,6 @@ class POLICY_EXPORT TypeCheckingPolicyHandler
                         const base::Value** value);
 
  private:
-  // The name of the policy.
-  const char* policy_name_;
-
   // The type the value of the policy should have.
   base::Value::Type value_type_;
 
@@ -306,8 +316,7 @@ class POLICY_EXPORT IntPercentageToDoublePolicyHandler
 // Like TypeCheckingPolicyHandler, but validates against a schema instead of a
 // single type. |schema| is the schema used for this policy, and |strategy| is
 // the strategy used for schema validation errors.
-class POLICY_EXPORT SchemaValidatingPolicyHandler
-    : public ConfigurationPolicyHandler {
+class POLICY_EXPORT SchemaValidatingPolicyHandler : public NamedPolicyHandler {
  public:
   SchemaValidatingPolicyHandler(const char* policy_name,
                                 Schema schema,
@@ -318,8 +327,6 @@ class POLICY_EXPORT SchemaValidatingPolicyHandler
   bool CheckPolicySettings(const PolicyMap& policies,
                            PolicyErrorMap* errors) override;
 
-  const char* policy_name() const;
-
  protected:
   // Runs policy checks and returns the policy value if successful.
   bool CheckAndGetValue(const PolicyMap& policies,
@@ -327,7 +334,6 @@ class POLICY_EXPORT SchemaValidatingPolicyHandler
                         std::unique_ptr<base::Value>* output);
 
  private:
-  const char* policy_name_;
   const Schema schema_;
   const SchemaOnErrorStrategy strategy_;
 
@@ -380,7 +386,7 @@ class POLICY_EXPORT SimpleSchemaValidatingPolicyHandler
 // - You don't have to parse JSON every time you read it from the pref store.
 // - Nested dicts are simple, but nested JSON strings are complicated.
 class POLICY_EXPORT SimpleJsonStringSchemaValidatingPolicyHandler
-    : public ConfigurationPolicyHandler {
+    : public NamedPolicyHandler {
  public:
   SimpleJsonStringSchemaValidatingPolicyHandler(
       const char* policy_name,
@@ -431,7 +437,6 @@ class POLICY_EXPORT SimpleJsonStringSchemaValidatingPolicyHandler
   // Returns true if the schema root is a list.
   bool IsListSchema() const;
 
-  const char* policy_name_;
   const Schema schema_;
   const char* pref_path_;
   const bool allow_recommended_;
@@ -478,8 +483,8 @@ class POLICY_EXPORT SimpleDeprecatingPolicyHandler
     : public ConfigurationPolicyHandler {
  public:
   SimpleDeprecatingPolicyHandler(
-      std::unique_ptr<TypeCheckingPolicyHandler> legacy_policy_handler,
-      std::unique_ptr<TypeCheckingPolicyHandler> new_policy_handler);
+      std::unique_ptr<NamedPolicyHandler> legacy_policy_handler,
+      std::unique_ptr<NamedPolicyHandler> new_policy_handler);
   ~SimpleDeprecatingPolicyHandler() override;
   SimpleDeprecatingPolicyHandler(const SimpleDeprecatingPolicyHandler&) =
       delete;
@@ -500,8 +505,8 @@ class POLICY_EXPORT SimpleDeprecatingPolicyHandler
                            PrefValueMap* prefs) override;
 
  private:
-  std::unique_ptr<TypeCheckingPolicyHandler> legacy_policy_handler_;
-  std::unique_ptr<TypeCheckingPolicyHandler> new_policy_handler_;
+  std::unique_ptr<NamedPolicyHandler> legacy_policy_handler_;
+  std::unique_ptr<NamedPolicyHandler> new_policy_handler_;
 };
 
 }  // namespace policy