Implement device-local account policy pushing

Implement device-local account policy pushing

This CL implements policy pushing for device-local accounts. The
Tango connection comes from AffiliatedInvalidationServiceProvider,
which manages a single shared connection for device policy pushing
and device-local account policy pushing, conserving server resources.

BUG=442800
TEST=New unit test

TBR=dcheng (profile_invalidation_provider_factory.h)

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

[bartfab (slow), 2015-01-20 15:58:45]

bartfab@chromium.org changed reviewers:
+ mnissler@chromium.org, ygorshenin@chromium.org

[bartfab (slow), 2015-01-20 15:58:46]

Hi Mattias,

Could you take a look please? I had to make both DeviceSettingsService and
CrosSettings available via their respective singletons in the end. Otherwise, I
would always end up in an inconsistent state where some code uses the mock
objects while other code tries to Get() the singletons and ends up with a
nullptr.

Hi Yuri,

Could you take a look at:

chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc

[Mattias Nissler (ping if slow), 2015-01-21 13:07:33]

I'm really sad about the singleton proliferation - I'll need to take some more
time to think about whether this is really necessary.

[bartfab (slow), 2015-01-21 13:15:58]

On 2015/01/21 13:07:33, Mattias Nissler wrote:
> I'm really sad about the singleton proliferation - I'll need to take some more
> time to think about whether this is really necessary.

I did not keep detailed notes on what code ends up picking up what. But after
fighting the singletons for a while, I gave up :(.

[bartfab (slow), 2015-01-21 14:16:44]

On 2015/01/21 13:15:58, bartfab wrote:
> On 2015/01/21 13:07:33, Mattias Nissler wrote:
> > I'm really sad about the singleton proliferation - I'll need to take some
more
> > time to think about whether this is really necessary.
> 
> I did not keep detailed notes on what code ends up picking up what. But after
> fighting the singletons for a while, I gave up :(.

There is no way to run trybots here. The CL depends on the other two CLs you
reviewed which have no landed yet.

[bartfab (slow), 2015-01-27 15:35:51]

On 2015/01/21 14:16:44, bartfab wrote:
> On 2015/01/21 13:15:58, bartfab wrote:
> > On 2015/01/21 13:07:33, Mattias Nissler wrote:
> > > I'm really sad about the singleton proliferation - I'll need to take some
> more
> > > time to think about whether this is really necessary.
> > 
> > I did not keep detailed notes on what code ends up picking up what. But
after
> > fighting the singletons for a while, I gave up :(.
> 
> There is no way to run trybots here. The CL depends on the other two CLs you
> reviewed which have no landed yet.

Ping. Do you want to wait for the prerequisite CLs to land so we can see what
the trybots say before trying to reach a conclusion on what to do here?

[Mattias Nissler (ping if slow), 2015-01-28 09:04:28]

On 2015/01/27 15:35:51, bartfab wrote:
> On 2015/01/21 14:16:44, bartfab wrote:
> > On 2015/01/21 13:15:58, bartfab wrote:
> > > On 2015/01/21 13:07:33, Mattias Nissler wrote:
> > > > I'm really sad about the singleton proliferation - I'll need to take
some
> > more
> > > > time to think about whether this is really necessary.
> > > 
> > > I did not keep detailed notes on what code ends up picking up what. But
> after
> > > fighting the singletons for a while, I gave up :(.
> > 
> > There is no way to run trybots here. The CL depends on the other two CLs you
> > reviewed which have no landed yet.
> 
> Ping. Do you want to wait for the prerequisite CLs to land so we can see what
> the trybots say before trying to reach a conclusion on what to do here?

Looking now.

[bartfab (slow), 2015-01-29 14:38:21]

bartfab@chromium.org changed reviewers:
+ pneubeck@chromium.org, xiyuan@chromium.org
- ygorshenin@chromium.org

[bartfab (slow), 2015-01-29 14:38:21]

Hi Xiyuan,

Could you take a look at device_settings_test_helper.cc? Mattias is on vacation
and I need another reviewer.

Hi Philipp,

Could you take a look at the remainder of this CL? Mattias is on vacation and I
need another reviewer. I believe the CL should be ready to go. Mattias had
objected to the proliferation of singletons in unit tests that it causes and I
fixed that.

[xiyuan, 2015-01-29 17:40:58]

device_settings_test_helper.cc LGTM

[pneubeck (no reviews), 2015-01-30 14:23:59]

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc:204:
NULL));
nit: nullptr

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:67:
AffiliatedInvalidationServiceProvider* invalidation_service_provider,
nitty: 
 non const& arguments tend to be add the end of the argument list

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:129: bool
IsConnected();
could have a comment

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(left):

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:414:
broker->core()->client()->FetchPolicy();
you dropped this FetchPolicy call during the refactoring. Intentionally?

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:133:
void FlushDeviceLocalAccountPolicyFetch();
i think this function has enough side effect that it deserves a comment.

isn't this more of a 'FlushDeviceLocalAccountAndFetchPolicy()' or '...ToFetch..'
?

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:160:
new AffiliatedInvalidationServiceProvider);
you could make the relevant methods of the provider virtual and replace it by a
mock. If i see it correctly only the un-/register functions are used.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:539:
EXPECT_EQ(account_1_user_id_, broker->user_id());
this seems to be tested already in TEST_F(..., GetBroker), please don't
redundantly test this here or in ConnectBeforeInvalidationServiceAvailable

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:618:
TEST_F(DeviceLocalAccountPolicyServiceTest,
If I checked it correctly, by parameterizing DeviceCloudPolicyInvalidator with a
CloudPolicyCore* and DeviceRegisterRequest_Type, you could reuse it and the
lines 579 - 615 as well as this whole test wouldn't be necessary (or could be
moved to DeviceCloudPolicyInvalidator's unittest).

also most of the implementation might not be necessary anymore.
the IsConnected check will probably not hurt the current usage of
DeviceCloudPolicyInvalidator and could just be added there.

you know better than me how much code reduction that will be, so in the end it's
your call.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:633:
EXPECT_EQ(account_1_user_id_, broker->user_id());
same as above, please remove

[bartfab (slow), 2015-02-03 18:31:06]

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc:204:
NULL));
On 2015/01/30 14:23:58, pneubeck wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:67:
AffiliatedInvalidationServiceProvider* invalidation_service_provider,
On 2015/01/30 14:23:58, pneubeck wrote:
> nitty: 
>  non const& arguments tend to be add the end of the argument list

