Commit 584682e4 authored by Kyle Horimoto's avatar Kyle Horimoto Committed by Commit Bot

[CrOS MultiDevice] Fix two bugs related to removing the host device.

(1) HostVerifierImpl::IsHostVerified() returned true if the device had
    previously been verified but was pending removal. If a device is
    pending removal, the user has explicitly requested that it be
    removed as the host device, so it should not be considered verified.
(2) RemoteDeviceCache::SetRemoteDevices() skipped overwriting device
    metadata if that data was not newer than the previously-stored data.
    However, there is currently a bug on the CryptAuth back-end which
    does not update the device metadata update time when a feature has
    been changed from supported to enabled or vice versa. Thus, this
    check has been temporarily removed until the back-end bug has been
    fixed.

Bug: 870069, 824568
Change-Id: Ie748e5d662a9ea00cf1b5e6a42fc8354c986d61e
Reviewed-on: https://chromium-review.googlesource.com/1192417Reviewed-by: default avatarJames Hawkins <jhawkins@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586767}
parent cd0a5631
...@@ -113,6 +113,13 @@ bool HostVerifierImpl::IsHostVerified() { ...@@ -113,6 +113,13 @@ bool HostVerifierImpl::IsHostVerified() {
if (!current_host) if (!current_host)
return false; return false;
// If a host exists on the back-end but there is a pending request to remove
// that host, the device pending removal is no longer considered verified.
if (host_backend_delegate_->HasPendingHostRequest() &&
!host_backend_delegate_->GetPendingHostRequest()) {
return false;
}
// If one or more potential host sofware features is enabled, the host is // If one or more potential host sofware features is enabled, the host is
// considered verified. // considered verified.
for (const auto& software_feature : kPotentialHostFeatures) { for (const auto& software_feature : kPotentialHostFeatures) {
......
...@@ -134,6 +134,10 @@ class MultiDeviceSetupHostVerifierImplTest : public testing::Test { ...@@ -134,6 +134,10 @@ class MultiDeviceSetupHostVerifierImplTest : public testing::Test {
mock_timer_->Fire(); mock_timer_->Fire();
} }
FakeHostBackendDelegate* fake_host_backend_delegate() {
return fake_host_backend_delegate_.get();
}
private: private:
cryptauth::RemoteDeviceRef test_device_; cryptauth::RemoteDeviceRef test_device_;
...@@ -312,6 +316,22 @@ TEST_F(MultiDeviceSetupHostVerifierImplTest, ...@@ -312,6 +316,22 @@ TEST_F(MultiDeviceSetupHostVerifierImplTest,
kFirstRetryDeltaMs /* expected_retry_delta_value */); kFirstRetryDeltaMs /* expected_retry_delta_value */);
} }
TEST_F(MultiDeviceSetupHostVerifierImplTest,
StartWithVerifiedHost_PendingRemoval) {
CreateVerifier(HostState::kHostVerified);
VerifyState(true /* expected_is_verified */,
0u /* expected_num_verified_events */,
0 /* expected_retry_timestamp_value */,
0 /* expected_retry_delta_value */);
fake_host_backend_delegate()->AttemptToSetMultiDeviceHostOnBackend(
base::nullopt /* host_device */);
VerifyState(false /* expected_is_verified */,
0u /* expected_num_verified_events */,
0 /* expected_retry_timestamp_value */,
0 /* expected_retry_delta_value */);
}
} // namespace multidevice_setup } // namespace multidevice_setup
} // namespace chromeos } // namespace chromeos
...@@ -41,13 +41,8 @@ void RemoteDeviceCache::SetRemoteDevices( ...@@ -41,13 +41,8 @@ void RemoteDeviceCache::SetRemoteDevices(
const RemoteDeviceList& remote_devices) { const RemoteDeviceList& remote_devices) {
for (const auto& remote_device : remote_devices) { for (const auto& remote_device : remote_devices) {
if (base::ContainsKey(remote_device_map_, remote_device.GetDeviceId())) { if (base::ContainsKey(remote_device_map_, remote_device.GetDeviceId())) {
// Skip if the incoming remote device object contains // TODO(khorimoto): Do not overwrite device metadata if the new device
// a stale timestamp. // contains a stale timestamp; see https://crbug.com/856746.
if (remote_device.last_update_time_millis <=
remote_device_map_[remote_device.GetDeviceId()]
->last_update_time_millis) {
continue;
}
// Keep the same shared_ptr object, and simply // Keep the same shared_ptr object, and simply
// update the RemoteDevice it references. This transparently updates // update the RemoteDevice it references. This transparently updates
......
...@@ -87,9 +87,11 @@ TEST_F(RemoteDeviceCacheTest, ...@@ -87,9 +87,11 @@ TEST_F(RemoteDeviceCacheTest,
EXPECT_EQ(remote_device.name, remote_device_ref.name()); EXPECT_EQ(remote_device.name, remote_device_ref.name());
} }
// Currently disabled; will be re-enabled when https://crbug.com/856746 is
// fixed.
TEST_F( TEST_F(
RemoteDeviceCacheTest, RemoteDeviceCacheTest,
TestSetRemoteDevices_RemoteDeviceCacheDoesNotUpdateWithStaleRemoteDevice) { DISABLED_TestSetRemoteDevices_RemoteDeviceCacheDoesNotUpdateWithStaleRemoteDevice) {
// Store the device with a last update time of 1000. // Store the device with a last update time of 1000.
cryptauth::RemoteDevice remote_device = cryptauth::RemoteDevice remote_device =
cryptauth::CreateRemoteDeviceForTest(); cryptauth::CreateRemoteDeviceForTest();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment