Move DeviceManagementPolicyProvider into the profile.

Move DeviceManagementPolicyProvider into the profile.

Previously, we used only one provider, which would break in situations where there are two profiles (which ChromeOS already does), because it would apply the policy to both profiles and not only the logged in one. For this to work properly, we also need to pass the relevant TokenService instance the DeviceTokenFetcher, so the fetcher can decide whether the token is relevant to it by checking whether the TokenService that sent it is the one associated with the right profile.

BUG=63608
TEST=existing unit tests succeed

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

[Mattias Nissler (ping if slow), 2010-11-18 13:08:47]

Please review!

Markus, Gabor, Jakob: FYI (for the various dependencies)

[Nico, 2010-11-18 13:53:39]

Please update the CL description with why this is useful. Does it makes things
possible that weren't possible before? Does it makes things easier?

I'm dense, I don't understand that just from reading the diff.

Also, all the TokenService changes don't look immediately related to moving
something into the profile to me. Is this intentionally in this CL? (I guess a
better CL description can clear this up, too :-) ).

http://codereview.chromium.org/5174006/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5174006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider_unittest.cc:20: const
char kTestToken[] = "device_policy_provider_test_auth_token";
const things implicitly have internal linkage, no need for the anonymous
namespace

http://codereview.chromium.org/5174006/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5174006/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:187: //        be non-null if any
preferences are to be registered.
update comment?

[Mattias Nissler (ping if slow), 2010-11-18 14:20:15]

Addressed comments, new version is up. It now has a nice description and even an
associated bug that explains what is wrong with the current code that the CL
fixes. I really was too terse, thanks for pointing this out.

http://codereview.chromium.org/5174006/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/5174006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider_unittest.cc:20: const
char kTestToken[] = "device_policy_provider_test_auth_token";
On 2010/11/18 13:53:40, Nico wrote:
> const things implicitly have internal linkage, no need for the anonymous
> namespace

Done.

http://codereview.chromium.org/5174006/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5174006/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:187: //        be non-null if any
preferences are to be registered.
On 2010/11/18 13:53:40, Nico wrote:
> update comment?

Done.

[Nico, 2010-11-18 14:34:24]

Makes sense. LG.

Is there anything interesting that happens when the user is incognito? Should
the incognito profile call through to the normal profile? Does it do that by
default? Do you think there could be problems where someone gets the
DeviceManagementPolicyProvider from the incognito profile, then gets the (now
normal) profile from DeviceManagementPolicyProvider and changes that, so that
now the normal profile gets modified in incognito mode?

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

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:38: if (!HasAuthToken()) {
You check in |Observe|, so this isn't really necessary. I assume this event
doesn't happen all that often.

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.h:190: // management pref store in case of
policy refresh.
get rid of the newline above this, and make the formatting match the description
of the other parameters

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.h:194: // parameter be removed her.
can be removed here

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/profile_impl....
chrome/browser/profile_impl.cc:1337: }
I like early exits, they lead to less "indenty" code:

  CommandLine* command_line = CommandLine::ForCurrentProcess();
  if (!command_line->HasSwitch(switches::kDeviceManagementUrl))
    return NULL;

// ...rest

[Mattias Nissler (ping if slow), 2010-11-18 15:08:16]

Here's another update.

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

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:38: if (!HasAuthToken()) {
On 2010/11/18 14:34:24, Nico wrote:
> You check in |Observe|, so this isn't really necessary. I assume this event
> doesn't happen all that often.

Removed. Done.

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.h:190: // management pref store in case of
policy refresh.
On 2010/11/18 14:34:24, Nico wrote:
> get rid of the newline above this, and make the formatting match the
description
> of the other parameters

I disagree, the above comment talks about the pref stores "in decreasing order
of precedence", which profile isn't one of.

Done.

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.h:194: // parameter be removed her.
On 2010/11/18 14:34:24, Nico wrote:
> can be removed here

Done.

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5174006/diff/6001/chrome/browser/profile_impl....
chrome/browser/profile_impl.cc:1337: }
On 2010/11/18 14:34:24, Nico wrote:
> I like early exits, they lead to less "indenty" code:
> 
>   CommandLine* command_line = CommandLine::ForCurrentProcess();
>   if (!command_line->HasSwitch(switches::kDeviceManagementUrl))
>     return NULL;
> 
> // ...rest

Yay, me too! But there are many people who don't. Changed anyways :)

[Mattias Nissler (ping if slow), 2010-11-18 15:10:15]

On 2010/11/18 14:34:24, Nico wrote:
> Makes sense. LG.
> 
> Is there anything interesting that happens when the user is incognito? Should
> the incognito profile call through to the normal profile? Does it do that by
> default? Do you think there could be problems where someone gets the
> DeviceManagementPolicyProvider from the incognito profile, then gets the (now
> normal) profile from DeviceManagementPolicyProvider and changes that, so that
> now the normal profile gets modified in incognito mode?