Done.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:129: bool
IsConnected();
On 2015/01/30 14:23:58, pneubeck wrote:
> could have a comment

Added comment. Also, renamed this to HasClient(). The old name, IsConnected(),
was misleading as having a CloudPolicyClient in place does not mean that the
client actually managed to connect correctly.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(left):

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:414:
broker->core()->client()->FetchPolicy();
On 2015/01/30 14:23:58, pneubeck wrote:
> you dropped this FetchPolicy call during the refactoring. Intentionally?

Yes, this was intentional. We should test that the broker initiates a policy
fetch at the correct places, instead of requesting fetches on its behalf.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:133:
void FlushDeviceLocalAccountPolicyFetch();
On 2015/01/30 14:23:58, pneubeck wrote:
> i think this function has enough side effect that it deserves a comment.
> 
> isn't this more of a 'FlushDeviceLocalAccountAndFetchPolicy()' or
'...ToFetch..'
> ?

Obsolete.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:160:
new AffiliatedInvalidationServiceProvider);
On 2015/01/30 14:23:58, pneubeck wrote:
> you could make the relevant methods of the provider virtual and replace it by
a
> mock. If i see it correctly only the un-/register functions are used.

Done.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:539:
EXPECT_EQ(account_1_user_id_, broker->user_id());
On 2015/01/30 14:23:58, pneubeck wrote:
> this seems to be tested already in TEST_F(..., GetBroker), please don't
> redundantly test this here or in ConnectBeforeInvalidationServiceAvailable

