UserPolicyCache only becomes ready after policy has been fetched.

UserPolicyCache only becomes ready after policy has been fetched.

This behavior is only forced on enterprise devices, and for logged in users whose username matches the enrollment domain.

This requires waiting for a couple of things:
- oauth token fetching
- dm server register token fetching
- user policy fetching

The cache resumes initialization too if any of these steps fails. Fetching of OAuth tokens usually happens after Profile creation, but in this case it will be done earlier so that the policy fetching can proceed before the Profile is fully created.

This fixes the races between policy and sync, startup pages, etc.

BUG=chromium-os:17698
TEST=Logins of enterprise users on enrolled ChromeOS devices waits for a policy fetch before proceeding.

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

[Joao da Silva, 2011-11-10 14:03:12]

Hi all, please have a look.

@mnissler: policy/ changes, overall thing

@zelidrag: your name is all over login_utils.cc, and particularly on the
PolicyOAuthFetcher; can you have a look, or point me to someone who can review
those changes?

@nkostylev: owner review for chromeos/login; please have a look at those changes
too

I'm still writing unit tests and possibly an end-to-end test for these changes,
but I'd like some early feedback on the overall direction.

Thanks!

[Mattias Nissler (ping if slow), 2011-11-11 11:41:23]

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:582: scoped_ptr<PolicyOAuthFetcher>
policy_oauth_early_fetcher_;
I don't see why we need two pointers here, wouldn't one do?

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:1147: LOG(WARNING) << "Failed
fetching OAuth request token";
This logged error.state() before, why did you drop this? It might also be
helpful to log more info about the fetch errors in the various LOG(WARNING)
incantations above.

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.h:102: // creation until a user
policy fetch is performed.
This return value is easy to miss and the results may be quite fatal ;) I'd
rather see a GetUserAffiliation() call from LoginUtils, make the decision there
and then call a to-be-created SetWaitForUserPolicy().

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_cache.cc:80: VLOG(1) << "SetFetchingDone,
cache is now fetch_ready_";
all VLOGs should probably be DVLOGs per that recent discussion on chromium-dev.

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_cache_unittest.cc (right):

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_cache_unittest.cc:381: UserPolicyCache
cache(test_file(), false  /* wait_for_policy_fetch */);
It might be good to also have a test that passes the true flag and checks that
IsReady() behaves accordingly.

[Joao da Silva, 2011-11-11 12:55:14]

@mnissler: PTAL

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:582: scoped_ptr<PolicyOAuthFetcher>
policy_oauth_early_fetcher_;
On 2011/11/11 11:41:23, Mattias Nissler wrote:
> I don't see why we need two pointers here, wouldn't one do?

The 2nd pointer was used to distinguish early policy token fetches from those
done later or triggered through StartTokenServices. However, this fetch only has
to be performed once, so I've changed this to use only one fetcher now.

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:1147: LOG(WARNING) << "Failed
fetching OAuth request token";
On 2011/11/11 11:41:23, Mattias Nissler wrote:
> This logged error.state() before, why did you drop this? It might also be
> helpful to log more info about the fetch errors in the various LOG(WARNING)
> incantations above.

Done.

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.h:102: // creation until a user
policy fetch is performed.
On 2011/11/11 11:41:23, Mattias Nissler wrote:
> This return value is easy to miss and the results may be quite fatal ;) I'd
> rather see a GetUserAffiliation() call from LoginUtils, make the decision
there
> and then call a to-be-created SetWaitForUserPolicy().

Done, in a slightly different form. The decision is made in LoginUtils, but is
passed into this function as a parameter. This is required when the
UserPolicyCache is created.

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_cache.cc:80: VLOG(1) << "SetFetchingDone,
cache is now fetch_ready_";
On 2011/11/11 11:41:23, Mattias Nissler wrote:
> all VLOGs should probably be DVLOGs per that recent discussion on
chromium-dev.

Done.

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_cache_unittest.cc (right):

http://codereview.chromium.org/8499021/diff/2001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_cache_unittest.cc:381: UserPolicyCache
cache(test_file(), false  /* wait_for_policy_fetch */);
On 2011/11/11 11:41:23, Mattias Nissler wrote:
> It might be good to also have a test that passes the true flag and checks that
> IsReady() behaves accordingly.

