[Easy signin] Wire up userClick auth attempt to easy unlock app and back

[Easy signin] Wire up userClick auth attempt to easy unlock app and back

BUG=401634
TEST=Easy unlock works
Easy signin works (woth some changes to the unlock app)

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

[tbarzic, 2014-09-20 02:45:23]

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

[tbarzic, 2014-09-20 02:45:24]

Not completely ready for review (I still have to make a pass over the cl, and
add some comments, and I plan to add some tests), but I wanted to see if you're
ok with the direction (I borrowed some bits from
https://codereview.chromium.org/558273002/ :) ).

[tbarzic, 2014-09-20 02:46:14]

tbarzic@chromium.org changed reviewers:
+ tengs@chromium.org

[xiyuan, 2014-09-20 03:30:20]

hoho.. we are coming to the almost the same thing. :) See my
https://codereview.chromium.org/590603003/...

I am okay the general direction of the CL. The only part I am not so sure is
whether to put the sign-in method in ScreenLockBridge::LockHandler. I thought
about that but then the ScreenLock part in the name makes it feel a bit strange.

I am not happy with my CL either, where I added a
EasyUnlockService::SignInHandler. But then SignInScreenHandler needs to register
both with ScreenLockBridge and EasyUnlockService. Feel a bit clumsy.

Should we rename ScreenLockBridge to something else, e.g. SignInScreenBridge?

[tbarzic, 2014-09-22 17:42:21]

On 2014/09/20 03:30:20, xiyuan wrote:
> hoho.. we are coming to the almost the same thing. :) See my
> https://codereview.chromium.org/590603003/...
> 
> I am okay the general direction of the CL. The only part I am not so sure is
> whether to put the sign-in method in ScreenLockBridge::LockHandler. I thought
> about that but then the ScreenLock part in the name makes it feel a bit
strange.
> 
> I am not happy with my CL either, where I added a
> EasyUnlockService::SignInHandler. But then SignInScreenHandler needs to
register
> both with ScreenLockBridge and EasyUnlockService. Feel a bit clumsy.
> 
> Should we rename ScreenLockBridge to something else, e.g. SignInScreenBridge?

yeah, I have similar concerns (with both approaches). Renaming ScreenlockBridge
to SignInScreenBridge sounds good (I had a similar idea myself)

[tbarzic, 2014-09-23 00:36:58]

