Remove SCHEMA_ALLOW_INVALID from SchemaOnErrorStrategy

Deprecate the policy schema SCHEMA_ALLOW_INVALID since it only has a different value from SCHEMA_ALLOW_UNKNOWN on Mac OS and Android and in a low percentage of its use cases.

Bug: 969706
Change-Id: Ie5251e1ceaa6a4bc8472e6bb1477b7a6de45fc2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2018946
Commit-Queue: Anina Koehler <aninak@google.com>
Reviewed-by: Alexander Hendrich <hendrich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742540}

components/policy/core/browser/configuration_policy_handler_unittest.cc

@@ -753,19 +753,9 @@ TEST(SchemaValidatingPolicyHandlerTest, CheckAndGetValueInvalid) {
   policy_map.LoadFrom(policy_map_dict, POLICY_LEVEL_RECOMMENDED,
                       POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD);
 
-  TestSchemaValidatingPolicyHandler handler(schema, SCHEMA_ALLOW_INVALID);
+  TestSchemaValidatingPolicyHandler handler(schema, SCHEMA_ALLOW_UNKNOWN);
   std::unique_ptr<base::Value> output_value;
-  ASSERT_TRUE(handler.CheckAndGetValueForTest(policy_map, &output_value));
-  ASSERT_TRUE(output_value);
-
-  base::DictionaryValue* dict = nullptr;
-  ASSERT_TRUE(output_value->GetAsDictionary(&dict));
-
-  // Test that CheckAndGetValue() actually dropped invalid properties.
-  int int_value = -1;
-  EXPECT_TRUE(dict->GetInteger("OneToThree", &int_value));
-  EXPECT_EQ(2, int_value);
-  EXPECT_FALSE(dict->HasKey("Colors"));
+  EXPECT_FALSE(handler.CheckAndGetValueForTest(policy_map, &output_value));
 }
 
 TEST(SchemaValidatingPolicyHandlerTest, CheckAndGetValueUnknown) {
@@ -807,7 +797,7 @@ TEST(SchemaValidatingPolicyHandlerTest, CheckAndGetValueUnknown) {
   policy_map.LoadFrom(policy_map_dict, POLICY_LEVEL_RECOMMENDED,
                       POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD);
 
-  TestSchemaValidatingPolicyHandler handler(schema, SCHEMA_ALLOW_INVALID);
+  TestSchemaValidatingPolicyHandler handler(schema, SCHEMA_ALLOW_UNKNOWN);
   std::unique_ptr<base::Value> output_value;
   ASSERT_TRUE(handler.CheckAndGetValueForTest(policy_map, &output_value));
   ASSERT_TRUE(output_value);

components/policy/core/common/schema.cc

@@ -221,10 +221,6 @@ bool SchemaTypeToValueType(const std::string& schema_type,
                                  kSchemaTypesToValueTypesEnd, value_type);
 }
 
-bool StrategyAllowInvalid(SchemaOnErrorStrategy strategy) {
-  return strategy == SCHEMA_ALLOW_INVALID;
-}
-
 bool StrategyAllowUnknown(SchemaOnErrorStrategy strategy) {
   return strategy != SCHEMA_STRICT;
 }
@@ -1219,7 +1215,6 @@ bool Schema::Validate(const base::Value& value,
         if (!StrategyAllowUnknown(strategy))
           return false;
       } else {
-        bool all_subschemas_are_valid = true;
         for (const auto& subschema : schema_list) {
           std::string new_error;
           const bool validation_result = subschema.Validate(
@@ -1230,13 +1225,10 @@ bool Schema::Validate(const base::Value& value,
           }
           if (!validation_result) {
             // Invalid property was detected.
-            all_subschemas_are_valid = false;
-            if (!StrategyAllowInvalid(strategy))
-              return false;
+            return false;
           }
         }
-        if (all_subschemas_are_valid)
-          present_properties.insert(dict_item.first);
+        present_properties.insert(dict_item.first);
       }
     }
 
@@ -1259,7 +1251,7 @@ bool Schema::Validate(const base::Value& value,
         AddListIndexPrefixToPath(index, error_path);
         *error = std::move(new_error);
       }
-      if (!validation_result && !StrategyAllowInvalid(strategy))
+      if (!validation_result)
         return false;  // Invalid list item was detected.
     }
   } else if (value.is_int()) {
@@ -1314,7 +1306,6 @@ bool Schema::Normalize(base::Value* value,
           return false;
         drop_list.push_back(dict_item.first);
       } else {
-        bool all_subschemas_are_valid = true;
         for (const auto& subschema : schema_list) {
           std::string new_error;
           const bool normalization_result = subschema.Normalize(
@@ -1325,15 +1316,12 @@ bool Schema::Normalize(base::Value* value,
           }
           if (!normalization_result) {
             // Invalid property was detected.
-            all_subschemas_are_valid = false;
-            if (!StrategyAllowInvalid(strategy))
-              return false;
+            return false;
             drop_list.push_back(dict_item.first);
             break;
           }
         }
-        if (all_subschemas_are_valid)
-          present_properties.insert(dict_item.first);
+        present_properties.insert(dict_item.first);
       }
     }
 
@@ -1370,8 +1358,7 @@ bool Schema::Normalize(base::Value* value,
       }
       if (!normalization_result) {
         // Invalid list item was detected.
-        if (!StrategyAllowInvalid(strategy))
-          return false;
+        return false;
       } else {
         if (write_index != index)
           list[write_index] = std::move(list_item);

components/policy/core/common/schema.h

@@ -41,8 +41,6 @@ enum SchemaOnErrorStrategy {
   SCHEMA_STRICT = 0,
   // Unknown properties in any dictionary will be ignored.
   SCHEMA_ALLOW_UNKNOWN,
-  // Mismatched values will be ignored.
-  SCHEMA_ALLOW_INVALID,
 };
 
 // Schema validation options for Schema::ParseToDictAndValidate().

components/policy/core/common/schema_unittest.cc

@@ -794,7 +794,7 @@ TEST(SchemaTest, Validate) {
   // Invalid top level property.
   bundle.SetInteger("Boolean", 12345);
   TestSchemaValidation(schema, bundle, SCHEMA_STRICT, false);
-  TestSchemaValidation(schema, bundle, SCHEMA_ALLOW_INVALID, true);
+  TestSchemaValidation(schema, bundle, SCHEMA_ALLOW_UNKNOWN, false);
   TestSchemaValidationWithPath(schema, bundle, "Boolean");
   bundle.SetBoolean("Boolean", true);
 
@@ -808,7 +808,6 @@ TEST(SchemaTest, Validate) {
     root.SetBoolean("Object.three", false);
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, true);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, true);
     TestSchemaValidationWithPath(subschema, root, "Object");
     root.Remove("Object.three", nullptr);
 
@@ -816,7 +815,6 @@ TEST(SchemaTest, Validate) {
     root.SetInteger("Object.one", 12345);
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, false);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, true);
     TestSchemaValidationWithPath(subschema, root, "Object.one");
     root.Remove("Object.one", nullptr);
   }
