Add CloudPolicyService

Add CloudPolicyService

CloudPolicyService glues together a cloud policy client and a CloudPolicyStore, taking care of pushing policy to the store when new policy from the cloud arrived. It also handles coordination for policy refresh operations.

BUG=chromium:108928
TEST=Unit tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139014

[Mattias Nissler (ping if slow), 2012-04-16 14:31:18]

I feel this is now far enough for sharing. Feedback is appreciated. Let me know
if you want to discuss something, as the modern-art boxes and lines drawing on
my desk may be helpful :)

[Joao da Silva, 2012-04-17 00:30:58]

Looks good! I'd like to see how you plan to add the missing pieces.

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_service.cc (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service.cc:56: store_->Store(*policy);
I'd move this line after the refresh_state_ update, since Store() may
synchronously call OnStoreError/OnPolicyLoaded.

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_service.h (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service.h:25: class CloudPolicyService :
public CloudPolicyClient::Observer,
This doesn't currently handle periodic policy refreshes. Is that for another
class, or is this one a WIP?

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_service_unittest.cc (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service_unittest.cc:35: using
CloudPolicyStore::status_;
Now that I see this, this is a nice way to create mocks out of existing classes.
A lot of "friends" can be removed! :-)

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service_unittest.cc:45: : client_(new
MockCloudPolicyClient),
indent

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_store.h (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_store.h:34: virtual void
OnPolicyLoaded(CloudPolicyStore* store) = 0;
The comments refer to this as OnPolicyUpdated(). Since this can be invoked from
both Load() and Store(), I guess "Updated" makes more sense. Your call, just
make it consistent :-)

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/mock_clo...
File chrome/browser/policy/mock_cloud_policy_client.cc (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/mock_clo...
chrome/browser/policy/mock_cloud_policy_client.cc:13: : CloudPolicyClient("",
"", USER_AFFILIATION_NONE,
indent

http://codereview.chromium.org/10092010/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10092010/diff/1/chrome/chrome_browser.gypi#new...
chrome/chrome_browser.gypi:1546: 'browser/policy/cloud_policy_store.h',
These come before the subsystem

[Mattias Nissler (ping if slow), 2012-05-22 14:14:26]

Ready for a close look.

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_service.cc (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service.cc:56: store_->Store(*policy);
On 2012/04/17 00:30:58, Joao da Silva wrote:
> I'd move this line after the refresh_state_ update, since Store() may
> synchronously call OnStoreError/OnPolicyLoaded.

Good catch. Done.

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_service.h (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service.h:25: class CloudPolicyService :
public CloudPolicyClient::Observer,
On 2012/04/17 00:30:58, Joao da Silva wrote:
> This doesn't currently handle periodic policy refreshes. Is that for another
> class, or is this one a WIP?

I decided to put that into a separate class so the refresh behavior can be
enabled separately (useful at enrollment).

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_service_unittest.cc (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_service_unittest.cc:45: : client_(new
MockCloudPolicyClient),
On 2012/04/17 00:30:58, Joao da Silva wrote:
> indent

Done.

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
File chrome/browser/policy/cloud_policy_store.h (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/cloud_po...
chrome/browser/policy/cloud_policy_store.h:34: virtual void
OnPolicyLoaded(CloudPolicyStore* store) = 0;
On 2012/04/17 00:30:58, Joao da Silva wrote:
> The comments refer to this as OnPolicyUpdated(). Since this can be invoked
from
> both Load() and Store(), I guess "Updated" makes more sense. Your call, just
> make it consistent :-)

Done.

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/mock_clo...
File chrome/browser/policy/mock_cloud_policy_client.cc (right):

http://codereview.chromium.org/10092010/diff/1/chrome/browser/policy/mock_clo...
chrome/browser/policy/mock_cloud_policy_client.cc:13: : CloudPolicyClient("",
"", USER_AFFILIATION_NONE,
On 2012/04/17 00:30:58, Joao da Silva wrote:
> indent

Done.

http://codereview.chromium.org/10092010/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10092010/diff/1/chrome/chrome_browser.gypi#new...
chrome/chrome_browser.gypi:1546: 'browser/policy/cloud_policy_store.h',
On 2012/04/17 00:30:58, Joao da Silva wrote:
> These come before the subsystem

Done.

[Joao da Silva, 2012-05-22 22:01:44]

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_constants.h (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_constants.h:96: CLOUD_POLICY_STATUS_SUCCESS,
Is this needed? Haven't found a use for it, except a
CloudPolicyService::GetStatus() declaration that isn't defined.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service.cc (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service.cc:17: store_(store.Pass()) {
initialize refresh_state_(REFRESH_NONE). The valgrind bot found an uninitialized
read in OnPolicyFetched.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service.h (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service.h:37: CloudPolicyStatus GetStatus()
const;
This is not defined, is it still needed?

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service_unittest.cc (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service_unittest.cc:27:
scoped_ptr<CloudPolicyStore>(store_)) {}
I'm surprised this compiles, I thought it'd need an explicit .Pass().

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_store.cc (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.cc:12: : status_(STATUS_OK) {}
Initialize is_initialized_

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_store.h (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.h:43: // Validation failure: Time
stamp.
Timestamp in the future?

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.h:91: const
enterprise_management::PolicyFetchResponse& policy) = 0;
I'm assuming |policy| only has to be valid during this call; otherwise the
lifetime expectation should be documented.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.h:117: void NotifyStoreError();
Nit: methods before data fields.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/mock...
File chrome/browser/policy/mock_cloud_policy_store.h (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/mock...
chrome/browser/policy/mock_cloud_policy_store.h:16: MockCloudPolicyStore() {}
Clang says this complex ctor must be out-of-line, and an out-of-line virtual
dtor is needed as well.

[Mattias Nissler (ping if slow), 2012-05-24 10:12:24]

All comments addressed, please take another look.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_constants.h (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_constants.h:96: CLOUD_POLICY_STATUS_SUCCESS,
On 2012/05/22 22:01:44, Joao da Silva wrote:
> Is this needed? Haven't found a use for it, except a
> CloudPolicyService::GetStatus() declaration that isn't defined.

The intention here is to put some code that figures out the overall status of
cloud policy from the client and provider into CloudPolicyService. That'll be
helpful for showing proper error messages in about:policy and the enrollment
screen. I've removed this for now and plan to re-introduce it once I get to
actually implement the error reporting bits.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service.cc (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service.cc:17: store_(store.Pass()) {
On 2012/05/22 22:01:44, Joao da Silva wrote:
> initialize refresh_state_(REFRESH_NONE). The valgrind bot found an
uninitialized
> read in OnPolicyFetched.

Done.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service.h (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service.h:37: CloudPolicyStatus GetStatus()
const;
On 2012/05/22 22:01:44, Joao da Silva wrote:
> This is not defined, is it still needed?

Removed, see other comment for details.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_service_unittest.cc (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_service_unittest.cc:27:
scoped_ptr<CloudPolicyStore>(store_)) {}
On 2012/05/22 22:01:44, Joao da Silva wrote:
> I'm surprised this compiles, I thought it'd need an explicit .Pass().

base/move.h has the details :)

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_store.cc (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.cc:12: : status_(STATUS_OK) {}
On 2012/05/22 22:01:44, Joao da Silva wrote:
> Initialize is_initialized_

Done.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_store.h (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.h:43: // Validation failure: Time
stamp.
On 2012/05/22 22:01:44, Joao da Silva wrote:
> Timestamp in the future?

Unless we make the check also reject policy that is too old... Or we might want
to introduce a new error code. For now, I've updated the comment.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.h:91: const
enterprise_management::PolicyFetchResponse& policy) = 0;
On 2012/05/22 22:01:44, Joao da Silva wrote:
> I'm assuming |policy| only has to be valid during this call; otherwise the
> lifetime expectation should be documented.

Yes, it's meant to be copied to whatever internal representation the
implementation wants to use.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_store.h:117: void NotifyStoreError();
On 2012/05/22 22:01:44, Joao da Silva wrote:
> Nit: methods before data fields.

Done.

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/mock...
File chrome/browser/policy/mock_cloud_policy_store.h (right):

http://codereview.chromium.org/10092010/diff/19002/chrome/browser/policy/mock...
chrome/browser/policy/mock_cloud_policy_store.h:16: MockCloudPolicyStore() {}
On 2012/05/22 22:01:44, Joao da Silva wrote:
> Clang says this complex ctor must be out-of-line, and an out-of-line virtual
> dtor is needed as well.

Done.

[Joao da Silva, 2012-05-24 15:06:18]

lgtm with a little nit

http://codereview.chromium.org/10092010/diff/23002/chrome/browser/policy/mock...
File chrome/browser/policy/mock_cloud_policy_store.h (right):

http://codereview.chromium.org/10092010/diff/23002/chrome/browser/policy/mock...
chrome/browser/policy/mock_cloud_policy_store.h:29: };
private:
  DISALLOW_COPY_AND_ASSIGN(MockCloudPolicyStore)

[Mattias Nissler (ping if slow), 2012-05-24 18:05:20]

Thanks for the review. I'll commit this once the first part has landed.

http://codereview.chromium.org/10092010/diff/23002/chrome/browser/policy/mock...
File chrome/browser/policy/mock_cloud_policy_store.h (right):

http://codereview.chromium.org/10092010/diff/23002/chrome/browser/policy/mock...
chrome/browser/policy/mock_cloud_policy_store.h:29: };
On 2012/05/24 15:06:18, Joao da Silva wrote:
> private:
>   DISALLOW_COPY_AND_ASSIGN(MockCloudPolicyStore)

Done.

[commit-bot: I haz the power, 2012-05-25 08:32:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/10092010/35002

[commit-bot: I haz the power, 2012-05-25 09:40:21]

Change committed as 139014