Obsolete.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:618:
TEST_F(DeviceLocalAccountPolicyServiceTest,
On 2015/01/30 14:23:58, pneubeck wrote:
> If I checked it correctly, by parameterizing DeviceCloudPolicyInvalidator with
a
> CloudPolicyCore* and DeviceRegisterRequest_Type, you could reuse it and the
> lines 579 - 615 as well as this whole test wouldn't be necessary (or could be
> moved to DeviceCloudPolicyInvalidator's unittest).
> 
> also most of the implementation might not be necessary anymore.
> the IsConnected check will probably not hurt the current usage of
> DeviceCloudPolicyInvalidator and could just be added there.
> 
> you know better than me how much code reduction that will be, so in the end
it's
> your call.

Done. After making this change, I renamed DeviceCloudPolicyInvalidator to
AffiliatedCloudPolicyInvalidator. All testing code moved to a
AffiliatedCloudPolicyInvalidator unit test. The Broker now simply owns a
AffiliatedCloudPolicyInvalidator instead of reimplementing its functionality.

https://codereview.chromium.org/822523003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:633:
EXPECT_EQ(account_1_user_id_, broker->user_id());
On 2015/01/30 14:23:58, pneubeck wrote:
> same as above, please remove

Obsolete.

[pneubeck (no reviews), 2015-02-04 10:02:14]

lgtm, thanks for the clean up.

https://codereview.chromium.org/822523003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h
(right):

https://codereview.chromium.org/822523003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h:30: // the
|CloudPolicyInvalidator| destroyed and re-created.
comment nit: s/destroyed/is destroyed/

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.cc
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.cc:12:
#include "policy/proto/device_management_backend.pb.h"
nit: included in .h already

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h:55:
CloudPolicyCore* core_;
nit: CloudPolicyCore* const core_

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h:57:
AffiliatedInvalidationServiceProvider* invalidation_service_provider_;
nit: +const

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator_unittest.cc
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator_unittest.cc:74:
FakeCloudPolicyStore store;
if possible to describe briefly, please do so for lines 74-88.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h:46:
virtual ~Consumer();
nitty: + DISALLOW_ASSIGN (which will require a c'tor)

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h:71:
DISALLOW_COPY_AND_ASSIGN(AffiliatedInvalidationServiceProvider);
nit: DISALLOW_ASSIGN is sufficient as this class is abstract

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:24:
#include
"chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h"
shouldn't be required as you're not actually accessing the provider but only
forwarding the pointer.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:124:
AffiliatedInvalidationServiceProvider* invalidation_service_provider_;
nit: +const

[bartfab (slow), 2015-02-04 12:33:24]

New patchsets have been uploaded after l-g-t-m from pneubeck@chromium.org

[bartfab (slow), 2015-02-04 12:34:52]

https://codereview.chromium.org/822523003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h
(right):

https://codereview.chromium.org/822523003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h:30: // the
|CloudPolicyInvalidator| destroyed and re-created.
On 2015/02/04 10:02:13, pneubeck wrote:
> comment nit: s/destroyed/is destroyed/

Done.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.cc
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.cc:12:
#include "policy/proto/device_management_backend.pb.h"
On 2015/02/04 10:02:13, pneubeck wrote:
> nit: included in .h already

Done.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h:55:
CloudPolicyCore* core_;
On 2015/02/04 10:02:13, pneubeck wrote:
> nit: CloudPolicyCore* const core_

Done.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator.h:57:
AffiliatedInvalidationServiceProvider* invalidation_service_provider_;
On 2015/02/04 10:02:13, pneubeck wrote:
> nit: +const

Done.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator_unittest.cc
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_cloud_policy_invalidator_unittest.cc:74:
FakeCloudPolicyStore store;
On 2015/02/04 10:02:14, pneubeck wrote:
> if possible to describe briefly, please do so for lines 74-88.

Done.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h:46:
virtual ~Consumer();
On 2015/02/04 10:02:14, pneubeck wrote:
> nitty: + DISALLOW_ASSIGN (which will require a c'tor)

Done.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h:71:
DISALLOW_COPY_AND_ASSIGN(AffiliatedInvalidationServiceProvider);
On 2015/02/04 10:02:14, pneubeck wrote:
> nit: DISALLOW_ASSIGN is sufficient as this class is abstract

Done.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:24:
#include
"chrome/browser/chromeos/policy/affiliated_invalidation_service_provider.h"
On 2015/02/04 10:02:14, pneubeck wrote:
> shouldn't be required as you're not actually accessing the provider but only
> forwarding the pointer.

Done.

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/822523003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:124:
AffiliatedInvalidationServiceProvider* invalidation_service_provider_;
On 2015/02/04 10:02:14, pneubeck wrote:
> nit: +const

Done.

[bartfab (slow), 2015-02-04 12:36:20]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2015-02-04 12:37:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/822523003/100001

[commit-bot: I haz the power, 2015-02-04 12:40:03]

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

[commit-bot: I haz the power, 2015-02-04 12:40:04]

Try jobs failed on following builders:
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bartfab (slow), 2015-02-04 13:05:38]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2015-02-04 13:06:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/822523003/120001

[commit-bot: I haz the power, 2015-02-04 14:37:56]

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

[commit-bot: I haz the power, 2015-02-04 14:37:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bartfab (slow), 2015-02-04 14:42:13]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2015-02-04 14:43:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/822523003/120001

[commit-bot: I haz the power, 2015-02-04 15:18:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-04 15:19:39]

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