Enable fetching of admin policies for login screen apps

Enable fetching of admin policies for login screen apps

This introduces fetching of admin policies for login screen apps. The
policy is fetched using a special policy domain, and is fetched as a
part of the device policy.

BUG=644304
TEST=new unit and browser tests

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

[emaxx, 2016-10-21 03:35:57]

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

[commit-bot: I haz the power, 2016-10-21 03:36:19]

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 03:40:38]

Description was changed from

==========
Enable fetching of admin policies for login screen apps

This introduces fetching of admin policies for login screen apps. The
policy is fetched using a special policy domain, and is fetched as a
part of the device policy.

BUG=644304
==========

to

==========
Enable fetching of admin policies for login screen apps

This introduces fetching of admin policies for login screen apps. The
policy is fetched using a special policy domain, and is fetched as a
part of the device policy.

BUG=644304
TEST=new unit and browser tests
==========

[emaxx, 2016-10-21 03:45:04]

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

[emaxx, 2016-10-21 03:45:04]

Hi Bartosz,

Could you PTAL? I've split the original CL, so this part is now 2x smaller then
what I asked you to review in the beginning.

[commit-bot: I haz the power, 2016-10-21 04:36:29]

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

[commit-bot: I haz the power, 2016-10-21 04:36:30]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-10-24 15:48:32]

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

[emaxx, 2016-10-24 15:48:33]

Drew, PTAL.

[Andrew T Wilson (Slow), 2016-10-28 14:49:56]

LGTM with a few comments

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:108:
command_line->AppendSwitchASCII(::switches::kDisableExtensionsExcept,
Why is this required? Maybe add a comment here?

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:132: //
crbug.com/576464).
This bug doesn't talk about a command line switch.

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:243:
manager_->RefreshPolicies();
This is fine, but curious why StartConnection() doesn't refresh policies if this
is a known side effect?

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:609:
CreateJob(DeviceManagementRequestJob::TYPE_POLICY_FETCH, _))
I'm somewhat confused - doesn't TestingDeviceCloudPolicyManager turn off
component policy fetching?

https://codereview.chromium.org/2441653003/diff/1/components/policy/core/comm...
File components/policy/core/common/schema_registry.cc (right):

https://codereview.chromium.org/2441653003/diff/1/components/policy/core/comm...
components/policy/core/common/schema_registry.cc:240: void
ForwardingSchemaRegistry::UpdateReadiness() {
How can UpdateReadiness() be called when wrapped_ is NULL? You remove the
observer before setting it to null.

Also, not totally clear when you *wouldn't* want forward_readiness_ = true. Can
you clarify this in the class documentation (i.e. exactly when it becomes ready
when forward_readiness_ = false)?

[emaxx, 2016-10-28 21:06:05]

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

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

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-28 21:09:19]

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

[commit-bot: I haz the power, 2016-10-28 21:09:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[emaxx, 2016-10-28 22:17:53]

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

[commit-bot: I haz the power, 2016-10-28 22:18:26]

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-28 22:21:21]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[emaxx, 2016-10-28 23:05:03]

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

[commit-bot: I haz the power, 2016-10-28 23:05:24]

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-28 23:08:39]

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

[commit-bot: I haz the power, 2016-10-28 23:08:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emaxx, 2016-10-29 12:02:24]

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

[commit-bot: I haz the power, 2016-10-29 12:02:30]

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-29 12:05:18]

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

[commit-bot: I haz the power, 2016-10-29 12:05:19]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[emaxx, 2016-10-31 14:59:13]

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

[commit-bot: I haz the power, 2016-10-31 14:59:32]

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 15:50:25]

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

[commit-bot: I haz the power, 2016-10-31 15:50:27]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-10-31 15:51:20]

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:108:
command_line->AppendSwitchASCII(::switches::kDisableExtensionsExcept,
On 2016/10/28 14:49:56, Andrew T Wilson (Slow) wrote:
> Why is this required? Maybe add a comment here?

I'll add a comment that without this switch the test will fail to install the
extension due to some checks in |ExtensionService|.


Some details:
These checks currently reside in |ExtensionService::AddExtension|:
|ExtensionService::extensions_enabled| is false for the signin profile, and the
test extension is neither component nor has external location.
These checks work, if I'm correct, as a second level of defense that prevents
extensions from being installed in unwanted environments - like in the signin
profile.

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:132: //
crbug.com/576464).
On 2016/10/28 14:49:56, Andrew T Wilson (Slow) wrote:
> This bug doesn't talk about a command line switch.

Good catch, that's true. I'll remove this TODO.

