Add strictness to Schema::Validate()

components/policy/core/common/schema.cc

Index: components/policy/core/common/schema.cc
diff --git a/components/policy/core/common/schema.cc b/components/policy/core/common/schema.cc
index d9b400436a604bd6928747a93bb8dcc37e3185fd..954618ca3ebe4a2a2e9205dfdb4b48772e1cea81 100644
--- a/components/policy/core/common/schema.cc
+++ b/components/policy/core/common/schema.cc
@@ -82,6 +82,28 @@ bool SchemaTypeToValueType(const std::string& type_string,
   return false;
 }
 
+bool StrategyAllowInvalidOnTopLevel(SchemaOnErrorStrategy strategy) {
+  return strategy == SCHEMA_ALLOW_INVALID ||
+         strategy == SCHEMA_ALLOW_INVALID_TOPLEVEL ||
+         strategy == SCHEMA_ALLOW_INVALID_TOPLEVEL_AND_ALLOW_UNKNOWN;
+}
+
+bool StrategyAllowUnknownOnTopLevel(SchemaOnErrorStrategy strategy) {
+  return strategy != SCHEMA_VERY_STRICT;
+}
+
+SchemaOnErrorStrategy StrategyForNextLevel(SchemaOnErrorStrategy strategy) {
+  static SchemaOnErrorStrategy next_level_strategy[] = {
+    SCHEMA_VERY_STRICT,    // SCHEMA_VERY_STRICT
+    SCHEMA_VERY_STRICT,    // SCHEMA_ALLOW_UNKNOWN_TOPLEVEL
+    SCHEMA_ALLOW_UNKNOWN,  // SCHEMA_ALLOW_UNKNOWN
+    SCHEMA_VERY_STRICT,    // SCHEMA_ALLOW_INVALID_TOPLEVEL
+    SCHEMA_ALLOW_UNKNOWN,  // SCHEMA_ALLOW_INVALID_TOPLEVEL_AND_ALLOW_UNKNOWN
+    SCHEMA_ALLOW_INVALID,  // SCHEMA_ALLOW_INVALID
+  };
+  return next_level_strategy[(int)strategy];
+}
+
 }  // namespace
 
 // Contains the internal data representation of a Schema. This can either wrap
@@ -609,14 +631,18 @@ Schema Schema::Wrap(const SchemaData* data) {
   return Schema(storage, storage->root_node());
 }
 
-bool Schema::Validate(const base::Value& value) const {
+bool Schema::Validate(const base::Value& value,
+                      SchemaOnErrorStrategy strategy,
+                      std::string *error) const {
   if (!valid()) {
-    // Schema not found, invalid entry.
+    *error = "Fatal error: The schema itself is invalid.";

[Joao da Silva, 2014/01/22 10:49:53]

Just "The schema is invalid."

[binjin, 2014/01/23 12:01:31]

Done.

     return false;
   }
 
-  if (!value.IsType(type()))
+  if (!value.IsType(type())) {
+    *error = "The schema type and value type is not matched.";

[Joao da Silva, 2014/01/22 10:49:53]

The value type doesn't match the schema type.

[binjin, 2014/01/23 12:01:31]

Done.

     return false;
+  }
 
   const base::DictionaryValue* dict = NULL;
   const base::ListValue* list = NULL;
@@ -625,26 +651,111 @@ bool Schema::Validate(const base::Value& value) const {
   if (value.GetAsDictionary(&dict)) {
     for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd();
          it.Advance()) {
-      if (!GetProperty(it.key()).Validate(it.value()))
-        return false;
+      Schema subschema = GetProperty(it.key());
+      if (!subschema.valid()) {
+        // Unknown property was detected.
+        if (!StrategyAllowUnknownOnTopLevel(strategy)) {
+          *error = "Unknown property: " + it.key();
+          return false;
+        }
+      } else {

[Joao da Silva, 2014/01/22 10:49:53]

The else part should just be:

  std::string sub_error;
  if (!subschema.Validate(it.value(), StrategyForNextLevel(strategy),
&sub_error) {
    if (!StrategyAllowInvalidOnTopLevel(strategy)) {
      *error = sub_error;
      return false;
    }
  }

Is there a reason why you look at the subschema.node_->type below? I may be
missing something. Note that the subtype may be a list with dictionaries inside,
so you shouldn't  pass STRICT directly, because we may allow invalid or unknown
in those inner dictionaries.

[binjin, 2014/01/22 12:46:53]

The current approach is result of trade-off. In short words, I dropped support
for dict-list-dict-nested schema. The current approach support dict-dict-nested,
dict-list-nested, list-dict-nested schema well, but for dict-list-dict-nested
schema, it will use user defined strategy for outer dict and strict strategy for
inner one.

For the first question, I need separate routines to handle this since I think
StrategyAllowInvalidOnTopLevel(strategy) is only supposed to take effect on
properties. It's obvious, otherwise SCHEME_ALLOW_INVALID_TOPLEVEL will always
return true even if it's supposed to be strict for deeper level invalids.

For the second question, the current approach is based on an assumption that
every properties in a dict which is not a dict type are supposed to strictly
follow the schema. This is a result of trade-off. Take the dict-list-dict-nested
schema as example:
1) There is no cheap way to verify if "some-propery-name-list-dict" is dict type
instead of list type, but it's necessary as stated before for
StrategyAllowInvalidOnTopLevel(strategy) check.
2) I think strategy is meaningful for outer dict(or dict-dict-nested) only. It
looks odd no matter we reset it to root status or just keep it unchanged on the
inner dict node, and for the former case it complicated the code a lot.
3) Basically we are combining several policies into "A.B.C" paths in a dict,
it's a corner case to have dict-list-dict nested schema, and for inner dict
there are no proper path names referring it as well.

+        std::string property_error;
+        if (subschema.node_->type == base::Value::TYPE_DICTIONARY) {
+          if (!subschema.Validate(it.value(),
+                StrategyForNextLevel(strategy), error)) {
+            return false;
+          }
+        } else if (!subschema.Validate(it.value(),
+            SCHEMA_VERY_STRICT, &property_error)) {
+          // Invalid property was detected.
+          if (!StrategyAllowInvalidOnTopLevel(strategy)) {
+            *error = property_error;
+            return false;
+          }
+        }
+      }
     }
   } else if (value.GetAsList(&list)) {
     for (base::ListValue::const_iterator it = list->begin();
          it != list->end(); ++it) {
-      if (!*it || !GetItems().Validate(**it))
+      if (!*it || !GetItems().Validate(**it, strategy, error))

[Joao da Silva, 2014/01/22 10:49:53]

Replace "strategy" with StrategyForNextLevel(strategy). The list may contain
dictionaries.

[binjin, 2014/01/22 12:46:53]

For list-dict-nested schema(with list/list-list-nested in the root), I think
it's better to keep strategy unchanged since it just means nothing for list. 

For dict-list-nested, following the assumption stated above I passed the strict
strategy to lower levels.

         return false;
     }
   } else if (value.GetAsInteger(&int_value)) {
-    return node_->extra == kInvalid ||
-        ValidateIntegerRestriction(node_->extra, int_value);
+    if (node_->extra != kInvalid &&
+        !ValidateIntegerRestriction(node_->extra, int_value)) {
+      *error = "Invalid value for integer";
+      return false;
+    }
   } else if (value.GetAsString(&str_value)) {
-    return node_->extra == kInvalid ||
-        ValidateStringRestriction(node_->extra, str_value.c_str());
+    if (node_->extra != kInvalid &&
+        !ValidateStringRestriction(node_->extra, str_value.c_str())) {
+      *error = "Invalid value for string";
+      return false;
+    }
   }
 
   return true;
 }
 
