fido: move FidoAuthenticator instantiation into FidoDiscovery

This change extracts an abstract base class called FidoDiscoveryBase
from FidoDiscovery. The FidoDiscovery::Observer interface is moved into
the base class, and its signature is changed to allow observing the
instantiation and destruction of FidoAuthenticator instances rather than
FidoDevice. I.e. FidoDiscovery::Observer::Device{Added,Removed} becomes
FidoDiscoveryBase::Observer::Authenticator{Added,Removed}.

FidoDiscovery takes on the responsibility of instantiating and owning
FidoAuthenticator instances (more specifically, FidoDeviceAuthenticator
instances). The AuthenticatorMap in FidoRequestHandlerBase now only
holds plain FidoAuthenticator pointers, rather than owning the
unique_ptrs.

This gets us closer to getting platform-y authenticators (Touch ID,
Windows) injected via Discovery implementations, rather than
implementing custom interaction between FidoRequestHandlerBase and
AuthenticatorImpl for them (see
SetPlatformAuthenticatorOrMarkUnavailable).

Better naming might have been "FidoDeviceDiscovery" for FidoDiscovery
and perhaps "FidoDiscovery" for FidoDiscoveryBase, but that would be a
rather large diff.

A sensible additional refactoring might be to make
FidoDeviceAuthenticator own its associated FidoDevice.

Change-Id: I05d9c24819745c784fef3898bec8d6bb63d2f503
Reviewed-on: https://chromium-review.googlesource.com/c/1256016
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596882}

device/fido/BUILD.gn