The command line flag, which I was talking about, was actually planned as a
temporary solution until all security mitigations are in place. Therefore this
test shouldn't really depend on it (and even though some alternative way of
enabling extensions may appear after crbug.com/576464 is done, it probably
doesn't make sense to change this test).

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc:252: void
StartPolicyTestService() {
nit (for myself): s/Service/Server/.

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:243:
manager_->RefreshPolicies();
On 2016/10/28 14:49:56, Andrew T Wilson (Slow) wrote:
> This is fine, but curious why StartConnection() doesn't refresh policies if
this
> is a known side effect?

Usually the first policy fetch happens as the result of the
DeviceCloudPolicyManagerChromeOS::StartConnection call: it calls
CloudPolicyCore::StartRefreshScheduler which creates a
CloudPolicyRefreshScheduler that schedules an immediate refresh because there's
no previous policy fetch time.

During enrollment, the first policy fetch happens earlier, before calling
DeviceCloudPolicyManagerChromeOS::StartConnection, that's why the new
CloudPolicyRefreshScheduler instance doesn't schedule an immediate policy
refresh.


Though I agree this my solution looks more like a workaround.
Maybe you have an idea for a better approach?

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:609:
CreateJob(DeviceManagementRequestJob::TYPE_POLICY_FETCH, _))
On 2016/10/28 14:49:56, Andrew T Wilson (Slow) wrote:
> I'm somewhat confused - doesn't TestingDeviceCloudPolicyManager turn off
> component policy fetching?

Right, the comment was misleading. The second policy refresh actually comes from
the new code in the enrollment completion handler.

https://codereview.chromium.org/2441653003/diff/1/components/policy/core/comm...
File components/policy/core/common/schema_registry.cc (right):

https://codereview.chromium.org/2441653003/diff/1/components/policy/core/comm...
components/policy/core/common/schema_registry.cc:240: void
ForwardingSchemaRegistry::UpdateReadiness() {
On 2016/10/28 14:49:56, Andrew T Wilson (Slow) wrote:
> How can UpdateReadiness() be called when wrapped_ is NULL? You remove the
> observer before setting it to null.

You are right, check for NULL wrapped_ is unnecessary. I've remove it.


> Also, not totally clear when you *wouldn't* want forward_readiness_ = true.
Can
> you clarify this in the class documentation (i.e. exactly when it becomes
ready
> when forward_readiness_ = false)?

I introduced this flag because I was afraid to break the existing code by this
change.

The original behavior of this class was to have separate readiness state for the
wrapped registry and for the forwarding registry.
This may make sense in some scenarios: imagine a piece of code that accepts a
registry and is then controlling its readiness in one part and observing it in
another part - then it's really important to not have any unexpected switches of
the readiness.

So what I found in the existing code was the following structure:

1. The DeviceLocalAccountPolicyBroker instance owns a SchemaRegistry instance.
2. The DeviceLocalAccountExtensionTracker class contains a code that calls
SetReady method on this SchemaRegistry.
3. The ComponentCloudPolicyService instance observes the readiness state of that
same SchemaRegistry.
4. ProfileImpl constructor calls SchemaRegistryServiceFactory::CreateForContext,
which internally creates a ForwardingSchemaRegistry instance that wraps that
same SchemaRegistry.
5. Now all the profile-tied stuff that works with SchemaRegistry* is in case of
device-local account really working with a ForwardingSchemaRegistry instance.
For instance, a ManagedValueStoreCache::ExtensionTracker instance is constructed
that calls SetReady on the registry. And there is a number of observers of the
registry state.

So I really don't know how to make sure that none of the observers mentioned in
#5 could actually break if the forwarding schema registry may become ready
earlier...

[Andrew T Wilson (Slow), 2016-10-31 20:30:04]

On 2016/10/31 15:51:20, emaxx wrote:
> So I really don't know how to make sure that none of the observers mentioned
in
> #5 could actually break if the forwarding schema registry may become ready
> earlier...

It's your call, and it's LGTM either way. But if I were writing this, I'd remove
the forward_readiness_ flag, especially since we can't really explain how a new
user of the class should use it.

If the tests break, then you can see when it's useful and document it.
If the tests don't break, then land your change and we'll find out if we break
anything - at the very worst, we'll find out that our test coverage isn't very
good and we can add a new test later if a bug shows up.

I'd just err more towards taking a chance of breaking functionality on the Dev
channel for a while over making the code harder to maintain with a "do what this
class did in October 2016" flag.

[Andrew T Wilson (Slow), 2016-10-31 20:44:11]

still LGTM

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2441653003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:243:
manager_->RefreshPolicies();
On 2016/10/31 15:51:20, emaxx wrote:
> On 2016/10/28 14:49:56, Andrew T Wilson (Slow) wrote:
> > This is fine, but curious why StartConnection() doesn't refresh policies if
> this
> > is a known side effect?
> 
> Usually the first policy fetch happens as the result of the
> DeviceCloudPolicyManagerChromeOS::StartConnection call: it calls
> CloudPolicyCore::StartRefreshScheduler which creates a
> CloudPolicyRefreshScheduler that schedules an immediate refresh because
there's
> no previous policy fetch time.
> 
> During enrollment, the first policy fetch happens earlier, before calling
> DeviceCloudPolicyManagerChromeOS::StartConnection, that's why the new
> CloudPolicyRefreshScheduler instance doesn't schedule an immediate policy
> refresh.
> 
> 
> Though I agree this my solution looks more like a workaround.
> Maybe you have an idea for a better approach?

I don't have a great idea here - I've never really had to dive into the
enrollment code so I don't have a deep understanding about how the
initialization order works.

[emaxx, 2016-11-03 15:13:47]

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

[commit-bot: I haz the power, 2016-11-03 15:14:06]

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

[emaxx, 2016-11-03 15:51:28]

Updated the CL:
1. Make the policy refresh after enrollment in a bit more natural way.
2. Removed parametrization from ForwardingSchemaRegistry.

Some details below.


On 2016/10/31 20:30:04, Andrew T Wilson (Slow) wrote:
> On 2016/10/31 15:51:20, emaxx wrote:
> > So I really don't know how to make sure that none of the observers mentioned
> in
> > #5 could actually break if the forwarding schema registry may become ready
> > earlier...
> 
> It's your call, and it's LGTM either way. But if I were writing this, I'd
remove
> the forward_readiness_ flag, especially since we can't really explain how a
new
> user of the class should use it.
> 
> If the tests break, then you can see when it's useful and document it.
> If the tests don't break, then land your change and we'll find out if we break
> anything - at the very worst, we'll find out that our test coverage isn't very
> good and we can add a new test later if a bug shows up.
> 
> I'd just err more towards taking a chance of breaking functionality on the Dev
> channel for a while over making the code harder to maintain with a "do what
this
> class did in October 2016" flag.

This makes sense. After more studying of the code and playing with dev
builds, I think there should be no problem in making the new behavior
(i.e. forwarding the readiness state in ForwardingSchemaRegistry)
shouldn't break anything.

The only piece of code that I found that could depend on this is
SchemaRegistryTrackingPolicyProvider. And if my understanding is
correct, it's not a problem for it to set the readiness flag earlier, as
all the initial extensions should have already been registered in the
wrapped registry by the device local account broker.



On 2016/10/31 20:44:11, Andrew T Wilson (Slow) wrote:
> On 2016/10/31 15:51:20, emaxx wrote:
> > During enrollment, the first policy fetch happens earlier [...]
> 
> I don't have a great idea here - I've never really had to dive into the
> enrollment code so I don't have a deep understanding about how the
> initialization order works.

I think I've found a bit cleaner approach to request this policy refresh
- see the Patch Set 5. Though the bug fix from
http://crrev.com/2472593003 is required for it to work.

[commit-bot: I haz the power, 2016-11-03 16:12:16]

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

[commit-bot: I haz the power, 2016-11-03 16:12:17]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-11-03 18:22:09]

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

