Use schemas bundled in extensions to convert policies loaded on Windows.

Use schemas bundled in extensions to convert policies loaded on Windows.

Extensions that support policies must declare those policies in a JSON
schema. All the policy providers get a mapping from components to their
schemas. This change makes the Windows policy provider use those schemas
when loading policies from GPO or the registry.

The previous behavior was to load the schema from GPO/the registry too;
this behavior is still used if the extension doesn't include a schema,
to support the Legacy Browser Support extension for the next releases.

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

[Joao da Silva, 2013-12-03 17:59:26]

Please have a look, thanks!

[Mattias Nissler (ping if slow), 2013-12-03 20:21:59]

Code looks reasonable to me, but there's one thing I don't understand.

https://codereview.chromium.org/102493002/diff/20001/components/policy/core/c...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/102493002/diff/20001/components/policy/core/c...
components/policy/core/common/schema.h:57: // updated to use a proper schema.
I don't get why LBS needs this - shouldn't arrays that don't declare their item
type just use the natural type we pull from the registry (which is  string for
string-typed registry keys?). What am I missing? Would be could to clarify the
comment to elaborate on why exactly LBS needs this.

[Joao da Silva, 2013-12-04 10:20:06]

https://codereview.chromium.org/102493002/diff/20001/components/policy/core/c...
File components/policy/core/common/schema.h (right):

https://codereview.chromium.org/102493002/diff/20001/components/policy/core/c...
components/policy/core/common/schema.h:57: // updated to use a proper schema.
On 2013/12/03 20:22:00, Mattias Nissler wrote:
> I don't get why LBS needs this - shouldn't arrays that don't declare their
item
> type just use the natural type we pull from the registry (which is  string for
> string-typed registry keys?). What am I missing? Would be could to clarify the
> comment to elaborate on why exactly LBS needs this.

LBS does not include the "items" schema because otherwise it would exceed the
maximum size of registry values in ADM templates. Without this workaround,
Schema::InternalStorage::ParseList would break because it requires an "items"
subschema for "type"="array" schemas.

The main question is whether we should be strict and require "complete" schemas,
or be more lax and let whatever doesn't match the schema just stay as is found
by the loaders.

I opted for the strict approach, so that the GUI builder for CPanel and similar
tools have complete information and the expected types. This also forces authors
to completely define the policies' format. LBS will be updated during the
M34-stable weeks to declare a complete schema too, and then this code will be
removed.

If you agree then I'll clarify this comment and leave the code as is. WDYT?

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

