Hook up ProximityAuthSystem in EasyUnlockService.

Hook up ProximityAuthSystem in EasyUnlockService.

ProximityAuthSystem is intended to replace the background page of the easy_unlock Chrome app.
Currently, this functionality is implemented behind the kEnableBluetoothLowEnergyDiscovery flag.

BUG=527558
TEST=manual

Committed: https://crrev.com/163fa44c84f8d4041f2d2e3b2fb359f3643b03c3
Cr-Commit-Position: refs/heads/master@{#351707}

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

[Tim Song, 2015-09-29 03:52:14]

Patchset #1 (id:1) has been deleted

[Tim Song, 2015-09-29 03:52:21]

Patchset #1 (id:20001) has been deleted

[Tim Song, 2015-09-29 03:54:14]

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

[sacomoto, 2015-09-29 18:45:57]

lgtm

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_app_manager.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_app_manager.cc:92: void
EasyUnlockAppManagerImpl::LoadApp() {
Please add a TODO here.

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

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:434: void
EasyUnlockServiceRegular::OnSyncStarted() {}
nit: Add an empty implementation to methods in
|CryptAuthDeviceManager::Observer|, instead of this.

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:618: }
Why does the order matter?

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.h (right):

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.h:181: bool
deferring_device_load_;
What exactly do you mean by security reasons?

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:397: PA_LOG(ERROR)
<< "Loaded Remote Device:\n"
nit: s/ERROR/INFO/.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/messenger_impl.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/messenger_impl.cc:78: if
(connection_->remote_device().bluetooth_address.empty())
Please use the RemoteDevice::bluetooth_type field.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/messenger_impl.h (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/messenger_impl.h:88: // For iOS, we can not get the
screen lock state reliably without polling.
nit: We need to poll the iOS device to keep the application alive on the
background.

[sacomoto, 2015-09-29 20:58:33]

More nits.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/proximity_auth_system.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.cc:91:
ScreenlockBridge::LockHandler::ScreenType screen_type) {}
Shouldn't you start the |remote_device_life_cycle_| here too?

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/proximity_auth_system.h (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.h:43: // Calls when the system
wakes up from a suspended state.
nit: s/Calls/Called/.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.h:58: // Resumes |life_cycle_|
after device wakes up and waits a timeout.
nit: |remote_device_life_cycle_|

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/remote_device_life_cycle_impl.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/remote_device_life_cycle_impl.cc:79: if
(remote_device_.bluetooth_address.empty()) {
Please use |remove_device_.bluetooth_type| and remove the TODO.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/remote_device_life_cycle_impl.cc:146: //
|OnSendComplete()| as an observer call for |messenger_|.
nit: s/OnSendComplete/OnSendCompleted/.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/remote_device_life_cycle_impl.h (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/remote_device_life_cycle_impl.h:84:
base::ObserverList<Observer> observers_;
Please add a comment saying the observers are |NOTIFY_EXISTING_ONLY|, as this is
not the default.

[Tim Song, 2015-09-30 00:05:05]

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_app_manager.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_app_manager.cc:92: void
EasyUnlockAppManagerImpl::LoadApp() {
On 2015/09/29 18:45:57, sacomoto wrote:
> Please add a TODO here.

Done.

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

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:434: void
EasyUnlockServiceRegular::OnSyncStarted() {}
On 2015/09/29 18:45:57, sacomoto wrote:
> nit: Add an empty implementation to methods in
> |CryptAuthDeviceManager::Observer|, instead of this.

Done.

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:618: }
On 2015/09/29 18:45:57, sacomoto wrote:
> Why does the order matter?

I want to avoid these managers being returned as null in
ChromeProximityAuthClient during initialization. We should be able to read the
existing prefs before the syncing is started.

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.h (right):

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.h:181: bool
deferring_device_load_;
On 2015/09/29 18:45:57, sacomoto wrote:
> What exactly do you mean by security reasons?

Basically, we don't want a network event to change things on the lock screen.
For example, if an attacker gets access to your Google account, they can
register their own unlock key. We also have another flow where syncing a new
device while logged in will require you to enter your password once after
locking.

I clarified the comments.

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:397: PA_LOG(ERROR)
<< "Loaded Remote Device:\n"
On 2015/09/29 18:45:57, sacomoto wrote:
> nit: s/ERROR/INFO/.

Done.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/messenger_impl.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/messenger_impl.cc:78: if
(connection_->remote_device().bluetooth_address.empty())
On 2015/09/29 18:45:57, sacomoto wrote:
> Please use the RemoteDevice::bluetooth_type field.

Done.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/messenger_impl.h (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/messenger_impl.h:88: // For iOS, we can not get the
screen lock state reliably without polling.
On 2015/09/29 18:45:57, sacomoto wrote:
> nit: We need to poll the iOS device to keep the application alive on the
> background.

Done.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/proximity_auth_system.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.cc:91:
ScreenlockBridge::LockHandler::ScreenType screen_type) {}
On 2015/09/29 20:58:33, sacomoto wrote:
> Shouldn't you start the |remote_device_life_cycle_| here too?

Done.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/proximity_auth_system.h (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.h:43: // Calls when the system
wakes up from a suspended state.
On 2015/09/29 20:58:33, sacomoto wrote:
> nit: s/Calls/Called/.

Done.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.h:58: // Resumes |life_cycle_|
after device wakes up and waits a timeout.
On 2015/09/29 20:58:33, sacomoto wrote:
> nit: |remote_device_life_cycle_|

Done.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/remote_device_life_cycle_impl.cc (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/remote_device_life_cycle_impl.cc:79: if
(remote_device_.bluetooth_address.empty()) {
On 2015/09/29 20:58:33, sacomoto wrote:
> Please use |remove_device_.bluetooth_type| and remove the TODO.

Done.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/remote_device_life_cycle_impl.cc:146: //
|OnSendComplete()| as an observer call for |messenger_|.
On 2015/09/29 20:58:33, sacomoto wrote:
> nit: s/OnSendComplete/OnSendCompleted/.

Done.

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
File components/proximity_auth/remote_device_life_cycle_impl.h (right):

https://codereview.chromium.org/1372283002/diff/40001/components/proximity_au...
components/proximity_auth/remote_device_life_cycle_impl.h:84:
base::ObserverList<Observer> observers_;
On 2015/09/29 20:58:33, sacomoto wrote:
> Please add a comment saying the observers are |NOTIFY_EXISTING_ONLY|, as this
is
> not the default.

Done.

[Tim Song, 2015-09-30 07:45:15]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2015-09-30 07:45:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
sacomoto@chromium.org
Link to the patchset: https://codereview.chromium.org/1372283002/#ps120001
(title: "fix compile")

[commit-bot: I haz the power, 2015-09-30 07:45:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372283002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372283002/120001

[commit-bot: I haz the power, 2015-09-30 08:48:40]

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

[commit-bot: I haz the power, 2015-09-30 08:48:41]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sacomoto, 2015-09-30 16:46:40]

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_app_manager.cc (right):

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_app_manager.cc:97: return;
The setup app doesn't load without loading the background app. Do you know why?

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_regular.cc:94: void
EasyUnlockServiceRegular::LoadRemoteDevices() {
After the setup is completed we need to sync the devices from CryptAuth. Maybe
you could trigger it from |SetBleRemoteDevices|, which is the last thing called
from the setup flow.

[sacomoto, 2015-09-30 17:40:53]

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_app_manager.cc (right):

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_app_manager.cc:97: return;
On 2015/09/30 16:46:40, sacomoto wrote:
> The setup app doesn't load without loading the background app. Do you know
why?

Ok, I found the issue. The background app registers an observer to launch the
paring_dialog:

https://cs.corp.google.com/#piper///depot/google3/googleclient/chrome/proximi...

Maybe remove this, and change the background.js not call |initEasyUnlock()|. We
can hide everything behind the BLE discovery flag
(userInfo[0].bleDiscoveryEnabled).

[Tim Song, 2015-09-30 19:13:39]

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_app_manager.cc (right):

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_app_manager.cc:97: return;
On 2015/09/30 16:46:40, sacomoto wrote:
> The setup app doesn't load without loading the background app. Do you know
why?

Okay, I removed this check.

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_app_manager.cc:97: return;
On 2015/09/30 17:40:53, sacomoto wrote:
> On 2015/09/30 16:46:40, sacomoto wrote:
> > The setup app doesn't load without loading the background app. Do you know
> why?
> 
> Ok, I found the issue. The background app registers an observer to launch the
> paring_dialog:
> 
>
https://cs.corp.google.com/#piper///depot/google3/googleclient/chrome/proximi...
> 
> Maybe remove this, and change the background.js not call |initEasyUnlock()|.
We
> can hide everything behind the BLE discovery flag
> (userInfo[0].bleDiscoveryEnabled). 

Acknowledged.

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/1372283002/diff/120001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_regular.cc:94: void
EasyUnlockServiceRegular::LoadRemoteDevices() {
On 2015/09/30 16:46:40, sacomoto wrote:
> After the setup is completed we need to sync the devices from CryptAuth. Maybe
> you could trigger it from |SetBleRemoteDevices|, which is the last thing
called
> from the setup flow.

That sounds good.

[Tim Song, 2015-10-01 02:14:23]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2015-10-01 02:14:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
sacomoto@chromium.org
Link to the patchset: https://codereview.chromium.org/1372283002/#ps160001
(title: "fix unlock manager crash")

[commit-bot: I haz the power, 2015-10-01 02:15:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372283002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372283002/160001

[commit-bot: I haz the power, 2015-10-01 02:23:20]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-10-01 02:24:21]

Patchset 7 (id:??) landed as
https://crrev.com/163fa44c84f8d4041f2d2e3b2fb359f3643b03c3
Cr-Commit-Position: refs/heads/master@{#351707}

[Tim Song, 2015-10-01 04:15:01]

A revert of this CL (patchset #7 id:160001) has been created in
https://codereview.chromium.org/1377313002/ by tengs@chromium.org.

The reason for reverting is: MSAN test failure:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests....

[sacomoto, 2015-10-01 16:09:08]

I fixed those issues in this CL:

https://codereview.chromium.org/1375633004/

Feel free to integrate that in your CL. 

Or if you prefer, review it and if you think it looks good (and applies without
conflicts after you land this CL), please send it directly to the CQ.

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
File components/proximity_auth/messenger_impl.cc (right):

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
components/proximity_auth/messenger_impl.cc:267:
connection_->SendMessage(make_scoped_ptr(new WireMessage(kPollScreenState)));
We should encrypt the messages to iOS too.

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
components/proximity_auth/messenger_impl.cc:293: std::string payload =
wire_message.payload();
This message should be encrypted too.

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
File components/proximity_auth/unlock_manager.cc (right):

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
components/proximity_auth/unlock_manager.cc:307:
reject_auth_attempt_weak_ptr_factory_.GetWeakPtr(), false),
It should be "true" here.

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
components/proximity_auth/unlock_manager.cc:308:
base::TimeDelta::FromSeconds(kAuthAttemptTimeoutSecs));
Why do you need this delay?

[sacomoto, 2015-10-01 16:56:47]

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
File components/proximity_auth/unlock_manager.cc (right):

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
components/proximity_auth/unlock_manager.cc:307:
reject_auth_attempt_weak_ptr_factory_.GetWeakPtr(), false),
On 2015/10/01 16:09:08, sacomoto wrote:
> It should be "true" here.

Oh, I see the issue here, it's my fault. The unlock protocol is sightly
different than what I was assuming. 

Even if the phone already sent a message saying it's unlocked CrOS sends a
message "unlock_request" message and only unlocks after receiving an
"unlock_response" message, right? 

I can implement it on the iOS app tomorrow, it should not be hard. 

But I think it's not worth it. We are already sending the screen status every
few seconds, and when it changes while we are connected. Moreover, these two
extra messages would add an extra delay to the (already slow) unlock process. 

What do you think? What if for BLE devices we unlocked directly whenever the
screen is unlocked?

https://codereview.chromium.org/1372283002/diff/180001/components/proximity_a...
components/proximity_auth/unlock_manager.cc:308:
base::TimeDelta::FromSeconds(kAuthAttemptTimeoutSecs));
On 2015/10/01 16:09:08, sacomoto wrote:
> Why do you need this delay?

Please disregard this comment.

[Tim Song, 2015-10-01 17:36:15]

MSAN tests pass, recommitting

[Tim Song, 2015-10-01 17:36:21]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2015-10-01 17:36:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
sacomoto@chromium.org
Link to the patchset: https://codereview.chromium.org/1372283002/#ps180001
(title: "fix msan tests")

[commit-bot: I haz the power, 2015-10-01 17:37:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372283002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372283002/180001

[commit-bot: I haz the power, 2015-10-01 17:53:13]

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

[commit-bot: I haz the power, 2015-10-01 17:53:14]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Tim Song, 2015-10-01 18:06:16]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2015-10-01 18:08:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372283002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372283002/180001

[commit-bot: I haz the power, 2015-10-01 18:12:48]

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

[commit-bot: I haz the power, 2015-10-01 18:12:49]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tim Song, 2015-10-02 03:11:21]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2015-10-02 03:11:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
sacomoto@chromium.org
Link to the patchset: https://codereview.chromium.org/1372283002/#ps240001
(title: "remove log")

[commit-bot: I haz the power, 2015-10-02 03:11:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372283002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372283002/240001

[commit-bot: I haz the power, 2015-10-02 04:00:54]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-10-02 04:01:44]

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