Reauthenticate the user before launching Smart Lock setup app.

Reauthenticate the user before launching Smart Lock setup app.

The reauth is needed to acquire the user's password to add a new cryptohome
key for sign-in.

BUG=409427
Committed: https://crrev.com/174ab92a101d02bce1301d8fdc8ec15ec5557137
Cr-Commit-Position: refs/heads/master@{#302541}

[Tim Song, 2014-10-23 16:57:59]

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

[Tim Song, 2014-10-23 16:57:59]

+nkostylev@ for ui/login
+tbarzic@ for the rest

[Nikita (slow), 2014-10-27 15:15:10]

ui/login lgtm

https://codereview.chromium.org/668213003/diff/40001/ui/login/account_picker/...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/668213003/diff/40001/ui/login/account_picker/...
ui/login/account_picker/user_pod_row.js:483: // TODO(tengs): Introduce a special
reauth state for the account picker,
nit: Makes sense to mention crbug issue  # here.

[tbarzic, 2014-10-27 21:44:42]

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

[tbarzic, 2014-10-27 21:44:44]

+xiyuan (I'd like to get his opinion on this, too)

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:40: // Only
primary profile's can set up Easy Unlock right now.
the comment doesn't really match the logic.
I don't think that generally only primary profile is allowed to lock screen.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:79:
DCHECK(lock_users.size() == 1);
I don't think this necessary holds

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.h (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.h:27: // Helper
class performing the actual reauth flow.
as far as I can see, the whole private block can be moved to .cc file scope

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc
(right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc:17: const
int64 kUserContextTimeToLiveMin = 10;
Do you mean Max?

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc:28:
base::TimeDelta::FromMinutes(kUserContextTimeToLiveMin));
What if the setup exceeds 10 min? Does setup fail in any way; or do we just
fallback to current behaviour?

Is there a way to avoid the magic constant here (maybe bind the lifetime to
setup app being active)?

Maybe it would make more sense to ask for credentials right before we need it
(i.e. before saving list of remote devices)?

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:591: if (paired_devices ==
devices_in_cryptohome) {
I don't think this is the right thing to do in general case.
I'm particularly concerned about two cases:
1. Chrome OS public key is changed, which would change PSK (even though phone
public key stays the same).
2. User turns off easy unlock, and then easy unlock gets reenabled due to sync
(before user ever providing credentials to update cryptohome state) It is an
edge case, but still, I'm not too happy with introducing it :/

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.cc:151: !devices.empty()) {
I concerned about the case where user does not finish setup, and
SetRemoteDevices gets called due to sync, in which case easy signin would end up
being available on next login (with no real feedback to the user)

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.cc:162:
weak_ptr_factory_.GetWeakPtr()));
reset user context?
also, can we make sure the context is reset when setup app window closes.

[xiyuan, 2014-10-28 20:38:03]

I agree with Toni's comments and don't have more to add.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:40: // Only
primary profile's can set up Easy Unlock right now.
On 2014/10/27 21:44:43, tbarzic wrote:
> the comment doesn't really match the logic.
> I don't think that generally only primary profile is allowed to lock screen.

Yep, let's do primary user check here since we only support primary user ATM.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:79:
DCHECK(lock_users.size() == 1);
On 2014/10/27 21:44:43, tbarzic wrote:
> I don't think this necessary holds

We probably should pass in the reauth user and pick the right pod here.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:591: if (paired_devices ==
devices_in_cryptohome) {
On 2014/10/27 21:44:43, tbarzic wrote:
> I don't think this is the right thing to do in general case.
> I'm particularly concerned about two cases:
> 1. Chrome OS public key is changed, which would change PSK (even though phone
> public key stays the same).
> 2. User turns off easy unlock, and then easy unlock gets reenabled due to sync
> (before user ever providing credentials to update cryptohome state) It is an
> edge case, but still, I'm not too happy with introducing it :/

For 1, maybe we should consider using psk instead of phone key for the change
detection.

2 should be addressed too. We should clear the hardlock state somewhere else,
e.g. in EasyUnlockServiceRegular::OnKeysRefreshedForSetDevices.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.cc:151: !devices.empty()) {
On 2014/10/27 21:44:44, tbarzic wrote:
> I concerned about the case where user does not finish setup, and
> SetRemoteDevices gets called due to sync, in which case easy signin would end
up
> being available on next login (with no real feedback to the user)

Agree. We probably should make sure |short_lived_user_context_| get reset when
the setup window is dismissed, either by user or from sync.

[Tim Song, 2014-10-31 17:57:37]

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:40: // Only
primary profile's can set up Easy Unlock right now.
On 2014/10/28 20:38:03, xiyuan wrote:
> On 2014/10/27 21:44:43, tbarzic wrote:
> > the comment doesn't really match the logic.
> > I don't think that generally only primary profile is allowed to lock screen.
> 
> Yep, let's do primary user check here since we only support primary user ATM.

Done.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:79:
DCHECK(lock_users.size() == 1);
On 2014/10/28 20:38:03, xiyuan wrote:
> On 2014/10/27 21:44:43, tbarzic wrote:
> > I don't think this necessary holds
> 
> We probably should pass in the reauth user and pick the right pod here.

I plan on introducing a new Reauth state to the screen locker so we can pass in
the users we want rather than rely on UserManager::GetUnlockUsers().

I also added a check above to check that we're not in a multiprofile session.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.h (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.h:27: // Helper
class performing the actual reauth flow.
On 2014/10/27 21:44:43, tbarzic wrote:
> as far as I can see, the whole private block can be moved to .cc file scope

Done.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc
(right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc:17: const
int64 kUserContextTimeToLiveMin = 10;
On 2014/10/27 21:44:43, tbarzic wrote:
> Do you mean Max?

This should say kUserContextTimeToLiveMinutes.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc:28:
base::TimeDelta::FromMinutes(kUserContextTimeToLiveMin));
On 2014/10/27 21:44:43, tbarzic wrote:
> What if the setup exceeds 10 min? Does setup fail in any way; or do we just
> fallback to current behaviour?
> 
> Is there a way to avoid the magic constant here (maybe bind the lifetime to
> setup app being active)?
> 
> Maybe it would make more sense to ask for credentials right before we need it
> (i.e. before saving list of remote devices)?


The UI decision was to show the reauth before the app is launched, so we have to
cache the user context. Additionally, we will need to cache the user context
briefly. when checking for the lure notification. I think the fallback is okay,
since it's pretty rare for someone to spend 10 minutes on this setup. We want to
avoid keeping the credentials around if the user keeps this window open
indefinitely.

I added logic binding the lifetime of the user context to the lifetime of the
app window.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:591: if (paired_devices ==
devices_in_cryptohome) {
On 2014/10/28 20:38:03, xiyuan wrote:
> On 2014/10/27 21:44:43, tbarzic wrote:
> > I don't think this is the right thing to do in general case.
> > I'm particularly concerned about two cases:
> > 1. Chrome OS public key is changed, which would change PSK (even though
phone
> > public key stays the same).
> > 2. User turns off easy unlock, and then easy unlock gets reenabled due to
sync
> > (before user ever providing credentials to update cryptohome state) It is an
> > edge case, but still, I'm not too happy with introducing it :/
> 
> For 1, maybe we should consider using psk instead of phone key for the change
> detection.
> 
> 2 should be addressed too. We should clear the hardlock state somewhere else,
> e.g. in EasyUnlockServiceRegular::OnKeysRefreshedForSetDevices.

For 1, I did as Xiyuan suggested and made the check based on the psk.

I'm not sure I understand 2, though. Turning off easy unlock should clear the
cryptohome keys. If new keys were ever synced, that would result in the
preference keys not matching the cryptohome keys, and give us the expected
behaviour.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.cc:151: !devices.empty()) {
On 2014/10/28 20:38:03, xiyuan wrote:
> On 2014/10/27 21:44:44, tbarzic wrote:
> > I concerned about the case where user does not finish setup, and
> > SetRemoteDevices gets called due to sync, in which case easy signin would
end
> up
> > being available on next login (with no real feedback to the user)
> 
> Agree. We probably should make sure |short_lived_user_context_| get reset when
> the setup window is dismissed, either by user or from sync.

Done. I put the logic binding the user context to the app window in
ShortLivedUserContext.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.cc:162:
weak_ptr_factory_.GetWeakPtr()));
On 2014/10/27 21:44:43, tbarzic wrote:
> reset user context?
> also, can we make sure the context is reset when setup app window closes.

Done.

https://codereview.chromium.org/668213003/diff/40001/ui/login/account_picker/...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/668213003/diff/40001/ui/login/account_picker/...
ui/login/account_picker/user_pod_row.js:483: // TODO(tengs): Introduce a special
reauth state for the account picker,
On 2014/10/27 15:15:10, Nikita (OOO Oct 29 - Nov 5) wrote:
> nit: Makes sense to mention crbug issue  # here.

Done.

[tbarzic, 2014-10-31 18:24:09]

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc
(right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc:28:
base::TimeDelta::FromMinutes(kUserContextTimeToLiveMin));
On 2014/10/31 17:57:37, Tim Song wrote:
> On 2014/10/27 21:44:43, tbarzic wrote:
> > What if the setup exceeds 10 min? Does setup fail in any way; or do we just
> > fallback to current behaviour?
> > 
> > Is there a way to avoid the magic constant here (maybe bind the lifetime to
> > setup app being active)?
> > 
> > Maybe it would make more sense to ask for credentials right before we need
it
> > (i.e. before saving list of remote devices)?
> 
> 
> The UI decision was to show the reauth before the app is launched, so we have
to
> cache the user context. Additionally, we will need to cache the user context
> briefly. when checking for the lure notification. I think the fallback is
okay,
> since it's pretty rare for someone to spend 10 minutes on this setup. We want
to
> avoid keeping the credentials around if the user keeps this window open
> indefinitely.
> 

Yeah, I was thinking that we may show an error message in the setup UI (and
clear the user context when that happens; I agree that we should not keep the
credentials indefinitely) rather than just continue setup and commit the keys on
next authentication. Though, I guess this is good enough for now.

Maybe we could also add some metrics for the time the credentials are kept
around in order to see how long "short-lived" should be (10 min is not that
short :) )

> I added logic binding the lifetime of the user context to the lifetime of the
> app window.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:591: if (paired_devices ==
devices_in_cryptohome) {
On 2014/10/31 17:57:37, Tim Song wrote:
> On 2014/10/28 20:38:03, xiyuan wrote:
> > On 2014/10/27 21:44:43, tbarzic wrote:
> > > I don't think this is the right thing to do in general case.
> > > I'm particularly concerned about two cases:
> > > 1. Chrome OS public key is changed, which would change PSK (even though
> phone
> > > public key stays the same).
> > > 2. User turns off easy unlock, and then easy unlock gets reenabled due to
> sync
> > > (before user ever providing credentials to update cryptohome state) It is
an
> > > edge case, but still, I'm not too happy with introducing it :/
> > 
> > For 1, maybe we should consider using psk instead of phone key for the
change
> > detection.
> > 
> > 2 should be addressed too. We should clear the hardlock state somewhere
else,
> > e.g. in EasyUnlockServiceRegular::OnKeysRefreshedForSetDevices.
> 
> For 1, I did as Xiyuan suggested and made the check based on the psk.
> 
> I'm not sure I understand 2, though. Turning off easy unlock should clear the
> cryptohome keys. If new keys were ever synced, that would result in the
> preference keys not matching the cryptohome keys, and give us the expected
> behaviour.

cryptohome keys are actually cleared on the next user authentication (afaik, we
need user credentials for this too), so if we sync before that, the values in
user prefs and cryptohome would match (provided that the set of setup phones is
the same)

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:48:
DCHECK(false) << "Only primary users in non-multiprofile sessions are "
no DCHECK; afaik this is realistic possibility.

[Tim Song, 2014-11-01 03:16:53]

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc
(right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc:28:
base::TimeDelta::FromMinutes(kUserContextTimeToLiveMin));
On 2014/10/31 18:24:08, tbarzic wrote:
> On 2014/10/31 17:57:37, Tim Song wrote:
> > On 2014/10/27 21:44:43, tbarzic wrote:
> > > What if the setup exceeds 10 min? Does setup fail in any way; or do we
just
> > > fallback to current behaviour?
> > > 
> > > Is there a way to avoid the magic constant here (maybe bind the lifetime
to
> > > setup app being active)?
> > > 
> > > Maybe it would make more sense to ask for credentials right before we need
> it
> > > (i.e. before saving list of remote devices)?
> > 
> > 
> > The UI decision was to show the reauth before the app is launched, so we
have
> to
> > cache the user context. Additionally, we will need to cache the user context
> > briefly. when checking for the lure notification. I think the fallback is
> okay,
> > since it's pretty rare for someone to spend 10 minutes on this setup. We
want
> to
> > avoid keeping the credentials around if the user keeps this window open
> > indefinitely.
> > 
> 
> Yeah, I was thinking that we may show an error message in the setup UI (and
> clear the user context when that happens; I agree that we should not keep the
> credentials indefinitely) rather than just continue setup and commit the keys
on
> next authentication. Though, I guess this is good enough for now.
> 
> Maybe we could also add some metrics for the time the credentials are kept
> around in order to see how long "short-lived" should be (10 min is not that
> short :) )
> 
> > I added logic binding the lifetime of the user context to the lifetime of
the
> > app window.
> 

I can add a check in the app to close the window in 10 minutes. If I recall, we
actually had this logic a while back when the setup was encrypted, so we can
timeout the temporary key.

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/668213003/diff/40001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:591: if (paired_devices ==
devices_in_cryptohome) {
On 2014/10/31 18:24:09, tbarzic wrote:
> On 2014/10/31 17:57:37, Tim Song wrote:
> > On 2014/10/28 20:38:03, xiyuan wrote:
> > > On 2014/10/27 21:44:43, tbarzic wrote:
> > > > I don't think this is the right thing to do in general case.
> > > > I'm particularly concerned about two cases:
> > > > 1. Chrome OS public key is changed, which would change PSK (even though
> > phone
> > > > public key stays the same).
> > > > 2. User turns off easy unlock, and then easy unlock gets reenabled due
to
> > sync
> > > > (before user ever providing credentials to update cryptohome state) It
is
> an
> > > > edge case, but still, I'm not too happy with introducing it :/
> > > 
> > > For 1, maybe we should consider using psk instead of phone key for the
> change
> > > detection.
> > > 
> > > 2 should be addressed too. We should clear the hardlock state somewhere
> else,
> > > e.g. in EasyUnlockServiceRegular::OnKeysRefreshedForSetDevices.
> > 
> > For 1, I did as Xiyuan suggested and made the check based on the psk.
> > 
> > I'm not sure I understand 2, though. Turning off easy unlock should clear
the
> > cryptohome keys. If new keys were ever synced, that would result in the
> > preference keys not matching the cryptohome keys, and give us the expected
> > behaviour.
> 
> cryptohome keys are actually cleared on the next user authentication (afaik,
we
> need user credentials for this too), so if we sync before that, the values in
> user prefs and cryptohome would match (provided that the set of setup phones
is
> the same)

Okay that makes sense. I changed the hardlock state in EasyUnlockServiceRegular
instead and reverted this check back to what it was before.

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:48:
DCHECK(false) << "Only primary users in non-multiprofile sessions are "
On 2014/10/31 18:24:09, tbarzic wrote:
> no DCHECK; afaik this is realistic possibility.

This should be rare, so let's fall back to the hardlock state for now. I need to
add the explicit reauth state so we can select the user to reauth rather than
rely on GetUnlockUsers()

[xiyuan, 2014-11-01 03:51:37]

LGTM but please wait for Toni.

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:48:
DCHECK(false) << "Only primary users in non-multiprofile sessions are "
On 2014/11/01 03:16:52, Tim Song wrote:
> On 2014/10/31 18:24:09, tbarzic wrote:
> > no DCHECK; afaik this is realistic possibility.
> 
> This should be rare, so let's fall back to the hardlock state for now. I need
to
> add the explicit reauth state so we can select the user to reauth rather than
> rely on GetUnlockUsers()

I think Toni meant to replace the DCHECK with a LOG(WARNING) or something since
it could happen. DCHECK might be too drastic.

[Tim Song, 2014-11-01 04:57:39]

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:48:
DCHECK(false) << "Only primary users in non-multiprofile sessions are "
On 2014/11/01 03:51:37, xiyuan wrote:
> On 2014/11/01 03:16:52, Tim Song wrote:
> > On 2014/10/31 18:24:09, tbarzic wrote:
> > > no DCHECK; afaik this is realistic possibility.
> > 
> > This should be rare, so let's fall back to the hardlock state for now. I
need
> to
> > add the explicit reauth state so we can select the user to reauth rather
than
> > rely on GetUnlockUsers()
> 
> I think Toni meant to replace the DCHECK with a LOG(WARNING) or something
since
> it could happen. DCHECK might be too drastic.

Done. Sorry, I misread this comment.

[tbarzic, 2014-11-01 23:21:46]

LGTM

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:40:
DCHECK(false) << "Screen should not be locked when attempting reauth.";
notreached instead of DCHECK(false)

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:44: // Only
primary profile's can set up Easy Unlock right now.
s/profile's/user

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:44: // Only
primary profile's can set up Easy Unlock right now.
nit: I'd remove the comment; this is explained in LOG(WARNING) message

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:114: if
(g_reauth_handler)
since you have DCHECK just before this, I'd remove if (g_auth_handler)

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc
(right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc:46:
app_lifetime_monitor_->RemoveObserver(this);
this doesn't really belong in "DeleteUserContext"
Maybe you could rename the method to Reset, or move RemoveObserver to dtor.

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (left):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:394: if (GetHardlockState() ==
EasyUnlockScreenlockStateHandler::PAIRING_CHANGED)
can we add this back now?

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:70: if
(short_lived_user_context_ && short_lived_user_context_->user_context()) {
maybe we could also check that the credentials are not just about to get expired
(or maybe reset the deletion timeout); not necessarily in this cl

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:164: // We may already have
the password cached, so proceed to create the
my main concern here is that SetRemoteDevices could get called by sync logic
while the app window is still open (In particular if the user does not finish
the setup). We'd treat it the same as if the user set up those devices during
the setup.
I'm not sure that we can handle this without more context from the app, though.
Can you at least open a bug for this and fix it in a follow up?

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:172:
SetHardlockStateForUser(GetUserEmail(),
why is this needed here?

[tbarzic, 2014-11-01 23:29:05]

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:164: // We may already have
the password cached, so proceed to create the
On 2014/11/01 23:21:45, tbarzic wrote:
> my main concern here is that SetRemoteDevices could get called by sync logic
> while the app window is still open (In particular if the user does not finish
> the setup). We'd treat it the same as if the user set up those devices during
> the setup.
> I'm not sure that we can handle this without more context from the app,
though.
> Can you at least open a bug for this and fix it in a follow up?

actually, a workaround could be to check whether SetRemoteDevice was called from
the background page (which would mean that it's set by sync).
I think this should work for determining if the function was called from the
background page (in EasyUnlockPrivateFunction implementation):
content::WebContents::FromRenderViewHost(render_view_host())->GetDelegate()->IsNeverVisible().

I'm not completely sure this would work, though :)

[tbarzic, 2014-11-01 23:40:51]

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:70: if
(short_lived_user_context_ && short_lived_user_context_->user_context()) {
On 2014/11/01 23:21:45, tbarzic wrote:
> maybe we could also check that the credentials are not just about to get
expired
> (or maybe reset the deletion timeout); not necessarily in this cl

you can ignore this (we should not be keeping user context from a stale setup
attempt, right).

[Tim Song, 2014-11-03 19:07:51]

Patchset #7 (id:120001) has been deleted

[Tim Song, 2014-11-03 19:10:44]

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:40:
DCHECK(false) << "Screen should not be locked when attempting reauth.";
On 2014/11/01 23:21:44, tbarzic wrote:
> notreached instead of DCHECK(false)

Done.

https://codereview.chromium.org/668213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:44: // Only
primary profile's can set up Easy Unlock right now.
On 2014/11/01 23:21:44, tbarzic wrote:
> s/profile's/user

Done.

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc (right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:44: // Only
primary profile's can set up Easy Unlock right now.
On 2014/11/01 23:21:45, tbarzic wrote:
> nit: I'd remove the comment; this is explained in LOG(WARNING) message

Done.

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_reauth.cc:114: if
(g_reauth_handler)
On 2014/11/01 23:21:45, tbarzic wrote:
> since you have DCHECK just before this, I'd remove if (g_auth_handler)

Done.

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc
(right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/short_lived_user_context.cc:46:
app_lifetime_monitor_->RemoveObserver(this);
On 2014/11/01 23:21:45, tbarzic wrote:
> this doesn't really belong in "DeleteUserContext"
> Maybe you could rename the method to Reset, or move RemoveObserver to dtor.

Done. Renamed to Reset().

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (left):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:394: if (GetHardlockState() ==
EasyUnlockScreenlockStateHandler::PAIRING_CHANGED)
On 2014/11/01 23:21:45, tbarzic wrote:
> can we add this back now?

Done.

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:70: if
(short_lived_user_context_ && short_lived_user_context_->user_context()) {
On 2014/11/01 23:40:51, tbarzic wrote:
> On 2014/11/01 23:21:45, tbarzic wrote:
> > maybe we could also check that the credentials are not just about to get
> expired
> > (or maybe reset the deletion timeout); not necessarily in this cl
> 
> you can ignore this (we should not be keeping user context from a stale setup
> attempt, right).

Acknowledged

[tbarzic, 2014-11-03 22:50:13]

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:164: // We may already have
the password cached, so proceed to create the
On 2014/11/01 23:29:05, tbarzic wrote:
> On 2014/11/01 23:21:45, tbarzic wrote:
> > my main concern here is that SetRemoteDevices could get called by sync logic
> > while the app window is still open (In particular if the user does not
finish
> > the setup). We'd treat it the same as if the user set up those devices
during
> > the setup.
> > I'm not sure that we can handle this without more context from the app,
> though.
> > Can you at least open a bug for this and fix it in a follow up?
> 
> actually, a workaround could be to check whether SetRemoteDevice was called
from
> the background page (which would mean that it's set by sync).
> I think this should work for determining if the function was called from the
> background page (in EasyUnlockPrivateFunction implementation):
>
content::WebContents::FromRenderViewHost(render_view_host())->GetDelegate()->IsNeverVisible().
> 
> I'm not completely sure this would work, though :)

any update on this? (as I said, at least a TODO would be nice)

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:172:
SetHardlockStateForUser(GetUserEmail(),
On 2014/11/01 23:21:45, tbarzic wrote:
> why is this needed here?

how about this one?

[Tim Song, 2014-11-04 00:23:08]

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:164: // We may already have
the password cached, so proceed to create the
On 2014/11/03 22:50:13, tbarzic wrote:
> On 2014/11/01 23:29:05, tbarzic wrote:
> > On 2014/11/01 23:21:45, tbarzic wrote:
> > > my main concern here is that SetRemoteDevices could get called by sync
logic
> > > while the app window is still open (In particular if the user does not
> finish
> > > the setup). We'd treat it the same as if the user set up those devices
> during
> > > the setup.
> > > I'm not sure that we can handle this without more context from the app,
> > though.
> > > Can you at least open a bug for this and fix it in a follow up?
> > 
> > actually, a workaround could be to check whether SetRemoteDevice was called
> from
> > the background page (which would mean that it's set by sync).
> > I think this should work for determining if the function was called from the
> > background page (in EasyUnlockPrivateFunction implementation):
> >
>
content::WebContents::FromRenderViewHost(render_view_host())->GetDelegate()->IsNeverVisible().
> > 
> > I'm not completely sure this would work, though :)
> 
> any update on this? (as I said, at least a TODO would be nice)

Sorry, missed these comments.

I'm not sure if this is actually a problem. I'll put a TODO to look into this,
but I don't think the privacy/security concerns really apply in this case.

https://codereview.chromium.org/668213003/diff/100001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:172:
SetHardlockStateForUser(GetUserEmail(),
On 2014/11/03 22:50:13, tbarzic wrote:
> On 2014/11/01 23:21:45, tbarzic wrote:
> > why is this needed here?
> 
> how about this one?

Done. Sorry that was a left over.

[Tim Song, 2014-11-04 00:23:42]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-11-04 00:24:27]

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

[commit-bot: I haz the power, 2014-11-04 01:42:44]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2014-11-04 01:43:23]

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