Added a PolicySchema class.

Added a PolicySchema class.

A PolicySchema describes the known policy keys (for Chrome, or for an extension)
and maps them to their expected base::Value::Type. This class parses a buffer
as a JSON schema and additionally verifies that it can be used to describe a
policy map (e.g. the schema version is defined, the top-level is an "object",
and other checks).

BUG=108992
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201707

[Joao da Silva, 2013-05-14 16:43:29]

This depends on https://codereview.chromium.org/14830007.

@mnissler: please review

@kalman: FYI

[Mattias Nissler (ping if slow), 2013-05-15 10:04:12]

I love the fact that these changes come in handy pieces for review! Good job on
that.

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_schema.cc (right):

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema.cc:57: *error = "Must declare JSON Schema v3
version in \"$schema\"";
Is this a good idea? This will prevent upgrading to later Schema versions, even
if they are forward-compatible.

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema.cc:72: "supported at the main schema";
Ditto: Should this be a hard error or merely a warning if you consider
forward-compatibility?

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema.cc:91: *error = "The type of main properties
can't be \"any\"";
Forward-compatibility?

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_schema.h (right):

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema.h:18: // type is declared; the types of list
and dictionary entries are not declared.
Is there a plan for further type checks? In particular, can you elaborate on
"currently"? Are there plans or not?

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_schema_unittest.cc (right):

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema_unittest.cc:40:
EXPECT_TRUE(ParseFails("{}"));
Many of these seem redundant, given that you rely on the generic schema
validator (which I assume has reasonable test coverage already.

[Joao da Silva, 2013-05-19 13:17:08]

@bartfab: passing this on to you; please have a look. Thanks!

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_schema.cc (right):

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema.cc:57: *error = "Must declare JSON Schema v3
version in \"$schema\"";
On 2013/05/15 10:04:12, Mattias Nissler wrote:
> Is this a good idea? This will prevent upgrading to later Schema versions,
even
> if they are forward-compatible.

If a new format is introduced then Chrome X will start supporting that version,
and it becomes publicly supported in Chrome X+1 or X+2.

I don't think it's a good idea to relax this and accept schemas that "look like
v3". It's possible that "v4" parses as v3 but has different semantics.

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema.cc:72: "supported at the main schema";
On 2013/05/15 10:04:12, Mattias Nissler wrote:
> Ditto: Should this be a hard error or merely a warning if you consider
> forward-compatibility?

This restriction is forced because some policy loaders need the exact policy
name to find it. If that isn't needed anymore in the future then we do the X+1
or 2 roll to relax this.

If we relaxed this now then policy names that aren't declared in "properties"
are never loaded, which would be more confusing.

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema.cc:91: *error = "The type of main properties
can't be \"any\"";
On 2013/05/15 10:04:12, Mattias Nissler wrote:
> Forward-compatibility?

Same rationale again; we can roll an updated version, and for now we need the
exact type.

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_schema.h (right):

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema.h:18: // type is declared; the types of list
and dictionary entries are not declared.
On 2013/05/15 10:04:12, Mattias Nissler wrote:
> Is there a plan for further type checks? In particular, can you elaborate on
> "currently"? Are there plans or not?

That comment made me think more about this, and I realized that the types of
sub-schemas is also needed to load dictionaries and lists correctly on Windows.

This class has been expanded to represent properties and list items.

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_schema_unittest.cc (right):

https://codereview.chromium.org/15133005/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_schema_unittest.cc:40:
EXPECT_TRUE(ParseFails("{}"));
On 2013/05/15 10:04:12, Mattias Nissler wrote:
> Many of these seem redundant, given that you rely on the generic schema
> validator (which I assume has reasonable test coverage already.

It's true that the JSONSchemaValidator tests cover some of these, but I'd rather
err on the side of too-many tests than too-few. The validator code and tests can
be modified without us noticing, for example.

[bartfab (slow), 2013-05-21 11:02:39]

lgtm

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
File chrome/browser/policy/policy_schema.cc (right):

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:7: #include "base/json/json_reader.h"
What is this used for?

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:27: virtual const PolicySchemaMap*
GetProperties() const OVERRIDE;
#include "base/compiler_specific.h"

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:69: { json_schema_constants::kObject,    
  base::Value::TYPE_DICTIONARY  },
nit: Why a single space after { and a double space before }?

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:91: *error = "The \"any\" type can't be
used.";
You can't be sure that the schema was "any" here. It was some unsupported value,
maybe "any", maybe some typo...

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:198: PolicySchemaMap::const_iterator it =
properties->find(key);
nit: You can additionally make |it| const.

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:221: *error = "Must declare JSON Schema
v3 version in \"$schema\"";
Nit: Above this line, all error strings were full sentences, ending in a full
stop. From here on, they no longer have full stops at the end.

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
File chrome/browser/policy/policy_schema.h (right):

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.h:62: base::Value::Type type_;
nit: make this const?

[Joao da Silva, 2013-05-21 17:16:25]

Thanks for the review!

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
File chrome/browser/policy/policy_schema.cc (right):

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:7: #include "base/json/json_reader.h"
On 2013/05/21 11:02:40, bartfab wrote:
> What is this used for?

It's not used, removed.

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:27: virtual const PolicySchemaMap*
GetProperties() const OVERRIDE;
On 2013/05/21 11:02:40, bartfab wrote:
> #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:69: { json_schema_constants::kObject,    
  base::Value::TYPE_DICTIONARY  },
On 2013/05/21 11:02:40, bartfab wrote:
> nit: Why a single space after { and a double space before }?

Ha, that was vi aligning at tab boundaries. Fixed.

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:91: *error = "The \"any\" type can't be
used.";
On 2013/05/21 11:02:40, bartfab wrote:
> You can't be sure that the schema was "any" here. It was some unsupported
value,
> maybe "any", maybe some typo...

Actually only "any" could trigger this, because |schema| was previously parsed
by JSONSchemaValidator, which only accepts the valid, known types. The only one
that is not allowed here is "any".

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:198: PolicySchemaMap::const_iterator it =
properties->find(key);
On 2013/05/21 11:02:40, bartfab wrote:
> nit: You can additionally make |it| const.

A const_iterator is already a const pointer to something (i.e. it can't modify
what it points to). Making |it| const as well (so that it can't point to another
position) doesn't seem idiomatic in chrome code, afaict.

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:221: *error = "Must declare JSON Schema
v3 version in \"$schema\"";
On 2013/05/21 11:02:40, bartfab wrote:
> Nit: Above this line, all error strings were full sentences, ending in a full
> stop. From here on, they no longer have full stops at the end.

Done.

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
File chrome/browser/policy/policy_schema.h (right):

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.h:62: base::Value::Type type_;
On 2013/05/21 11:02:40, bartfab wrote:
> nit: make this const?

Done.

[bartfab (slow), 2013-05-21 18:09:56]

still lgtm

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
File chrome/browser/policy/policy_schema.cc (right):

https://codereview.chromium.org/15133005/diff/20005/chrome/browser/policy/pol...
chrome/browser/policy/policy_schema.cc:198: PolicySchemaMap::const_iterator it =
properties->find(key);
On 2013/05/21 17:16:25, Joao da Silva wrote:
> On 2013/05/21 11:02:40, bartfab wrote:
> > nit: You can additionally make |it| const.
> 
> A const_iterator is already a const pointer to something (i.e. it can't modify
> what it points to). Making |it| const as well (so that it can't point to
another
> position) doesn't seem idiomatic in chrome code, afaict.

Yes, preventing |it| from being reset is what I was thinking about. But you are
right, this does not seem to be usually done in Chrome code (which is a shame -
it would catch bugs at compile time where you try to reset an iterator that was
meant to be const).

[commit-bot: I haz the power, 2013-05-22 11:54:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/15133005/63001

[commit-bot: I haz the power, 2013-05-22 13:45:55]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-05-22 13:50:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/15133005/63001

[commit-bot: I haz the power, 2013-05-23 05:25:54]

Change committed as 201707