[emaxx, 2016-11-03 18:22:10]

Adding more reviewers for OWNERS review. PTAL.

Devlin: chrome/browser/extensions/api/storage/**

Steven: chromeos/chromeos_paths.*

[stevenjb, 2016-11-03 18:26:55]

RS lgtm

[emaxx, 2016-11-04 15:40:13]

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

[commit-bot: I haz the power, 2016-11-04 15:40:45]

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-11-04 17:10:05]

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

[commit-bot: I haz the power, 2016-11-04 17:10:06]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-05 02:03:44]

extensions lgtm

[emaxx, 2016-11-09 21:42:01]

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

[commit-bot: I haz the power, 2016-11-09 21:42:46]

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-11-09 21:45:56]

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

[commit-bot: I haz the power, 2016-11-09 21:45:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-10 02:08:43]

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

[commit-bot: I haz the power, 2016-11-10 02:09: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-11-10 02:24:20]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-10 02:39:53]

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

[commit-bot: I haz the power, 2016-11-10 02:40:15]

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-11-10 03:24:12]

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

[commit-bot: I haz the power, 2016-11-10 03:24:13]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-11-10 03:34:46]

The CQ bit was checked by emaxx@chromium.org

[emaxx, 2016-11-10 03:34:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, stevenjb@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2441653003/#ps130001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-10 03:35:06]

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-11-10 03:40:00]

Description was changed from

==========
Enable fetching of admin policies for login screen apps

This introduces fetching of admin policies for login screen apps. The
policy is fetched using a special policy domain, and is fetched as a
part of the device policy.

BUG=644304
TEST=new unit and browser tests
==========

to

==========
Enable fetching of admin policies for login screen apps

This introduces fetching of admin policies for login screen apps. The
policy is fetched using a special policy domain, and is fetched as a
part of the device policy.

BUG=644304
TEST=new unit and browser tests
==========

[commit-bot: I haz the power, 2016-11-10 03:40:02]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-11-10 03:42:20]

Description was changed from

==========
Enable fetching of admin policies for login screen apps

This introduces fetching of admin policies for login screen apps. The
policy is fetched using a special policy domain, and is fetched as a
part of the device policy.

BUG=644304
TEST=new unit and browser tests
==========

to

==========
Enable fetching of admin policies for login screen apps

This introduces fetching of admin policies for login screen apps. The
policy is fetched using a special policy domain, and is fetched as a
part of the device policy.

BUG=644304
TEST=new unit and browser tests

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

[commit-bot: I haz the power, 2016-11-10 03:42:21]

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