@@ -73,6 +73,8 @@ component("fido") {
     "fido_device_authenticator.h",
     "fido_discovery.cc",
     "fido_discovery.h",
+    "fido_discovery_base.cc",
+    "fido_discovery_base.h",
     "fido_parsing_utils.cc",
     "fido_parsing_utils.h",
     "fido_request_handler.h",

device/fido/ble/fido_ble_discovery.cc

@@ -14,6 +14,7 @@
 #include "device/bluetooth/bluetooth_uuid.h"
 #include "device/fido/ble/fido_ble_device.h"
 #include "device/fido/ble/fido_ble_uuids.h"
+#include "device/fido/fido_authenticator.h"
 
 namespace device {
 
@@ -108,8 +109,8 @@ void FidoBleDiscovery::DeviceAddressChanged(BluetoothAdapter* adapter,
   devices_.erase(it);
 
   if (observer()) {
-    observer()->DeviceIdChanged(this, previous_device_id,
-                                std::move(new_device_id));
+    observer()->AuthenticatorIdChanged(this, previous_device_id,
+                                       std::move(new_device_id));
   }
 }
 

device/fido/ble/fido_ble_discovery_unittest.cc

@@ -16,6 +16,7 @@
 #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/fido_authenticator.h"
 #include "device/fido/mock_fido_discovery_observer.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -82,8 +83,8 @@ TEST_F(BluetoothTest, FidoBleDiscoveryNoAdapter) {
   MockFidoDiscoveryObserver observer;
   discovery.set_observer(&observer);
   EXPECT_CALL(observer, DiscoveryStarted(&discovery, _)).Times(0);
-  EXPECT_CALL(observer, DeviceAdded(&discovery, _)).Times(0);
-  EXPECT_CALL(observer, DeviceRemoved(&discovery, _)).Times(0);
+  EXPECT_CALL(observer, AuthenticatorAdded(&discovery, _)).Times(0);
+  EXPECT_CALL(observer, AuthenticatorRemoved(&discovery, _)).Times(0);
 }
 
 TEST_F(BluetoothTest, FidoBleDiscoveryFindsKnownDevice) {
@@ -104,9 +105,10 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsKnownDevice) {
   {
     base::RunLoop run_loop;
     auto quit = run_loop.QuitClosure();
-    EXPECT_CALL(observer,
-                DeviceAdded(&discovery,
-                            IdMatches(BluetoothTestBase::kTestDeviceAddress1)));
+    EXPECT_CALL(
+        observer,
+        AuthenticatorAdded(&discovery,
+                           IdMatches(BluetoothTestBase::kTestDeviceAddress1)));
     EXPECT_CALL(observer, DiscoveryStarted(&discovery, true))
         .WillOnce(ReturnFromAsyncCall(quit));
 
@@ -139,9 +141,10 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsNewDevice) {
   {
     base::RunLoop run_loop;
     auto quit = run_loop.QuitClosure();
-    EXPECT_CALL(observer,
-                DeviceAdded(&discovery,
-                            IdMatches(BluetoothTestBase::kTestDeviceAddress1)))
+    EXPECT_CALL(
+        observer,
+        AuthenticatorAdded(&discovery,
+                           IdMatches(BluetoothTestBase::kTestDeviceAddress1)))
         .WillOnce(ReturnFromAsyncCall(quit));
 
     SimulateLowEnergyDevice(4);  // This device should be ignored.
@@ -184,9 +187,10 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsUpdatedDevice) {
   {
     base::RunLoop run_loop;
     auto quit = run_loop.QuitClosure();
-    EXPECT_CALL(observer,
-                DeviceAdded(&discovery,
-                            IdMatches(BluetoothTestBase::kTestDeviceAddress1)))
+    EXPECT_CALL(
+        observer,
+        AuthenticatorAdded(&discovery,
+                           IdMatches(BluetoothTestBase::kTestDeviceAddress1)))
         .WillOnce(ReturnFromAsyncCall(quit));
 
     // This will update properties for device 3.
@@ -222,7 +226,7 @@ TEST_F(BluetoothTest, FidoBleDiscoveryRejectsCableDevice) {
     run_loop.Run();
   }
 
-  EXPECT_CALL(observer, DeviceAdded(&discovery, _)).Times(0);
+  EXPECT_CALL(observer, AuthenticatorAdded(&discovery, _)).Times(0);
 
   // Simulates a discovery of two Cable devices one of which is an Android Cable
   // authenticator and other is IOS Cable authenticator.
@@ -231,7 +235,7 @@ TEST_F(BluetoothTest, FidoBleDiscoveryRejectsCableDevice) {
 
   // Simulates a device change update received from the BluetoothAdapter. As the
   // updated device has an address that we know is an Cable device, this should
-  // not trigger DeviceAdded().
+  // not trigger AuthenticatorAdded().
   SimulateLowEnergyDevice(7);
 }
 
@@ -261,8 +265,8 @@ TEST_F(BluetoothTest, DiscoveryDoesNotAddDuplicateDeviceOnAddressChanged) {
           std::vector<BluetoothUUID>{BluetoothUUID(kFidoServiceUUID)}));
 
   BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter.get());
-  EXPECT_CALL(observer, DeviceIdChanged(&discovery, kAuthenticatorId,
-                                        kAuthenticatorChangedId));
+  EXPECT_CALL(observer, AuthenticatorIdChanged(&discovery, kAuthenticatorId,
+                                               kAuthenticatorChangedId));
   discovery.Start();
 
   EXPECT_CALL(*mock_device.get(), GetAddress)

device/fido/cable/fido_cable_discovery_unittest.cc

@@ -317,7 +317,7 @@ class FidoCableDiscoveryTest : public ::testing::Test {
 TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewDevice) {
   auto cable_discovery = CreateDiscovery();
   NiceMock<MockFidoDiscoveryObserver> mock_observer;
-  EXPECT_CALL(mock_observer, DeviceAdded(_, _));
+  EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
   cable_discovery->set_observer(&mock_observer);
 
   auto mock_adapter =
@@ -336,7 +336,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewDevice) {
 TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewAppleDevice) {
   auto cable_discovery = CreateDiscovery();
   NiceMock<MockFidoDiscoveryObserver> mock_observer;
-  EXPECT_CALL(mock_observer, DeviceAdded(_, _));
+  EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
   cable_discovery->set_observer(&mock_observer);
 
   auto mock_adapter =
@@ -357,7 +357,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewAppleDevice) {
 TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsIncorrectDevice) {
   auto cable_discovery = CreateDiscovery();
   NiceMock<MockFidoDiscoveryObserver> mock_observer;
-  EXPECT_CALL(mock_observer, DeviceAdded(_, _)).Times(0);
+  EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0);
   cable_discovery->set_observer(&mock_observer);
 
   auto mock_adapter =
@@ -395,7 +395,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithMultipleEids) {
   mock_adapter->ExpectDiscoveryWithScanCallback(kAuthenticatorEid);
 
   NiceMock<MockFidoDiscoveryObserver> mock_observer;
-  EXPECT_CALL(mock_observer, DeviceAdded(_, _));
+  EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
   cable_discovery->set_observer(&mock_observer);
 
   Sequence sequence;
@@ -424,7 +424,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithPartialAdvertisementSuccess) {
   auto cable_discovery =
       std::make_unique<FakeFidoCableDiscovery>(std::move(discovery_data));
   NiceMock<MockFidoDiscoveryObserver> mock_observer;
-  EXPECT_CALL(mock_observer, DeviceAdded(_, _));
+  EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
   cable_discovery->set_observer(&mock_observer);
 
   auto mock_adapter =
@@ -456,7 +456,7 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithAdvertisementFailures) {
       std::make_unique<FakeFidoCableDiscovery>(std::move(discovery_data));
 
   NiceMock<MockFidoDiscoveryObserver> mock_observer;
-  EXPECT_CALL(mock_observer, DeviceAdded(_, _)).Times(0);
+  EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0);
   cable_discovery->set_observer(&mock_observer);
 
   auto mock_adapter =
@@ -504,7 +504,7 @@ TEST_F(FidoCableDiscoveryTest, TestUnregisterAdvertisementUponDestruction) {
 TEST_F(FidoCableDiscoveryTest, TestResumeDiscoveryAfterPoweredOn) {
   auto cable_discovery = CreateDiscovery();
   NiceMock<MockFidoDiscoveryObserver> mock_observer;
-  EXPECT_CALL(mock_observer, DeviceAdded);
+  EXPECT_CALL(mock_observer, AuthenticatorAdded);
   cable_discovery->set_observer(&mock_observer);
 
   auto mock_adapter =

device/fido/fake_fido_discovery_unittest.cc

@@ -20,6 +20,8 @@
 namespace device {
 namespace test {
 
+using ::testing::_;
+
 class FakeFidoDiscoveryTest : public ::testing::Test {
  public:
   FakeFidoDiscoveryTest() = default;
@@ -119,7 +121,7 @@ TEST_F(FakeFidoDiscoveryTest, AddDevice) {
   auto device0 = std::make_unique<MockFidoDevice>();
   EXPECT_CALL(*device0, GetId()).WillOnce(::testing::Return("device0"));
   base::RunLoop device0_done;
-  EXPECT_CALL(observer, DeviceAdded(&discovery, ::testing::_))
+  EXPECT_CALL(observer, AuthenticatorAdded(&discovery, _))
       .WillOnce(testing::InvokeWithoutArgs(
           [&device0_done]() { device0_done.Quit(); }));
   discovery.AddDevice(std::move(device0));
@@ -133,7 +135,7 @@ TEST_F(FakeFidoDiscoveryTest, AddDevice) {
   auto device1 = std::make_unique<MockFidoDevice>();
   EXPECT_CALL(*device1, GetId()).WillOnce(::testing::Return("device1"));
   base::RunLoop device1_done;
-  EXPECT_CALL(observer, DeviceAdded(&discovery, ::testing::_))
+  EXPECT_CALL(observer, AuthenticatorAdded(&discovery, _))
       .WillOnce(testing::InvokeWithoutArgs(
           [&device1_done]() { device1_done.Quit(); }));
   discovery.AddDevice(std::move(device1));

device/fido/fido_device_authenticator.h

@@ -34,9 +34,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
 
   // FidoAuthenticator:
   void InitializeAuthenticator(base::OnceClosure callback) override;
-  void MakeCredential(
-      CtapMakeCredentialRequest request,
-      MakeCredentialCallback callback) override;
+  void MakeCredential(CtapMakeCredentialRequest request,
+                      MakeCredentialCallback callback) override;
   void GetAssertion(CtapGetAssertionRequest request,
                     GetAssertionCallback callback) override;
   void Cancel() override;
@@ -46,6 +45,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   FidoTransportProtocol AuthenticatorTransport() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 
+  FidoDevice* GetDeviceForTesting() { return device_; }
+
  protected:
   void OnCtapMakeCredentialResponseReceived(
       MakeCredentialCallback callback,

device/fido/fido_discovery.cc

@@ -10,7 +10,9 @@
 #include "build/build_config.h"
 #include "device/fido/ble/fido_ble_discovery.h"
 #include "device/fido/cable/fido_cable_discovery.h"
+#include "device/fido/fido_authenticator.h"
 #include "device/fido/fido_device.h"
+#include "device/fido/fido_device_authenticator.h"
 
 // HID is not supported on Android.
 #if !defined(OS_ANDROID)
@@ -81,7 +83,7 @@ std::unique_ptr<FidoDiscovery> FidoDiscovery::CreateCable(
 }
 
 FidoDiscovery::FidoDiscovery(FidoTransportProtocol transport)
-    : transport_(transport), weak_factory_(this) {}
+    : FidoDiscoveryBase(transport), weak_factory_(this) {}
 
 FidoDiscovery::~FidoDiscovery() = default;
 
@@ -98,30 +100,31 @@ void FidoDiscovery::NotifyDiscoveryStarted(bool success) {
   DCHECK_EQ(state_, State::kStarting);
   if (success)
     state_ = State::kRunning;
-  if (!observer_)
+  if (!observer())
     return;
-  observer_->DiscoveryStarted(this, success);
+  observer()->DiscoveryStarted(this, success);
 }
 
-void FidoDiscovery::NotifyDeviceAdded(FidoDevice* device) {
+void FidoDiscovery::NotifyAuthenticatorAdded(FidoAuthenticator* authenticator) {
   DCHECK_NE(state_, State::kIdle);
-  if (!observer_)
+  if (!observer())
     return;
-  observer_->DeviceAdded(this, device);
+  observer()->AuthenticatorAdded(this, authenticator);
 }
 
-void FidoDiscovery::NotifyDeviceRemoved(FidoDevice* device) {
+void FidoDiscovery::NotifyAuthenticatorRemoved(
+    FidoAuthenticator* authenticator) {
   DCHECK_NE(state_, State::kIdle);
-  if (!observer_)
+  if (!observer())
     return;
-  observer_->DeviceRemoved(this, device);
+  observer()->AuthenticatorRemoved(this, authenticator);
 }
 
 std::vector<FidoDevice*> FidoDiscovery::GetDevices() {
   std::vector<FidoDevice*> devices;
   devices.reserve(devices_.size());
   for (const auto& device : devices_)
-    devices.push_back(device.second.get());
+    devices.push_back(device.second.second.get());
   return devices;
 }
 
@@ -129,7 +132,7 @@ std::vector<const FidoDevice*> FidoDiscovery::GetDevices() const {
   std::vector<const FidoDevice*> devices;
   devices.reserve(devices_.size());
   for (const auto& device : devices_)
-    devices.push_back(device.second.get());
+    devices.push_back(device.second.second.get());
   return devices;
 }
 
@@ -140,17 +143,20 @@ FidoDevice* FidoDiscovery::GetDevice(base::StringPiece device_id) {
 
 const FidoDevice* FidoDiscovery::GetDevice(base::StringPiece device_id) const {
   auto found = devices_.find(device_id);
-  return found != devices_.end() ? found->second.get() : nullptr;
+  return found != devices_.end() ? found->second.second.get() : nullptr;
 }
 
 bool FidoDiscovery::AddDevice(std::unique_ptr<FidoDevice> device) {
   std::string device_id = device->GetId();
-  const auto result = devices_.emplace(std::move(device_id), std::move(device));
+  auto authenticator = std::make_unique<FidoDeviceAuthenticator>(device.get());
+  const auto result = devices_.emplace(
+      std::move(device_id),
+      std::make_pair(std::move(authenticator), std::move(device)));
   if (!result.second) {
     return false;  // Duplicate device id.
   }
 
-  NotifyDeviceAdded(result.first->second.get());
+  NotifyAuthenticatorAdded(result.first->second.first.get());
   return true;
 }
 
@@ -159,9 +165,9 @@ bool FidoDiscovery::RemoveDevice(base::StringPiece device_id) {
   if (found == devices_.end())
     return false;
 
-  auto device = std::move(found->second);
+  auto authenticator_device_pair = std::move(found->second);
   devices_.erase(found);
-  NotifyDeviceRemoved(device.get());
+  NotifyAuthenticatorRemoved(authenticator_device_pair.first.get());
   return true;
 }
 

device/fido/fido_discovery.h

@@ -18,6 +18,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string_piece.h"
 #include "device/fido/cable/cable_discovery_data.h"
+#include "device/fido/fido_discovery_base.h"
 #include "device/fido/fido_transport_protocol.h"
 
 namespace service_manager {
@@ -32,7 +33,7 @@ namespace internal {
 class ScopedFidoDiscoveryFactory;
 }
 
-class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
+class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery : public FidoDiscoveryBase {
  public:
   enum class State {
     kIdle,
@@ -40,32 +41,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
     kRunning,
   };
 
-  class COMPONENT_EXPORT(DEVICE_FIDO) Observer {
-   public:
-    virtual ~Observer();
-
-    // It is guaranteed that this is never invoked synchronously from Start().
-    virtual void DiscoveryStarted(FidoDiscovery* discovery, bool success) {}
-
-    // It is guaranteed that DeviceAdded/DeviceRemoved() will not be invoked
-    // before the client of FidoDiscovery calls FidoDiscovery::Start(). However,
-    // for devices already known to the system at that point, DeviceAdded()
-    // might already be called to reported already known devices.
-    //
-    // The supplied FidoDevice instance is guaranteed to have its protocol
-    // version initialized. I.e., FidoDiscovery calls
-    // FidoDevice::DiscoverSupportedProtocolAndDeviceInfo() before notifying
-    // the Observer.
-    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
   // the given |transport| protocol. The first variant is for everything except
   // for cloud-assisted BLE which is handled by the second variant.
@@ -78,32 +53,27 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
   static std::unique_ptr<FidoDiscovery> CreateCable(
       std::vector<CableDiscoveryData> cable_data);
 
-  virtual ~FidoDiscovery();
-
-  Observer* observer() const { return observer_; }
-  void set_observer(Observer* observer) {
-    DCHECK(!observer_ || !observer) << "Only one observer is supported.";
-    observer_ = observer;
-  }
+  ~FidoDiscovery() override;
 
-  FidoTransportProtocol transport() const { return transport_; }
   bool is_start_requested() const { return state_ != State::kIdle; }
   bool is_running() const { return state_ == State::kRunning; }
 
-  void Start();
-
   std::vector<FidoDevice*> GetDevices();
   std::vector<const FidoDevice*> GetDevices() const;
 
+  // TODO(martinkr): Rename to GetDeviceForTesting.
   FidoDevice* GetDevice(base::StringPiece device_id);
   const FidoDevice* GetDevice(base::StringPiece device_id) const;
 
+  // FidoDiscoveryBase:
+  void Start() override;
+
  protected:
   FidoDiscovery(FidoTransportProtocol transport);
 
   void NotifyDiscoveryStarted(bool success);
-  void NotifyDeviceAdded(FidoDevice* device);
-  void NotifyDeviceRemoved(FidoDevice* device);
+  void NotifyAuthenticatorAdded(FidoAuthenticator* authenticator);
+  void NotifyAuthenticatorRemoved(FidoAuthenticator* authenticator);
 
   bool AddDevice(std::unique_ptr<FidoDevice> device);
   bool RemoveDevice(base::StringPiece device_id);
@@ -115,8 +85,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
   // the discovery is s tarted.
   virtual void StartInternal() = 0;
 
-  std::map<std::string, std::unique_ptr<FidoDevice>, std::less<>> devices_;
-  Observer* observer_ = nullptr;
+  std::map<std::string,
+           std::pair<std::unique_ptr<FidoAuthenticator>,
+                     std::unique_ptr<FidoDevice>>,
+           std::less<>>
+      devices_;
 
  private:
   friend class internal::ScopedFidoDiscoveryFactory;
@@ -127,7 +100,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
   static FactoryFuncPtr g_factory_func_;
   static CableFactoryFuncPtr g_cable_factory_func_;
 
-  const FidoTransportProtocol transport_;
   State state_ = State::kIdle;
   base::WeakPtrFactory<FidoDiscovery> weak_factory_;
 

device/fido/fido_discovery_base.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "device/fido/fido_discovery_base.h"
+
+namespace device {
+
+FidoDiscoveryBase::FidoDiscoveryBase(FidoTransportProtocol transport)
+    : transport_(transport) {}
+FidoDiscoveryBase::~FidoDiscoveryBase() = default;
+
+}  // namespace device

device/fido/fido_discovery_base.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DEVICE_FIDO_FIDO_DISCOVERY_BASE_H_
+#define DEVICE_FIDO_FIDO_DISCOVERY_BASE_H_
+
+#include "base/component_export.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "device/fido/fido_transport_protocol.h"
+
+namespace device {
+
+class FidoAuthenticator;
+
+class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryBase {
+ public:
+  virtual ~FidoDiscoveryBase();
+
+  class COMPONENT_EXPORT(DEVICE_FIDO) Observer {
+   public:
+    virtual ~Observer();
+
+    // It is guaranteed that this is never invoked synchronously from Start().
+    virtual void DiscoveryStarted(FidoDiscoveryBase* discovery, bool success) {}
+
+    // It is guaranteed that AuthenticatorAdded/AuthenticatorRemoved() will not
+    // be invoked before the client of FidoDiscoveryBase calls
+    // FidoDiscoveryBase::Start(). However, for authenticators already known to
+    // the system at that point, AuthenticatorAdded() might already be called to
+    // reported already known devices.
+    virtual void AuthenticatorAdded(FidoDiscoveryBase* discovery,
+                                    FidoAuthenticator* authenticator) = 0;
+    virtual void AuthenticatorRemoved(FidoDiscoveryBase* discovery,
+                                      FidoAuthenticator* authenticator) = 0;
+
+    // Invoked when address of a connected FIDO Bluetooth device changes due
+    // to pairing.
+    virtual void AuthenticatorIdChanged(FidoDiscoveryBase* discovery,
+                                        const std::string& previous_id,
+                                        std::string new_id) = 0;
+  };
+
+  // Start authenticator discovery. The Observer must have been set before this
+  // method is invoked. DiscoveryStarted must be invoked asynchronously from
+  // this method.
+  virtual void Start() = 0;
+
+  Observer* observer() const { return observer_; }
+  void set_observer(Observer* observer) {
+    DCHECK(!observer_ || !observer) << "Only one observer is supported.";
+    observer_ = observer;
+  }
+  FidoTransportProtocol transport() const { return transport_; }
+
+ protected:
+  FidoDiscoveryBase(FidoTransportProtocol transport);
+
+ private:
+  const FidoTransportProtocol transport_;
+  Observer* observer_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(FidoDiscoveryBase);
+};
+
+}  // namespace device
+
+#endif  // DEVICE_FIDO_FIDO_DISCOVERY_BASE_H_

device/fido/fido_discovery_unittest.cc

@@ -9,6 +9,8 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/test/scoped_task_environment.h"
+#include "device/fido/fido_authenticator.h"
+#include "device/fido/fido_device_authenticator.h"
 #include "device/fido/fido_test_data.h"
 #include "device/fido/mock_fido_device.h"
 #include "device/fido/mock_fido_discovery_observer.h"
@@ -20,7 +22,9 @@ namespace device {
 namespace {
 
 using ::testing::_;
+using ::testing::DoAll;
 using ::testing::Return;
+using ::testing::SaveArg;
 using ::testing::UnorderedElementsAre;
 
 // A minimal implementation of FidoDiscovery that is no longer abstract.
@@ -33,11 +37,8 @@ class ConcreteFidoDiscovery : public FidoDiscovery {
   MOCK_METHOD0(StartInternal, void());
 
   using FidoDiscovery::AddDevice;
-  using FidoDiscovery::RemoveDevice;
-
   using FidoDiscovery::NotifyDiscoveryStarted;
-  using FidoDiscovery::NotifyDeviceAdded;
-  using FidoDiscovery::NotifyDeviceRemoved;
+  using FidoDiscovery::RemoveDevice;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ConcreteFidoDiscovery);
@@ -102,12 +103,17 @@ TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
   // Expect successful insertion.
   auto device0 = std::make_unique<MockFidoDevice>();
   auto* device0_raw = device0.get();
+  FidoAuthenticator* authenticator0 = nullptr;
   base::RunLoop device0_done;
-  EXPECT_CALL(observer, DeviceAdded(&discovery, device0_raw))
-      .WillOnce(testing::InvokeWithoutArgs(
-          [&device0_done]() { device0_done.Quit(); }));
-  EXPECT_CALL(*device0, GetId()).WillOnce(Return("device0"));
+  EXPECT_CALL(observer, AuthenticatorAdded(&discovery, _))
+      .WillOnce(DoAll(SaveArg<1>(&authenticator0),
+                      testing::InvokeWithoutArgs(
+                          [&device0_done]() { device0_done.Quit(); })));
+  EXPECT_CALL(*device0, GetId()).WillRepeatedly(Return("device0"));
   EXPECT_TRUE(discovery.AddDevice(std::move(device0)));
+  EXPECT_EQ("device0", authenticator0->GetId());
+  EXPECT_EQ(device0_raw, static_cast<FidoDeviceAuthenticator*>(authenticator0)
+                             ->GetDeviceForTesting());
   device0_done.Run();
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 
@@ -115,17 +121,22 @@ TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
   base::RunLoop device1_done;
   auto device1 = std::make_unique<MockFidoDevice>();
   auto* device1_raw = device1.get();
-  EXPECT_CALL(observer, DeviceAdded(&discovery, device1_raw))
-      .WillOnce(testing::InvokeWithoutArgs(
-          [&device1_done]() { device1_done.Quit(); }));
-  EXPECT_CALL(*device1, GetId()).WillOnce(Return("device1"));
+  FidoAuthenticator* authenticator1 = nullptr;
+  EXPECT_CALL(observer, AuthenticatorAdded(&discovery, _))
+      .WillOnce(DoAll(SaveArg<1>(&authenticator1),
+                      testing::InvokeWithoutArgs(
+                          [&device1_done]() { device1_done.Quit(); })));
+  EXPECT_CALL(*device1, GetId()).WillRepeatedly(Return("device1"));
   EXPECT_TRUE(discovery.AddDevice(std::move(device1)));
+  EXPECT_EQ("device1", authenticator1->GetId());
+  EXPECT_EQ(device1_raw, static_cast<FidoDeviceAuthenticator*>(authenticator1)
+                             ->GetDeviceForTesting());
   device1_done.Run();
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   // Inserting a device with an already present id should be prevented.
   auto device1_dup = std::make_unique<MockFidoDevice>();
-  EXPECT_CALL(observer, DeviceAdded(_, _)).Times(0);
+  EXPECT_CALL(observer, AuthenticatorAdded(_, _)).Times(0);
   EXPECT_CALL(*device1_dup, GetId()).WillOnce(Return("device1"));
   EXPECT_FALSE(discovery.AddDevice(std::move(device1_dup)));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
@@ -142,15 +153,15 @@ TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
               UnorderedElementsAre(device0_raw, device1_raw));
 
   // Trying to remove a non-present device should fail.
-  EXPECT_CALL(observer, DeviceRemoved(_, _)).Times(0);
+  EXPECT_CALL(observer, AuthenticatorRemoved(_, _)).Times(0);
   EXPECT_FALSE(discovery.RemoveDevice("device2"));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 
-  EXPECT_CALL(observer, DeviceRemoved(&discovery, device1_raw));
+  EXPECT_CALL(observer, AuthenticatorRemoved(&discovery, authenticator1));
   EXPECT_TRUE(discovery.RemoveDevice("device1"));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 
-  EXPECT_CALL(observer, DeviceRemoved(&discovery, device0_raw));
+  EXPECT_CALL(observer, AuthenticatorRemoved(&discovery, authenticator0));
   EXPECT_TRUE(discovery.RemoveDevice("device0"));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 }

device/fido/fido_request_handler.h

@@ -41,7 +41,7 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
 
   ~FidoRequestHandler() override {
     if (!is_complete())
-      CancelOngoingTasks();
+      CancelActiveAuthenticators();
   }
 
   bool is_complete() const { return completion_callback_.is_null(); }
@@ -72,7 +72,7 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
 
     // Once response has been passed to the relying party, cancel all other on
     // going requests.
-    CancelOngoingTasks(authenticator->GetId());
+    CancelActiveAuthenticators(authenticator->GetId());
     std::move(completion_callback_)
         .Run(*return_code, std::move(response_data),
              authenticator->AuthenticatorTransport());

device/fido/fido_request_handler_base.cc

@@ -14,8 +14,6 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
 #include "device/fido/ble_adapter_manager.h"
-#include "device/fido/fido_device.h"
-#include "device/fido/fido_task.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 namespace device {
@@ -121,14 +119,14 @@ FidoRequestHandlerBase::~FidoRequestHandlerBase() = default;
 
 void FidoRequestHandlerBase::StartAuthenticatorRequest(
     const std::string& authenticator_id) {
-  auto authenticator = active_authenticators_.find(authenticator_id);
-  if (authenticator == active_authenticators_.end())
+  auto authenticator_it = active_authenticators_.find(authenticator_id);
+  if (authenticator_it == active_authenticators_.end())
     return;
 
-  InitializeAuthenticatorAndDispatchRequest(authenticator->second.get());
+  InitializeAuthenticatorAndDispatchRequest(authenticator_it->second);
 }
 
-void FidoRequestHandlerBase::CancelOngoingTasks(
+void FidoRequestHandlerBase::CancelActiveAuthenticators(
     base::StringPiece exclude_device_id) {
   for (auto task_it = active_authenticators_.begin();
        task_it != active_authenticators_.end();) {
@@ -136,6 +134,10 @@ void FidoRequestHandlerBase::CancelOngoingTasks(
     if (task_it->first != exclude_device_id) {
       DCHECK(task_it->second);
       task_it->second->Cancel();
+
+      // Note that the pointer being erased is non-owning. The actual
+      // FidoAuthenticator instance is owned by its discovery (which in turn is
+      // owned by |discoveries_|.
       task_it = active_authenticators_.erase(task_it);
     } else {
       ++task_it;
@@ -183,34 +185,32 @@ void FidoRequestHandlerBase::Start() {
     discovery->Start();
 }
 
-void FidoRequestHandlerBase::DeviceAdded(FidoDiscovery* discovery,
-                                         FidoDevice* device) {
-  DCHECK(!base::ContainsKey(active_authenticators(), device->GetId()));
-  AddAuthenticator(CreateAuthenticatorFromDevice(device));
-}
-
-std::unique_ptr<FidoDeviceAuthenticator>
-FidoRequestHandlerBase::CreateAuthenticatorFromDevice(FidoDevice* device) {
-  return std::make_unique<FidoDeviceAuthenticator>(device);
+void FidoRequestHandlerBase::AuthenticatorAdded(
+    FidoDiscoveryBase* discovery,
+    FidoAuthenticator* authenticator) {
+  DCHECK(!base::ContainsKey(active_authenticators(), authenticator->GetId()));
+  AddAuthenticator(authenticator);
 }
 
-void FidoRequestHandlerBase::DeviceRemoved(FidoDiscovery* discovery,
-                                           FidoDevice* device) {
+void FidoRequestHandlerBase::AuthenticatorRemoved(
+    FidoDiscoveryBase* discovery,
+    FidoAuthenticator* authenticator) {
   // Device connection has been lost or device has already been removed.
   // Thus, calling CancelTask() is not necessary. Also, below
   // ongoing_tasks_.erase() will have no effect for the devices that have been
   // already removed due to processing error or due to invocation of
   // CancelOngoingTasks().
-  DCHECK(device);
-  active_authenticators_.erase(device->GetId());
+  DCHECK(authenticator);
+  active_authenticators_.erase(authenticator->GetId());
 
   if (observer_)
-    observer_->FidoAuthenticatorRemoved(device->GetId());
+    observer_->FidoAuthenticatorRemoved(authenticator->GetId());
 }
 
-void FidoRequestHandlerBase::DeviceIdChanged(FidoDiscovery* discovery,
-                                             const std::string& previous_id,
-                                             std::string new_id) {
+void FidoRequestHandlerBase::AuthenticatorIdChanged(
+    FidoDiscoveryBase* 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())
@@ -224,21 +224,19 @@ void FidoRequestHandlerBase::DeviceIdChanged(FidoDiscovery* discovery,
 }
 
 void FidoRequestHandlerBase::AddAuthenticator(
-    std::unique_ptr<FidoAuthenticator> authenticator) {
+    FidoAuthenticator* authenticator) {
   DCHECK(authenticator &&
          !base::ContainsKey(active_authenticators(), authenticator->GetId()));
-  FidoAuthenticator* authenticator_ptr = authenticator.get();
-  active_authenticators_.emplace(authenticator->GetId(),
-                                 std::move(authenticator));
+  active_authenticators_.emplace(authenticator->GetId(), authenticator);
 
-  // If |observer_| exists, dispatching request to |authenticator_ptr| is
-  // delegated to |observer_|. Else, dispatch request to |authenticator_ptr|
+  // If |observer_| exists, dispatching request to |authenticator| is
+  // delegated to |observer_|. Else, dispatch request to |authenticator|
   // immediately.
   bool embedder_controls_dispatch = false;
   if (observer_) {
     embedder_controls_dispatch =
-        observer_->EmbedderControlsAuthenticatorDispatch(*authenticator_ptr);
-    observer_->FidoAuthenticatorAdded(*authenticator_ptr);
+        observer_->EmbedderControlsAuthenticatorDispatch(*authenticator);
+    observer_->FidoAuthenticatorAdded(*authenticator);
   }
 
   if (!embedder_controls_dispatch) {
@@ -249,12 +247,13 @@ void FidoRequestHandlerBase::AddAuthenticator(
         FROM_HERE,
         base::BindOnce(
             &FidoRequestHandlerBase::InitializeAuthenticatorAndDispatchRequest,
-            GetWeakPtr(), authenticator_ptr));
+            GetWeakPtr(), authenticator));
   }
 }
 
 void FidoRequestHandlerBase::SetPlatformAuthenticatorOrMarkUnavailable(
     base::Optional<PlatformAuthenticatorInfo> platform_authenticator_info) {
+  DCHECK(!platform_authenticator_);
   if (platform_authenticator_info &&
       base::ContainsKey(transport_availability_info_.available_transports,
                         FidoTransportProtocol::kInternal)) {
@@ -264,7 +263,9 @@ void FidoRequestHandlerBase::SetPlatformAuthenticatorOrMarkUnavailable(
          FidoTransportProtocol::kInternal));
     transport_availability_info_.has_recognized_mac_touch_id_credential =
         platform_authenticator_info->has_recognized_mac_touch_id_credential;
-    AddAuthenticator(std::move(platform_authenticator_info->authenticator));
+    platform_authenticator_ =
+        std::move(platform_authenticator_info->authenticator);
+    AddAuthenticator(platform_authenticator_.get());
   } else {
     transport_availability_info_.available_transports.erase(
         FidoTransportProtocol::kInternal);

device/fido/fido_request_handler_base.h

@@ -31,8 +31,6 @@ namespace device {
 
 class BleAdapterManager;
 class FidoAuthenticator;
-class FidoDevice;
-class FidoTask;
 
 struct COMPONENT_EXPORT(DEVICE_FIDO) PlatformAuthenticatorInfo {
   PlatformAuthenticatorInfo(std::unique_ptr<FidoAuthenticator> authenticator,
@@ -45,16 +43,16 @@ struct COMPONENT_EXPORT(DEVICE_FIDO) PlatformAuthenticatorInfo {
   bool has_recognized_mac_touch_id_credential;
 };
 
-// Base class that handles device discovery/removal. Each FidoRequestHandlerBase
-// is owned by FidoRequestManager and its lifetime is equivalent to that of a
-// single WebAuthn request. For each authenticator, the per-device work is
-// carried out by one FidoTask instance, which is constructed on DeviceAdded(),
-// and destroyed either on DeviceRemoved() or CancelOutgoingTaks().
+// Base class that handles authenticator discovery/removal. Its lifetime is
+// equivalent to that of a single WebAuthn request. For each authenticator, the
+// per-device work is carried out by one FidoAuthenticator instance, which is
+// constructed in a FidoDiscoveryBase and passed to the request handler via its
+// Observer interface.
 class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
-    : public FidoDiscovery::Observer {
+    : public FidoDiscoveryBase::Observer {
  public:
   using AuthenticatorMap =
-      std::map<std::string, std::unique_ptr<FidoAuthenticator>, std::less<>>;
+      std::map<std::string, FidoAuthenticator*, std::less<>>;
   using RequestCallback = base::RepeatingCallback<void(const std::string&)>;
 
   enum class RequestType { kMakeCredential, kGetAssertion };
@@ -128,17 +126,16 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   // FidoAuthenticator with given |authenticator_id|.
   void StartAuthenticatorRequest(const std::string& authenticator_id);
 
-  // Triggers cancellation of all per-device FidoTasks, except for the device
-  // with |exclude_device_id|, if one is provided. Cancelled tasks are
-  // immediately removed from |ongoing_tasks_|.
+  // Invokes |FidoAuthenticator::Cancel| on all authenticators, except if
+  // matching |exclude_id|, if one is provided. Cancelled authenticators are
+  // immediately removed from |active_authenticators_|.
   //
-  // This function is invoked either when:
-  //  (a) the entire WebAuthn API request is canceled or,
-  //  (b) a successful response or "invalid state error" is received from the
-  //  any one of the connected authenticators, in which case all other
-  //  per-device tasks are cancelled.
+  // This function is invoked either when: (a) the entire WebAuthn API request
+  // is canceled or, (b) a successful response or "invalid state error" is
+  // received from the any one of the connected authenticators, in which case
+  // all other authenticators are cancelled.
   // https://w3c.github.io/webauthn/#iface-pkcredential
-  void CancelOngoingTasks(base::StringPiece exclude_device_id = nullptr);
+  void CancelActiveAuthenticators(base::StringPiece exclude_id = nullptr);
   void OnBluetoothAdapterEnumerated(bool is_present,
                                     bool is_powered_on,
                                     bool can_power_on);
@@ -173,12 +170,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
 
   void Start();
 
-  // Testing seam to allow unit tests to inject a fake authenticator.
-  virtual std::unique_ptr<FidoDeviceAuthenticator>
-  CreateAuthenticatorFromDevice(FidoDevice* device);
-
   AuthenticatorMap& active_authenticators() { return active_authenticators_; }
-  std::vector<std::unique_ptr<FidoDiscovery>>& discoveries() {
+  std::vector<std::unique_ptr<FidoDiscoveryBase>>& discoveries() {
     return discoveries_;
   }
   TransportAvailabilityObserver* observer() const { return observer_; }
@@ -186,14 +179,16 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
  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;
+  // FidoDiscoveryBase::Observer
+  void AuthenticatorAdded(FidoDiscoveryBase* discovery,
+                          FidoAuthenticator* authenticator) final;
+  void AuthenticatorRemoved(FidoDiscoveryBase* discovery,
+                            FidoAuthenticator* authenticator) final;
+  void AuthenticatorIdChanged(FidoDiscoveryBase* discovery,
+                              const std::string& previous_id,
+                              std::string new_id) final;
 
-  void AddAuthenticator(std::unique_ptr<FidoAuthenticator> authenticator);
+  void AddAuthenticator(FidoAuthenticator* authenticator);
   void NotifyObserverTransportAvailability();
 
   // Invokes FidoAuthenticator::InitializeAuthenticator(), followed by
@@ -204,11 +199,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   void ConstructBleAdapterPowerManager();
 
   AuthenticatorMap active_authenticators_;
-  std::vector<std::unique_ptr<FidoDiscovery>> discoveries_;
+  std::vector<std::unique_ptr<FidoDiscoveryBase>> discoveries_;
   TransportAvailabilityObserver* observer_ = nullptr;
   TransportAvailabilityInfo transport_availability_info_;
   base::RepeatingClosure notify_observer_callback_;
   std::unique_ptr<BleAdapterManager> bluetooth_adapter_manager_;
+  // TODO(martinkr): Inject platform authenticators through FidoDiscovery and
+  // hold ownership there.
+  std::unique_ptr<FidoAuthenticator> platform_authenticator_;
 
   base::WeakPtrFactory<FidoRequestHandlerBase> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(FidoRequestHandlerBase);

device/fido/fido_request_handler_unittest.cc

@@ -189,11 +189,6 @@ class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> {
                            weak_factory_.GetWeakPtr(), authenticator));
   }
 
-  std::unique_ptr<FidoDeviceAuthenticator> CreateAuthenticatorFromDevice(
-      FidoDevice* device) override {
-    return std::make_unique<FakeFidoAuthenticator>(device);
-  }
-
  private:
   base::WeakPtrFactory<FakeFidoRequestHandler> weak_factory_;
 };
@@ -244,8 +239,8 @@ class FidoRequestHandlerTest : public ::testing::Test {
   void ChangeAuthenticatorId(FakeFidoRequestHandler* request_handler,
                              FidoDevice* device,
                              std::string new_authenticator_id) {
-    request_handler->DeviceIdChanged(ble_discovery_, device->GetId(),
-                                     std::move(new_authenticator_id));
+    request_handler->AuthenticatorIdChanged(ble_discovery_, device->GetId(),
+                                            std::move(new_authenticator_id));
   }
 
   test::FakeFidoDiscovery* discovery() const { return discovery_; }

device/fido/hid/fido_hid_discovery_unittest.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/test/scoped_task_environment.h"
+#include "device/fido/fido_authenticator.h"
 #include "device/fido/hid/fake_hid_impl_for_testing.h"
 #include "device/fido/hid/fido_hid_device.h"
 #include "device/fido/mock_fido_discovery_observer.h"
@@ -91,26 +92,26 @@ TEST_F(FidoHidDiscoveryTest, TestAddRemoveDevice) {
 
   // Devices initially known to the service before discovery started should be
   // reported as KNOWN.
-  EXPECT_CALL(observer, DeviceAdded(&discovery, IdMatches("known")));
+  EXPECT_CALL(observer, AuthenticatorAdded(&discovery, IdMatches("known")));
   scoped_task_environment().RunUntilIdle();
 
   // Devices added during the discovery should be reported as ADDED.
-  EXPECT_CALL(observer, DeviceAdded(&discovery, IdMatches("added")));
+  EXPECT_CALL(observer, AuthenticatorAdded(&discovery, IdMatches("added")));
   fake_hid_manager_->AddDevice(MakeFidoHidDevice("added"));
   scoped_task_environment().RunUntilIdle();
 
   // Added non-U2F devices should not be reported at all.
-  EXPECT_CALL(observer, DeviceAdded(_, _)).Times(0);
+  EXPECT_CALL(observer, AuthenticatorAdded(_, _)).Times(0);
   fake_hid_manager_->AddDevice(MakeOtherDevice("other"));
 
   // Removed non-U2F devices should not be reported at all.
-  EXPECT_CALL(observer, DeviceRemoved(_, _)).Times(0);
+  EXPECT_CALL(observer, AuthenticatorRemoved(_, _)).Times(0);
   fake_hid_manager_->RemoveDevice("other");
   scoped_task_environment().RunUntilIdle();
 
   // Removed U2F devices should be reported as REMOVED.
-  EXPECT_CALL(observer, DeviceRemoved(&discovery, IdMatches("known")));
-  EXPECT_CALL(observer, DeviceRemoved(&discovery, IdMatches("added")));
+  EXPECT_CALL(observer, AuthenticatorRemoved(&discovery, IdMatches("known")));
+  EXPECT_CALL(observer, AuthenticatorRemoved(&discovery, IdMatches("added")));
   fake_hid_manager_->RemoveDevice("known");
   fake_hid_manager_->RemoveDevice("added");
   scoped_task_environment().RunUntilIdle();

device/fido/mock_fido_discovery_observer.h

@@ -14,19 +14,21 @@
 
 namespace device {
 
-class FidoDevice;
+class FidoAuthenticator;
 
-class MockFidoDiscoveryObserver : public FidoDiscovery::Observer {
+class MockFidoDiscoveryObserver : public FidoDiscoveryBase::Observer {
  public:
   MockFidoDiscoveryObserver();
   ~MockFidoDiscoveryObserver() override;
 
-  MOCK_METHOD2(DiscoveryStarted, void(FidoDiscovery*, bool));
-  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));
+  MOCK_METHOD2(DiscoveryStarted, void(FidoDiscoveryBase*, bool));
+  MOCK_METHOD2(DiscoveryStopped, void(FidoDiscoveryBase*, bool));
+  MOCK_METHOD2(AuthenticatorAdded,
+               void(FidoDiscoveryBase*, FidoAuthenticator*));
+  MOCK_METHOD2(AuthenticatorRemoved,
+               void(FidoDiscoveryBase*, FidoAuthenticator*));
+  MOCK_METHOD3(AuthenticatorIdChanged,
+               void(FidoDiscoveryBase*, const std::string&, std::string));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockFidoDiscoveryObserver);