Enable user policy handling through the new cloud policy stack.

Enable user policy handling through the new cloud policy stack.

Implement UserCloudPolicyManager, which wraps an instance of the new
cloud policy stack and implements the ConfigurationPolicyProvider
interface. Hook it up in BrowserPolicyConnector to be used for user
policy instead of the old code if enabled through a command line switch.

BUG=chromium:108929
TEST=With --enable-cloud-policy-service, user policy on Chrome OS still works as before.


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

[Mattias Nissler (ping if slow), 2012-06-01 09:27:18]

Hi Joao, here is the CL that finally enables this. Please take a look. There is
one remaining issue, namely BrowserPolicyConnector::user_cloud_policy_manager_
not being initialized when the policy service gets constructed. I think the
solution to that one would be to have a proxy provider for user policy in
BrowserPolicyConnector that facilitates swapping in the actual provider at
runtime, which is in particular useful at the point where UserCloudPolicyManager
moves to Profile (which I think is the way to go). Also note that this will
finally solve the issue of IsInitializationComplete returning bogus values on
the global PolicyService that you ran into a while ago.

[Joao da Silva, 2012-06-01 12:27:47]

lgtm after fixing the nits.

This is much better than the current cloud policy system. Thanks for doing all
this nice work :-)

The initialization issue is unfortunate. A proxy provider can work around it,
but I'd like to explore other options first. Another possibility is making
providers pluggable at the PolicyService (the challenge would then be to set
their relative priorities).

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

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.cc:99: }
// namespace

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
File chrome/browser/policy/user_cloud_policy_manager.cc (right):

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.cc:13: #include
"chrome/browser/browser_process.h"
Not needed

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.cc:15: #include
"chrome/browser/policy/cloud_policy_store.h"
Already in .h

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.cc:64:
scoped_ptr<UserCloudPolicyManager> UserCloudPolicyManager::Create(
// static

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:13: #include
"chrome/browser/policy/cloud_policy_service.h"
CloudPolicyService can be forward-declared in this header.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:20: class
SessionManagerClient;
Not needed

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:54: CloudPolicyService*
cloud_policy_service() { return service_.get(); }
const method?

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:72: #endif
Put this declaration after the dtor, to preserve the same order as in the .cc.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
File chrome/browser/policy/user_cloud_policy_manager_unittest.cc (right):

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager_unittest.cc:221:
EXPECT_TRUE(manager_->IsInitializationComplete());
Isn't this test like the previous?

[Mattias Nissler (ping if slow), 2012-06-04 17:42:36]

Here's the big CL again, this time with a proxy provider. I also have a very
crude hack to move UserCloudPolicyManager to the profile (which we'll need for
desktop cloud policy), but that'll probably need further work regarding the
Profile initialization sequence, so I'd like to land this in its current form.

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

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.cc:99: }
On 2012/06/01 12:27:47, Joao da Silva wrote:
> // namespace

Done.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
File chrome/browser/policy/user_cloud_policy_manager.cc (right):

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.cc:13: #include
"chrome/browser/browser_process.h"
On 2012/06/01 12:27:47, Joao da Silva wrote:
> Not needed

Done.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.cc:15: #include
"chrome/browser/policy/cloud_policy_store.h"
On 2012/06/01 12:27:47, Joao da Silva wrote:
> Already in .h

Done.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.cc:64:
scoped_ptr<UserCloudPolicyManager> UserCloudPolicyManager::Create(
On 2012/06/01 12:27:47, Joao da Silva wrote:
> // static

Done.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:13: #include
"chrome/browser/policy/cloud_policy_service.h"
On 2012/06/01 12:27:47, Joao da Silva wrote:
> CloudPolicyService can be forward-declared in this header.

Done.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:20: class
SessionManagerClient;
On 2012/06/01 12:27:47, Joao da Silva wrote:
> Not needed

Done.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:54: CloudPolicyService*
cloud_policy_service() { return service_.get(); }
On 2012/06/01 12:27:47, Joao da Silva wrote:
> const method?

You mean a const variant of the getter? Let's add it once we need it.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:72: #endif
On 2012/06/01 12:27:47, Joao da Silva wrote:
> Put this declaration after the dtor, to preserve the same order as in the .cc.

Done.

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
File chrome/browser/policy/user_cloud_policy_manager_unittest.cc (right):

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager_unittest.cc:221:
EXPECT_TRUE(manager_->IsInitializationComplete());
On 2012/06/01 12:27:47, Joao da Silva wrote:
> Isn't this test like the previous?

Yes, I meant to actually to a policy reload. Fixed.

[Joao da Silva, 2012-06-05 07:21:46]

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10449071/diff/9004/chrome/browser/policy/user_...
chrome/browser/policy/user_cloud_policy_manager.h:54: CloudPolicyService*
cloud_policy_service() { return service_.get(); }
On 2012/06/04 17:42:37, Mattias Nissler wrote:
> On 2012/06/01 12:27:47, Joao da Silva wrote:
> > const method?
> 
> You mean a const variant of the getter? Let's add it once we need it.

I mean that this method can be const:

CloudPolicyService* cloud_policy_service() const { return service_.get(); }

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:181:
providers.push_back(user_cloud_policy_manager_.get());
I think this should be removed now that the proxy is in place. Is there a reason
to directly add the user_cloud_policy_manager?

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:33: class ProxyPolicyProvider;
Nit: header is included

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:181: ProxyPolicyProvider
user_cloud_policy_provider_;
This will be deleted after the |handler_list_|, leading to the use-after-free
crashes we saw before.

I'd move |handler_list_| to the top, and make it the last thing to be destructed
since it doesn't have other dependencies. If you do that, then there's no need
for the explicit reset() of the providers in the dtor; and the comment there
should be next to |handler_list_| to explain why it's there.

[Mattias Nissler (ping if slow), 2012-06-05 10:27:21]

Addressed comments. Please take another look.

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:181:
providers.push_back(user_cloud_policy_manager_.get());
On 2012/06/05 07:21:46, Joao da Silva wrote:
> I think this should be removed now that the proxy is in place. Is there a
reason
> to directly add the user_cloud_policy_manager?

Yes, for wait_for_policy_fetch to work correctly. Profiles should use the user
policy manager as their provider, while the global policy service should use the
proxy. I've updated the code and put a comment explaining it.

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:33: class ProxyPolicyProvider;
On 2012/06/05 07:21:46, Joao da Silva wrote:
> Nit: header is included

Done.

http://codereview.chromium.org/10449071/diff/16001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:181: ProxyPolicyProvider
user_cloud_policy_provider_;
On 2012/06/05 07:21:46, Joao da Silva wrote:
> This will be deleted after the |handler_list_|, leading to the use-after-free
> crashes we saw before.
> 
> I'd move |handler_list_| to the top, and make it the last thing to be
destructed
> since it doesn't have other dependencies. If you do that, then there's no need
> for the explicit reset() of the providers in the dtor; and the comment there
> should be next to |handler_list_| to explain why it's there.

Done.

[Joao da Silva, 2012-06-05 11:33:27]

lgtm

http://codereview.chromium.org/10449071/diff/27002/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/10449071/diff/27002/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.h:161: // will will result in
|handler_list_| being consulted for policy translation.
Nit: will will

[commit-bot: I haz the power, 2012-06-05 11:47:29]

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

[commit-bot: I haz the power, 2012-06-05 12:52:36]

Change committed as 140519