Add policy domain for signin screen apps

Add policy domain for signin screen apps

This introduces a new policy domain that is intended for fetching and
providing admin policies for the login screen apps.

This is preparation CL for the following CLs that will add actual
fetching of the policies for the login screen apps.

No behavior change is expected to be introduced by this CL.

BUG=644304
TEST=existing tests

Committed: https://crrev.com/f1884f8297f6c1c4d1f3a67d8271fd1d1c7385ae
Cr-Commit-Position: refs/heads/master@{#428845}

[emaxx, 2016-10-21 02:14:43]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 02:15:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 02:37:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 02:37:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[emaxx, 2016-10-21 02:42:32]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 02:43:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 03:01:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 03:01:07]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[emaxx, 2016-10-21 03:37:53]

emaxx@chromium.org changed reviewers:
+ atwilson@chromium.org

[emaxx, 2016-10-21 03:37:53]

Drew, PTAL.

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc
(left):

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc:40:
schema_registry_->SetReady(POLICY_DOMAIN_EXTENSIONS);
It seems that these two lines may be actually safely removed: the same operation
is also performed in the DeviceLocalAccountPolicyBroker class (in its
constructor), which is the only owner of instances of this class.

[Andrew T Wilson (Slow), 2016-10-21 13:56:30]

LGTM assuming you have reasonable responses to my comments

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:229:
schema_registry_->SetReady(policy::POLICY_DOMAIN_EXTENSIONS);
Why are we doing this instead of SetReady(policy_domain_) ?

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:238:
policy_domain_(policy::POLICY_DOMAIN_EXTENSIONS),
I guess the idea is that eventually the caller will specify this in the
constructor? Because otherwise keeping this in a member variable makes no sense.

https://codereview.chromium.org/2440473004/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/2440473004/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.h:33: class
SchemaRegistry;
Why is this added? We aren't using it?

[emaxx, 2016-10-21 14:32:44]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 14:32:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2016-10-21 14:33:10]

Thanks a lot for the prompt review!

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:229:
schema_registry_->SetReady(policy::POLICY_DOMAIN_EXTENSIONS);
On 2016/10/21 13:56:30, Andrew T Wilson (Slow) wrote:
> Why are we doing this instead of SetReady(policy_domain_) ?

That's an unfortunate side effect of introducing a new policy domain.
The problem was that without marking _both_ extension-related domains as ready,
the whole schema registry would stay non-ready and therefore block the observers
in the policy stack:
https://cs.chromium.org/chromium/src/components/policy/core/common/schema_reg...

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:238:
policy_domain_(policy::POLICY_DOMAIN_EXTENSIONS),
On 2016/10/21 13:56:30, Andrew T Wilson (Slow) wrote:
> I guess the idea is that eventually the caller will specify this in the
> constructor? Because otherwise keeping this in a member variable makes no
sense.

Yes, it was planned either to add a constructor argument or to introduce a new
static method and call it here (I currently chose the second approach).

https://codereview.chromium.org/2440473004/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/2440473004/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.h:33: class
SchemaRegistry;
On 2016/10/21 13:56:30, Andrew T Wilson (Slow) wrote:
> Why is this added? We aren't using it?

Thanks, good catch. This line leaked from the second part of the big change.

I'll remove it from this CL.

[emaxx, 2016-10-21 14:46:24]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 14:46:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2016-10-21 14:59:38]

emaxx@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[emaxx, 2016-10-21 14:59:39]

Hi Devlin, could you PTAL at changes under
chrome/browser/extensions/api/storage/ ?

Note that this CL is the first half of the bigger change, that would turn on
ManagedValueStoreCache for the extensions running under the signin profile.

So this CL is just preparing the ground for this bigger change, by making the
existing code flexible enough for working in both modes.

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:229:
schema_registry_->SetReady(policy::POLICY_DOMAIN_EXTENSIONS);
On 2016/10/21 14:33:10, emaxx wrote:
> On 2016/10/21 13:56:30, Andrew T Wilson (Slow) wrote:
> > Why are we doing this instead of SetReady(policy_domain_) ?
> 
> That's an unfortunate side effect of introducing a new policy domain.
> The problem was that without marking _both_ extension-related domains as
ready,
> the whole schema registry would stay non-ready and therefore block the
observers
> in the policy stack:
>
https://cs.chromium.org/chromium/src/components/policy/core/common/schema_reg...

I added a comment that should clarify this tricky place.

[commit-bot: I haz the power, 2016-10-21 15:41:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 15:41:32]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-10-27 00:50:34]

Sorry for the delay - missed this one come in.  I'm not super familiar with this
code, so I'm mostly relying on atwilson's lg, but this still strikes me as a bit
odd.

https://codereview.chromium.org/2440473004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:230: // Note
that instead of using the |policy_domain_| value here, both of the
I'm confused by this, despite the helpful comment. :)

If there is some other cache corresponding to the other policy domain, why can't
we use that?
If there's not, should there be one?
If there shouldn't, can we default no cache to being ready? (100% of the 0
caches are ready. :))

[emaxx, 2016-10-27 13:34:22]

