Chromad: Allow offline login.

Chromad: Allow offline login.

When user types password on the pod screen it tries to unlock cryptohome
and authenticate against Active Directory server (get TGT) at the same
time. In case decrypting fail - it cleans authpolicyd state (by
restart).
The point of Active Directory authentication is to get TGT (if possible) with the password user typed. Failure here is OK - that could mean e.g. server is not reachable.
We don't want to have user wait for the Active Directory Authentication on the pod screen. In the follow-up CL we're gonna to create KeyedService inside the session which would get status about last authentication and handle possible failures (e.g. suggest to re-login in case server became reachable)


BUG=662400
TEST=manual

Review-Url: https://codereview.chromium.org/2835473002
Cr-Commit-Position: refs/heads/master@{#466664}
Committed: https://chromium.googlesource.com/chromium/src/+/41f61625dbac455f9e5876fc3ccbbb4952f21647

[Roman Sorokin (ftl), 2017-04-20 16:16:08]

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

[commit-bot: I haz the power, 2017-04-20 16:16:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2017-04-20 16:19:07]

rsorokin@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Roman Sorokin (ftl), 2017-04-20 16:19:07]

Hey Xiyuan, PTAL. Thanks!

[commit-bot: I haz the power, 2017-04-20 16:45:04]

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

[commit-bot: I haz the power, 2017-04-20 16:45:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Roman Sorokin (ftl), 2017-04-20 16:51:03]

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

[commit-bot: I haz the power, 2017-04-20 16:51:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-04-20 17:03:24]

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

https://codereview.chromium.org/2835473002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:1468: if (error !=
authpolicy::ERROR_NONE)
We would do nothing on auth error?

https://codereview.chromium.org/2835473002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:1470:
SetDisplayAndGivenName(account_data.display_name(),
How would we handle the race between OnAuthSuccess and this callback?

[commit-bot: I haz the power, 2017-04-20 18:35:06]

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

[commit-bot: I haz the power, 2017-04-20 18:35:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Roman Sorokin (ftl), 2017-04-21 09:01:37]

Description was changed from

==========
Chromad: Allow offline login.

When user types password on the pod screen it tries to unlock cryptohome
and authenticate against Active Directory server (get TGT) at the same
time. In case decrypting fail - it cleans authpolicyd state (by
restart).

BUG=662400
TEST=manual
==========

to

==========
Chromad: Allow offline login.

When user types password on the pod screen it tries to unlock cryptohome
and authenticate against Active Directory server (get TGT) at the same
time. In case decrypting fail - it cleans authpolicyd state (by
restart).
The point of Active Directory authentication is to get TGT (if possible) with
the password user typed. Failure here is OK - that could mean e.g. server is not
reachable.
We don't want to have user wait for the Active Directory Authentication on the
pod screen. In the follow-up CL we're gonna to create KeyedService inside the
session which would get status about last authentication and handle possible
failures (e.g. suggest to re-login in case server became reachable)


BUG=662400
TEST=manual
==========

[Roman Sorokin (ftl), 2017-04-21 09:06:01]

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

[commit-bot: I haz the power, 2017-04-21 09:06:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2017-04-21 09:07:34]

Hey Xiyuan, thanks for the fast review! PTAL

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

https://codereview.chromium.org/2835473002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:1468: if (error !=
authpolicy::ERROR_NONE)
On 2017/04/20 17:03:24, xiyuan wrote:
> We would do nothing on auth error?

KeyedService in the follow-up CL would handle that.

https://codereview.chromium.org/2835473002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:1470:
SetDisplayAndGivenName(account_data.display_name(),
On 2017/04/20 17:03:24, xiyuan wrote:
> How would we handle the race between OnAuthSuccess and this callback?

We're actually OK if this won't be called. I added section in the description
about follow-up CL. Maybe we should just ignore callback?

[commit-bot: I haz the power, 2017-04-21 09:41:29]

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

[commit-bot: I haz the power, 2017-04-21 09:41:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Roman Sorokin (ftl), 2017-04-21 13:53:06]

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

[commit-bot: I haz the power, 2017-04-21 13:53:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 14:26:00]

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

[commit-bot: I haz the power, 2017-04-21 14:26:01]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-21 14:48:14]

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

https://codereview.chromium.org/2835473002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:1468: if (error !=
authpolicy::ERROR_NONE)
On 2017/04/21 09:07:34, Roman Sorokin (fast) wrote:
> On 2017/04/20 17:03:24, xiyuan wrote:
> > We would do nothing on auth error?
> 
> KeyedService in the follow-up CL would handle that.

Please put the relevant part in CL description. It is not easy to dig into
history and check out the CL description.

