Prevent login while cros settings are untrusted

Prevent login while cros settings are untrusted

This CL is a prerequisite for device disabling:

If a device has been disabled by its owner, a corresponding bit will be
set in cros settings. If that bit is set, the device should show a
warning screen and prevent any sessions from being started. We could
guarantee this by waiting for cros settings to become trusted and checking
the bit before showing the login screen. However, that would increase
the time-to-login-screen for all users. This CL takes a different
approach. It allows the login screen to show but ensures that any login
attempts made will wait until the cros settings have become trusted
(checking for the device disabled bit will be added in a follow-up CL).

To ensure that no login path bypasses this check, the various Login*()
methods are made private, forcing everyone to go through the central,
publich Login() method.

BUG=425574
TEST=Added browser tests for all account types

Committed: https://crrev.com/a92230048d293d6d5c8f49b940e91bd6bec3f2a5
Cr-Commit-Position: refs/heads/master@{#303073}

[bartfab (slow), 2014-11-03 14:44:55]

bartfab@chromium.org changed reviewers:
+ antrim@chromium.org, mnissler@chromium.org, rkc@chromium.org

[bartfab (slow), 2014-11-03 14:44:55]

Hi Denis,

Could you take a look at:
chrome/browser/chromeos/login/*

Hi Rahul,

Could you take a look at:
chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc

Hi Mattias,

Could you take a look at:
chrome/browser/chromeos/policy/device_local_account_browsertest.cc

[rkc, 2014-11-03 22:42:50]

lgtm

chrome/browser/chromeos/kiosk_mode/kiosk_mode_screensaver.cc

lgtm

[Mattias Nissler (ping if slow), 2014-11-04 08:38:52]

device_local_account_browsertest.cc LGTM

On a related note: Are you positive we haven't waited for trusted settings
before logging in? If so, that'd be a security regression. I know the code to
wait for a trusted login whitelist existed a long while ago. If it's still
present, you might now want to remove redundancy. If it's no longer present,
please file a security bug.

[bartfab (slow), 2014-11-04 08:51:06]

On 2014/11/04 08:38:52, Mattias Nissler wrote:
> device_local_account_browsertest.cc LGTM
> 
> On a related note: Are you positive we haven't waited for trusted settings
> before logging in? If so, that'd be a security regression. I know the code to
> wait for a trusted login whitelist existed a long while ago. If it's still
> present, you might now want to remove redundancy. If it's no longer present,
> please file a security bug.

I found scattered code that waited for trusted settings and consolidated it. It
is a bit hard to see in existing_user_controller.cc because I moved some methods
around to match their declaration order. That code was present in regular user
logins but e.g. kiosk auto-launch had absolutely no trusted settings on its
critical path.

[Mattias Nissler (ping if slow), 2014-11-04 09:12:44]

On 2014/11/04 08:51:06, bartfab wrote:
> On 2014/11/04 08:38:52, Mattias Nissler wrote:
> > device_local_account_browsertest.cc LGTM
> > 
> > On a related note: Are you positive we haven't waited for trusted settings
> > before logging in? If so, that'd be a security regression. I know the code
to
> > wait for a trusted login whitelist existed a long while ago. If it's still
> > present, you might now want to remove redundancy. If it's no longer present,
> > please file a security bug.
> 
> I found scattered code that waited for trusted settings and consolidated it.
It
> is a bit hard to see in existing_user_controller.cc because I moved some
methods
> around to match their declaration order. That code was present in regular user
> logins but e.g. kiosk auto-launch had absolutely no trusted settings on its
> critical path.

OK, saw that now, and also found the whitelist check in
LoginPerformer::PerformLogin. This code is rather hard to follow :-/

[bartfab (slow), 2014-11-04 10:59:36]

> This code is rather hard to follow :-/

+1

[bartfab (slow), 2014-11-06 15:51:31]

Hi Denis,

Friendly review ping. This is aimed at M40.

[Denis Kuznetsov (DE-MUC), 2014-11-06 17:09:11]

lgtm

[bartfab (slow), 2014-11-06 17:11:58]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-06 17:12:47]

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

[commit-bot: I haz the power, 2014-11-06 18:28:29]

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

[commit-bot: I haz the power, 2014-11-06 18:28:29]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bartfab (slow), 2014-11-06 19:00:11]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-06 19:01:04]

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

[commit-bot: I haz the power, 2014-11-06 19:47:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-06 19:47:57]

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