https://codereview.chromium.org/2440473004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:230: // Note
that instead of using the |policy_domain_| value here, both of the
On 2016/10/27 00:50:34, Devlin wrote:
> I'm confused by this, despite the helpful comment. :)
> 
> If there is some other cache corresponding to the other policy domain, why
can't
> we use that?
> If there's not, should there be one?
> If there shouldn't, can we default no cache to being ready? (100% of the 0
> caches are ready. :))

OK, so probably my comment is not clarifying the things well enough... Or maybe
that's because this CL is just a first step in the bigger change and therefore
lacks some context.


Answering your question - there will be only one cache for each profile. For
most of the cases, the cache instance will have
policy_domain_==POLICY_DOMAIN_EXTENSIONS (and therefore behave exactly as before
my changes); and for the signin profile the cache would have
policy_domain_==POLICY_DOMAIN_SIGNIN_EXTENSIONS.


Now the tricky thing that is happening here in lines 237-238 is that _both_ the
old POLICY_DOMAIN_EXTENSIONS and the new POLICY_DOMAIN_SIGNIN_EXTENSIONS domains
are marked as ready. And this is done regardless of the type of the profile.

The explanation for this, which I tried to fit into the new comment, is the
following: a lot of policy-related code depends on the value of
|SchemaRegistry::IsReady|, which returns true only after
|SchemaRegistry::SetReady| was called at least once for each domain. There's no
way to tell to |SchemaRegistry| or its clients "don't wait for readiness of
POLICY_DOMAIN_SIGNIN_EXTENSIONS for the normal user profile" or "don't wait for
readiness of POLICY_DOMAIN_EXTENSIONS for the signing profile". That's why the
only solution I found was calling |SchemaRegistry::SetReady| with both of the
domains here.

[Devlin, 2016-10-27 14:47:34]

https://codereview.chromium.org/2440473004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:230: // Note
that instead of using the |policy_domain_| value here, both of the
On 2016/10/27 13:34:22, emaxx wrote:
> On 2016/10/27 00:50:34, Devlin wrote:
> > I'm confused by this, despite the helpful comment. :)
> > 
> > If there is some other cache corresponding to the other policy domain, why
> can't
> > we use that?
> > If there's not, should there be one?
> > If there shouldn't, can we default no cache to being ready? (100% of the 0
> > caches are ready. :))
> 
> OK, so probably my comment is not clarifying the things well enough... Or
maybe
> that's because this CL is just a first step in the bigger change and therefore
> lacks some context.
> 
> 
> Answering your question - there will be only one cache for each profile. For
> most of the cases, the cache instance will have
> policy_domain_==POLICY_DOMAIN_EXTENSIONS (and therefore behave exactly as
before
> my changes); and for the signin profile the cache would have
> policy_domain_==POLICY_DOMAIN_SIGNIN_EXTENSIONS.
> 
> 
> Now the tricky thing that is happening here in lines 237-238 is that _both_
the
> old POLICY_DOMAIN_EXTENSIONS and the new POLICY_DOMAIN_SIGNIN_EXTENSIONS
domains
> are marked as ready. And this is done regardless of the type of the profile.
> 
> The explanation for this, which I tried to fit into the new comment, is the
> following: a lot of policy-related code depends on the value of
> |SchemaRegistry::IsReady|, which returns true only after
> |SchemaRegistry::SetReady| was called at least once for each domain. There's
no
> way to tell to |SchemaRegistry| or its clients "don't wait for readiness of
> POLICY_DOMAIN_SIGNIN_EXTENSIONS for the normal user profile" or "don't wait
for
> readiness of POLICY_DOMAIN_EXTENSIONS for the signing profile". That's why the
> only solution I found was calling |SchemaRegistry::SetReady| with both of the
> domains here.

Thanks for the explanation.  If this is the only place that will be calling this
with these values, and they must always be called together, could we make a
SetExtensionCachesReady() method or similar on the schema registry that toggles
both these?  That way the logic for the fact that there will only be one and
they should be toggled together is locked in related code, which seems a little
better to me.  WDYT?

[emaxx, 2016-10-27 15:27:45]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 15:28:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 16:12:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 16:12:55]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-10-27 16:15:48]

Made some more refactoring according to feedback from Devlin.

Drew, please take a look too.

