Introduce CloudPolicyClient

Introduce CloudPolicyClient

This is part of a clean up of the policy code. CloudPolicyClient moves
the core policy interactions into a single class with a well defined
interface. Subsequent CLs will remove CloudPolicyController and
DeviceTokenFetcher and introduce cleaner separation of policy fetching
and caching.

BUG=chromium:108928
TEST=Compiles and passes tests.


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

[Mattias Nissler (ping if slow), 2012-01-05 11:11:54]

Here's a first cut at CloudPolicyClient, early comments appreciated.

[Joao da Silva, 2012-01-05 15:35:00]

Looks good, I like a lot how much simpler it is to follow :-) I'm only unsure
about how the dm_token and client_id are going to be cached, see inline.

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

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.cc:84: if (!machine_id_.empty())
if (submit_machine_id && !machine_id_.empty()) ? Should that flag also guard
machine_model_?

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

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:60: int version() { return version_;
}
Nit: const

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:79: virtual void OnError() = 0;
This is great for the CloudPolicyProvider::FetchPolicies :-)

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:88: // Retrieves in status
information to send with the next policy fetch.
Document what the bool return value is used for. Returns true if |status| was
filled, and false otherwise?

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:96: virtual void
OnSubmittedSuccessfully() = 0;
Didn't find any calls to this.

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:113: const std::string& client_id);
Where is |dm_token| supposed to come from? I guess it's from a token_cache
that's owned by the other of |this| too (a super-duper-new-CloudPolicyProvider
that does what the current CloudPolicySubsystem does, or something similar). In
that case, how can it retrieve the dm_token from this class for storage in the
token_cache?

It seems to me there should be an accessor for |dm_token_|. Same for
|client_id_|.

(I just noticed later that this data is also available from the protobuf from
policy(); is that how it's meant to be retrieved?)

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:126: bool FetchPolicy();
The part about canceling previous requests turned out much simpler in this
implementation, very cool!

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

Here's the current version of this code right from my file system :) Nothing
new, just some iteration.

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

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.cc:84: if (!machine_id_.empty())
On 2012/01/05 15:35:01, Joao da Silva wrote:
> if (submit_machine_id && !machine_id_.empty()) ? Should that flag also guard
> machine_model_?

Nah, that flag is just for policy fetches. For registration, we always want to
submit the machine id.

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

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:60: int version() { return version_;
}
On 2012/01/05 15:35:01, Joao da Silva wrote:
> Nit: const

Done.

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:88: // Retrieves in status
information to send with the next policy fetch.
On 2012/01/05 15:35:01, Joao da Silva wrote:
> Document what the bool return value is used for. Returns true if |status| was
> filled, and false otherwise?

Done.

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:96: virtual void
OnSubmittedSuccessfully() = 0;
On 2012/01/05 15:35:01, Joao da Silva wrote:
> Didn't find any calls to this.

True. I had missed to implement this properly. Updated the unit tests as well.

http://codereview.chromium.org/9109009/diff/2004/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_client.h:113: const std::string& client_id);
On 2012/01/05 15:35:01, Joao da Silva wrote:
> Where is |dm_token| supposed to come from? I guess it's from a token_cache
> that's owned by the other of |this| too (a super-duper-new-CloudPolicyProvider
> that does what the current CloudPolicySubsystem does, or something similar).
In
> that case, how can it retrieve the dm_token from this class for storage in the
> token_cache?
> 
> It seems to me there should be an accessor for |dm_token_|. Same for
> |client_id_|.
> 
> (I just noticed later that this data is also available from the protobuf from
> policy(); is that how it's meant to be retrieved?)

Yes, the intention is to make the owner of the client responsible to fill in the
token.  And yes, I intend to pull the token and client id directly from the
protobuf.

[Joao da Silva, 2012-04-16 23:45:00]

Looks good! Let me know once you plan to land it, and I'll look for the nits :-)

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:74: client_id_ =
guid::GenerateGUID();
CHECK(dm_token.empty())?

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.h (right):

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:45: POLICY_SCOPE_DEVICE,
Use the constants from policy_map.h?

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:105: UserAffiliation
user_affiliation,
This changes after enrollment. Will the owner of the client create a new one?
Maybe this should come from the StatusProvider interface instead?

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_device_management_service.cc (right):

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.cc:11: using
testing::Matcher;
These 2 are not used

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

Here's a new version that should be good for a proper review.

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:74: client_id_ =
guid::GenerateGUID();
On 2012/04/16 23:45:00, Joao da Silva wrote:
> CHECK(dm_token.empty())?

Probably a good idea. We probably want to support re-registration eventually as
well, but then we can change the code here.

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.h (right):

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:45: POLICY_SCOPE_DEVICE,
On 2012/04/16 23:45:00, Joao da Silva wrote:
> Use the constants from policy_map.h?

I'm on the fence, because PolicyMap isn't a dependency of this code. I've now
moved the PolicyScope declaration to a new header that both include to avoid the
inappropriate dependency. WDYT?

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:105: UserAffiliation
user_affiliation,
On 2012/04/16 23:45:00, Joao da Silva wrote:
> This changes after enrollment. Will the owner of the client create a new one?
> Maybe this should come from the StatusProvider interface instead?

The assumption is that we bring up the client once we start a user session. At
that point, the affiliation cannot change. If we have reason to change the
affiliation in the future, I'd prefer to add a setter. The intention behind
StatusProvider is to supply information that's changing for every request.

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_device_management_service.cc (right):

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.cc:11: using
testing::Matcher;
On 2012/04/16 23:45:00, Joao da Silva wrote:
> These 2 are not used

Done.

[Joao da Silva, 2012-05-22 20:59:30]

lgtm after cleaning up the nits.

I've made a couple of other suggestions that's really up to you, I'm cool with
either way.

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.h (right):

http://codereview.chromium.org/9109009/diff/11003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:45: POLICY_SCOPE_DEVICE,
On 2012/05/22 14:05:26, Mattias Nissler wrote:
> On 2012/04/16 23:45:00, Joao da Silva wrote:
> > Use the constants from policy_map.h?
> 
> I'm on the fence, because PolicyMap isn't a dependency of this code. I've now
> moved the PolicyScope declaration to a new header that both include to avoid
the
> inappropriate dependency. WDYT?

That's cool.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/auto_...
File chrome/browser/policy/auto_enrollment_client_unittest.cc (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/auto_...
chrome/browser/policy/auto_enrollment_client_unittest.cc:32: using
::testing::Invoke;
Nit: not used anymore

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:54: request_job_(NULL),
Nit: I wouldn't explicitly initialize a NULL scoped_ptr, your call

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:64: 
DCHECK(!is_registered()) ?

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:75: client_id_ =
guid::GenerateGUID();
This is the same that the current code is doing, but it means a crazy client
that keeps registering (and failing) will use a different client_id per request.
Maybe generate the GUID at the ctor, or only if it's empty?

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:95: CHECK(!dm_token_.empty());
Rename to CHECK(is_registered()) ?

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:214:
response.policy_response().response(0)));
I'd like to avoid the copy if possible, and I think we could do this:

