Commit 8fb52d77 authored by Jordy Greenblatt's avatar Jordy Greenblatt Committed by Commit Bot

[CrOS MultiDevice] Reserve 'existing user' notifications for verified hosts

This CL adapts the MultiDevice notification functionality to the new
spec that only considers a new phone added or switched to once it is
verified rather than just set.

I added tests for this behavior for both of the 'existing host' events
and an extra test for the edge case of a set (but unverified) host
Phone A is replace by a different host Phone B that is verified.

Bug: 891822
Change-Id: Iebd9d0d209b020602a284de200fae168f34759c2
Reviewed-on: https://chromium-review.googlesource.com/c/1263425
Commit-Queue: Jordy Greenblatt <jordynass@chromium.org>
Reviewed-by: default avatarKyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597628}
parent 2387ead4
......@@ -13,9 +13,6 @@
#include "chromeos/components/proximity_auth/logging/logging.h"
#include "chromeos/services/multidevice_setup/host_status_provider_impl.h"
#include "chromeos/services/multidevice_setup/setup_flow_completion_recorder.h"
#include "components/cryptauth/proto/cryptauth_api.pb.h"
#include "components/cryptauth/remote_device_ref.h"
#include "components/cryptauth/software_feature_state.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
......@@ -76,7 +73,7 @@ void AccountStatusChangeDelegateNotifierImpl::RegisterPrefs(
registry->RegisterInt64Pref(kExistingUserChromebookAddedPrefName,
kTimestampNotSet);
registry->RegisterStringPref(
kHostDeviceIdFromMostRecentHostStatusUpdatePrefName, kNoHost);
kVerifiedHostDeviceIdFromMostRecentHostStatusUpdatePrefName, kNoHost);
}
AccountStatusChangeDelegateNotifierImpl::
......@@ -103,9 +100,12 @@ const char AccountStatusChangeDelegateNotifierImpl::
kExistingUserChromebookAddedPrefName[] =
"multidevice_setup.existing_user_chromebook_added";
// Note that, despite the pref string name, this pref only records the IDs of
// verified hosts. In particular, if a host has been set but is waiting for
// verification, it will not recorded.
// static
const char AccountStatusChangeDelegateNotifierImpl::
kHostDeviceIdFromMostRecentHostStatusUpdatePrefName[] =
kVerifiedHostDeviceIdFromMostRecentHostStatusUpdatePrefName[] =
"multidevice_setup.host_device_id_from_most_recent_sync";
AccountStatusChangeDelegateNotifierImpl::
......@@ -118,7 +118,7 @@ AccountStatusChangeDelegateNotifierImpl::
pref_service_(pref_service),
setup_flow_completion_recorder_(setup_flow_completion_recorder),
clock_(clock) {
host_device_id_from_most_recent_update_ =
verified_host_device_id_from_most_recent_update_ =
LoadHostDeviceIdFromEndOfPreviousSession();
host_status_provider_->AddObserver(this);
}
......@@ -137,27 +137,30 @@ void AccountStatusChangeDelegateNotifierImpl::CheckForMultiDeviceEvents(
return;
}
// Track and update host info.
base::Optional<std::string> host_device_id_before_update =
host_device_id_from_most_recent_update_;
// Track and update verified host info.
base::Optional<std::string> verified_host_device_id_before_update =
verified_host_device_id_from_most_recent_update_;
// Check if a host has been set.
if (host_status_with_device.host_device()) {
host_device_id_from_most_recent_update_ =
// Check if a host has been verified.
if (host_status_with_device.host_status() ==
mojom::HostStatus::kHostVerified) {
verified_host_device_id_from_most_recent_update_ =
host_status_with_device.host_device()->GetDeviceId();
pref_service_->SetString(
kHostDeviceIdFromMostRecentHostStatusUpdatePrefName,
*host_device_id_from_most_recent_update_);
kVerifiedHostDeviceIdFromMostRecentHostStatusUpdatePrefName,
*verified_host_device_id_from_most_recent_update_);
} else {
// No host set.
host_device_id_from_most_recent_update_.reset();
verified_host_device_id_from_most_recent_update_.reset();
pref_service_->SetString(
kVerifiedHostDeviceIdFromMostRecentHostStatusUpdatePrefName, kNoHost);
}
CheckForNewUserPotentialHostExistsEvent(host_status_with_device);
CheckForExistingUserHostSwitchedEvent(host_status_with_device,
host_device_id_before_update);
CheckForExistingUserChromebookAddedEvent(host_status_with_device,
host_device_id_before_update);
verified_host_device_id_before_update);
CheckForExistingUserChromebookAddedEvent(
host_status_with_device, verified_host_device_id_before_update);
}
void AccountStatusChangeDelegateNotifierImpl::
......@@ -165,7 +168,7 @@ void AccountStatusChangeDelegateNotifierImpl::
const HostStatusProvider::HostStatusWithDevice&
host_status_with_device) {
// We only check for new user events if there is no enabled host.
if (host_device_id_from_most_recent_update_)
if (verified_host_device_id_from_most_recent_update_)
return;
// If the observer has been notified of this event before, the user is not
......@@ -190,14 +193,17 @@ void AccountStatusChangeDelegateNotifierImpl::
void AccountStatusChangeDelegateNotifierImpl::
CheckForExistingUserHostSwitchedEvent(
const HostStatusProvider::HostStatusWithDevice& host_status_with_device,
const base::Optional<std::string>& host_device_id_before_update) {
// The host switched event requires both a pre-update and a post-update host.
if (!host_device_id_from_most_recent_update_ ||
!host_device_id_before_update) {
const base::Optional<std::string>&
verified_host_device_id_before_update) {
// The host switched event requires both a pre-update and a post-update
// verified host.
if (!verified_host_device_id_from_most_recent_update_ ||
!verified_host_device_id_before_update) {
return;
}
// If the host stayed the same, there was no switch.
if (*host_device_id_from_most_recent_update_ == *host_device_id_before_update)
if (*verified_host_device_id_from_most_recent_update_ ==
*verified_host_device_id_before_update)
return;
delegate()->OnConnectedHostSwitchedForExistingUser(
......@@ -209,11 +215,13 @@ void AccountStatusChangeDelegateNotifierImpl::
void AccountStatusChangeDelegateNotifierImpl::
CheckForExistingUserChromebookAddedEvent(
const HostStatusProvider::HostStatusWithDevice& host_status_with_device,
const base::Optional<std::string>& host_device_id_before_update) {
// The Chromebook added event requires that a set host was found by the
// update, i.e. there was no host before the host status update but afterward
// there is a set host.
if (!host_device_id_from_most_recent_update_ || host_device_id_before_update)
const base::Optional<std::string>&
verified_host_device_id_before_update) {
// The Chromebook added event requires that a verified host was found by the
// update, i.e. there was no verified host before the host status update but
// afterward there was a verified host.
if (!verified_host_device_id_from_most_recent_update_ ||
verified_host_device_id_before_update)
return;
delegate()->OnNewChromebookAddedForExistingUser(
......@@ -224,11 +232,12 @@ void AccountStatusChangeDelegateNotifierImpl::
base::Optional<std::string> AccountStatusChangeDelegateNotifierImpl::
LoadHostDeviceIdFromEndOfPreviousSession() {
std::string host_device_id_from_most_recent_update = pref_service_->GetString(
kHostDeviceIdFromMostRecentHostStatusUpdatePrefName);
if (host_device_id_from_most_recent_update.empty())
std::string verified_host_device_id_from_most_recent_update =
pref_service_->GetString(
kVerifiedHostDeviceIdFromMostRecentHostStatusUpdatePrefName);
if (verified_host_device_id_from_most_recent_update.empty())
return base::nullopt;
return host_device_id_from_most_recent_update;
return verified_host_device_id_from_most_recent_update;
}
} // namespace multidevice_setup
......
......@@ -62,7 +62,8 @@ class AccountStatusChangeDelegateNotifierImpl
static const char kExistingUserHostSwitchedPrefName[];
static const char kExistingUserChromebookAddedPrefName[];
static const char kHostDeviceIdFromMostRecentHostStatusUpdatePrefName[];
static const char
kVerifiedHostDeviceIdFromMostRecentHostStatusUpdatePrefName[];
AccountStatusChangeDelegateNotifierImpl(
HostStatusProvider* host_status_provider,
......@@ -84,17 +85,17 @@ class AccountStatusChangeDelegateNotifierImpl
const HostStatusProvider::HostStatusWithDevice& host_status_with_device);
void CheckForExistingUserHostSwitchedEvent(
const HostStatusProvider::HostStatusWithDevice& host_status_with_device,
const base::Optional<std::string>& host_device_id_before_update);
const base::Optional<std::string>& verified_host_device_id_before_update);
void CheckForExistingUserChromebookAddedEvent(
const HostStatusProvider::HostStatusWithDevice& host_status_with_device,
const base::Optional<std::string>& host_device_id_before_update);
const base::Optional<std::string>& verified_host_device_id_before_update);
// Loads data from previous session using PrefService.
base::Optional<std::string> LoadHostDeviceIdFromEndOfPreviousSession();
// Set to base::nullopt if there was no enabled host in the most recent
// host status update.
base::Optional<std::string> host_device_id_from_most_recent_update_;
base::Optional<std::string> verified_host_device_id_from_most_recent_update_;
mojom::AccountStatusChangeDelegatePtr delegate_ptr_;
HostStatusProvider* host_status_provider_;
......
......@@ -15,6 +15,7 @@
#include "chromeos/services/multidevice_setup/fake_host_status_provider.h"
#include "chromeos/services/multidevice_setup/fake_setup_flow_completion_recorder.h"
#include "chromeos/services/multidevice_setup/public/mojom/multidevice_setup.mojom.h"
#include "components/cryptauth/remote_device_ref.h"
#include "components/cryptauth/remote_device_test_util.h"
#include "components/sync_preferences/testing_pref_service_syncable.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -90,7 +91,7 @@ class MultiDeviceSetupAccountStatusChangeDelegateNotifierTest
cryptauth::RemoteDevice::GenerateDeviceId(old_host_key);
test_pref_service_->SetString(
AccountStatusChangeDelegateNotifierImpl::
kHostDeviceIdFromMostRecentHostStatusUpdatePrefName,
kVerifiedHostDeviceIdFromMostRecentHostStatusUpdatePrefName,
old_host_device_id);
}
......@@ -112,6 +113,12 @@ class MultiDeviceSetupAccountStatusChangeDelegateNotifierTest
kExistingUserChromebookAddedPrefName);
}
std::string GetMostRecentVerifiedHostDeviceIdPref() {
return test_pref_service_->GetString(
AccountStatusChangeDelegateNotifierImpl::
kVerifiedHostDeviceIdFromMostRecentHostStatusUpdatePrefName);
}
FakeAccountStatusChangeDelegate* fake_delegate() {
return fake_delegate_.get();
}
......@@ -216,35 +223,35 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
NotifiesObserverForHostSwitchEvents) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Verify the delegate initializes to 0.
// Check the delegate initializes to 0.
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
// Set initial host.
// Set initially verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
// Host was set but has never been switched.
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
// Switch hosts.
// Switch to new verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified,
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "#1")
.SetName(kFakePhoneName + "#1")
.SetPublicKey(kFakePhoneKey + "A")
.SetName(kFakePhoneName + "A")
.Build());
EXPECT_EQ(1u,
fake_delegate()->num_existing_user_host_switched_events_handled());
// Switch to another new host.
// Switch to a different new verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified,
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "#2")
.SetName(kFakePhoneName + "#2")
.SetPublicKey(kFakePhoneKey + "B")
.SetName(kFakePhoneName + "B")
.Build());
EXPECT_EQ(2u,
fake_delegate()->num_existing_user_host_switched_events_handled());
// Switch back to initial host.
// Switch back to initial host (verified).
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
EXPECT_EQ(3u,
fake_delegate()->num_existing_user_host_switched_events_handled());
......@@ -254,7 +261,7 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
SettingSameHostTriggersNoHostSwitchedEvent) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Set initial host.
// Set initially verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
// Set to host with identical information.
SetHostWithStatus(mojom::HostStatus::kHostVerified,
......@@ -270,9 +277,9 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
ChangingHostDevicesTriggersHostSwitchEventWhenHostNameIsUnchanged) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Set initial host.
// Set initially verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
// Set to host with same name but different key.
// Set to verified host with same name but different key.
SetHostWithStatus(mojom::HostStatus::kHostVerified,
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "alternate")
......@@ -283,13 +290,70 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
}
TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
VerifyingHostTriggersNoHostSwtichEvent) {
VerifyingSetHostTriggersNoHostSwtichEvent) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Set initial host but do not verify
// Set initial host but do not verify.
SetHostWithStatus(mojom::HostStatus::kHostSetButNotYetVerified, kFakePhone);
// Verify host
// Verify host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
}
TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
OnlyVerifiedHostCausesHostSwitchedEvent) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Set initially verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
// Set a new host without verifying.
SetHostWithStatus(mojom::HostStatus::kHostSetButNotYetVerified,
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "A")
.SetName(kFakePhoneName + "A")
.Build());
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
// Set a different new host without confirming with backend so host is
// unverified.
SetHostWithStatus(
mojom::HostStatus::kHostSetLocallyButWaitingForBackendConfirmation,
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "B")
.SetName(kFakePhoneName + "B")
.Build());
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
}
TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
ForgettingAndThenSwitchingHostsDoesNotTriggerHostSwitchedEvent) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Set initially verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
// Forget host.
SetHostWithStatus(mojom::HostStatus::kEligibleHostExistsButNoHostSet,
base::nullopt /* host_device */);
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
// Set a new verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified,
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "alternate")
.SetName(kFakePhoneName + "alternate")
.Build());
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
}
......@@ -298,7 +362,7 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
HostSwitchedBetweenSessions) {
SetHostFromPreviousSession(kFakePhoneKey + "-old");
BuildAccountStatusChangeDelegateNotifier();
// Host switched between sessions.
// Host switched and verified between sessions.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
SetAccountStatusChangeDelegatePtr();
EXPECT_EQ(1u,
......@@ -312,7 +376,7 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
// No enabled host initially.
SetHostWithStatus(mojom::HostStatus::kEligibleHostExistsButNoHostSet,
base::nullopt /* host_device */);
// Set host.
// Set and verify host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
......@@ -321,7 +385,7 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
NoHostSwitchedEventWithoutObserverSet) {
BuildAccountStatusChangeDelegateNotifier();
// Set initial host.
// Set initially verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
// All conditions for host switched event are satisfied except for setting a
// delegate.
......@@ -338,20 +402,66 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
NotifiesObserverForChromebookAddedEvents) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Verify the delegate initializes to 0.
// Check the delegate initializes to 0.
EXPECT_EQ(
0u, fake_delegate()->num_existing_user_chromebook_added_events_handled());
// Host is set from another Chromebook while this one is logged in.
// Host is set and verified from another Chromebook while this one is logged
// in.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
EXPECT_EQ(
1u, fake_delegate()->num_existing_user_chromebook_added_events_handled());
}
TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
OnlyVerifiedHostCausesChromebookAddedEvent) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Start with potential hosts but none set.
SetHostWithStatus(mojom::HostStatus::kEligibleHostExistsButNoHostSet,
base::nullopt /* host_device */);
// Set a host without verifying.
SetHostWithStatus(mojom::HostStatus::kHostSetButNotYetVerified, kFakePhone);
EXPECT_EQ(
0u, fake_delegate()->num_existing_user_chromebook_added_events_handled());
// Verify the new host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
EXPECT_EQ(
1u, fake_delegate()->num_existing_user_chromebook_added_events_handled());
}
TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
ReplacingUnverifiedHostAWithVerifiedHostBCausesChromebookAddedEvent) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Start with potential hosts but none set.
// Set initial host but do not verify.
SetHostWithStatus(mojom::HostStatus::kHostSetButNotYetVerified,
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "A")
.SetName(kFakePhoneName + "A")
.Build());
// Replace unverified Phone A with verified Phone B.
SetHostWithStatus(mojom::HostStatus::kHostVerified,
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "B")
.SetName(kFakePhoneName + "B")
.Build());
// This causes a 'Chromebook added' event.
EXPECT_EQ(
1u, fake_delegate()->num_existing_user_chromebook_added_events_handled());
// It does not cause a 'host switched' event.
EXPECT_EQ(0u,
fake_delegate()->num_existing_user_host_switched_events_handled());
}
TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
ChromebookAddedBetweenSessionsTriggersEvents) {
BuildAccountStatusChangeDelegateNotifier();
// Host is set before this Chromebook is logged in.
// Host is set and verified before this Chromebook is logged in.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
SetAccountStatusChangeDelegatePtr();
......@@ -387,6 +497,40 @@ TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
0u, fake_delegate()->num_existing_user_chromebook_added_events_handled());
}
TEST_F(MultiDeviceSetupAccountStatusChangeDelegateNotifierTest,
VerifiedHostIdStaysUpToDateInPrefs) {
BuildAccountStatusChangeDelegateNotifier();
SetAccountStatusChangeDelegatePtr();
// Check the delegate initializes to empty.
EXPECT_EQ(GetMostRecentVerifiedHostDeviceIdPref(), "");
// Set initially verified host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhone);
EXPECT_EQ(GetMostRecentVerifiedHostDeviceIdPref(), kFakePhone.GetDeviceId());
const cryptauth::RemoteDeviceRef kFakePhoneAlternate =
cryptauth::RemoteDeviceRefBuilder()
.SetPublicKey(kFakePhoneKey + "alternate")
.SetName(kFakePhoneName + "alternate")
.Build();
// Switch to an unverified host.
SetHostWithStatus(mojom::HostStatus::kHostSetButNotYetVerified,
kFakePhoneAlternate);
// The host is set but not verified so the pref should be set to empty.
EXPECT_EQ(GetMostRecentVerifiedHostDeviceIdPref(), "");
// Verify the new host.
SetHostWithStatus(mojom::HostStatus::kHostVerified, kFakePhoneAlternate);
EXPECT_EQ(GetMostRecentVerifiedHostDeviceIdPref(),
kFakePhoneAlternate.GetDeviceId());
// Forget host.
SetHostWithStatus(mojom::HostStatus::kEligibleHostExistsButNoHostSet,
base::nullopt /* host_device */);
EXPECT_EQ(GetMostRecentVerifiedHostDeviceIdPref(), "");
}
} // namespace multidevice_setup
} // namespace chromeos
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