+bool Schema::Normalize(base::Value* value,
+                       SchemaOnErrorStrategy strategy,
+                       std::string *error) const {
+  if (!valid()) {
+    *error = "Fatal error: The schema itself is invalid.";

[Joao da Silva, 2014/01/22 10:49:53]

Just "The schema is invalid."

[binjin, 2014/01/23 12:01:31]

Done.

+    return false;
+  }
+
+  if (!value->IsType(type())) {
+    *error = "The schema type and value type is not matched.";

[Joao da Silva, 2014/01/22 10:49:53]

The value type doesn't match the schema type.

[binjin, 2014/01/23 12:01:31]

Done.

+    return false;
+  }
+
+  base::DictionaryValue* dict = NULL;
+  if (value->GetAsDictionary(&dict)) {
+    std::vector<std::string> drop_list;  // Contains the keys to remove.
+    for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd();
+         it.Advance()) {
+      Schema subschema = GetProperty(it.key());
+      if (!subschema.valid()) {
+        // Unknown property was detected.
+        if (StrategyAllowUnknownOnTopLevel(strategy)) {
+          drop_list.push_back(it.key());
+        } else {
+          *error = "Unknown property: " + it.key();
+          return false;
+        }
+      } else {
+        std::string property_error;
+        if (subschema.node_->type == base::Value::TYPE_DICTIONARY) {
+          base::DictionaryValue* subdict = NULL;
+          dict->GetDictionary(it.key(), &subdict);
+          if (!subschema.Normalize(subdict,
+              StrategyForNextLevel(strategy), error)) {
+            return false;
+          }
+        } else if (!subschema.Validate(it.value(),
+              SCHEMA_VERY_STRICT, &property_error)) {
+          // Invalid property was detected.
+          if (StrategyAllowInvalidOnTopLevel(strategy)) {
+            drop_list.push_back(it.key());
+          } else {
+            *error = property_error;
+            return false;
+          }
+        }
+      }
+    }
+    for (std::vector<std::string>::const_iterator it = drop_list.begin();
+         it != drop_list.end(); ++it) {
+      dict->Remove(*it, NULL);
+    }
+    return true;
+  }

[Joao da Silva, 2014/01/22 10:49:53]

This needs to handle lists of dictionaries; the inner dictionaries must be
normalized too.

[binjin, 2014/01/22 12:46:53]

Yes, it did break normalizing by list-dict-nested schema, I will fix it.

+
+  return Validate(*value, strategy, error);
+}
+
 // static
 Schema Schema::Parse(const std::string& content, std::string* error) {
   // Validate as a generic JSON schema, and ignore unknown attributes; they