Commit 48c2b799 authored by Jun Choi's avatar Jun Choi Committed by Commit Bot

Handle Bluetooth device address change

BluetoothAdapter randomizes device address for unpaired devices while
pairing. As so, handle BluetoothDevice address change so that
FidoBleDevice advertising on a different address on pairing mode is not
recognized as a new FidoBleDevice.

Bug: 877344
Change-Id: Ie1dca3d01bbe4c0ca751062b1fa6f446e7055aaa
Reviewed-on: https://chromium-review.googlesource.com/c/1239202Reviewed-by: default avatarJohn Abd-El-Malek <jam@chromium.org>
Reviewed-by: default avatarBalazs Engedy <engedy@chromium.org>
Reviewed-by: default avatarJan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595981}
parent 8f62f362
......@@ -329,3 +329,15 @@ void AuthenticatorRequestDialogModel::DispatchRequestAsync(
delay);
authenticator->dispatched = true;
}
void AuthenticatorRequestDialogModel::UpdateAuthenticatorReferenceId(
base::StringPiece old_authenticator_id,
std::string new_authenticator_id) {
auto it = std::find_if(
saved_authenticators_.begin(), saved_authenticators_.end(),
[old_authenticator_id](const auto& authenticator) {
return authenticator.authenticator_id == old_authenticator_id;
});
if (it != saved_authenticators_.end())
it->authenticator_id = std::move(new_authenticator_id);
}
......@@ -258,6 +258,9 @@ class AuthenticatorRequestDialogModel {
void SetBluetoothAdapterPowerOnCallback(
base::RepeatingClosure bluetooth_adapter_power_on_callback);
void UpdateAuthenticatorReferenceId(base::StringPiece old_authenticator_id,
std::string new_authenticator_id);
std::vector<AuthenticatorReference>& saved_authenticators() {
return saved_authenticators_;
}
......
......@@ -319,6 +319,16 @@ void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorRemoved(
});
}
void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorIdChanged(
base::StringPiece old_authenticator_id,
std::string new_authenticator_id) {
if (!weak_dialog_model_)
return;
weak_dialog_model_->UpdateAuthenticatorReferenceId(
old_authenticator_id, std::move(new_authenticator_id));
}
void ChromeAuthenticatorRequestDelegate::BluetoothAdapterPowerChanged(
bool is_powered_on) {
if (!weak_dialog_model_)
......
......@@ -86,6 +86,8 @@ class ChromeAuthenticatorRequestDelegate
void FidoAuthenticatorAdded(
const device::FidoAuthenticator& authenticator) override;
void FidoAuthenticatorRemoved(base::StringPiece authenticator_id) override;
void FidoAuthenticatorIdChanged(base::StringPiece old_authenticator_id,
std::string new_authenticator_id) override;
void BluetoothAdapterPowerChanged(bool is_powered_on) override;
// AuthenticatorRequestDialogModel::Observer:
......
......@@ -66,4 +66,8 @@ void AuthenticatorRequestClientDelegate::FidoAuthenticatorAdded(
void AuthenticatorRequestClientDelegate::FidoAuthenticatorRemoved(
base::StringPiece device_id) {}
void AuthenticatorRequestClientDelegate::FidoAuthenticatorIdChanged(
base::StringPiece old_authenticator_id,
std::string new_authenticator_id) {}
} // namespace content
......@@ -115,6 +115,8 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
void FidoAuthenticatorAdded(
const device::FidoAuthenticator& authenticator) override;
void FidoAuthenticatorRemoved(base::StringPiece device_id) override;
void FidoAuthenticatorIdChanged(base::StringPiece old_authenticator_id,
std::string new_authenticator_id) override;
private:
DISALLOW_COPY_AND_ASSIGN(AuthenticatorRequestClientDelegate);
......
......@@ -471,6 +471,13 @@ void FidoBleConnection::OnStartNotifySessionError(
std::move(pending_connection_callback_).Run(false);
}
void FidoBleConnection::DeviceAddressChanged(BluetoothAdapter* adapter,
BluetoothDevice* device,
const std::string& old_address) {
if (address_ == old_address)
address_ = device->GetAddress();
}
void FidoBleConnection::GattCharacteristicValueChanged(
BluetoothAdapter* adapter,
BluetoothRemoteGattCharacteristic* characteristic,
......
......@@ -77,6 +77,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleConnection
private:
// BluetoothAdapter::Observer:
void DeviceAddressChanged(BluetoothAdapter* adapter,
BluetoothDevice* device,
const std::string& old_address) override;
void GattCharacteristicValueChanged(
BluetoothAdapter* adapter,
BluetoothRemoteGattCharacteristic* characteristic,
......
......@@ -209,6 +209,15 @@ class FidoBleConnectionTest : public ::testing::Test {
adapter_->NotifyGattServicesDiscovered(fido_device_);
}
void ChangeDeviceAddressAndNotifyObservers(std::string new_address) {
auto old_address = fido_device_->GetAddress();
EXPECT_CALL(*fido_device_, GetAddress)
.WillRepeatedly(::testing::Return(new_address));
for (auto& observer : adapter_->GetObservers())
observer.DeviceAddressChanged(adapter_.get(), fido_device_,
std::move(old_address));
}
void SetNextReadControlPointLengthReponse(bool success,
const std::vector<uint8_t>& value) {
EXPECT_CALL(*fido_control_point_length_, ReadRemoteCharacteristic(_, _))
......@@ -735,4 +744,15 @@ TEST_F(FidoBleConnectionTest, ReadsAndWriteFailWhenDisconnected) {
EXPECT_FALSE(write_callback.value());
}
TEST_F(FidoBleConnectionTest, ConnectionAddressChangeWhenDeviceAddressChanges) {
const std::string device_address = BluetoothTest::kTestDeviceAddress1;
static constexpr char kTestDeviceAddress2[] = "test_device_address_2";
AddFidoDevice(device_address);
SetupConnectingFidoDevice(device_address);
FidoBleConnection connection(adapter(), device_address, base::DoNothing());
ChangeDeviceAddressAndNotifyObservers(kTestDeviceAddress2);
EXPECT_EQ(kTestDeviceAddress2, connection.address());
}
} // namespace device
......@@ -93,6 +93,26 @@ void FidoBleDiscovery::AdapterPoweredChanged(BluetoothAdapter* adapter,
OnSetPowered();
}
void FidoBleDiscovery::DeviceAddressChanged(BluetoothAdapter* adapter,
BluetoothDevice* device,
const std::string& old_address) {
auto previous_device_id = FidoBleDevice::GetId(old_address);
auto new_device_id = FidoBleDevice::GetId(device->GetAddress());
auto it = devices_.find(previous_device_id);
if (it == devices_.end())
return;
VLOG(2) << "Discovered FIDO BLE device address change from old address : "
<< old_address << " to new address : " << device->GetAddress();
devices_.emplace(new_device_id, std::move(it->second));
devices_.erase(it);
if (observer()) {
observer()->DeviceIdChanged(this, previous_device_id,
std::move(new_device_id));
}
}
bool FidoBleDiscovery::CheckForExcludedDeviceAndCacheAddress(
const BluetoothDevice* device) {
std::string device_address = device->GetAddress();
......
......@@ -38,6 +38,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleDiscovery
void DeviceRemoved(BluetoothAdapter* adapter,
BluetoothDevice* device) override;
void AdapterPoweredChanged(BluetoothAdapter* adapter, bool powered) override;
void DeviceAddressChanged(BluetoothAdapter* adapter,
BluetoothDevice* device,
const std::string& old_address) override;
// Returns true if |device| is a Cable device. If so, add address of |device|
// to |blacklisted_cable_device_addresses_|.
......
......@@ -5,6 +5,7 @@
#include "device/fido/ble/fido_ble_discovery.h"
#include <string>
#include <vector>
#include "base/bind.h"
#include "base/run_loop.h"
......@@ -12,7 +13,9 @@
#include "device/bluetooth/bluetooth_adapter_factory.h"
#include "device/bluetooth/test/bluetooth_test.h"
#include "device/bluetooth/test/mock_bluetooth_adapter.h"
#include "device/bluetooth/test/mock_bluetooth_device.h"
#include "device/fido/ble/fido_ble_device.h"
#include "device/fido/ble/fido_ble_uuids.h"
#include "device/fido/mock_fido_discovery_observer.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -232,4 +235,52 @@ TEST_F(BluetoothTest, FidoBleDiscoveryRejectsCableDevice) {
SimulateLowEnergyDevice(7);
}
TEST_F(BluetoothTest, DiscoveryDoesNotAddDuplicateDeviceOnAddressChanged) {
using TestMockDevice = ::testing::NiceMock<MockBluetoothDevice>;
static constexpr char kDeviceName[] = "device_name";
static constexpr char kDeviceAddress[] = "device_address";
static constexpr char kAuthenticatorId[] = "ble:device_address";
static constexpr char kDeviceChangedAddress[] = "device_changed_address";
static constexpr char kAuthenticatorChangedId[] =
"ble:device_changed_address";
MockFidoDiscoveryObserver observer;
FidoBleDiscovery discovery;
discovery.set_observer(&observer);
auto mock_adapter =
base::MakeRefCounted<::testing::NiceMock<MockBluetoothAdapter>>();
EXPECT_CALL(*mock_adapter, IsPresent()).WillOnce(::testing::Return(true));
auto mock_device = std::make_unique<TestMockDevice>(
mock_adapter.get(), 0 /* bluetooth_class */, kDeviceName, kDeviceAddress,
false /* paired */, false /* connected */);
EXPECT_CALL(*mock_device.get(), GetUUIDs)
.WillRepeatedly(::testing::Return(
std::vector<BluetoothUUID>{BluetoothUUID(kFidoServiceUUID)}));
BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter.get());
EXPECT_CALL(observer, DeviceIdChanged(&discovery, kAuthenticatorId,
kAuthenticatorChangedId));
discovery.Start();
EXPECT_CALL(*mock_device.get(), GetAddress)
.WillRepeatedly(::testing::Return(kDeviceAddress));
mock_adapter->NotifyDeviceChanged(mock_device.get());
ASSERT_TRUE(::testing::Mock::VerifyAndClearExpectations(mock_device.get()));
EXPECT_CALL(*mock_device.get(), GetAddress)
.WillRepeatedly(::testing::Return(kDeviceChangedAddress));
for (auto& observer : mock_adapter->GetObservers()) {
observer.DeviceAddressChanged(mock_adapter.get(), mock_device.get(),
kDeviceAddress);
}
mock_adapter->NotifyDeviceChanged(mock_device.get());
EXPECT_EQ(1u, discovery.GetDevices().size());
EXPECT_TRUE(discovery.GetDevice(kAuthenticatorChangedId));
}
} // namespace device
......@@ -76,4 +76,16 @@ void FidoBlePairingDelegate::StoreBlePinCodeForDevice(
std::move(pin_code));
}
void FidoBlePairingDelegate::ChangeStoredDeviceAddress(
const std::string& old_address,
std::string new_address) {
auto it = bluetooth_device_pincode_map_.find(old_address);
if (it != bluetooth_device_pincode_map_.end()) {
std::string pincode = std::move(it->second);
bluetooth_device_pincode_map_.erase(it);
bluetooth_device_pincode_map_.insert_or_assign(std::move(new_address),
std::move(pincode));
}
}
} // namespace device
......@@ -40,8 +40,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBlePairingDelegate
void StoreBlePinCodeForDevice(std::string device_address,
std::string pin_code);
void ChangeStoredDeviceAddress(const std::string& old_address,
std::string new_address);
private:
friend class FidoBlePairingDelegateTest;
base::flat_map<std::string, std::string> bluetooth_device_pincode_map_;
DISALLOW_COPY_AND_ASSIGN(FidoBlePairingDelegate);
......
......@@ -20,6 +20,8 @@ constexpr char kTestPinCode[] = "1234_abcd";
constexpr uint32_t kTestPassKey = 1234;
constexpr char kTestBluetoothDeviceName[] = "device_name";
} // namespace
class FidoBlePairingDelegateTest : public ::testing::Test {
public:
FidoBlePairingDelegate* pairing_delegate() { return pairing_delegate_.get(); }
......@@ -27,6 +29,11 @@ class FidoBlePairingDelegateTest : public ::testing::Test {
return mock_bluetooth_device_.get();
}
const base::flat_map<std::string, std::string>&
pairing_delegate_pincode_map() {
return pairing_delegate_->bluetooth_device_pincode_map_;
}
private:
std::unique_ptr<FidoBlePairingDelegate> pairing_delegate_ =
std::make_unique<FidoBlePairingDelegate>();
......@@ -39,8 +46,6 @@ class FidoBlePairingDelegateTest : public ::testing::Test {
false /* connected */);
};
} // namespace
TEST_F(FidoBlePairingDelegateTest, PairingWithPin) {
pairing_delegate()->StoreBlePinCodeForDevice(kTestFidoBleDeviceId,
kTestPinCode);
......@@ -89,4 +94,22 @@ TEST_F(FidoBlePairingDelegateTest, RejectConfirmPassKey) {
pairing_delegate()->ConfirmPasskey(mock_bluetooth_device(), kTestPassKey);
}
TEST_F(FidoBlePairingDelegateTest, ChangeStoredDeviceAddress) {
static constexpr char kTestNewBleDeviceAddress[] =
"ble:test_changed_device_address";
pairing_delegate()->StoreBlePinCodeForDevice(kTestFidoBleDeviceId,
kTestPinCode);
EXPECT_TRUE(
base::ContainsKey(pairing_delegate_pincode_map(), kTestFidoBleDeviceId));
EXPECT_FALSE(base::ContainsKey(pairing_delegate_pincode_map(),
kTestNewBleDeviceAddress));
pairing_delegate()->ChangeStoredDeviceAddress(kTestFidoBleDeviceId,
kTestNewBleDeviceAddress);
EXPECT_FALSE(
base::ContainsKey(pairing_delegate_pincode_map(), kTestFidoBleDeviceId));
EXPECT_TRUE(base::ContainsKey(pairing_delegate_pincode_map(),
kTestNewBleDeviceAddress));
}
} // namespace device
......@@ -33,6 +33,9 @@ class MockTransportAvailabilityObserver
MOCK_METHOD1(FidoAuthenticatorAdded,
void(const FidoAuthenticator& authenticator));
MOCK_METHOD1(FidoAuthenticatorRemoved, void(base::StringPiece device_id));
MOCK_METHOD2(FidoAuthenticatorIdChanged,
void(base::StringPiece old_authenticator_id,
std::string new_authenticator_id));
private:
DISALLOW_COPY_AND_ASSIGN(MockTransportAvailabilityObserver);
......
......@@ -9,6 +9,7 @@
#include <map>
#include <memory>
#include <string>
#include <utility>
#include <vector>
#include "base/component_export.h"
......@@ -58,6 +59,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
virtual void DeviceAdded(FidoDiscovery* discovery, FidoDevice* device) = 0;
virtual void DeviceRemoved(FidoDiscovery* discovery,
FidoDevice* device) = 0;
// Invoked when address of the connected FIDO Bluetooth device changes due
// to pairing.
virtual void DeviceIdChanged(FidoDiscovery* discovery,
const std::string& previous_id,
std::string new_id) = 0;
};
// Factory functions to construct an instance that discovers authenticators on
......
......@@ -208,6 +208,21 @@ void FidoRequestHandlerBase::DeviceRemoved(FidoDiscovery* discovery,
observer_->FidoAuthenticatorRemoved(device->GetId());
}
void FidoRequestHandlerBase::DeviceIdChanged(FidoDiscovery* discovery,
const std::string& previous_id,
std::string new_id) {
DCHECK_EQ(FidoTransportProtocol::kBluetoothLowEnergy, discovery->transport());
auto it = active_authenticators_.find(previous_id);
if (it == active_authenticators_.end())
return;
active_authenticators_.emplace(new_id, std::move(it->second));
active_authenticators_.erase(it);
if (observer_)
observer_->FidoAuthenticatorIdChanged(previous_id, std::move(new_id));
}
void FidoRequestHandlerBase::AddAuthenticator(
std::unique_ptr<FidoAuthenticator> authenticator) {
DCHECK(authenticator &&
......
......@@ -110,6 +110,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
virtual void FidoAuthenticatorAdded(
const FidoAuthenticator& authenticator) = 0;
virtual void FidoAuthenticatorRemoved(base::StringPiece device_id) = 0;
virtual void FidoAuthenticatorIdChanged(
base::StringPiece old_authenticator_id,
std::string new_authenticator_id) = 0;
};
// TODO(https://crbug.com/769631): Remove the dependency on Connector once
......@@ -181,9 +184,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
TransportAvailabilityObserver* observer() const { return observer_; }
private:
friend class FidoRequestHandlerTest;
// FidoDiscovery::Observer
void DeviceAdded(FidoDiscovery* discovery, FidoDevice* device) final;
void DeviceRemoved(FidoDiscovery* discovery, FidoDevice* device) final;
void DeviceIdChanged(FidoDiscovery* discovery,
const std::string& previous_id,
std::string new_id) final;
void AddAuthenticator(std::unique_ptr<FidoAuthenticator> authenticator);
void NotifyObserverTransportAvailability();
......
......@@ -3,6 +3,7 @@
// found in the LICENSE file.
#include <memory>
#include <string>
#include <utility>
#include <vector>
......@@ -53,6 +54,8 @@ class TestTransportAvailabilityObserver
public:
using TransportAvailabilityNotificationReceiver = test::TestCallbackReceiver<
FidoRequestHandlerBase::TransportAvailabilityInfo>;
using AuthenticatorIdChangeNotificationReceiver =
test::TestCallbackReceiver<std::string>;
TestTransportAvailabilityObserver() {}
~TestTransportAvailabilityObserver() override {}
......@@ -72,6 +75,14 @@ class TestTransportAvailabilityObserver
}
}
void WaitForAuthenticatorIdChangeNotification(
base::StringPiece expected_new_authenticator_id) {
authenticator_id_change_notification_receiver_.WaitForCallback();
auto result =
std::get<0>(*authenticator_id_change_notification_receiver_.result());
EXPECT_EQ(expected_new_authenticator_id, result);
}
protected:
// FidoRequestHandlerBase::TransportAvailabilityObserver:
void OnTransportAvailabilityEnumerated(
......@@ -88,10 +99,17 @@ class TestTransportAvailabilityObserver
void FidoAuthenticatorAdded(const FidoAuthenticator& authenticator) override {
}
void FidoAuthenticatorRemoved(base::StringPiece device_id) override {}
void FidoAuthenticatorIdChanged(base::StringPiece old_authenticator_id,
std::string new_authenticator_id) override {
authenticator_id_change_notification_receiver_.callback().Run(
std::move(new_authenticator_id));
}
private:
TransportAvailabilityNotificationReceiver
transport_availability_notification_receiver_;
AuthenticatorIdChangeNotificationReceiver
authenticator_id_change_notification_receiver_;
DISALLOW_COPY_AND_ASSIGN(TestTransportAvailabilityObserver);
};
......@@ -223,6 +241,13 @@ class FidoRequestHandlerTest : public ::testing::Test {
return handler;
}
void ChangeAuthenticatorId(FakeFidoRequestHandler* request_handler,
FidoDevice* device,
std::string new_authenticator_id) {
request_handler->DeviceIdChanged(ble_discovery_, device->GetId(),
std::move(new_authenticator_id));
}
test::FakeFidoDiscovery* discovery() const { return discovery_; }
test::FakeFidoDiscovery* ble_discovery() const { return ble_discovery_; }
scoped_refptr<::testing::NiceMock<MockBluetoothAdapter>> adapter() {
......@@ -587,4 +612,20 @@ TEST_F(FidoRequestHandlerTest,
FidoTransportProtocol::kBluetoothLowEnergy});
}
TEST_F(FidoRequestHandlerTest, EmbedderNotifiedWhenAuthenticatorIdChanges) {
static constexpr char kNewAuthenticatorId[] = "new_authenticator_id";
TestTransportAvailabilityObserver observer;
auto request_handler = CreateFakeHandler();
request_handler->set_observer(&observer);
ble_discovery()->WaitForCallToStartAndSimulateSuccess();
auto device = std::make_unique<MockFidoDevice>();
auto* device_ptr = device.get();
EXPECT_CALL(*device, GetId()).WillRepeatedly(testing::Return("device0"));
discovery()->AddDevice(std::move(device));
ChangeAuthenticatorId(request_handler.get(), device_ptr, kNewAuthenticatorId);
observer.WaitForAuthenticatorIdChangeNotification(kNewAuthenticatorId);
}
} // namespace device
......@@ -5,6 +5,8 @@
#ifndef DEVICE_FIDO_MOCK_FIDO_DISCOVERY_OBSERVER_H_
#define DEVICE_FIDO_MOCK_FIDO_DISCOVERY_OBSERVER_H_
#include <string>
#include "base/component_export.h"
#include "base/macros.h"
#include "device/fido/fido_discovery.h"
......@@ -23,6 +25,8 @@ class MockFidoDiscoveryObserver : public FidoDiscovery::Observer {
MOCK_METHOD2(DiscoveryStopped, void(FidoDiscovery*, bool));
MOCK_METHOD2(DeviceAdded, void(FidoDiscovery*, FidoDevice*));
MOCK_METHOD2(DeviceRemoved, void(FidoDiscovery*, FidoDevice*));
MOCK_METHOD3(DeviceIdChanged,
void(FidoDiscovery*, const std::string&, std::string));
private:
DISALLOW_COPY_AND_ASSIGN(MockFidoDiscoveryObserver);
......
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