policy_.reset(response.policy_response().mutable_response()->ReleaseLast())

It'd also require swapping the first and last elements above, after checking
that response_size() > 1:

em::PolicyFetchResponse* r = response.policy_response().mutable_response();
r->SwapElements(0, r->size()-1);

Got this from [1]. I'm not sure the complication is worth the effort, so it's
your call :-)

[1]
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:216:
status_provider_->OnSubmittedSuccessfully();
Will this be the right thing to do if FetchPolicy is called twice and the 2nd
call drops the first? The status_provider_ will be asked for data twice but this
callback only refers to the data from the 2nd call. I guess that is correct,
just checking :-)

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.h (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:31: class MockCloudPolicyClient;
Nit: not needed

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:46: // Constructs and invalid public
key version.
Nit: s/and/an

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:215: 
private:

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_device_management_service.h (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.h:15: class
MockDeviceManagementService;
Not needed?

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.h:33: const std::string&
client_id,
#include <string>

[Joao da Silva, 2012-05-22 21:23:36]

Additional comments for the unit test.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client_unittest.cc (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client_unittest.cc:23: namespace {
Nit: newline

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client_unittest.cc:35:
MOCK_METHOD1(OnClientError, void(CloudPolicyClient*));
Nit: similar mocks declare DISALLOW_COPY_AND_ASSIGN (e.g.
MockDeviceManagementRequestJob at mock_device_management_service.cc).

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

Thanks for the review, new version is up. Please take another look; the only
significant change is the removal of class PublicKeyVersion.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/auto_...
File chrome/browser/policy/auto_enrollment_client_unittest.cc (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/auto_...
chrome/browser/policy/auto_enrollment_client_unittest.cc:32: using
::testing::Invoke;
On 2012/05/22 20:59:30, Joao da Silva wrote:
> Nit: not used anymore

Done.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:54: request_job_(NULL),
On 2012/05/22 20:59:30, Joao da Silva wrote:
> Nit: I wouldn't explicitly initialize a NULL scoped_ptr, your call

Done.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:64: 
On 2012/05/22 20:59:30, Joao da Silva wrote:
> DCHECK(!is_registered()) ?

Done.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:75: client_id_ =
guid::GenerateGUID();
On 2012/05/22 20:59:30, Joao da Silva wrote:
> This is the same that the current code is doing, but it means a crazy client
> that keeps registering (and failing) will use a different client_id per
request.
> Maybe generate the GUID at the ctor, or only if it's empty?

There's a good reason to re-generate the ID: privacy. We should only keep the ID
if registration is successful. Added a comment to that effect.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:95: CHECK(!dm_token_.empty());
On 2012/05/22 20:59:30, Joao da Silva wrote:
> Rename to CHECK(is_registered()) ?

Done.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:214:
response.policy_response().response(0)));
On 2012/05/22 20:59:30, Joao da Silva wrote:
> I'd like to avoid the copy if possible, and I think we could do this:
> 
> policy_.reset(response.policy_response().mutable_response()->ReleaseLast())
> 
> It'd also require swapping the first and last elements above, after checking
> that response_size() > 1:
> 
> em::PolicyFetchResponse* r = response.policy_response().mutable_response();
> r->SwapElements(0, r->size()-1);
> 
> Got this from [1]. I'm not sure the complication is worth the effort, so it's
> your call :-)
> 
> [1]
>
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...

That would require to pass |response| non-const. I'd rather keep the interface
clean as long as we don't run into performance issues here.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:216:
status_provider_->OnSubmittedSuccessfully();
On 2012/05/22 20:59:30, Joao da Silva wrote:
> Will this be the right thing to do if FetchPolicy is called twice and the 2nd
> call drops the first? The status_provider_ will be asked for data twice but
this
> callback only refers to the data from the 2nd call. I guess that is correct,
> just checking :-)

The idea is that the status provider should only flush state once it sees this
callback. If it has never seen a confirmation for the first request, it should
include the data shipped with the first request also in the second.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.h (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:31: class MockCloudPolicyClient;
On 2012/05/22 20:59:30, Joao da Silva wrote:
> Nit: not needed

Done.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:46: // Constructs and invalid public
key version.
On 2012/05/22 20:59:30, Joao da Silva wrote:
> Nit: s/and/an

Obsolete. I removed this class entirely, it turned out to be not very useful
after all.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.h:215: 
On 2012/05/22 20:59:30, Joao da Silva wrote:
> private:

Done.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client_unittest.cc (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client_unittest.cc:23: namespace {
On 2012/05/22 21:23:36, Joao da Silva wrote:
> Nit: newline

Done.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client_unittest.cc:35:
MOCK_METHOD1(OnClientError, void(CloudPolicyClient*));
On 2012/05/22 21:23:36, Joao da Silva wrote:
> Nit: similar mocks declare DISALLOW_COPY_AND_ASSIGN (e.g.
> MockDeviceManagementRequestJob at mock_device_management_service.cc).

Done.

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_device_management_service.h (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.h:15: class
MockDeviceManagementService;
On 2012/05/22 20:59:30, Joao da Silva wrote:
> Not needed?

Indeed. Not sure how it ended up here :)

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.h:33: const std::string&
client_id,
On 2012/05/22 20:59:30, Joao da Silva wrote:
> #include <string>

Done.

[Joao da Silva, 2012-05-24 15:00:47]

lgtm!

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:64: 
On 2012/05/24 10:12:01, Mattias Nissler wrote:
> On 2012/05/22 20:59:30, Joao da Silva wrote:
> > DCHECK(!is_registered()) ?
> 
> Done.

I meant that as an additional DCHECK, not in place of the others. Can the
arguments be empty?

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:75: client_id_ =
guid::GenerateGUID();
On 2012/05/24 10:12:01, Mattias Nissler wrote:
> On 2012/05/22 20:59:30, Joao da Silva wrote:
> > This is the same that the current code is doing, but it means a crazy client
> > that keeps registering (and failing) will use a different client_id per
> request.
> > Maybe generate the GUID at the ctor, or only if it's empty?
> 
> There's a good reason to re-generate the ID: privacy. We should only keep the
ID
> if registration is successful. Added a comment to that effect.

Ah true. Thanks for the clarification.

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

Thanks for the review. Ticking the commit checkbox...

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_client.cc (right):

http://codereview.chromium.org/9109009/diff/30015/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_client.cc:64: 
On 2012/05/24 15:00:47, Joao da Silva wrote:
> On 2012/05/24 10:12:01, Mattias Nissler wrote:
> > On 2012/05/22 20:59:30, Joao da Silva wrote:
> > > DCHECK(!is_registered()) ?
> > 
> > Done.
> 
> I meant that as an additional DCHECK, not in place of the others. Can the
> arguments be empty?

Ah true, and we want to avoid the arguments to be empty. I restored the checks.

[commit-bot: I haz the power, 2012-05-24 18:01:27]

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

[commit-bot: I haz the power, 2012-05-24 20:02:00]

Change committed as 138873