ChromeOS: user whitelist check should happen before entering password.

ChromeOS: user whitelist check should happen before entering password.

BUG=153241
TEST=manual

Committed: https://crrev.com/6d8a37d4cd6269010ad55e2dd9f937be1e3c5d43
Cr-Commit-Position: refs/heads/master@{#338590}

[Alexander Alekseev, 2015-06-18 04:46:59]

alemate@chromium.org changed reviewers:
+ antrim@chromium.org, xiyuan@chromium.org

[Alexander Alekseev, 2015-06-18 04:47:00]

Please review.

xiyuan@ : chrome/browser/resources/gaia_auth_host/authenticator.js

antrim@ : all

[Denis Kuznetsov (DE-MUC), 2015-06-18 11:13:31]

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:460: return
ChromeLoginPerformer(this).IsUserWhitelisted(user_id, &wildcard_match);
What is the case when we will not have login_performer_ at this point?
Does it really makes sense to create new object for just one check?

[Alexander Alekseev, 2015-06-19 00:54:08]

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:460: return
ChromeLoginPerformer(this).IsUserWhitelisted(user_id, &wildcard_match);
On 2015/06/18 11:13:31, Denis Kuznetsov wrote:
> What is the case when we will not have login_performer_ at this point?
> Does it really makes sense to create new object for just one check?

login_performer is created only in PerformLogin(), and should be used for one
user only. So at this point (before user has entered password), login_performer
is NULL.

I've added re-use of existing login_performer just in case this method would be
called from somewhere during active login process.

[Denis Kuznetsov (DE-MUC), 2015-06-25 19:21:11]

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:460: return
ChromeLoginPerformer(this).IsUserWhitelisted(user_id, &wildcard_match);
On 2015/06/19 00:54:08, Alexander Alekseev wrote:
> On 2015/06/18 11:13:31, Denis Kuznetsov wrote:
> > What is the case when we will not have login_performer_ at this point?
> > Does it really makes sense to create new object for just one check?
> 
> login_performer is created only in PerformLogin(), and should be used for one
> user only. So at this point (before user has entered password),
login_performer
> is NULL.
> 
> I've added re-use of existing login_performer just in case this method would
be
> called from somewhere during active login process.

First, ChromeLoginPerfomer is somewhat large object to create (it has
not-so-obvious constructor).
Second, this approach is incorrect - if you create LoginPerformer in this way
there is a chance that it will not load policy, and you will get wrong results.
Third, this example shows that ChomeLoginPerformer is a wrong place for 
IsUserWhitelisted check - UserManager seem to be a more appropriate class for
that.

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:439:
ShowWhitelistCheckFailedError()
";" is missing

[Alexander Alekseev, 2015-07-11 01:02:05]

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:460: return
ChromeLoginPerformer(this).IsUserWhitelisted(user_id, &wildcard_match);
On 2015/06/25 19:21:11, Denis Kuznetsov wrote:
> On 2015/06/19 00:54:08, Alexander Alekseev wrote:
> > On 2015/06/18 11:13:31, Denis Kuznetsov wrote:
> > > What is the case when we will not have login_performer_ at this point?
> > > Does it really makes sense to create new object for just one check?
> > 
> > login_performer is created only in PerformLogin(), and should be used for
one
> > user only. So at this point (before user has entered password),
> login_performer
> > is NULL.
> > 
> > I've added re-use of existing login_performer just in case this method would
> be
> > called from somewhere during active login process.
> 
> First, ChromeLoginPerfomer is somewhat large object to create (it has
> not-so-obvious constructor).
> Second, this approach is incorrect - if you create LoginPerformer in this way
> there is a chance that it will not load policy, and you will get wrong
results.
> Third, this example shows that ChomeLoginPerformer is a wrong place for 
> IsUserWhitelisted check - UserManager seem to be a more appropriate class for
> that.

components/ cannot include chrome/ . But the check can be directly here.

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1183893005/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:439:
ShowWhitelistCheckFailedError()
On 2015/06/25 19:21:11, Denis Kuznetsov wrote:
> ";" is missing

Done.

[Alexander Alekseev, 2015-07-11 01:02:54]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-11 01:03:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1183893005/20001

[commit-bot: I haz the power, 2015-07-11 01:41:16]

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

[commit-bot: I haz the power, 2015-07-11 01:41:17]

Dry run: This issue passed the CQ dry run.

[Denis Kuznetsov (DE-MUC), 2015-07-13 11:30:49]

lgtm

[Alexander Alekseev, 2015-07-13 21:26:23]

xiyuan@, Please review.

[xiyuan, 2015-07-13 21:30:39]

lgtm

I thought I've already stamped it. Sorry about that.

[Alexander Alekseev, 2015-07-13 22:01:51]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-07-13 22:03:26]

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

[commit-bot: I haz the power, 2015-07-13 23:37:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-13 23:38:44]

Patchset 2 (id:??) landed as
https://crrev.com/6d8a37d4cd6269010ad55e2dd9f937be1e3c5d43
Cr-Commit-Position: refs/heads/master@{#338590}