Add support for loading user cloud policy on desktop.

Add support for loading user cloud policy on desktop, behind the --load-cloud-policy-on-signin flag.

Move UserCloudPolicyManager to be owned by the Profile as a step towards supporting multiple profiles.

Added TestingProfile::Builder as a start towards taming the madness of all the different constructors and setters.

BUG=141123
TEST=none


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

[Andrew T Wilson (Slow), 2012-08-02 21:43:17]

Mattias, PTAL at stuff under policy/ and chromeos/ (feel free to look at the
changes under profiles/ also). I'll start looping in OWNERS for other sections
once you've had a chance to look at the main change.

[Mattias Nissler (ping if slow), 2012-08-03 12:19:08]

Looks pretty good in general, the only two items I'm a bit unhappy about are
short circuiting for tests that are now littered around the code (meaning the
test setup is still pretty different from production), and the
wait_for_policy_fetch hack. I don't have good ideas on how to address these from
the top of my head though.

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

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:132: // AsyncPlatformLoader
requires deletion on the FILE thread.
In policy, we usually just spin up a fake FILE thread in the tests to keep the
production code clean. How many tests are affected?

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.cc:61: Profile* profile, bool
wait_for_policy_fetch) {
parameters on separate lines.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:44: virtual void
Initialize(PrefService* local_prefs,
Wait, are you passing g_browser_process->local_state() here? That is incorrect
as the Profile PrefService can have different policy than local_state.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_store_base.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_base.cc:9: #include
"chrome/browser/signin/signin_manager.h"
used?

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_base.cc:11: using
enterprise_management::PolicyData;
Used? We have the convention to declare a namespace alias for
enterprise_management in implementation files, i.e.

namespace em = enterprise_management;

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_store_base.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_base.h:11: #include
"base/compiler_specific.h"
needed?

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_base.h:12: #include
"base/file_path.h"
needed?

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:26: // TODO(atwilson): Move
this once we add OAuth token support to TokenService.
So we still don't have it? I was under the impression it got implemented, but I
may be wrong.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:38: Profile* profile,
UserCloudPolicyManager* manager)
arguments on separate lines (as per Chromium style)

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:121:
manager_->Initialize(g_browser_process->local_state(),
I think you want the Profile's PrefService here to get the correct refresh rate
policy.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:30: #if !defined(OS_CHROMEOS)
Seems like this should rather use an exclude rule in chrome_browser.gypi?

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service_unittest.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service_unittest.cc:36: class
TestingProfileWithUserCloudPolicyManager : public TestingProfile {
Used?

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service_unittest.cc:53:
((TestingBrowserProcess*) g_browser_process)->SetLocalState(
Should use static_cast, but you may actually not need this if you switch to the
Profile's PrefService

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service_unittest.cc:171: 
remove trailing newline.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_member.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_member.cc:38:
DCHECK(prefs_->FindPreference(pref_name_.c_str())) <<
There seems to be the convention that the << operation should be on the next
line:

$ git grep '<<$' -- *.cc | wc -l
275
$ git grep '^\s\+<<' -- *.cc | wc -l
2212

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:243:
OffTheRecordProfileImpl::GetUserCloudPolicyManager() {
indent by 4 (as elsewhere in this file).

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:298: policy::USER_AFFILIATION_MANAGED;
Ah, I still feel bad for this hack. It would be nice if we could pass the
wait_for_policy_fetch flag on profile creation instead, not sure whether that's
possible though.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:307: // will cause it to get downloaded
eventually.
Not sure this is going to fly long term. If we force signing in to make sure the
user gets policy, the admin will also expect that settings relevant to startup
(i.e. the RestoreOnStartup policy) takes effect. For that case, waiting for the
sign in and the policy fetch to complete before opening the browser window might
be the way to go.

I think it's fair for now though to ignore the issue.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_mock.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_mock.cc:6: #include
"chrome/browser/prefs/pref_service_mock_builder.h"
not needed any longer?

http://codereview.chromium.org/10693022/diff/52055/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/common/pref_names.c...
chrome/common/pref_names.cc:1933: const char kLoadCloudPolicyOnSignin[] =
"policy.load_cloud_policy_on_signin";
This (and the two above) doesn't belong here (this is the "SERVICE PREFS"
section, but rather in the LOCAL STATE or PROFILE PREFS sections.

I'm fighting a losing battle against entropy in this file :)

[Andrew T Wilson (Slow), 2012-08-03 20:57:34]

On 2012/08/03 12:19:08, Mattias Nissler wrote:
> Looks pretty good in general, the only two items I'm a bit unhappy about are
> short circuiting for tests that are now littered around the code (meaning the
> test setup is still pretty different from production), and the
> wait_for_policy_fetch hack. I don't have good ideas on how to address these
from
> the top of my head though.

One of the reasons why we do this is because I'm trying to keep as much of the
initialization code intact as possible while running unit tests. Previously,
lots of tests only worked because we were essentially not initializing the
BrowserPolicyConnector for most unit tests, so we're actually *closer* to
matching our production behavior now.

We could go back to not initializing the BPC again, or having an earlier short
circuit in BPC to have fewer short-circuits elsewhere. Unit tests just do some
really screwy stuff (one common technique is making a fake FILE thread that
shares the UI thread's non-IO MessageLoop) that can end up causing DCHECKs on
various platforms.

So, yeah, it's a tradeoff - we can run more init code in every unit test, but
have more short circuits, or run less init code and have an early short circuit.

[Andrew T Wilson (Slow), 2012-08-04 00:33:26]

Anyhow, as to how many tests - basically every test that uses a ProfileImpl
instead of a TestingProfile could conceivably hit this, since the ProfileImpl
constructor causes BPC::Init() to get called now.

[Andrew T Wilson (Slow), 2012-08-04 00:54:41]

Thanks for looking at this so quickly.

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

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:132: // AsyncPlatformLoader
requires deletion on the FILE thread.
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> In policy, we usually just spin up a fake FILE thread in the tests to keep the
> production code clean. How many tests are affected?

Basically, any test that results in a call to
g_browser_process->browser_policy_connector() would be affected. This includes
most of the policy stuff (like PolicyUITest, for example) and any test that
creates a ProfileImpl (I don't have an exhaustive list, since a crashing test
exits the unit tests).

Doing the fake FILE thread is not a panacea, because we still hit more DCHECKs
later when we initialize things like ConfigDirPolicyLoader and they expect their
"FILE" thread to have an IO-enabled MessageLoop. And again, I don't think it's a
good option to change every Profile-using test to have to fake up a FILE thread.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.cc:61: Profile* profile, bool
wait_for_policy_fetch) {
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> parameters on separate lines.

Done.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:44: virtual void
Initialize(PrefService* local_prefs,
As far as I can tell, the only thing local_state is used for is to pull in
kUserPolicyRefreshRate, which lives in the local prefs. Honestly, it probably
would be better to have the refresh rate passed in, rather than plumb the
local_prefs all the way down into CloudPolicyRefreshScheduler. What do you
think?

The prefs accessed by by the CloudPolicyStore are still the profile prefs (they
are pulled directly from the profile in Create()).

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_store_base.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_base.cc:9: #include
"chrome/browser/signin/signin_manager.h"
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> used?

Done.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_base.cc:11: using
enterprise_management::PolicyData;
We don't need the using directive, so I've removed it.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_store_base.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_base.h:11: #include
"base/compiler_specific.h"
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> needed?

Done.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_store_base.h:12: #include
"base/file_path.h"
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> needed?

Done.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:26: // TODO(atwilson): Move
this once we add OAuth token support to TokenService.
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> So we still don't have it? I was under the impression it got implemented, but
I
> may be wrong.

There are two separate but related issues:

a) Using ClientOAuth in SigninManager to allow logging in with an OTP. This
generates an OAuth token that we turn around and generate a sid/lsid token with,
and then fetch a bunch of old-style ClientLogin tokens for various services.
b) Switching over our chrome services to use OAuth tokens, and changing
TokenService to fetch/persist these OAuth tokens so each service doesn't have to
fetch their own.

a) is somewhat done, although disabled because the GAIA server-side isn't ready
to support it at scale yet.
b) is not done at all. Right now, even though we generate an OAuth login refresh
token, the TokenService doesn't automatically fetch these tokens for services
(since it only knows about ClientLogin), nor does it refresh them if they time
out, etc. So every service has to wrangle their own, which is inefficient and
fragile.

I'm talking about b) in this comment.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:38: Profile* profile,
UserCloudPolicyManager* manager)
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> arguments on separate lines (as per Chromium style)

Done.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:121:
manager_->Initialize(g_browser_process->local_state(),
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> I think you want the Profile's PrefService here to get the correct refresh
rate
> policy.

That pref is only registered as part of browser local state
(CloudPolicySubsystem::RegisterPrefs() is called from
BrowserPrefs::RegisterLocalState(), which means it doesn't exist on profile
prefs. We can move this to be per-profile in a separate CL if we think this
really should not be a global browser pref.

Anyhow, I think the right thing to do here is to pass in the refresh rate rather
than pass in prefs - that will make it simpler if we want to allow this to vary
per-profile, while still allowing us to reload device policy using browser
prefs?

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:30: #if !defined(OS_CHROMEOS)
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> Seems like this should rather use an exclude rule in chrome_browser.gypi?

I do exclude this via chrome_browser.gypi, but all that does is massage the
dependency generation/list of linked files (it doesn't prevent people from
directly #including it). The goal of adding the #if !defined() here is to cause
a compile (not link) error if someone accidentally includes this on cros, with
the expectation that those are easier to diagnose.

If you'd like me to remove this #if or if there's a better idiom to use
(COMPILE_ASSERT?) let me know.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service_unittest.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service_unittest.cc:36: class
TestingProfileWithUserCloudPolicyManager : public TestingProfile {
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> Used?

Good catch - this became obsolete when I added TestingProfile::Builder.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service_unittest.cc:53:
((TestingBrowserProcess*) g_browser_process)->SetLocalState(
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> Should use static_cast, but you may actually not need this if you switch to
the
> Profile's PrefService

Changed to use static_cast.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service_unittest.cc:171: 
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> remove trailing newline.

Done.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_member.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_member.cc:38:
DCHECK(prefs_->FindPreference(pref_name_.c_str())) <<
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> There seems to be the convention that the << operation should be on the next
> line:
> 
> $ git grep '<<$' -- *.cc | wc -l
> 275
> $ git grep '^\s\+<<' -- *.cc | wc -l
> 2212

Done.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:243:
OffTheRecordProfileImpl::GetUserCloudPolicyManager() {
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> indent by 4 (as elsewhere in this file).
Done. Hmm, first time I've seen that style in chrome, but it seems that we've
decided that this is indeed the right approach:

https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de...

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:298: policy::USER_AFFILIATION_MANAGED;
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> Ah, I still feel bad for this hack. It would be nice if we could pass the
> wait_for_policy_fetch flag on profile creation instead, not sure whether
that's
> possible though.
Well, ultimately PolicyService and UserCloudPolicyManager should be
ProfileKeyedServices - the problem is that they need to get initialized *before*
PrefService, which is not yet a ProfileKeyedService.

As an interim move, I will move all of this code into a
BrowserPolicyConnector::CreateUserCloudPolicyManager() function - it's not as
good as injecting these classes as constructor arguments, but I don't want to do
that work when the real long-term solution is to make them PKSes.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:307: // will cause it to get downloaded
eventually.
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> Not sure this is going to fly long term. If we force signing in to make sure
the
> user gets policy, the admin will also expect that settings relevant to startup
> (i.e. the RestoreOnStartup policy) takes effect. For that case, waiting for
the
> sign in and the policy fetch to complete before opening the browser window
might
> be the way to go.

Yeah, what we do here is very dependent on what kind of force-login UI we come
up with. Ideally, we could still just pass "wait_for_policy_fetch=false" to let
the Profile load up, then display some kind of blocking UI while we wait for the
policy to load - I think that's a better UX anyway, although it presumes that
Chrome can dynamically apply all of the policy updates without requiring a
restart (or at least the most important ones).
> 
> I think it's fair for now though to ignore the issue.

Agreed.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_mock.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_mock.cc:6: #include
"chrome/browser/prefs/pref_service_mock_builder.h"
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> not needed any longer?

Done.

http://codereview.chromium.org/10693022/diff/52055/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/common/pref_names.c...
chrome/common/pref_names.cc:1933: const char kLoadCloudPolicyOnSignin[] =
"policy.load_cloud_policy_on_signin";
On 2012/08/03 12:19:08, Mattias Nissler wrote:
> This (and the two above) doesn't belong here (this is the "SERVICE PREFS"
> section, but rather in the LOCAL STATE or PROFILE PREFS sections.
> 
> I'm fighting a losing battle against entropy in this file :)

Done. I didn't even realize there were different sections, and I'm surprised
that the service process cares about the policy refresh rate prefs.

I wonder if perhaps it makes sense to break this file up into multiple files if
we want to keep these separate.

[Mattias Nissler (ping if slow), 2012-08-06 08:33:52]

Regarding the tests vs. BPC::Init() vs. threading discussion: Previously, we'd
just not run any policy code in most tests. Now we do (and it doesn't crash),
but we will still run into subtle behavioral issues that may be much harder to
diagnose than tests that simply don't see policy. One example from the top of my
head be the RefreshPolicies() call, which will now never report completion due
to the non-existence of the FILE thread in most tests, so stuff may hang...

Anyhow, we can discuss this to death unless we find a way to bring up a fully
functional policy subsystem in tests (which we probably don't want to do
actually). For now, I'm OK with trying your approach of doing more policy
initialization in tests, we can still go back or do something entirely different
if that causes to many headaches.

LGTM

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:44: virtual void
Initialize(PrefService* local_prefs,
On 2012/08/04 00:54:41, Andrew T Wilson wrote:
> As far as I can tell, the only thing local_state is used for is to pull in
> kUserPolicyRefreshRate, which lives in the local prefs. 

Ah, correct, I was mistaken. FWIW, in the future we might have different refresh
rates for different Profile instances in case each Profile has its own policy.
In that case we'd have to read from the profile PrefService. Nothing to worry
about right now though.

Honestly, it probably
> would be better to have the refresh rate passed in, rather than plumb the
> local_prefs all the way down into CloudPolicyRefreshScheduler. What do you
> think?

You can do that as well, but then you need an external entity that reacts to
pref changes and updates the value.
 
> The prefs accessed by by the CloudPolicyStore are still the profile prefs
(they
> are pulled directly from the profile in Create()).

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:26: // TODO(atwilson): Move
this once we add OAuth token support to TokenService.
On 2012/08/04 00:54:41, Andrew T Wilson wrote:
> On 2012/08/03 12:19:08, Mattias Nissler wrote:
> > So we still don't have it? I was under the impression it got implemented,
but
> I
> > may be wrong.
> 
> There are two separate but related issues:
> 
> a) Using ClientOAuth in SigninManager to allow logging in with an OTP. This
> generates an OAuth token that we turn around and generate a sid/lsid token
with,
> and then fetch a bunch of old-style ClientLogin tokens for various services.
> b) Switching over our chrome services to use OAuth tokens, and changing
> TokenService to fetch/persist these OAuth tokens so each service doesn't have
to
> fetch their own.
> 
> a) is somewhat done, although disabled because the GAIA server-side isn't
ready
> to support it at scale yet.
> b) is not done at all. Right now, even though we generate an OAuth login
refresh
> token, the TokenService doesn't automatically fetch these tokens for services
> (since it only knows about ClientLogin), nor does it refresh them if they time
> out, etc. So every service has to wrangle their own, which is inefficient and
> fragile.
> 
> I'm talking about b) in this comment.

Ah, thanks for the clarification!

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.cc:121:
manager_->Initialize(g_browser_process->local_state(),
On 2012/08/04 00:54:41, Andrew T Wilson wrote:
> On 2012/08/03 12:19:08, Mattias Nissler wrote:
> > I think you want the Profile's PrefService here to get the correct refresh
> rate
> > policy.
> 
> That pref is only registered as part of browser local state
> (CloudPolicySubsystem::RegisterPrefs() is called from
> BrowserPrefs::RegisterLocalState(), which means it doesn't exist on profile
> prefs. We can move this to be per-profile in a separate CL if we think this
> really should not be a global browser pref.

Agree (see also other comment).

> Anyhow, I think the right thing to do here is to pass in the refresh rate
rather
> than pass in prefs - that will make it simpler if we want to allow this to
vary
> per-profile, while still allowing us to reload device policy using browser
> prefs?

Device policy could continue using local_state. Also, as mentioned elsewhere, we
want to be able to pick up changes to the pref. If you pass in the refresh rate,
you'll have to handle that separately.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:30: #if !defined(OS_CHROMEOS)
On 2012/08/04 00:54:41, Andrew T Wilson wrote:
> On 2012/08/03 12:19:08, Mattias Nissler wrote:
> > Seems like this should rather use an exclude rule in chrome_browser.gypi?
> 
> I do exclude this via chrome_browser.gypi, but all that does is massage the
> dependency generation/list of linked files (it doesn't prevent people from
> directly #including it). The goal of adding the #if !defined() here is to
cause
> a compile (not link) error if someone accidentally includes this on cros, with
> the expectation that those are easier to diagnose.

Ah, that's definitely an interesting idea. Then again, we have lots of code
that's cros-only or non-cros (respectively), and I can't remember having seen
guards like this. Are they common in other parts of the code base?
(Unsignificant) data point: I personally never ran into a situation where I
tried to use something that I shouldn't have been using since it's non-cros or
cros-only.

If we start doing this, we should probably start a concerted effort to convert
over at least all of chrome/browser/policy to this style.

> 
> If you'd like me to remove this #if or if there's a better idiom to use
> (COMPILE_ASSERT?) let me know.

I'd remove it for consistency for now, unless you can point me at precedents in
the code base where this is commonly used.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:307: // will cause it to get downloaded
eventually.
On 2012/08/04 00:54:41, Andrew T Wilson wrote:
> On 2012/08/03 12:19:08, Mattias Nissler wrote:
> > Not sure this is going to fly long term. If we force signing in to make sure
> the
> > user gets policy, the admin will also expect that settings relevant to
startup
> > (i.e. the RestoreOnStartup policy) takes effect. For that case, waiting for
> the
> > sign in and the policy fetch to complete before opening the browser window
> might
> > be the way to go.
> 
> Yeah, what we do here is very dependent on what kind of force-login UI we come
> up with. Ideally, we could still just pass "wait_for_policy_fetch=false" to
let
> the Profile load up, then display some kind of blocking UI while we wait for
the
> policy to load - I think that's a better UX anyway, although it presumes that
> Chrome can dynamically apply all of the policy updates without requiring a
> restart (or at least the most important ones).

Reapplying policy when it changes is generally our goal when writing policy
code, so this might actually be OK. As I said, I'm more concerned with policies
that influence startup behavior.

> > 
> > I think it's fair for now though to ignore the issue.
> 
> Agreed.
>

http://codereview.chromium.org/10693022/diff/52055/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/common/pref_names.c...
chrome/common/pref_names.cc:1933: const char kLoadCloudPolicyOnSignin[] =
"policy.load_cloud_policy_on_signin";
On 2012/08/04 00:54:41, Andrew T Wilson wrote:
> On 2012/08/03 12:19:08, Mattias Nissler wrote:
> > This (and the two above) doesn't belong here (this is the "SERVICE PREFS"
> > section, but rather in the LOCAL STATE or PROFILE PREFS sections.
> > 
> > I'm fighting a losing battle against entropy in this file :)
> 
> Done. I didn't even realize there were different sections, and I'm surprised
> that the service process cares about the policy refresh rate prefs.

It doesn't, they're in the wrong location, and it was probably my mistake :)


> 
> I wonder if perhaps it makes sense to break this file up into multiple files
if
> we want to keep these separate.

That's one option. A good start would be to restore order in this file, but
that's a non-trivial task already :-/

[Andrew T Wilson (Slow), 2012-08-06 20:30:23]

erg: PTAL browser/profiles + testing_profile.*
sky: PTAL chrome/ and chrome/test
jhawkins: PTAL chrome/browser/ui/webui
nkostylev: PTAL chrome/browser/chromeos/login

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_cloud_policy_manager.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_cloud_policy_manager.h:44: virtual void
Initialize(PrefService* local_prefs,
On 2012/08/06 08:33:52, Mattias Nissler wrote:
> Honestly, it probably
> > would be better to have the refresh rate passed in, rather than plumb the
> > local_prefs all the way down into CloudPolicyRefreshScheduler. What do you
> > think?
> 
> You can do that as well, but then you need an external entity that reacts to
> pref changes and updates the value.

Hmmm. Good point (or we could ignore policy polling frequency changes until the
next restart). But I agree we're fine for now.

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
File chrome/browser/policy/user_policy_signin_service.h (right):

http://codereview.chromium.org/10693022/diff/52055/chrome/browser/policy/user...
chrome/browser/policy/user_policy_signin_service.h:30: #if !defined(OS_CHROMEOS)
On 2012/08/06 08:33:52, Mattias Nissler wrote:
> On 2012/08/04 00:54:41, Andrew T Wilson wrote:
> > On 2012/08/03 12:19:08, Mattias Nissler wrote:
> > > Seems like this should rather use an exclude rule in chrome_browser.gypi?
> > 
> > I do exclude this via chrome_browser.gypi, but all that does is massage the
> > dependency generation/list of linked files (it doesn't prevent people from
> > directly #including it). The goal of adding the #if !defined() here is to
> cause
> > a compile (not link) error if someone accidentally includes this on cros,
with
> > the expectation that those are easier to diagnose.
> 
> Ah, that's definitely an interesting idea. Then again, we have lots of code
> that's cros-only or non-cros (respectively), and I can't remember having seen
> guards like this. Are they common in other parts of the code base?
> (Unsignificant) data point: I personally never ran into a situation where I
> tried to use something that I shouldn't have been using since it's non-cros or
> cros-only.
> 
> If we start doing this, we should probably start a concerted effort to convert
> over at least all of chrome/browser/policy to this style.
> 
> > 
> > If you'd like me to remove this #if or if there's a better idiom to use
> > (COMPILE_ASSERT?) let me know.
> 
> I'd remove it for consistency for now, unless you can point me at precedents
in
> the code base where this is commonly used.

OK, removed.

[James Hawkins, 2012-08-06 21:17:51]

webui LGTM

[Elliot Glaysher, 2012-08-06 22:22:09]

lgtm I guess.

[sky, 2012-08-06 22:28:27]

http://codereview.chromium.org/10693022/diff/46070/chrome/test/base/testing_p...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/10693022/diff/46070/chrome/test/base/testing_p...
chrome/test/base/testing_profile.h:124: TestingProfile(const FilePath& path,
We already have so many constructors in this class. It is possible to nuke this
and force creation to go through Build. Build in term would use one of the
existing constructors and set up whatever state it needs to.

[Andrew T Wilson (Slow), 2012-08-06 23:12:25]

http://codereview.chromium.org/10693022/diff/46070/chrome/test/base/testing_p...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/10693022/diff/46070/chrome/test/base/testing_p...
chrome/test/base/testing_profile.h:124: TestingProfile(const FilePath& path,
On 2012/08/06 22:28:27, sky wrote:
> We already have so many constructors in this class. It is possible to nuke
this
> and force creation to go through Build. Build in term would use one of the
> existing constructors and set up whatever state it needs to.

I have a TODO to do exactly this (see above), but I want to do it in a separate
CL since this one is already too unwieldy.

[Andrew T Wilson (Slow), 2012-08-06 23:59:14]

sky: let me know if this does not address your concerns.

http://codereview.chromium.org/10693022/diff/46070/chrome/test/base/testing_p...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/10693022/diff/46070/chrome/test/base/testing_p...
chrome/test/base/testing_profile.h:124: TestingProfile(const FilePath& path,
On 2012/08/06 23:12:25, Andrew T Wilson wrote:
> On 2012/08/06 22:28:27, sky wrote:
> > We already have so many constructors in this class. It is possible to nuke
> this
> > and force creation to go through Build. Build in term would use one of the
> > existing constructors and set up whatever state it needs to.
> 
To be clear, my intent is to remove all other constructors, have all use go
through Builder::Build(), and move this constructor to be private/only
accessible to Builder.

[sky, 2012-08-07 00:55:43]

TODO sg, LGTM

[Nikita (slow), 2012-08-07 16:35:09]

http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:500: Profile* user_profile =
ProfileManager::GetDefaultProfile();
Please make sure that you're grabbing the correct profile.
Default profile path has been switched already?

http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/policy_oauth_fetcher.cc (right):

http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/policy_oauth_fetcher.cc:104:
ProfileManager::GetDefaultProfile()->GetUserCloudPolicyManager();
User's default profile in context of Chrome OS when sign in hasn't finished yet
is "login Default profile".
Are you sure that it is the one that you need?

[Andrew T Wilson (Slow), 2012-08-07 16:55:44]

http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:500: Profile* user_profile =
ProfileManager::GetDefaultProfile();
On 2012/08/07 16:35:09, Nikita Kostylev wrote:
> Please make sure that you're grabbing the correct profile.
> Default profile path has been switched already?

Yes. Before we call this, the code above calls UserManager::UserLoggedIn() which
sends out the notification which ProfileManager uses to switch to the correct
user-data-dir.

http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/policy_oauth_fetcher.cc (right):

http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/policy_oauth_fetcher.cc:104:
ProfileManager::GetDefaultProfile()->GetUserCloudPolicyManager();
On 2012/08/07 16:35:09, Nikita Kostylev wrote:
> User's default profile in context of Chrome OS when sign in hasn't finished
yet
> is "login Default profile".
> Are you sure that it is the one that you need?

Yes. Once we've called UserManager->UserLoggedIn(), it's safe to invoke this.
The PolicyOAuthFetcher is only invoked after the user has logged in (either at
the end of LoginUtilsImpl::PrepareProfile() after UserLoggedIn() has already
been invoked and the UserCloudPolicyManager has been initialized, or
LoginUtilsImpl::OnProfileCreated() is invoked (again, after UserLoggedIn() has
been called).

[Nikita (slow), 2012-08-07 17:03:58]

That's right, thanks for confirming.

lgtm

On 2012/08/07 16:55:44, Andrew T Wilson wrote:
>
http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
> File chrome/browser/chromeos/login/login_utils.cc (right):
> 
>
http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
> chrome/browser/chromeos/login/login_utils.cc:500: Profile* user_profile =
> ProfileManager::GetDefaultProfile();
> On 2012/08/07 16:35:09, Nikita Kostylev wrote:
> > Please make sure that you're grabbing the correct profile.
> > Default profile path has been switched already?
> 
> Yes. Before we call this, the code above calls UserManager::UserLoggedIn()
which
> sends out the notification which ProfileManager uses to switch to the correct
> user-data-dir.
> 
>
http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
> File chrome/browser/chromeos/login/policy_oauth_fetcher.cc (right):
> 
>
http://codereview.chromium.org/10693022/diff/46070/chrome/browser/chromeos/lo...
> chrome/browser/chromeos/login/policy_oauth_fetcher.cc:104:
> ProfileManager::GetDefaultProfile()->GetUserCloudPolicyManager();
> On 2012/08/07 16:35:09, Nikita Kostylev wrote:
> > User's default profile in context of Chrome OS when sign in hasn't finished
> yet
> > is "login Default profile".
> > Are you sure that it is the one that you need?
> 
> Yes. Once we've called UserManager->UserLoggedIn(), it's safe to invoke this.
> The PolicyOAuthFetcher is only invoked after the user has logged in (either at
> the end of LoginUtilsImpl::PrepareProfile() after UserLoggedIn() has already
> been invoked and the UserCloudPolicyManager has been initialized, or
> LoginUtilsImpl::OnProfileCreated() is invoked (again, after UserLoggedIn() has
> been called).

[commit-bot: I haz the power, 2012-08-07 17:06:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/10693022/46070

[commit-bot: I haz the power, 2012-08-07 18:50:10]

Change committed as 150380