Add initial device policy infrastructure.

Add initial device policy infrastructure.

This refactors the cloud policy-related code to support device policy
that gets associated with the whole browser session. Device policy
information will show up in g_browser_process->local_state(). Also,
start supporting recommended policy from the cloud.

BUG=chromium-os:11259, chromium-os:11257, chromium-os:11256
TEST=Enable device polcy by passing --device-policy-cache-dir, claim a device and verify that policy gets downloaded.

[Mattias Nissler (ping if slow), 2011-02-03 15:18:04]

Note: This is pending updates to the various unit tests, so the test binaries
don't compile at the moment.

I'd like you to take a look at this nevertheless so we can discuss the design
changes early and you are in a better position of completing the work while I'm
on vacation :-p

[danno, 2011-02-04 16:01:33]

here's some feedback to keep you busy :)

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_context.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_context.h:24: class BrowserPolicyContext {
How about s/Context/Custodian/ :-)

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_context.h:29: BrowserPolicyContext(
protected?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.cc:191: return
managed_policy_provider_.get();
Make this class nonthreadsafe?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.h:29: class CloudPolicyCache {
CloudPolicyOracle? :-)

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_unittest.cc:19: #if 0
uuuuuhhhhh, might need some rework here?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.cc:23: // non-managed domain names.
Returns false if |username| is empty or its
s/its/it's/

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_client.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:32: class CloudPolicyClient
CloudPolicyBroker? CloudPolicyController?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_client_unittest.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client_unittest.cc:24: #if 0
Whoa. needs work here, too :-)

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_context.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_context.h:29: class CloudPolicyContext :
public NotificationObserver {
CloudPolicyCustodian

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_controller.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:16: // Manages a device
management token, i.e. an identifier that represents a
It does more than that... better comment?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:18: class CloudPolicyController
{
CloudPolicyIdentityStrategy?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:26: virtual void
OnTokenChanged() = 0;
s/OnTokenChanged/OnDeviceTokenChanged/

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:57: // Notifies the provider
that a new token has been fetched. It is up to the
I don't think you mean provider, you mean controller, right?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:58: // provider to decide that
token is going to be used, in which case it should
provider -> controller

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:59: // send a OnTokenChanged()
notification an return the new token in
s/an/and

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:59: // send a OnTokenChanged()
notification an return the new token in
Couldn't you also have OnTokenChanged return a boolean that determines whether
NotifyTokenChanged is called?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:61: virtual void
OnTokenAvailable(const std::string& token) = 0;
s/OnTokenAvailable/OnDeviceTokenAvailable/ to disambiguate from auth token.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:65: void NotifyTokenChanged() {
s/NotifyTokenChanged/NotifyDeviceTokenChanged/

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:734:
context->GetManagedPlatformProvider());
indentation

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:756:
context->GetRecommendedPlatformProvider());
indentation

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:58: // Construct a new backend,
which will discard any previous requests.
You should validate that the state machine is in the right place. Maybe an
external and internal fetch token. Passing device_id_ to yourself in the retry
case is a bit confusing since it immediately gets set again.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:115: void
DeviceTokenFetcher::SetState(FetcherState state) {
Please include state diagram in header documentation the states transitions.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:153: void
DeviceTokenFetcher::DoDelayedWork() {
Please include state diagram in header documentation the states transitions.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.h:92: void DoDelayedWork();
What kind of work. Why? What's the motivation?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher_unittest.cc:19: #if 0
tests here please :-)

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_controller.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_controller.cc:41: const
base::WeakPtr<UserPolicyController> provider_;
controller

[Jakob Kummerow, 2011-02-07 16:00:09]

One round of feedback from me...

We'll need to discuss the naming of the various new classes.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:104: created_sidebar_manager_(false),
+ created_browser_policy_context_(false),

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_client.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:33: :  public
DeviceManagementBackend::DevicePolicyResponseDelegate,
nit: one space after ':'

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:92: // the case in
InitializeAfterIOThreadExists.
nit: update comment: I don't see InitializeAfterIOThreadExists() anywhere.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_context.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_context.h:27: // policy. It glues together
the backend, the policy client and manages the live
nit: s/live/life/

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_context.h:48: 3 * 60 * 60 * 1000; // 3 hours.
nit: two spaces before comment

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_policy_controller.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_controller.cc:53: // Only fetch policy when
the owner is logged in.
s/policy/token/ ? Or am I missing something here?

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:135: delayed_work_at =
cache_->last_policy_refresh_time() +
I think this should be s/cache_->last_policy_refresh_time()/base::Time::Now()/

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.h:92: void DoDelayedWork();
On 2011/02/04 16:01:33, danno wrote:
> What kind of work. Why? What's the motivation?

Seeing what this method does, I suggest we name it "RetryFetching()",
"ExecuteRetryTask()" or something to that effect.

[Jakob Kummerow, 2011-02-14 13:50:34]

Next patch set is at http://codereview.chromium.org/6520008. It's rebased onto
my CloudPolicyCache-changing CL. Please have a look.

Other changes you'll find there:

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:104: created_sidebar_manager_(false),
On 2011/02/07 16:00:09, jkummerow wrote:
> + created_browser_policy_context_(false),

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_context.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_context.h:24: class BrowserPolicyContext {
On 2011/02/04 16:01:33, danno wrote:
> How about s/Context/Custodian/ :-)

Done -- as discussed offline, s/Context/Connector/.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_context.h:29: BrowserPolicyContext(
On 2011/02/04 16:01:33, danno wrote:
> protected?

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.cc:191: return
managed_policy_provider_.get();
On 2011/02/04 16:01:33, danno wrote:
> Make this class nonthreadsafe?

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.h:29: class CloudPolicyCache {
On 2011/02/04 16:01:33, danno wrote:
> CloudPolicyOracle? :-)

Nope.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.cc:23: // non-managed domain names.
Returns false if |username| is empty or its
On 2011/02/04 16:01:33, danno wrote:
> s/its/it's/

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_client.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:32: class CloudPolicyClient
On 2011/02/04 16:01:33, danno wrote:
> CloudPolicyBroker? CloudPolicyController?

Done. CloudPolicyController.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:33: :  public
DeviceManagementBackend::DevicePolicyResponseDelegate,
On 2011/02/07 16:00:09, jkummerow wrote:
> nit: one space after ':'

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:92: // the case in
InitializeAfterIOThreadExists.
On 2011/02/07 16:00:09, jkummerow wrote:
> nit: update comment: I don't see InitializeAfterIOThreadExists() anywhere.

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_context.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_context.h:27: // policy. It glues together
the backend, the policy client and manages the live
On 2011/02/07 16:00:09, jkummerow wrote:
> nit: s/live/life/

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_context.h:29: class CloudPolicyContext :
public NotificationObserver {
On 2011/02/04 16:01:33, danno wrote:
> CloudPolicyCustodian

Done -- CloudPolicySubsystem.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_context.h:48: 3 * 60 * 60 * 1000; // 3 hours.
On 2011/02/07 16:00:09, jkummerow wrote:
> nit: two spaces before comment

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_controller.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:16: // Manages a device
management token, i.e. an identifier that represents a
On 2011/02/04 16:01:33, danno wrote:
> It does more than that... better comment?

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:18: class CloudPolicyController
{
On 2011/02/04 16:01:33, danno wrote:
> CloudPolicyIdentityStrategy?

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:26: virtual void
OnTokenChanged() = 0;
On 2011/02/04 16:01:33, danno wrote:
> s/OnTokenChanged/OnDeviceTokenChanged/

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:57: // Notifies the provider
that a new token has been fetched. It is up to the
On 2011/02/04 16:01:33, danno wrote:
> I don't think you mean provider, you mean controller, right?

Done. (s/provider/identity strategy/, to be precise.)

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:58: // provider to decide that
token is going to be used, in which case it should
On 2011/02/04 16:01:33, danno wrote:
> provider -> controller

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:59: // send a OnTokenChanged()
notification an return the new token in
On 2011/02/04 16:01:33, danno wrote:
> s/an/and

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:59: // send a OnTokenChanged()
notification an return the new token in
On 2011/02/04 16:01:33, danno wrote:
> Couldn't you also have OnTokenChanged return a boolean that determines whether
> NotifyTokenChanged is called?

I don't currently see a use case for that...

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:61: virtual void
OnTokenAvailable(const std::string& token) = 0;
On 2011/02/04 16:01:33, danno wrote:
> s/OnTokenAvailable/OnDeviceTokenAvailable/ to disambiguate from auth token.

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller.h:65: void NotifyTokenChanged() {
On 2011/02/04 16:01:33, danno wrote:
> s/NotifyTokenChanged/NotifyDeviceTokenChanged/

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:734:
context->GetManagedPlatformProvider());
On 2011/02/04 16:01:33, danno wrote:
> indentation

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:756:
context->GetRecommendedPlatformProvider());
On 2011/02/04 16:01:33, danno wrote:
> indentation

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_policy_controller.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_controller.cc:53: // Only fetch policy when
the owner is logged in.
On 2011/02/07 16:00:09, jkummerow wrote:
> s/policy/token/ ? Or am I missing something here?

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:58: // Construct a new backend,
which will discard any previous requests.
On 2011/02/04 16:01:33, danno wrote:
> You should validate that the state machine is in the right place. Maybe an
> external and internal fetch token. Passing device_id_ to yourself in the retry
> case is a bit confusing since it immediately gets set again.

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:115: void
DeviceTokenFetcher::SetState(FetcherState state) {
On 2011/02/04 16:01:33, danno wrote:
> Please include state diagram in header documentation the states transitions.

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:135: delayed_work_at =
cache_->last_policy_refresh_time() +
On 2011/02/07 16:00:09, jkummerow wrote:
> I think this should be s/cache_->last_policy_refresh_time()/base::Time::Now()/

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:153: void
DeviceTokenFetcher::DoDelayedWork() {
On 2011/02/04 16:01:33, danno wrote:
> Please include state diagram in header documentation the states transitions.

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.h:92: void DoDelayedWork();
On 2011/02/07 16:00:09, jkummerow wrote:
> On 2011/02/04 16:01:33, danno wrote:
> > What kind of work. Why? What's the motivation?
> 
> Seeing what this method does, I suggest we name it "RetryFetching()",
> "ExecuteRetryTask()" or something to that effect.

Done.

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_controller.cc (right):

http://codereview.chromium.org/6312121/diff/4001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_controller.cc:41: const
base::WeakPtr<UserPolicyController> provider_;
On 2011/02/04 16:01:33, danno wrote:
> controller

Done.