Incognito shares the prefs with the underlying profile. Since the provider is
only relevant to prefs, there is nothing to do here.

[markusheintz_, 2010-11-18 15:42:33]

LGTM with comments addressed

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:94: return new
DummyConfigurationPolicyProvider(
Do you really want to create a DummyConfigurationPolicyProvider? Is this guy
replaced when the next policy refresh happens?

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:191: // state preferences.
I'm so sorry to bother you with comments, but I don't understand this comment
:(. What is a replacement device management pref store? and why do we need it. 

Maybe something like this:

The |profile| parameter is needed to construct a device management pref store.
Everytime a policy refresh happens a new management pref store is created and
used to replace the old one. The is necessary to prevent concurrency issues.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:250: // The associated profile, if
applicable.
When is it not applicable? Maybe it's worth to add that. Or remove applicable if
not needed.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/profile_impl...
chrome/browser/profile_impl.cc:58: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
swap the two lines above pls (alphabetic order)

[Mattias Nissler (ping if slow), 2010-11-18 16:06:37]

Answer's to Markus questions. No comment changes so far, please indicate whether
you still want some and if so which.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:94: return new
DummyConfigurationPolicyProvider(
On 2010/11/18 15:42:33, markusheintz_ wrote:
> Do you really want to create a DummyConfigurationPolicyProvider? Is this guy
> replaced when the next policy refresh happens?

No. The factory method (see below) will only fall back to this if the
PrefValueStore doesn't have a profile (i.e. handling local state). I think
putting this here into the keeper is the right thing, since this is the location
we keep all our static stores. We now just have an override for the device
management store that is used instead for profile PrefServices.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:191: // state preferences.
On 2010/11/18 15:42:33, markusheintz_ wrote:
> I'm so sorry to bother you with comments, but I don't understand this comment
> :(. What is a replacement device management pref store? and why do we need it.

> 
> Maybe something like this:
> 
> The |profile| parameter is needed to construct a device management pref store.
> Everytime a policy refresh happens a new management pref store is created and
> used to replace the old one. The is necessary to prevent concurrency issues.
> 
> 

Na, the comment is really about this: The device management prefstore is passed
in here, but since we construct a new one when policy refreshes, we need to also
keep the profile parameter so we can use it then.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:250: // The associated profile, if
applicable.
On 2010/11/18 15:42:33, markusheintz_ wrote:
> When is it not applicable? Maybe it's worth to add that. Or remove applicable
if
> not needed.

It's only applicable if this pref_value_store is associated with a profile, i.e.
used for a profile PrefService and not local state. I hope you now understood
the comment above, so you probably also get this one? Or is there a need to
change?

[markusheintz_, 2010-11-19 09:53:07]

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:191: // state preferences.
On 2010/11/18 16:06:38, Mattias Nissler wrote:
> On 2010/11/18 15:42:33, markusheintz_ wrote:
> > I'm so sorry to bother you with comments, but I don't understand this
comment
> > :(. What is a replacement device management pref store? and why do we need
it.
> 
> > 
> > Maybe something like this:
> > 
> > The |profile| parameter is needed to construct a device management pref
store.
> > Everytime a policy refresh happens a new management pref store is created
and
> > used to replace the old one. The is necessary to prevent concurrency issues.
> > 
> > 
> 
> Na, the comment is really about this: The device management prefstore is
passed
> in here, but since we construct a new one when policy refreshes, we need to
also
> keep the profile parameter so we can use it then.

That's how I understood it too. But I had to read the code in other places
first. So why not make this comment more expressive, so that it becomes clear
that the passed managed_pref_store is replaced after a policy refresh. I think
that this is really important to mention.

How about something like this:
"After each cloud policy refresh a new pref-store instance is created that holds
the latest cloud policies. The new instance replaces the old
(device_management_pref) one. The |profile| parameter is needed to construct the
new replacement device management pref store."

If you want to keep this comment as it is then at least add something like the
following to line 180:

"The passed PrefStore is replaced be a new instance after cloud policies are
refreshed".

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:250: // The associated profile, if
applicable.
On 2010/11/18 16:06:38, Mattias Nissler wrote:
> On 2010/11/18 15:42:33, markusheintz_ wrote:
> > When is it not applicable? Maybe it's worth to add that. Or remove
applicable
> if
> > not needed.
> 
> It's only applicable if this pref_value_store is associated with a profile,
i.e.
> used for a profile PrefService and not local state. I hope you now understood
> the comment above, so you probably also get this one? Or is there a need to
> change?
> 

I understood this comment, but I think if someone reads it the first question
that pops up: What does that mean "if applicable" ? So either we clarify that or
we remove it.

s/if applicable/. No profile is needed if no cloud policies are read./

or something similar. Just that people now why when their read it.

[Mattias Nissler (ping if slow), 2010-11-19 10:49:38]

I missed your comment, sorry. Will fix in a follow-up CL.

On Fri, Nov 19, 2010 at 10:53 AM, <markusheintz@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
> File chrome/browser/prefs/pref_value_store.h (right):
>
>
>
http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
> chrome/browser/prefs/pref_value_store.h:191: // state preferences.
> On 2010/11/18 16:06:38, Mattias Nissler wrote:
>
>> On 2010/11/18 15:42:33, markusheintz_ wrote:
>> > I'm so sorry to bother you with comments, but I don't understand
>>
> this comment
>
>> > :(. What is a replacement device management pref store? and why do
>>
> we need it.
>
>  >
>> > Maybe something like this:
>> >
>> > The |profile| parameter is needed to construct a device management
>>
> pref store.
>
>> > Everytime a policy refresh happens a new management pref store is
>>
> created and
>
>> > used to replace the old one. The is necessary to prevent concurrency
>>
> issues.
>
>> >
>> >
>>
>
>  Na, the comment is really about this: The device management prefstore
>>
> is passed
>
>> in here, but since we construct a new one when policy refreshes, we
>>
> need to also
>
>> keep the profile parameter so we can use it then.
>>
>
> That's how I understood it too. But I had to read the code in other
> places first. So why not make this comment more expressive, so that it
> becomes clear that the passed managed_pref_store is replaced after a
> policy refresh. I think that this is really important to mention.
>
> How about something like this:
> "After each cloud policy refresh a new pref-store instance is created
> that holds the latest cloud policies. The new instance replaces the old
> (device_management_pref) one. The |profile| parameter is needed to
> construct the new replacement device management pref store."
>
> If you want to keep this comment as it is then at least add something
> like the following to line 180:
>
> "The passed PrefStore is replaced be a new instance after cloud policies
> are refreshed".
>
>
>
>
http://codereview.chromium.org/5174006/diff/27001/chrome/browser/prefs/pref_v...
> chrome/browser/prefs/pref_value_store.h:250: // The associated profile,
> if applicable.
> On 2010/11/18 16:06:38, Mattias Nissler wrote:
>
>> On 2010/11/18 15:42:33, markusheintz_ wrote:
>> > When is it not applicable? Maybe it's worth to add that. Or remove
>>
> applicable
>
>> if
>> > not needed.
>>
>
>  It's only applicable if this pref_value_store is associated with a
>>
> profile, i.e.
>
>> used for a profile PrefService and not local state. I hope you now
>>
> understood
>
>> the comment above, so you probably also get this one? Or is there a
>>
> need to
>
>> change?
>>
>
>
> I understood this comment, but I think if someone reads it the first
> question that pops up: What does that mean "if applicable" ? So either
> we clarify that or we remove it.
>
> s/if applicable/. No profile is needed if no cloud policies are read./
>
> or something similar. Just that people now why when their read it.
>
>
> http://codereview.chromium.org/5174006/
>

[markusheintz_, 2010-11-19 11:00:45]

If you address the other comments in your next CL Please also address these.
THANKS!!!!!!!

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:94: return new
DummyConfigurationPolicyProvider(
On 2010/11/18 16:06:38, Mattias Nissler wrote:
> On 2010/11/18 15:42:33, markusheintz_ wrote:
> > Do you really want to create a DummyConfigurationPolicyProvider? Is this guy
> > replaced when the next policy refresh happens?
> 
> No. The factory method (see below) will only fall back to this if the
> PrefValueStore doesn't have a profile (i.e. handling local state). I think
> putting this here into the keeper is the right thing, since this is the
location
> we keep all our static stores. We now just have an override for the device
> management store that is used instead for profile PrefServices.

I see. Sry I thought this was the factory method below.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:36: // testing. The fetcher uses
the directory in |token_dir| in which to store
I guess this should be token_path. Pls also address this if applicable.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:40: TokenService* token_service,
Please add an explaination for this parameter to the comment above.

[Mattias Nissler (ping if slow), 2010-11-19 11:40:02]

Let's switch discussion to the other CL.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:36: // testing. The fetcher uses
the directory in |token_dir| in which to store
On 2010/11/19 11:00:46, markusheintz_ wrote:
> I guess this should be token_path. Pls also address this if applicable.

Done.

http://codereview.chromium.org/5174006/diff/27001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:40: TokenService* token_service,
On 2010/11/19 11:00:46, markusheintz_ wrote:
> Please add an explaination for this parameter to the comment above.

Done.