https://codereview.chromium.org/2440473004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:230: // Note
that instead of using the |policy_domain_| value here, both of the
On 2016/10/27 14:47:33, Devlin wrote:
> On 2016/10/27 13:34:22, emaxx wrote:
> > On 2016/10/27 00:50:34, Devlin wrote:
> > > I'm confused by this, despite the helpful comment. :)
> > > 
> > > If there is some other cache corresponding to the other policy domain, why
> > can't
> > > we use that?
> > > If there's not, should there be one?
> > > If there shouldn't, can we default no cache to being ready? (100% of the 0
> > > caches are ready. :))
> > 
> > OK, so probably my comment is not clarifying the things well enough... Or
> maybe
> > that's because this CL is just a first step in the bigger change and
therefore
> > lacks some context.
> > 
> > 
> > Answering your question - there will be only one cache for each profile. For
> > most of the cases, the cache instance will have
> > policy_domain_==POLICY_DOMAIN_EXTENSIONS (and therefore behave exactly as
> before
> > my changes); and for the signin profile the cache would have
> > policy_domain_==POLICY_DOMAIN_SIGNIN_EXTENSIONS.
> > 
> > 
> > Now the tricky thing that is happening here in lines 237-238 is that _both_
> the
> > old POLICY_DOMAIN_EXTENSIONS and the new POLICY_DOMAIN_SIGNIN_EXTENSIONS
> domains
> > are marked as ready. And this is done regardless of the type of the profile.
> > 
> > The explanation for this, which I tried to fit into the new comment, is the
> > following: a lot of policy-related code depends on the value of
> > |SchemaRegistry::IsReady|, which returns true only after
> > |SchemaRegistry::SetReady| was called at least once for each domain. There's
> no
> > way to tell to |SchemaRegistry| or its clients "don't wait for readiness of
> > POLICY_DOMAIN_SIGNIN_EXTENSIONS for the normal user profile" or "don't wait
> for
> > readiness of POLICY_DOMAIN_EXTENSIONS for the signing profile". That's why
the
> > only solution I found was calling |SchemaRegistry::SetReady| with both of
the
> > domains here.
> 
> Thanks for the explanation.  If this is the only place that will be calling
this
> with these values, and they must always be called together, could we make a
> SetExtensionCachesReady() method or similar on the schema registry that
toggles
> both these?  That way the logic for the fact that there will only be one and
> they should be toggled together is locked in related code, which seems a
little
> better to me.  WDYT?

Sounds reasonable. I've updated the CL accordingly.

[Devlin, 2016-10-28 21:34:01]

nice; this looks better to me from an extensions perspective.  I'll wait for
Drew's lg just in case something needs to change again, but this seems good.

https://codereview.chromium.org/2440473004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:230: // Note
that instead of using the |policy_domain_| value here, all
Could we instead just say "There is only ever one policy extensions cache
(either for the sign in profile or normal profile), so mark all extensions
domains as ready here" or something similar?

[emaxx, 2016-10-31 15:43:34]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 15:43:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 16:32:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 16:32:34]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-10-31 17:22:43]

https://codereview.chromium.org/2440473004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/2440473004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:230: // Note
that instead of using the |policy_domain_| value here, all
On 2016/10/28 21:34:01, Devlin wrote:
> Could we instead just say "There is only ever one policy extensions cache
> (either for the sign in profile or normal profile), so mark all extensions
> domains as ready here" or something similar?

Thanks for the suggestion. I've updated the comment.

[Andrew T Wilson (Slow), 2016-10-31 20:54:21]

LGTM with one request for an expanded comment.

https://codereview.chromium.org/2440473004/diff/100001/components/policy/core...
File components/policy/core/common/cloud/component_cloud_policy_service.h
(right):

https://codereview.chromium.org/2440473004/diff/100001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service.h:60: //
|policy_type| specifies the policy type that should be fetched.
Can you give an example here (document what are the valid types?)

[emaxx, 2016-10-31 21:10:12]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 21:10:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-10-31 21:16:35]

extensions lgtm

[commit-bot: I haz the power, 2016-10-31 22:12:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 22:12:53]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-10-31 22:27:23]

https://codereview.chromium.org/2440473004/diff/100001/components/policy/core...
File components/policy/core/common/cloud/component_cloud_policy_service.h
(right):

https://codereview.chromium.org/2440473004/diff/100001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service.h:60: //
|policy_type| specifies the policy type that should be fetched.
On 2016/10/31 20:54:21, Andrew T Wilson (Slow) wrote:
> Can you give an example here (document what are the valid types?)

Done. (Added a comment and a DCHECK.)

[emaxx, 2016-10-31 22:27:37]

The CQ bit was checked by emaxx@chromium.org

[emaxx, 2016-10-31 22:27:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2440473004/#ps120001
(title: "More comments according to feedback")

[commit-bot: I haz the power, 2016-10-31 22:28:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 22:34:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-31 22:37:52]

Description was changed from

==========
Add policy domain for signin screen apps

This introduces a new policy domain that is intended for fetching and
providing admin policies for the login screen apps.

This is preparation CL for the following CLs that will add actual
fetching of the policies for the login screen apps.

No behavior change is expected to be introduced by this CL.

BUG=644304
TEST=existing tests
==========

to

==========
Add policy domain for signin screen apps

This introduces a new policy domain that is intended for fetching and
providing admin policies for the login screen apps.

This is preparation CL for the following CLs that will add actual
fetching of the policies for the login screen apps.

No behavior change is expected to be introduced by this CL.

BUG=644304
TEST=existing tests

Committed: https://crrev.com/f1884f8297f6c1c4d1f3a67d8271fd1d1c7385ae
Cr-Commit-Position: refs/heads/master@{#428845}
==========

[commit-bot: I haz the power, 2016-10-31 22:37:53]

Patchset 7 (id:??) landed as
https://crrev.com/f1884f8297f6c1c4d1f3a67d8271fd1d1c7385ae
Cr-Commit-Position: refs/heads/master@{#428845}