Add strictness to Schema::Validate()

components/policy/core/common/schema.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/policy/core/common/schema.h"
 
 #include <algorithm>
 #include <climits>
 #include <map>
 #include <utility>
@@ skipping 64 matching lines @@
   };
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kSchemaToValueTypeMap); ++i) {
     if (kSchemaToValueTypeMap[i].schema_type == type_string) {
       *type = kSchemaToValueTypeMap[i].value_type;
       return true;
     }
   }
   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
 // a SchemaData owned elsewhere (currently used to wrap the Chrome schema, which
 // is generated at compile time), or it can own its own SchemaData.
 class Schema::InternalStorage
     : public base::RefCountedThreadSafe<InternalStorage> {
  public:
   static scoped_refptr<const InternalStorage> Wrap(const SchemaData* data);
 
@@ skipping 507 matching lines @@
   node_ = schema.node_;
   return *this;
 }
 
 // static
 Schema Schema::Wrap(const SchemaData* data) {
   scoped_refptr<const InternalStorage> storage = InternalStorage::Wrap(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;
   int int_value;
   std::string str_value;
   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
   // may become used in a future version of the schema format.
   scoped_ptr<base::DictionaryValue> dict = JSONSchemaValidator::IsValidSchema(
       content, JSONSchemaValidator::OPTIONS_IGNORE_UNKNOWN_ATTRIBUTES, error);
   if (!dict)
     return Schema();
 
   // Validate the main type.
@@ skipping 95 matching lines @@
   const RestrictionNode* rnode = storage_->restriction(index);
   for (int i = rnode->enumeration_restriction.offset_begin;
        i < rnode->enumeration_restriction.offset_end; i++) {
     if (strcmp(*storage_->string_enums(i), str) == 0)
       return true;
   }
   return false;
 }
 
 }  // namespace policy