Commit 2104fb82 authored by Ryan Hansberry's avatar Ryan Hansberry Committed by Commit Bot

[SmartLock] Move life cycle observation responsibility to UnlockManager.

Make UnlockManagerImplTest more closely follow how UnlockManagerImpl
actually behaves in production by making UnlockManagerImpl directly
listen on RemoteDeviceLifeCycle::Observer events, and make
FakeRemoteDeviceLifeCycle::Start() notify observers. This all allows
the test RemoteDeviceLifeCycle object to notify UnlockManager as soon
as its state changes, instead of having to manually emulate state change
events.

This CL unblocks testing for the bug fix (crbug.com/890047) in the
subsequent CL. As it turns out, this refactor actually exposes that
bug -- had this test code been structured this way before, the bug
could have been fixed long ago. TODOs have been left in the test code
marking accommodations made for the bug -- they will be removed in the
subsequent CL.

Bug: 926608, 890047
Change-Id: Iee96974d9376bda1f75deba314f28efccae7052c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1716154
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: default avatarKyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680309}
parent 3ab3092e
......@@ -18,6 +18,7 @@ FakeRemoteDeviceLifeCycle::~FakeRemoteDeviceLifeCycle() {}
void FakeRemoteDeviceLifeCycle::Start() {
started_ = true;
ChangeState(RemoteDeviceLifeCycle::State::FINDING_CONNECTION);
}
chromeos::multidevice::RemoteDeviceRef
......
......@@ -122,12 +122,6 @@ ProximityAuthSystem::CreateRemoteDeviceLifeCycle(
remote_device, local_device, secure_channel_client_);
}
void ProximityAuthSystem::OnLifeCycleStateChanged(
RemoteDeviceLifeCycle::State old_state,
RemoteDeviceLifeCycle::State new_state) {
unlock_manager_->OnLifeCycleStateChanged();
}
void ProximityAuthSystem::OnScreenDidLock(
ScreenlockBridge::LockHandler::ScreenType screen_type) {
const AccountId& focused_account_id =
......@@ -182,7 +176,6 @@ void ProximityAuthSystem::OnFocusedUserChanged(const AccountId& account_id) {
<< account_id.Serialize();
remote_device_life_cycle_ =
CreateRemoteDeviceLifeCycle(remote_device, local_device);
remote_device_life_cycle_->AddObserver(this);
// UnlockManager listens for Bluetooth power change events, and is therefore
// responsible for starting RemoteDeviceLifeCycle when Bluetooth becomes
......
......@@ -10,7 +10,6 @@
#include "base/macros.h"
#include "chromeos/components/multidevice/remote_device_ref.h"
#include "chromeos/components/proximity_auth/remote_device_life_cycle.h"
#include "chromeos/components/proximity_auth/screenlock_bridge.h"
#include "components/account_id/account_id.h"
......@@ -23,6 +22,7 @@ class SecureChannelClient;
namespace proximity_auth {
class ProximityAuthClient;
class RemoteDeviceLifeCycle;
class UnlockManager;
// This is the main entry point to start Proximity Auth, the underlying system
......@@ -30,8 +30,7 @@ class UnlockManager;
// phone) for each registered user, the system will handle the connection,
// authentication, and messenging protocol when the screen is locked and the
// registered user is focused.
class ProximityAuthSystem : public RemoteDeviceLifeCycle::Observer,
public ScreenlockBridge::Observer {
class ProximityAuthSystem : public ScreenlockBridge::Observer {
public:
enum ScreenlockType { SESSION_LOCK, SIGN_IN };
......@@ -91,10 +90,6 @@ class ProximityAuthSystem : public RemoteDeviceLifeCycle::Observer,
chromeos::multidevice::RemoteDeviceRef remote_device,
base::Optional<chromeos::multidevice::RemoteDeviceRef> local_device);
// RemoteDeviceLifeCycle::Observer:
void OnLifeCycleStateChanged(RemoteDeviceLifeCycle::State old_state,
RemoteDeviceLifeCycle::State new_state) override;
// ScreenlockBridge::Observer:
void OnScreenDidLock(
ScreenlockBridge::LockHandler::ScreenType screen_type) override;
......
......@@ -68,7 +68,6 @@ class MockUnlockManager : public UnlockManager {
~MockUnlockManager() override {}
MOCK_METHOD0(IsUnlockAllowed, bool());
MOCK_METHOD1(SetRemoteDeviceLifeCycle, void(RemoteDeviceLifeCycle*));
MOCK_METHOD0(OnLifeCycleStateChanged, void());
MOCK_METHOD1(OnAuthAttempted, void(mojom::AuthType));
MOCK_METHOD0(CancelConnectionAttempt, void());
......@@ -395,20 +394,6 @@ TEST_F(ProximityAuthSystemTest, StopSystem_RegisteredUserFocused) {
EXPECT_EQ(kUser1, life_cycle()->GetRemoteDevice().user_id());
}
TEST_F(ProximityAuthSystemTest, OnLifeCycleStateChanged) {
FocusUser(kUser1);
EXPECT_CALL(*unlock_manager_, OnLifeCycleStateChanged());
life_cycle()->ChangeState(RemoteDeviceLifeCycle::State::FINDING_CONNECTION);
EXPECT_CALL(*unlock_manager_, OnLifeCycleStateChanged());
life_cycle()->ChangeState(RemoteDeviceLifeCycle::State::AUTHENTICATING);
EXPECT_CALL(*unlock_manager_, OnLifeCycleStateChanged());
life_cycle()->ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
}
TEST_F(ProximityAuthSystemTest, OnAuthAttempted) {
FocusUser(kUser1);
EXPECT_CALL(*unlock_manager_, OnAuthAttempted(_));
......
......@@ -25,9 +25,6 @@ class UnlockManager {
// authentication is inactive.
virtual void SetRemoteDeviceLifeCycle(RemoteDeviceLifeCycle* life_cycle) = 0;
// Called when the life cycle's state changes.
virtual void OnLifeCycleStateChanged() = 0;
// Called when the user pod is clicked for an authentication attempt of type
// |auth_type|.
// Exposed for testing.
......
......@@ -125,14 +125,13 @@ UnlockManagerImpl::UnlockManagerImpl(
}
UnlockManagerImpl::~UnlockManagerImpl() {
if (life_cycle_)
life_cycle_->RemoveObserver(this);
if (GetMessenger())
GetMessenger()->RemoveObserver(this);
if (proximity_monitor_)
proximity_monitor_->RemoveObserver(this);
chromeos::PowerManagerClient::Get()->RemoveObserver(this);
if (bluetooth_adapter_)
bluetooth_adapter_->RemoveObserver(this);
}
......@@ -153,11 +152,15 @@ void UnlockManagerImpl::SetRemoteDeviceLifeCycle(
PA_LOG(VERBOSE) << "Request received to change scan state to: "
<< (life_cycle == nullptr ? "inactive" : "active") << ".";
if (life_cycle_)
life_cycle_->RemoveObserver(this);
if (GetMessenger())
GetMessenger()->RemoveObserver(this);
life_cycle_ = life_cycle;
if (life_cycle_) {
life_cycle_->AddObserver(this);
attempt_secure_connection_start_time_ =
base::DefaultClock::GetInstance()->Now();
......@@ -174,11 +177,11 @@ void UnlockManagerImpl::SetRemoteDeviceLifeCycle(
UpdateLockScreen();
}
void UnlockManagerImpl::OnLifeCycleStateChanged() {
RemoteDeviceLifeCycle::State state = life_cycle_->GetState();
void UnlockManagerImpl::OnLifeCycleStateChanged(
RemoteDeviceLifeCycle::State old_state,
RemoteDeviceLifeCycle::State new_state) {
remote_screenlock_state_.reset();
if (state == RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED) {
if (new_state == RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED) {
DCHECK(life_cycle_->GetChannel());
DCHECK(GetMessenger());
if (!proximity_monitor_) {
......@@ -196,7 +199,7 @@ void UnlockManagerImpl::OnLifeCycleStateChanged() {
proximity_monitor_.reset();
}
if (state == RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED)
if (new_state == RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED)
SetIsPerformingInitialScan(false /* is_performing_initial_scan */);
UpdateLockScreen();
......
......@@ -34,8 +34,9 @@ class ProximityMonitor;
class UnlockManagerImpl : public UnlockManager,
public MessengerObserver,
public ProximityMonitorObserver,
chromeos::PowerManagerClient::Observer,
public device::BluetoothAdapter::Observer {
public chromeos::PowerManagerClient::Observer,
public device::BluetoothAdapter::Observer,
public RemoteDeviceLifeCycle::Observer {
public:
// The |proximity_auth_client| is not owned and should outlive the constructed
// unlock manager.
......@@ -46,7 +47,6 @@ class UnlockManagerImpl : public UnlockManager,
// UnlockManager:
bool IsUnlockAllowed() override;
void SetRemoteDeviceLifeCycle(RemoteDeviceLifeCycle* life_cycle) override;
void OnLifeCycleStateChanged() override;
void OnAuthAttempted(mojom::AuthType auth_type) override;
void CancelConnectionAttempt() override;
......@@ -92,6 +92,10 @@ class UnlockManagerImpl : public UnlockManager,
// chromeos::PowerManagerClient::Observer:
void SuspendDone(const base::TimeDelta& sleep_duration) override;
// RemoteDeviceLifeCycle::Observer:
void OnLifeCycleStateChanged(RemoteDeviceLifeCycle::State old_state,
RemoteDeviceLifeCycle::State new_state) override;
// Returns true if the BluetoothAdapter is present and powered.
bool IsBluetoothPresentAndPowered() const;
......
......@@ -195,7 +195,6 @@ class ProximityAuthUnlockManagerImplTest : public testing::Test {
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenUnlocked);
}
......@@ -244,7 +243,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenUnlocked);
EXPECT_TRUE(unlock_manager_->IsUnlockAllowed());
......@@ -256,7 +254,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest, IsUnlockAllowed_SignIn_AllGood) {
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
ON_CALL(messenger_, SupportsSignIn()).WillByDefault(Return(true));
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenUnlocked);
......@@ -271,7 +268,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
ON_CALL(messenger_, SupportsSignIn()).WillByDefault(Return(false));
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenUnlocked);
......@@ -286,7 +282,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
ON_CALL(*proximity_monitor(), IsUnlockAllowed()).WillByDefault(Return(false));
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenUnlocked);
......@@ -300,7 +295,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
life_cycle_.set_messenger(nullptr);
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::AUTHENTICATING);
unlock_manager_->OnLifeCycleStateChanged();
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenUnlocked);
EXPECT_FALSE(unlock_manager_->IsUnlockAllowed());
......@@ -323,7 +317,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenLocked);
EXPECT_FALSE(unlock_manager_->IsUnlockAllowed());
......@@ -335,7 +328,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest, IsUnlockAllowed_UserIsSecondary) {
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
unlock_manager_->OnRemoteStatusUpdate({USER_PRESENCE_SECONDARY,
SECURE_SCREEN_LOCK_ENABLED,
TRUST_AGENT_UNSUPPORTED});
......@@ -350,7 +342,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
unlock_manager_->OnRemoteStatusUpdate({USER_PRESENCE_BACKGROUND,
SECURE_SCREEN_LOCK_ENABLED,
TRUST_AGENT_UNSUPPORTED});
......@@ -365,7 +356,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenlockStateUnknown);
EXPECT_FALSE(unlock_manager_->IsUnlockAllowed());
......@@ -378,7 +368,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
unlock_manager_->OnRemoteStatusUpdate(kRemoteScreenlockDisabled);
EXPECT_FALSE(unlock_manager_->IsUnlockAllowed());
......@@ -391,7 +380,6 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
EXPECT_FALSE(unlock_manager_->IsUnlockAllowed());
}
......@@ -471,9 +459,7 @@ TEST_F(
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
SimulateUserPresentState();
unlock_manager_->OnLifeCycleStateChanged();
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::FINDING_CONNECTION);
unlock_manager_->OnLifeCycleStateChanged();
EXPECT_TRUE(proximity_monitor_destroyed());
}
......@@ -483,10 +469,8 @@ TEST_F(
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
unlock_manager_->OnLifeCycleStateChanged();
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
EXPECT_TRUE(proximity_monitor()->started());
}
......@@ -498,22 +482,18 @@ TEST_F(ProximityAuthUnlockManagerImplTest,
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
unlock_manager_->OnLifeCycleStateChanged();
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
EXPECT_TRUE(proximity_monitor()->started());
// Simulate the phone connection being lost. The ProximityMonitor is stale
// and should have been destroyed.
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::FINDING_CONNECTION);
unlock_manager_->OnLifeCycleStateChanged();
EXPECT_TRUE(proximity_monitor_destroyed());
life_cycle_.ChangeState(
RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
unlock_manager_->OnLifeCycleStateChanged();
EXPECT_FALSE(proximity_monitor_destroyed());
EXPECT_TRUE(proximity_monitor()->started());
}
......@@ -548,76 +528,58 @@ TEST_F(ProximityAuthUnlockManagerImplTest, BluetoothAdapterPowerChanges) {
EXPECT_TRUE(life_cycle_.started());
}
TEST_F(ProximityAuthUnlockManagerImplTest,
OnLifeCycleStateChanged_SecureChannelEstablished_RegistersAsObserver) {
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
SimulateUserPresentState();
EXPECT_CALL(messenger_, AddObserver(unlock_manager_.get()));
unlock_manager_->OnLifeCycleStateChanged();
}
TEST_F(ProximityAuthUnlockManagerImplTest,
OnLifeCycleStateChanged_StartsProximityMonitor) {
TEST_F(ProximityAuthUnlockManagerImplTest, StartsProximityMonitor) {
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
SimulateUserPresentState();
EXPECT_TRUE(proximity_monitor()->started());
unlock_manager_->OnLifeCycleStateChanged();
}
TEST_F(ProximityAuthUnlockManagerImplTest,
OnLifeCycleStateChanged_StopsProximityMonitor) {
OnAuthenticationFailed_StopsProximityMonitor) {
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
SimulateUserPresentState();
unlock_manager_->OnLifeCycleStateChanged();
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED);
unlock_manager_->OnLifeCycleStateChanged();
EXPECT_TRUE(proximity_monitor_destroyed());
}
TEST_F(ProximityAuthUnlockManagerImplTest,
OnLifeCycleStateChanged_AuthenticationFailed_UpdatesScreenlockState) {
AuthenticationFailed_UpdatesScreenlockState) {
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
SimulateUserPresentState();
EXPECT_CALL(proximity_auth_client_,
UpdateScreenlockState(ScreenlockState::PHONE_NOT_AUTHENTICATED));
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED);
unlock_manager_->OnLifeCycleStateChanged();
}
TEST_F(ProximityAuthUnlockManagerImplTest,
OnLifeCycleStateChanged_FindingConnection_UpdatesScreenlockState) {
FindingConnection_UpdatesScreenlockState) {
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
// TODO(crbug.com/890047): Remove this once the bug is resolved.
EXPECT_CALL(proximity_auth_client_,
UpdateScreenlockState(ScreenlockState::NO_PHONE));
EXPECT_CALL(proximity_auth_client_,
UpdateScreenlockState(ScreenlockState::BLUETOOTH_CONNECTING));
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
EXPECT_TRUE(life_cycle_.started());
}
TEST_F(ProximityAuthUnlockManagerImplTest,
OnLifeCycleStateChanged_Authenticating_UpdatesScreenlockState) {
Authenticating_UpdatesScreenlockState) {
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
// TODO(crbug.com/890047): Remove this once the bug is resolved.
EXPECT_CALL(proximity_auth_client_,
UpdateScreenlockState(ScreenlockState::NO_PHONE));
EXPECT_CALL(proximity_auth_client_,
UpdateScreenlockState(ScreenlockState::BLUETOOTH_CONNECTING));
unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
EXPECT_TRUE(life_cycle_.started());
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::AUTHENTICATING);
unlock_manager_->OnLifeCycleStateChanged();
}
TEST_F(ProximityAuthUnlockManagerImplTest,
OnDisconnected_UnregistersAsObserver) {
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
SimulateUserPresentState();
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED);
unlock_manager_->OnLifeCycleStateChanged();
EXPECT_CALL(messenger_, RemoveObserver(unlock_manager_.get()))
.Times(testing::AtLeast(1));
unlock_manager_.get()->OnDisconnected();
unlock_manager_->SetRemoteDeviceLifeCycle(nullptr);
}
TEST_F(ProximityAuthUnlockManagerImplTest,
......
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