https://codereview.chromium.org/2835473002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:1470:
SetDisplayAndGivenName(account_data.display_name(),
On 2017/04/21 09:07:34, Roman Sorokin (fast) wrote:
> On 2017/04/20 17:03:24, xiyuan wrote:
> > How would we handle the race between OnAuthSuccess and this callback?
> 
> We're actually OK if this won't be called. I added section in the description
> about follow-up CL. Maybe we should just ignore callback?

ExistingUserController is owned by LoginDisplayHostImpl. When cryptohome auth
succeeds, LoginDisplayHostImpl will be gone and so is ExistingUserController.
This callback could never be called.

I an inclined to just remove everything here and just put a comment here about
the follow up CL since the code here is not actually useful ATM.

https://codereview.chromium.org/2835473002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/2835473002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.h:45: }  // namespace
authpolicy
nit: Follow the style in line 37-39 for forward decl, i.e. no empty lines after
{ and before } and no need to have namespace comment.

[Roman Sorokin (ftl), 2017-04-24 16:20:48]

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

[commit-bot: I haz the power, 2017-04-24 16:21:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roman Sorokin (ftl), 2017-04-24 16:21:28]

Thanks Xiyuan! Please take another look!

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

https://codereview.chromium.org/2835473002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:1468: if (error !=
authpolicy::ERROR_NONE)
On 2017/04/21 14:48:14, xiyuan wrote:
> On 2017/04/21 09:07:34, Roman Sorokin (fast) wrote:
> > On 2017/04/20 17:03:24, xiyuan wrote:
> > > We would do nothing on auth error?
> > 
> > KeyedService in the follow-up CL would handle that.
> 
> Please put the relevant part in CL description. It is not easy to dig into
> history and check out the CL description.

Done.

https://codereview.chromium.org/2835473002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:1470:
SetDisplayAndGivenName(account_data.display_name(),
On 2017/04/21 14:48:14, xiyuan wrote:
> On 2017/04/21 09:07:34, Roman Sorokin (fast) wrote:
> > On 2017/04/20 17:03:24, xiyuan wrote:
> > > How would we handle the race between OnAuthSuccess and this callback?
> > 
> > We're actually OK if this won't be called. I added section in the
description
> > about follow-up CL. Maybe we should just ignore callback?
> 
> ExistingUserController is owned by LoginDisplayHostImpl. When cryptohome auth
> succeeds, LoginDisplayHostImpl will be gone and so is ExistingUserController.
> This callback could never be called.
> 
> I an inclined to just remove everything here and just put a comment here about
> the follow up CL since the code here is not actually useful ATM. 

Done.

https://codereview.chromium.org/2835473002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/2835473002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.h:45: }  // namespace
authpolicy
On 2017/04/21 14:48:14, xiyuan wrote:
> nit: Follow the style in line 37-39 for forward decl, i.e. no empty lines
after
> { and before } and no need to have namespace comment.

Done.

[xiyuan, 2017-04-24 16:23:25]

lgtm

[commit-bot: I haz the power, 2017-04-24 16:54:13]

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

[commit-bot: I haz the power, 2017-04-24 16:54:14]

Dry run: This issue passed the CQ dry run.

[Roman Sorokin (ftl), 2017-04-24 16:54:39]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2017-04-24 16:55:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 17:03:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493052879255390,
"parent_rev": "e52938b242948527f56802eb6871cdc65f7a10d7", "commit_rev":
"41f61625dbac455f9e5876fc3ccbbb4952f21647"}

[commit-bot: I haz the power, 2017-04-24 17:03:51]

Description was changed from

==========
Chromad: Allow offline login.

When user types password on the pod screen it tries to unlock cryptohome
and authenticate against Active Directory server (get TGT) at the same
time. In case decrypting fail - it cleans authpolicyd state (by
restart).
The point of Active Directory authentication is to get TGT (if possible) with
the password user typed. Failure here is OK - that could mean e.g. server is not
reachable.
We don't want to have user wait for the Active Directory Authentication on the
pod screen. In the follow-up CL we're gonna to create KeyedService inside the
session which would get status about last authentication and handle possible
failures (e.g. suggest to re-login in case server became reachable)


BUG=662400
TEST=manual
==========

to

==========
Chromad: Allow offline login.

When user types password on the pod screen it tries to unlock cryptohome
and authenticate against Active Directory server (get TGT) at the same
time. In case decrypting fail - it cleans authpolicyd state (by
restart).
The point of Active Directory authentication is to get TGT (if possible) with
the password user typed. Failure here is OK - that could mean e.g. server is not
reachable.
We don't want to have user wait for the Active Directory Authentication on the
pod screen. In the follow-up CL we're gonna to create KeyedService inside the
session which would get status about last authentication and handle possible
failures (e.g. suggest to re-login in case server became reachable)


BUG=662400
TEST=manual

Review-Url: https://codereview.chromium.org/2835473002
Cr-Commit-Position: refs/heads/master@{#466664}
Committed:
https://chromium.googlesource.com/chromium/src/+/41f61625dbac455f9e5876fc3ccb...
==========

[commit-bot: I haz the power, 2017-04-24 17:03:53]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/41f61625dbac455f9e5876fc3ccb...