@@ -834,7 +832,6 @@ TEST(SchemaTest, Validate) {
     root.Append(std::move(dict_value));
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, true);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, true);
     TestSchemaValidationWithPath(subschema, root, "items[0]");
     root.Remove(root.GetSize() - 1, nullptr);
 
@@ -844,7 +841,6 @@ TEST(SchemaTest, Validate) {
     root.Append(std::move(dict_value));
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, false);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, true);
     TestSchemaValidationWithPath(subschema, root, "items[0].two");
     root.Remove(root.GetSize() - 1, nullptr);
   }
@@ -862,13 +858,11 @@ TEST(SchemaTest, Validate) {
     list_value->AppendInteger(12345);
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, true);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, true);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, true);
 
     // Invalid list item.
     list_value->AppendString("blabla");
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, false);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, true);
     TestSchemaValidationWithPath(subschema, root, "List.items[1]");
   }
 
@@ -887,13 +881,11 @@ TEST(SchemaTest, Validate) {
     list_value->AppendString("blabla");
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, true);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, true);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, true);
 
     // Invalid list item.
     list_value->AppendInteger(12345);
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, false);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, true);
     TestSchemaValidationWithPath(subschema, root, "items[0].List.items[1]");
   }
 
@@ -972,13 +964,11 @@ TEST(SchemaTest, Validate) {
     root.SetDouble("Number", 3.14);
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, false);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, false);
 
     // Invalid required property.
     root.SetInteger("String", 123);
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, false);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, false);
     root.SetString("String", "a string");
 
     // Invalid subschema of required property with multiple subschemas.
@@ -991,12 +981,10 @@ TEST(SchemaTest, Validate) {
     root.SetInteger("Integer", 2);
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, false);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, false);
 
     root.SetInteger("Integer", 3);
     TestSchemaValidation(subschema, root, SCHEMA_STRICT, false);
     TestSchemaValidation(subschema, root, SCHEMA_ALLOW_UNKNOWN, false);
-    TestSchemaValidation(subschema, root, SCHEMA_ALLOW_INVALID, false);
   }
 
   // Test that integer to double promotion is allowed.