OK, I think this should be more or less ok.. Can you take a look (I'll do
ScreenlockBridge renaming separately:
https://codereview.chromium.org/592893002/)

[xiyuan, 2014-09-23 01:24:30]

lgtm

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1093: Profile*
profile = profile_helper->GetProfileByUser(unlock_user);
This call probably has side effect. I suspect that it would create a profile in
the user hash dir without mounting cryptohome.

Can we use the knowledge in line 1073-1077 above to determine the profile? That
is, if it is a lock screen, then we call this to get user profile. Otherwise,
use Profile::FromWebUI(web_ui()) and DCHECK it is the sign-in profile.

[tbarzic, 2014-09-23 01:50:22]

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1093: Profile*
profile = profile_helper->GetProfileByUser(unlock_user);
On 2014/09/23 01:24:29, xiyuan wrote:
> This call probably has side effect. I suspect that it would create a profile
in
> the user hash dir without mounting cryptohome.
> 
> Can we use the knowledge in line 1073-1077 above to determine the profile?
That
> is, if it is a lock screen, then we call this to get user profile. Otherwise,
> use Profile::FromWebUI(web_ui()) and DCHECK it is the sign-in profile. 

According to the method comment, it returns NULL if the user's profile is not
fully initialized.
"Returns profile of the |user| if it is created and fully initialized.
Otherwise, returns NULL."

Though, I guess I can go the other way, too.

[xiyuan, 2014-09-23 02:49:24]

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1093: Profile*
profile = profile_helper->GetProfileByUser(unlock_user);
On 2014/09/23 01:50:21, tbarzic wrote:
> On 2014/09/23 01:24:29, xiyuan wrote:
> > This call probably has side effect. I suspect that it would create a profile
> in
> > the user hash dir without mounting cryptohome.
> > 
> > Can we use the knowledge in line 1073-1077 above to determine the profile?
> That
> > is, if it is a lock screen, then we call this to get user profile.
Otherwise,
> > use Profile::FromWebUI(web_ui()) and DCHECK it is the sign-in profile. 
> 
> According to the method comment, it returns NULL if the user's profile is not
> fully initialized.
> "Returns profile of the |user| if it is created and fully initialized.
> Otherwise, returns NULL."
> 
> Though, I guess I can go the other way, too.

I see. In this case, I am okay leaving as is.

[tbarzic, 2014-09-23 02:59:31]

tbarzic@chromium.org changed reviewers:
+ nkostylev@chromium.org, yoz@chromium.org

[tbarzic, 2014-09-23 02:59:32]

I'll also need owner approval for
chromeos/login/auth/user_context.h (+nkostylev) and
chrome/browser/extensions/api/screenlock_private (+yoz)

[Nikita (slow), 2014-09-23 15:21:31]

On 2014/09/23 02:59:32, tbarzic wrote:
> I'll also need owner approval for
> chromeos/login/auth/user_context.h (+nkostylev) and

lgtm

[Nikita (slow), 2014-09-23 15:21:42]

https://codereview.chromium.org/585213002/diff/120001/chromeos/login/auth/use...
File chromeos/login/auth/user_context.h (right):

https://codereview.chromium.org/585213002/diff/120001/chromeos/login/auth/use...
chromeos/login/auth/user_context.h:30: // Offline authenticateion using and Easy
unlock device (e.g. a phone).
nit: authentication spelling

[Tim Song, 2014-09-23 19:37:59]

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:195:
RemoveKeys(op->user_context(), remove_start_index, RemoveKeysCallback());
There might be a race condition here if RemoveKeys() is called again before this
operation finishes, so the first operation will be canceled.

Perhaps it would be a good idea to enforce that this manager only performs one
operation at any time.

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/signin/s...
File chrome/browser/signin/screenlock_bridge.h (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/signin/s...
chrome/browser/signin/screenlock_bridge.h:139: virtual void
AttemptUserClickLogin(const std::string& user_email,
I was a bit confused by this name because the actual click is handled in
SignInScreenHandler. Maybe rename it to "AttemptEasyLogin" for clarity?

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1002:
delegate_->Login(user_context, SigninSpecifics());
Is the delegate hooked up yet for logging in with the secret?

[xiyuan, 2014-09-23 19:56:59]

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:195:
RemoveKeys(op->user_context(), remove_start_index, RemoveKeysCallback());
On 2014/09/23 19:37:59, Tim Song wrote:
> There might be a race condition here if RemoveKeys() is called again before
this
> operation finishes, so the first operation will be canceled.
> 
> Perhaps it would be a good idea to enforce that this manager only performs one
> operation at any time.

Currently, we have a poor man's DCHECK(!HasPendingOperations()) in line 70 to
protect the case. Since key operations should only happen after user presents
credentials (and after a user confirmation UI in the future), the chances of the
racing is low.

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1002:
delegate_->Login(user_context, SigninSpecifics());
On 2014/09/23 19:37:59, Tim Song wrote:
> Is the delegate hooked up yet for logging in with the secret?

The delegate should be set when sign-in screen is visible.

[tbarzic, 2014-09-23 19:58:35]

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:195:
RemoveKeys(op->user_context(), remove_start_index, RemoveKeysCallback());
On 2014/09/23 19:37:59, Tim Song wrote:
> There might be a race condition here if RemoveKeys() is called again before
this
> operation finishes, so the first operation will be canceled.
> 
> Perhaps it would be a good idea to enforce that this manager only performs one
> operation at any time.

I had a similar concern when this first got in, but afaik this is called only
once per session, so there should be ok (and there is a DCHECK for this in
RemoveKeys/RefreshKeys).

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/signin/s...
File chrome/browser/signin/screenlock_bridge.h (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/signin/s...
chrome/browser/signin/screenlock_bridge.h:139: virtual void
AttemptUserClickLogin(const std::string& user_email,
On 2014/09/23 19:37:59, Tim Song wrote:
> I was a bit confused by this name because the actual click is handled in
> SignInScreenHandler. Maybe rename it to "AttemptEasyLogin" for clarity?

I named it "AttemptUserClickLogin" because the AuthType it's supposed to handle
is called USER_CLICK, but I see your point. I'll rename it (especially since
auth flow was named EASY_UNLOCK)..

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1002:
delegate_->Login(user_context, SigninSpecifics());
On 2014/09/23 19:37:59, Tim Song wrote:
> Is the delegate hooked up yet for logging in with the secret?

Yeah, it should work fine.

[tbarzic, 2014-09-23 22:09:09]

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/signin/s...
File chrome/browser/signin/screenlock_bridge.h (right):

https://codereview.chromium.org/585213002/diff/120001/chrome/browser/signin/s...
chrome/browser/signin/screenlock_bridge.h:139: virtual void
AttemptUserClickLogin(const std::string& user_email,
On 2014/09/23 19:37:59, Tim Song wrote:
> I was a bit confused by this name because the actual click is handled in
> SignInScreenHandler. Maybe rename it to "AttemptEasyLogin" for clarity?

Done.

https://codereview.chromium.org/585213002/diff/120001/chromeos/login/auth/use...
File chromeos/login/auth/user_context.h (right):

https://codereview.chromium.org/585213002/diff/120001/chromeos/login/auth/use...
chromeos/login/auth/user_context.h:30: // Offline authenticateion using and Easy
unlock device (e.g. a phone).
On 2014/09/23 15:21:42, Nikita Kostylev wrote:
> nit: authentication spelling

Done.

[Yoyo Zhou, 2014-09-23 23:34:28]

screenlock_private LGTM

[Tim Song, 2014-09-24 00:12:35]

lgtm

[tbarzic, 2014-09-24 00:29:35]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:30:54]

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

[commit-bot: I haz the power, 2014-09-24 00:36:58]

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

[commit-bot: I haz the power, 2014-09-24 00:36:59]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[tbarzic, 2014-09-24 02:18:32]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-09-24 02:19:56]

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

[commit-bot: I haz the power, 2014-09-24 03:52:09]

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

[commit-bot: I haz the power, 2014-09-24 03:52:10]

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

[tbarzic, 2014-09-24 20:25:56]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-09-24 20:27:05]

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

[commit-bot: I haz the power, 2014-09-24 21:16:37]

Committed patchset #10 (id:170001) as 1923e87f01567533c3a7650652b65e6819c2ace0

[commit-bot: I haz the power, 2014-09-24 21:17:38]

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