Ignore unknown attributes when parsing JSON schemas.

Ignore unknown attributes when parsing JSON schemas.

JSON schemas are used by extensions that support enterprise management,
to declare the policies that they support. The current parser rejects
schemas with unknown attributes; this CL lifts that restriction for 2 reasons:

- the JSON schema spec does not mandate this behavior for validators

- it restricts future extensions to this format

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

[Joao da Silva, 2013-12-05 19:58:52]

@Benjamin: please have a look

@Mattias: FYI, as discussed earlier to support future changes, if any

Thanks!

[not at google - send to devlin, 2013-12-06 15:50:09]

In the case of an extension installed by policy, it'd be likely that the admin
would push the new extension, which would be installed, then push the new
policy, so it should always be up-to-date? I'm not totally convinced that's
true/safe though (maybe it's using some webstore that's offline or something).

I have actually been considering making a similar change for the extensions
system at large, but I'm not sure; is it better to silently do unexpected
behaviour, or loudly reject it? In the case of extension APIs, extensions can
gate on the version of Chrome that they use, particularly if/when we surface
that in the documentation. They can also use min_chrome_version in their
manifest or whatever that is. Still undecided though.

However in your case that's not an option obviously. I see a few options:
1 - don't change anything
2 - don't fail with unknown properties (this patch)
3 - remove unknown properties
4 - give the extension the option

3/4 don't sound very good to me (4 is basically 2 just less flexible) - so I
think 2. However perhaps (implementation wise) it should be an option to the
validator? Divergent behaviour in our JSON schema validators might bite us at
some point in the future.

[Joao da Silva, 2013-12-06 18:08:57]

On 2013/12/06 15:50:09, kalman wrote:
> In the case of an extension installed by policy, it'd be likely that the admin
> would push the new extension, which would be installed, then push the new
> policy, so it should always be up-to-date? I'm not totally convinced that's
> true/safe though (maybe it's using some webstore that's offline or something).
> 

The admin just pushes the extension ID and an optional update URL; then Chrome
always installs the latest version. So it's inevitable that some clients will
have older versions for some time.

Older versions of Chrome just ignore newer policies. If an admin configures some
policy that only shipped in M32 then older clients will simply ignore it, but
they won't break when it's present. For extensions we'd like to achieve the
same, though it's harder to get it right when the schema format changes.

We see 2 ways to handle such a change in the future:

1. a completely new format, not backwards compatible. If we ever have a good
reason to do this then we can have a new key in the manifest (e.g.
"storage.managed_schema_v2"), and it's up to the author to have both schemas if
he wants to support older versions of Chrome (we'd make this key require a
certain min_chrome_version). A policy should either have exactly the same type
in both schemas, or appear in only one of them (the v2 schema would use
different names). Chrome would have to recognize that key though, so it would
take a couple of releases to roll this out.

2. a small, incremental change that is backwards compatible; for example, adding
an option to hide the concrete policy value in about:policy. Older versions of
Chrome will still load and enforce the policies, they just won't honor this
option. If this is not acceptable to the author then he can use
min_chrome_version. This CL makes it possible to make evolutionary changes like
this; we don't foresee any such changes, but we'd like to keep the options open.

> I have actually been considering making a similar change for the extensions
> system at large, but I'm not sure; is it better to silently do unexpected
> behaviour, or loudly reject it? In the case of extension APIs, extensions can
> gate on the version of Chrome that they use, particularly if/when we surface
> that in the documentation. They can also use min_chrome_version in their
> manifest or whatever that is. Still undecided though.
> 
> However in your case that's not an option obviously. I see a few options:
> 1 - don't change anything
> 2 - don't fail with unknown properties (this patch)
> 3 - remove unknown properties
> 4 - give the extension the option
> 
> 3/4 don't sound very good to me (4 is basically 2 just less flexible) - so I
> think 2. However perhaps (implementation wise) it should be an option to the
> validator? Divergent behaviour in our JSON schema validators might bite us at
> some point in the future.

Loudly rejecting bad input makes many mistakes more obvious much sooner. I think
we're striking a good balance for policy-for-extensions: the extension won't get
any policies loaded if the type description isn't right, but we still allow new
options to be added. Typos in a "type" attribute will still be caught; but
having a typo in "additionalProperties" would be harder to spot, unfortunately.
We still think it's worth it.

So I'm going for 2, and I've added the option to the validator. PTAL, thanks!

[not at google - send to devlin, 2013-12-06 18:12:25]

lgtm

https://codereview.chromium.org/94043003/diff/20001/components/json_schema/js...
File components/json_schema/json_schema_validator.cc (right):

https://codereview.chromium.org/94043003/diff/20001/components/json_schema/js...
components/json_schema/json_schema_validator.cc:160: if (!ignore_unknown_attrs)
{
i.e. an option such that this would be

if (!(options & (1 << IGNORE_UNKNOWN_ATTRS))

https://codereview.chromium.org/94043003/diff/20001/components/json_schema/js...
File components/json_schema/json_schema_validator.h (right):

https://codereview.chromium.org/94043003/diff/20001/components/json_schema/js...
components/json_schema/json_schema_validator.h:111: bool
ignore_unknown_attributes,
pls make this an enum like the JSONWriter has (Options etc)

and consider providing two versions of this method, one with the options
argument and one without; that will clean up a lot of call sites.

[Joao da Silva, 2013-12-08 08:41:19]

https://codereview.chromium.org/94043003/diff/20001/components/json_schema/js...
File components/json_schema/json_schema_validator.h (right):

https://codereview.chromium.org/94043003/diff/20001/components/json_schema/js...
components/json_schema/json_schema_validator.h:111: bool
ignore_unknown_attributes,
On 2013/12/06 18:12:26, kalman wrote:
> pls make this an enum like the JSONWriter has (Options etc)
> 
> and consider providing two versions of this method, one with the options
> argument and one without; that will clean up a lot of call sites.

Done.

[commit-bot: I haz the power, 2013-12-08 08:41:27]

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

[commit-bot: I haz the power, 2013-12-08 22:23:47]

Change committed as 239407