Mattias, PTAL. This handles LBS explicitly now, so that other extensions can't
rely on the same special treatment and don't break by M35.

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:184: return
extension->id() == kLegacyBrowserSupportExtensionId;
This is important, so that other extensions don't get the special treatment
w.r.t. the registry. We don't want them to start relying on it.

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_win.cc:516: ParsePolicy(policy_dict,
kLevels[j].level, scope, schema, &policy);
Just a side comment: at this stage, |schema| is always valid, and this will be
enforced once we remove the LBS support. |schema| may be incomplete for LBS
though (doesn't declare all the policies supported by that extension), so we
keep this a bit more relaxed until LBS is updated with a proper schema.

https://codereview.chromium.org/102493002/diff/40001/components/policy/core/c...
File components/policy/core/common/registry_dict_win.cc (right):

https://codereview.chromium.org/102493002/diff/40001/components/policy/core/c...
components/policy/core/common/registry_dict_win.cc:131: return
scoped_ptr<base::Value>();
Returning a null here was problematic; if a policy is a map and expects a double
in some key but the conversion fails, then this would map its value to null,
which the extension does not expected.

SchemaMap::FilterBundle() would also filter out that entire map, because it
would be non-conforming.

[Joao da Silva, 2013-12-05 18:14:07]

@Benjamin: please review the change to managed_value_store_cache.cc. Thanks!

[not at google - send to devlin, 2013-12-05 18:15:43]

lgtm

[Mattias Nissler (ping if slow), 2013-12-06 10:25:31]

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_win.cc:516: ParsePolicy(policy_dict,
kLevels[j].level, scope, schema, &policy);
On 2013/12/05 18:12:08, Joao da Silva wrote:
> Just a side comment: at this stage, |schema| is always valid, and this will be
> enforced once we remove the LBS support. |schema| may be incomplete for LBS
> though (doesn't declare all the policies supported by that extension), so we
> keep this a bit more relaxed until LBS is updated with a proper schema.

I guess I'd feel better if we enforced adherence to the schema (if that's our
decision) from the start and added another special case for LBS here.

Edit: I found that SchemaMap::FilterBundle enforces adherence to the schema and
drops policies that don't conform - so LBS with additional undeclared policies
would fail there, no? Am I missing something? Have you verified that LBS
actually works?

[Joao da Silva, 2013-12-06 17:25:16]

Thanks for noticing the subtle behavior. The LBSSupport unittest is verifying
that Chrome is doing the right thing, but it wasn't obvious why; see inline.

This is good to go; it'll be much cleaner once we remove the LBS backwards
compat code.

@Mattias: PTAL

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_win.cc:516: ParsePolicy(policy_dict,
kLevels[j].level, scope, schema, &policy);
On 2013/12/06 10:25:31, Mattias Nissler wrote:
> On 2013/12/05 18:12:08, Joao da Silva wrote:
> > Just a side comment: at this stage, |schema| is always valid, and this will
be
> > enforced once we remove the LBS support. |schema| may be incomplete for LBS
> > though (doesn't declare all the policies supported by that extension), so we
> > keep this a bit more relaxed until LBS is updated with a proper schema.
> 
> I guess I'd feel better if we enforced adherence to the schema (if that's our
> decision) from the start and added another special case for LBS here.
> 

That is already the case. If a schema is not on the map then nothing will be
loaded; and schemas only end up on the map if they passed the extension manifest
validation, which only passes for proper enterprise extensions and LBS.

> Edit: I found that SchemaMap::FilterBundle enforces adherence to the schema
and
> drops policies that don't conform - so LBS with additional undeclared policies
> would fail there, no? Am I missing something? Have you verified that LBS
> actually works?

It works as expected. Note that the schema map has an entry for LBS but it's
going to be empty; and FilterBundle() skips validation for empty schemas. Other
extensions won't get this behavior because their schemas must be valid at load
time.

Note that the undeclared policies from LBS still work because the schema map
does not have the patched schema, it still contains the empty schema that makes
FilterBundle skip this extension. The patched schema is only used for the
ConvertToJSON call. This is verified by PolicyLoaderWinTest.LBSSupport.

I've also verified manually that undeclared policies get loaded as expected, and
only if the extension ID is LBS's.

Finally, if LBS gets loaded but bundles a valid schema then none of this
behavior will kick in, so that it can be updated during the M33/M34 stable time.

[Mattias Nissler (ping if slow), 2013-12-09 14:07:12]

LGTM

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/102493002/diff/40001/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_win.cc:516: ParsePolicy(policy_dict,
kLevels[j].level, scope, schema, &policy);
On 2013/12/06 17:25:16, Joao da Silva wrote:
> On 2013/12/06 10:25:31, Mattias Nissler wrote:
> > On 2013/12/05 18:12:08, Joao da Silva wrote:
> > > Just a side comment: at this stage, |schema| is always valid, and this
will
> be
> > > enforced once we remove the LBS support. |schema| may be incomplete for
LBS
> > > though (doesn't declare all the policies supported by that extension), so
we
> > > keep this a bit more relaxed until LBS is updated with a proper schema.
> > 
> > I guess I'd feel better if we enforced adherence to the schema (if that's
our
> > decision) from the start and added another special case for LBS here.
> > 
> 
> That is already the case. If a schema is not on the map then nothing will be
> loaded; and schemas only end up on the map if they passed the extension
manifest
> validation, which only passes for proper enterprise extensions and LBS.
> 
> > Edit: I found that SchemaMap::FilterBundle enforces adherence to the schema
> and
> > drops policies that don't conform - so LBS with additional undeclared
policies
> > would fail there, no? Am I missing something? Have you verified that LBS
> > actually works?
> 
> It works as expected. Note that the schema map has an entry for LBS but it's
> going to be empty; and FilterBundle() skips validation for empty schemas.
Other
> extensions won't get this behavior because their schemas must be valid at load
> time.
> 
> Note that the undeclared policies from LBS still work because the schema map
> does not have the patched schema, it still contains the empty schema that
makes
> FilterBundle skip this extension. The patched schema is only used for the
> ConvertToJSON call. This is verified by PolicyLoaderWinTest.LBSSupport.
> 
> I've also verified manually that undeclared policies get loaded as expected,
and
> only if the extension ID is LBS's.
> 
> Finally, if LBS gets loaded but bundles a valid schema then none of this
> behavior will kick in, so that it can be updated during the M33/M34 stable
time.

OK, thanks for explaining. This stuff is quite subtle, happy to see it go away
soon :)

[commit-bot: I haz the power, 2013-12-09 15:14:28]

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

[commit-bot: I haz the power, 2013-12-09 18:14:28]

Change committed as 239520