Make an online wildcard login check for enterprise devices.

Make an online wildcard login check for enterprise devices.

This checks with GAIA to reconfirm whether a given user is permitted
to log in to a device. The check complements the existing login
whitelist check based on device policy.

BUG=chromium:336284
TEST=Manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247829

[Mattias Nissler (ping if slow), 2014-01-28 12:44:51]

Guys, here's the next attempt. Can you review?

[Joao da Silva, 2014-01-28 14:45:45]

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_performer.cc:178: (new
policy::WildcardLoginChecker())->Start(
I've found code like this hard to get right in tests in the past, because the
instance is still live after the test ends and a leak is reported (unless the
test adds more boilerplate to make sure this check is done).

I'd just have LoginPerformer own the checker.

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_performer.cc:367: }  // namespace chromeos
nit: add a newline

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h:15: #include
"base/memory/weak_ptr.h"
Used?

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:210:
}
It's not clear to me why we do this. Is this meant to handle users that somehow
passed through login (or were already added to the device)?

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc
(right):

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc:80: 
We never revoke this token; I don't know if the ProfileOAuth2TokenService takes
care of that for us. If it doesn't then this may hit a token limit.

[Mattias Nissler (ping if slow), 2014-01-28 15:44:22]

Addressed Joao's comments.

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_performer.cc:178: (new
policy::WildcardLoginChecker())->Start(
On 2014/01/28 14:45:46, Joao da Silva wrote:
> I've found code like this hard to get right in tests in the past, because the
> instance is still live after the test ends and a leak is reported (unless the
> test adds more boilerplate to make sure this check is done).
> 
> I'd just have LoginPerformer own the checker.

All right, converting.

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_performer.cc:367: }  // namespace chromeos
On 2014/01/28 14:45:46, Joao da Silva wrote:
> nit: add a newline

Done.

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h:15: #include
"base/memory/weak_ptr.h"
On 2014/01/28 14:45:46, Joao da Silva wrote:
> Used?

Done.

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:210:
}
On 2014/01/28 14:45:46, Joao da Silva wrote:
> It's not clear to me why we do this. Is this meant to handle users that
somehow
> passed through login (or were already added to the device)?

Yes, this is a best-effort attempt at catching already-existing accounts for the
case where the whitelist changes.

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc
(right):

https://codereview.chromium.org/148843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc:80: 
On 2014/01/28 14:45:46, Joao da Silva wrote:
> We never revoke this token; I don't know if the ProfileOAuth2TokenService
takes
> care of that for us. If it doesn't then this may hit a token limit.

I don't think that's a problem here, ProfileOAuth2TokenService caches existing
tokens instead of recreating them all the time. Also, we're talking about an
access token here, which has limited validity time (60 min IIRC).

[Joao da Silva, 2014-01-28 15:47:46]

lgtm

https://codereview.chromium.org/148843002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_performer.h (right):

https://codereview.chromium.org/148843002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_performer.h:154:
scoped_ptr<policy::WildcardLoginChecker> wildcard_login_checker_;
#include scoped_ptr.h

[Andrew T Wilson (Slow), 2014-01-28 16:31:13]

lgtm

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

https://codereview.chromium.org/148843002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:165:
wildcard_username_ = username;
So, this breaks if for some reason EnableWildcardLoginCheck() is called after
the access token is available. I don't think it's possible for this to happen,
but you should probably add a DCHECK(access_token_.empty()) here to ensure it.

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

https://codereview.chromium.org/148843002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc:82:
token_service_->GetPrimaryAccountId()))
nit: just noticed indentation is wrong here.

[xiyuan, 2014-01-29 03:45:08]

lgtm

https://codereview.chromium.org/148843002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/wildcard_login_checker.cc (right):

https://codereview.chromium.org/148843002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/wildcard_login_checker.cc:59: LOG(ERROR) <<
"Failed to fetch policy token " << error.ToString();
nit: "Failed to get user info " since it's not about fetching policy token.

[Mattias Nissler (ping if slow), 2014-01-29 12:12:31]

Thanks for the reviews. I'm going to land this once I've completed a round of
testing with the latest version.

https://codereview.chromium.org/148843002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_performer.h (right):

https://codereview.chromium.org/148843002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_performer.h:154:
scoped_ptr<policy::WildcardLoginChecker> wildcard_login_checker_;
On 2014/01/28 15:47:46, Joao da Silva wrote:
> #include scoped_ptr.h

Done.

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

https://codereview.chromium.org/148843002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:165:
wildcard_username_ = username;
On 2014/01/28 16:31:13, Andrew T Wilson wrote:
> So, this breaks if for some reason EnableWildcardLoginCheck() is called after
> the access token is available. I don't think it's possible for this to happen,
> but you should probably add a DCHECK(access_token_.empty()) here to ensure it.

Done.

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

https://codereview.chromium.org/148843002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.cc:82:
token_service_->GetPrimaryAccountId()))
On 2014/01/28 16:31:13, Andrew T Wilson wrote:
> nit: just noticed indentation is wrong here.

This looks perfectly fine to me, and clang-format concurs.

https://codereview.chromium.org/148843002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/wildcard_login_checker.cc (right):

https://codereview.chromium.org/148843002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/wildcard_login_checker.cc:59: LOG(ERROR) <<
"Failed to fetch policy token " << error.ToString();
On 2014/01/29 03:45:09, xiyuan wrote:
> nit: "Failed to get user info " since it's not about fetching policy token.

Doh. Done.

[commit-bot: I haz the power, 2014-01-29 15:28:19]

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

[commit-bot: I haz the power, 2014-01-30 04:12:48]

Change committed as 247829