Refactor user pods to use authType property for distinct authentication modes.

Refactor user pods to use authType property for distinct authentication modes.

The authType of the user pod determines the authentication method to sign in
or unlock from the account picker screen. This refactoring aims to unify the
current two modes the user pod can use, and make the user pod more
extensible for other modes.

For example, the user pod may currently authenticate using a password or online
GAIA signin. This CL introduces a full-pod user click authType, and a PIN
code authType will be implemented in the future (see bug). This CL also adds
plumbing for the authType to be set by the ScreenLocker for the
chrome.screenlockPrivate API.

BUG=344179
TEST=visual inspection

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

[Tim Song, 2014-02-16 07:08:17]

Please take a look at the bug for the mocks of what I'm trying to accomplish.

This CL is split from the chrome.screenlockPrivate API implementation here:
https://codereview.chromium.org/168623003

+xiyuan@
Xiyuan, you may be interested in chrome.screenlockPrivate functions for
crbug.com/333979.

+nkostylev@
Nikita, I noticed you recently landed a change for kiosk app user pods. It seems
there is a trend towards user pod subclasses, and I would like your opinion on
the architecture for user pods.

[xiyuan, 2014-02-16 18:38:14]

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display.h (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display.h:23: class ScreenLocker;
nit: This seems not used.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display.h:32: SYSTEM_PASSWORD,
nit: OFFLINE_PASSWORD.  SYSTEM_PASSWORD is a bit confusing and does not
emphasize the key difference from this and Gaia one below.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:266:
UserManager::Get()->GetActiveUser());
This is not always the case when multi profile is enabled, e.g. with both
@google.com and @gmail.com, @google.com is always used to lock regardless of
which one is active. We probably should iterate through ScreenLocker's |users_|
to find the user and the profile.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:270:
router->OnAuthAttempted(auth_type, user_context.password);
How do we get feedback from the auth attempt? What if it is failed?

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:272: }
Should we move the whole block under "delegate_->OnAuthenticate();" at line 276?

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:325:
base::Int64ToString(base::Time::Now().ToInternalValue());
nit: prepend a "uniq=" to give the time a param name.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.h (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.h:16: #include
"chrome/browser/chromeos/extensions/screenlock_private_api.h"
nit: Is this needed?

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/user_pod_row.css (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.css:151: display:
-webkit-box;
.password-label has no children and "display: -webkit-box" only affects how an
element's children are laid out. We should use use "display: block" to avoid
confusion.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.css:210:
.pod[auth-type='onlineSignIn'] .signin-button {
Do this mean to show .signin-button always, regardless of whether the pod is
focused or not?

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.js:206:
this.user.initialAuthType : AUTH_TYPE.SYSTEM_PASSWORD;
nit: var initialAuthType = this.user.initialAuthType ||
AUTH_TYPE.SYSTEM_PASSWORD;

[Tim Song, 2014-02-18 23:32:44]

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display.h (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display.h:23: class ScreenLocker;
On 2014/02/16 18:38:14, xiyuan wrote:
> nit: This seems not used.

Done.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display.h:32: SYSTEM_PASSWORD,
On 2014/02/16 18:38:14, xiyuan wrote:
> nit: OFFLINE_PASSWORD.  SYSTEM_PASSWORD is a bit confusing and does not
> emphasize the key difference from this and Gaia one below. 

Done.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:266:
UserManager::Get()->GetActiveUser());
On 2014/02/16 18:38:14, xiyuan wrote:
> This is not always the case when multi profile is enabled, e.g. with both
> @google.com and @gmail.com, @google.com is always used to lock regardless of
> which one is active. We probably should iterate through ScreenLocker's
|users_|
> to find the user and the profile.

It seems that ScreenLocker already assumes there is only one user when
authenticating, so we can just use users_[0].

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:270:
router->OnAuthAttempted(auth_type, user_context.password);
On 2014/02/16 18:38:14, xiyuan wrote:
> How do we get feedback from the auth attempt? What if it is failed?

The app can unlock the screen if the auth attempt succeeds. Do we need feedback
when it fails?

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:272: }
On 2014/02/16 18:38:14, xiyuan wrote:
> Should we move the whole block under "delegate_->OnAuthenticate();" at line
276?

Done.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:325:
base::Int64ToString(base::Time::Now().ToInternalValue());
On 2014/02/16 18:38:14, xiyuan wrote:
> nit: prepend a "uniq=" to give the time a param name.

Done.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.h (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.h:16: #include
"chrome/browser/chromeos/extensions/screenlock_private_api.h"
On 2014/02/16 18:38:14, xiyuan wrote:
> nit: Is this needed?

Done.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/user_pod_row.css (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.css:151: display:
-webkit-box;
On 2014/02/16 18:38:14, xiyuan wrote:
> .password-label has no children and "display: -webkit-box" only affects how an
> element's children are laid out. We should use use "display: block" to avoid
> confusion.

The textContent node is a child, and I want it to be vertically centered.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.css:210:
.pod[auth-type='onlineSignIn'] .signin-button {
On 2014/02/16 18:38:14, xiyuan wrote:
> Do this mean to show .signin-button always, regardless of whether the pod is
> focused or not?

You're right, this should only be visible when the pod is focused.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.js:206:
this.user.initialAuthType : AUTH_TYPE.SYSTEM_PASSWORD;
On 2014/02/16 18:38:14, xiyuan wrote:
> nit: var initialAuthType = this.user.initialAuthType ||
> AUTH_TYPE.SYSTEM_PASSWORD;

Done.

[xiyuan, 2014-02-19 00:17:51]

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:266:
UserManager::Get()->GetActiveUser());
On 2014/02/18 23:32:44, Tim Song wrote:
> On 2014/02/16 18:38:14, xiyuan wrote:
> > This is not always the case when multi profile is enabled, e.g. with both
> > @google.com and @gmail.com, @google.com is always used to lock regardless of
> > which one is active. We probably should iterate through ScreenLocker's
> |users_|
> > to find the user and the profile.
> 
> It seems that ScreenLocker already assumes there is only one user when
> authenticating, so we can just use users_[0].

That is a bad assumption and we should not follow the pattern. It is used in
ScreenLocker::AuthenticateByPassword where the function is called from test
automation code and only has password. Here we have |user_context| and have the
actual user email, why not use it?

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:270:
router->OnAuthAttempted(auth_type, user_context.password);
On 2014/02/18 23:32:44, Tim Song wrote:
> On 2014/02/16 18:38:14, xiyuan wrote:
> > How do we get feedback from the auth attempt? What if it is failed?
> 
> The app can unlock the screen if the auth attempt succeeds. Do we need
feedback
> when it fails?

We do after we moved the "SetInputEnabled(false)" block before this and need to
re-enable the UI after auth failure. Otherwise, user would stuck with a
input-disabled locker screen.

[Tim Song, 2014-02-19 19:26:57]

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:266:
UserManager::Get()->GetActiveUser());
On 2014/02/19 00:17:52, xiyuan wrote:
> On 2014/02/18 23:32:44, Tim Song wrote:
> > On 2014/02/16 18:38:14, xiyuan wrote:
> > > This is not always the case when multi profile is enabled, e.g. with both
> > > @google.com and @gmail.com, @google.com is always used to lock regardless
of
> > > which one is active. We probably should iterate through ScreenLocker's
> > |users_|
> > > to find the user and the profile.
> > 
> > It seems that ScreenLocker already assumes there is only one user when
> > authenticating, so we can just use users_[0].
> 
> That is a bad assumption and we should not follow the pattern. It is used in
> ScreenLocker::AuthenticateByPassword where the function is called from test
> automation code and only has password. Here we have |user_context| and have
the
> actual user email, why not use it?

Done.

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:270:
router->OnAuthAttempted(auth_type, user_context.password);
On 2014/02/19 00:17:52, xiyuan wrote:
> On 2014/02/18 23:32:44, Tim Song wrote:
> > On 2014/02/16 18:38:14, xiyuan wrote:
> > > How do we get feedback from the auth attempt? What if it is failed?
> > 
> > The app can unlock the screen if the auth attempt succeeds. Do we need
> feedback
> > when it fails?
> 
> We do after we moved the "SetInputEnabled(false)" block before this and need
to
> re-enable the UI after auth failure. Otherwise, user would stuck with a
> input-disabled locker screen.

You're right. I added an acceptAuthAttempt() function to the screenlockPrivate
API to enable the input if the auth attempt is rejected.

[xiyuan, 2014-02-19 21:48:43]

LGTM

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://codereview.chromium.org/168813002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:270:
router->OnAuthAttempted(auth_type, user_context.password);
On 2014/02/19 19:26:57, Tim Song wrote:
> On 2014/02/19 00:17:52, xiyuan wrote:
> > On 2014/02/18 23:32:44, Tim Song wrote:
> > > On 2014/02/16 18:38:14, xiyuan wrote:
> > > > How do we get feedback from the auth attempt? What if it is failed?
> > > 
> > > The app can unlock the screen if the auth attempt succeeds. Do we need
> > feedback
> > > when it fails?
> > 
> > We do after we moved the "SetInputEnabled(false)" block before this and need
> to
> > re-enable the UI after auth failure. Otherwise, user would stuck with a
> > input-disabled locker screen.
> 
> You're right. I added an acceptAuthAttempt() function to the screenlockPrivate
> API to enable the input if the auth attempt is rejected.

SGTM. I saw you call ScreenLocker::EnableInput in the other CL but did not see
it here. Please make sure you don't lost any changes.

https://codereview.chromium.org/168813002/diff/570001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://codereview.chromium.org/168813002/diff/570001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:271:
UserManager::Get()->FindUser(user_context.username);
nit: It's better use the |users_| of ScreenLocker, which is a subset of the user
list in UserManager and contains only the user that shows up.

[Tim Song, 2014-02-20 00:06:45]

https://codereview.chromium.org/168813002/diff/570001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://codereview.chromium.org/168813002/diff/570001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screen_locker.cc:271:
UserManager::Get()->FindUser(user_context.username);
On 2014/02/19 21:48:44, xiyuan wrote:
> nit: It's better use the |users_| of ScreenLocker, which is a subset of the
user
> list in UserManager and contains only the user that shows up.

Done.

[Nikita (slow), 2014-02-20 15:12:17]

lgtm with nits

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display.h (right):

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display.h:28: enum AuthType {
It feels that this enum might leave somewhere else but I'm not sure where.

It's not like chrome/browser/chromeos/login/authenticator.h is a much better
place.

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/chromeos...
File chrome/browser/chromeos/login/webui_login_display.cc (right):

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/chromeos...
chrome/browser/chromeos/login/webui_login_display.cc:158: // Set the
authentication type to be used on the lock screen.
nit: Comment can be dropped here.

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.js:516: this.nameElement :
this.passwordElement;
When you return this.nameElement, how does it work next when mainInput.focus()
is called?

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.js:638: // Note: the
|mainInput| can be the pod itself.
Not the pod itself but nameElement?

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.js:655:
[this.user.username, '']);
nit: Fits on a single line?

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/screenlock_icon_provider.cc (right):

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/screenlock_icon_provider.cc:21: }
nit: drop {}

[Tim Song, 2014-02-20 22:14:00]

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/chromeos...
File chrome/browser/chromeos/login/webui_login_display.cc (right):

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/chromeos...
chrome/browser/chromeos/login/webui_login_display.cc:158: // Set the
authentication type to be used on the lock screen.
On 2014/02/20 15:12:18, Nikita Kostylev wrote:
> nit: Comment can be dropped here.

Done.

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.js:516: this.nameElement :
this.passwordElement;
On 2014/02/20 15:12:18, Nikita Kostylev wrote:
> When you return this.nameElement, how does it work next when mainInput.focus()
> is called?

Now that I think about it, the name element shouldn't be the main input element
at all.

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.js:638: // Note: the
|mainInput| can be the pod itself.
On 2014/02/20 15:12:18, Nikita Kostylev wrote:
> Not the pod itself but nameElement?

In the mocks, the entire pod is clickable, so I think it makes sense to consider
it as the "main input". This has some implications in terms of what gets
focused.

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/resource...
chrome/browser/resources/chromeos/login/user_pod_row.js:655:
[this.user.username, '']);
On 2014/02/20 15:12:18, Nikita Kostylev wrote:
> nit: Fits on a single line?

Done.

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/screenlock_icon_provider.cc (right):

https://codereview.chromium.org/168813002/diff/380002/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/screenlock_icon_provider.cc:21: }
On 2014/02/20 15:12:18, Nikita Kostylev wrote:
> nit: drop {}

Done.

[Tim Song, 2014-02-21 01:52:25]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-21 01:54:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168813002/830001

[commit-bot: I haz the power, 2014-02-21 02:45:39]

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

[commit-bot: I haz the power, 2014-02-21 02:45:40]

Retried try job too often on ios_rel_device_ninja for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[Tim Song, 2014-02-21 02:46:57]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-21 02:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168813002/830001

[commit-bot: I haz the power, 2014-02-21 02:54:39]

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

[commit-bot: I haz the power, 2014-02-21 02:54:40]

Retried try job too often on ios_rel_device_ninja for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[Tim Song, 2014-02-21 03:01:54]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-21 03:02:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168813002/830001

[commit-bot: I haz the power, 2014-02-21 05:09:24]

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

[commit-bot: I haz the power, 2014-02-21 05:09:24]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Tim Song, 2014-02-21 19:36:39]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-21 19:40:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168813002/1100001

[commit-bot: I haz the power, 2014-02-21 22:03:38]

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

[commit-bot: I haz the power, 2014-02-21 22:03:39]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Tim Song, 2014-02-21 22:29:57]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-21 22:31:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168813002/1100001

[commit-bot: I haz the power, 2014-02-22 00:49:24]

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

[commit-bot: I haz the power, 2014-02-22 00:49:25]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Tim Song, 2014-02-22 02:33:02]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-22 02:33:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168813002/1100001

[commit-bot: I haz the power, 2014-02-22 07:51:57]

Change committed as 252762