Done.

[Mattias Nissler (ping if slow), 2011-11-11 14:38:50]

LGTM from the policy side if you address my nits.

Zel, can you please take a look at the login_utils changes?

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:664: // Profile creation is also
resume if the fetch attempt fails.
s/resume/resumed/

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:279: :
kServiceInitializationStartupDelay;
never seen this style :) I must say I tend to like it, but I think breaking
after = is more common.

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher_unittest.cc:141:
TEST_F(DeviceTokenFetcherTest, DontSetFetchingDone0) {
more descriptive names.

[Joao da Silva, 2011-11-14 15:59:48]

Added login_utils_unittest.cc.

@mnissler: fixed nits; mind to have a look at the new test too? :-)

@zel: friendly ping :-)

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:664: // Profile creation is also
resume if the fetch attempt fails.
On 2011/11/11 14:38:50, Mattias Nissler wrote:
> s/resume/resumed/

Done.

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:279: :
kServiceInitializationStartupDelay;
On 2011/11/11 14:38:50, Mattias Nissler wrote:
> never seen this style :) I must say I tend to like it, but I think breaking
> after = is more common.

I like it too :-) Using a line break after = now.

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/8499021/diff/7005/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher_unittest.cc:141:
TEST_F(DeviceTokenFetcherTest, DontSetFetchingDone0) {
On 2011/11/11 14:38:50, Mattias Nissler wrote:
> more descriptive names.

Done.

[Nikita (slow), 2011-11-14 16:02:08]

On 2011/11/14 15:59:48, Joao da Silva wrote:
> Added login_utils_unittest.cc.

This should be called login_utils_browsertest.cc

[Nikita (slow), 2011-11-14 16:21:28]

We're thinking of resurrecting ClientLogin code path for some of the enterprise
users.
How would policy fetch would behave in that case? Is the previous code that was
fetching policy in ClientLogin auth case would still work?

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:659: // If this is an enterprise
device and the user belongs to the enterprise
This code path is used when we log in with the existing users too and in offline
case.
What will happen with policy fetcher?

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:681:
policy_oauth_fetcher_->Start();
PolicyOAuthFetcher::Start() will fetch in async mode. How that would delay
Profile creation which is next line after this block?

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:1252: // If we come online for the
first time after successful offline login,
Should you refetch policy tokens here too?
Applies for the case when existing user signed in when being offline.

[Joao da Silva, 2011-11-14 17:19:08]

@nkostylev: The ClientLogin path won't block with these changes, but if it is
introduced again I'll make a similar CL for that. Please have a look at the
comments inline.

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:659: // If this is an enterprise
device and the user belongs to the enterprise
On 2011/11/14 16:21:28, Nikita Kostylev wrote:
> This code path is used when we log in with the existing users too and in
offline
> case.
> What will happen with policy fetcher?

It will try to fetch the dmservice token again, but for existing users it can be
already locally cached. If the cache loads it, the policy subsystem will use it
and unblock login earlier. The fetch will refresh the token, if it succeeds.

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:681:
policy_oauth_fetcher_->Start();
On 2011/11/14 16:21:28, Nikita Kostylev wrote:
> PolicyOAuthFetcher::Start() will fetch in async mode. How that would delay
> Profile creation which is next line after this block?

Profile creation blocks on its PrefService becoming ready, which blocks on its
PrefStores. The PolicyPrefStore is the one that will not become ready until the
policy is fetched. Once the policy is fetched (or an error occurs), that
PrefStore will become ready and the rest of Profile creation will resume. The
relevant code for this is ProfileImpl::OnPrefsChanged, which is triggered from a
notification of NOTIFICATION_PREF_INITIALIZATION_COMPLETED.

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:1252: // If we come online for the
first time after successful offline login,
On 2011/11/14 16:21:28, Nikita Kostylev wrote:
> Should you refetch policy tokens here too?
> Applies for the case when existing user signed in when being offline.

Probably a good idea. The token only has to be fetched once, and it is then
immediately used to fetch yet another token from the DM service; it isn't needed
anymore once the DMToken is available. However, we should retry if the first
fetch fails.

I've made FetchPolicyToken() retry if the current fetcher doesn't have a policy
token. Notice that this method ends up calling FetchPolicyToken through
VerifyOAuth1AccessToken and FetchSecondaryTokens.

[Nikita (slow), 2011-11-15 09:32:16]

Please run linux_chromeos_clang trybot.

[Joao da Silva, 2011-11-15 11:55:17]

On 2011/11/15 09:32:16, Nikita Kostylev wrote:
> Please run linux_chromeos_clang trybot.

Done.

[Nikita (slow), 2011-11-15 12:09:23]

Zel, please take a quick look if I have overlooked something.

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/19007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:1252: // If we come online for the
first time after successful offline login,
On 2011/11/14 17:19:09, Joao da Silva wrote:
> On 2011/11/14 16:21:28, Nikita Kostylev wrote:
> > Should you refetch policy tokens here too?
> > Applies for the case when existing user signed in when being offline.
> 
> Probably a good idea. The token only has to be fetched once, and it is then
> immediately used to fetch yet another token from the DM service; it isn't
needed
> anymore once the DMToken is available. However, we should retry if the first
> fetch fails.
> 
> I've made FetchPolicyToken() retry if the current fetcher doesn't have a
policy
> token. Notice that this method ends up calling FetchPolicyToken through
> VerifyOAuth1AccessToken and FetchSecondaryTokens.

Ok, it will also be called on crash.
http://codereview.chromium.org/8537002/diff/13001/chrome/browser/chromeos/log...

http://codereview.chromium.org/8499021/diff/17009/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/17009/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:1236:
policy_oauth_fetcher_->policy_token().empty()) {
Could we distinguish these cases:

1. Policy token fetch hasn't been started > !policy_oauth_fetcher_.get()
2. Policy token fetch is in progress and we should restart it
3. Policy token fetch has failed > retry?

[Nikita (slow), 2011-11-15 12:16:19]

http://codereview.chromium.org/8499021/diff/17009/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/17009/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:1236:
policy_oauth_fetcher_->policy_token().empty()) {
On 2011/11/15 12:09:23, Nikita Kostylev wrote:
> Could we distinguish these cases:
> 
> 1. Policy token fetch hasn't been started > !policy_oauth_fetcher_.get()
> 2. Policy token fetch is in progress and we should restart it

should NOT restart it.

> 3. Policy token fetch has failed > retry?

[Joao da Silva, 2011-11-15 14:35:56]

@Nikita: changed to prevent new fetches when there are pending fetches.

http://codereview.chromium.org/8499021/diff/17009/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/8499021/diff/17009/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:1236:
policy_oauth_fetcher_->policy_token().empty()) {
On 2011/11/15 12:16:20, Nikita Kostylev wrote:
> On 2011/11/15 12:09:23, Nikita Kostylev wrote:
> > Could we distinguish these cases:
> > 
> > 1. Policy token fetch hasn't been started > !policy_oauth_fetcher_.get()
> > 2. Policy token fetch is in progress and we should restart it
> 
> should NOT restart it.
> 
> > 3. Policy token fetch has failed > retry?
> 

Done. This method makes a new fetch if it hasn't been done yet (non-enterprise
devices), or if the current fetcher has finished (doesn't have pending fetches)
but didn't get a token.

[Nikita (slow), 2011-11-15 15:31:20]

lgtm

[zel, 2011-11-15 15:43:02]

lgtm

[commit-bot: I haz the power, 2011-11-15 16:20:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8499021/31001

[commit-bot: I haz the power, 2011-11-15 17:29:31]

Change committed as 110108

[jochen (gone - plz use gerrit), 2012-11-13 13:25:03]

https://codereview.chromium.org/8499021/diff/31001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/8499021/diff/31001/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:2430:
'browser/chromeos/login/login_utils_browsertest.cc',
why was the file missing?

[jochen (gone - plz use gerrit), 2012-11-13 13:25:30]

disregard my comment, wrong CL :(