Notify FidoDevice::is_in_pairing_mode to WebAuthN UI embedder

Connected Bluetooth FidoAuthenticator can be in pairing mode when
discovered by the browser, or device can change to pairing mode anytime
during the lifetime of FidoDiscovery. As so add function to notify
embedder when connected FidoAuthenticator changes its state to pairing
mode.

Bug: 877344
Change-Id: I3dc46e67064fdf7361b6cf2ef144d7daa907e92a
Reviewed-on: https://chromium-review.googlesource.com/c/1246596
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597261}

chrome/browser/ui/webauthn/authenticator_dialog_browsertest.cc

@@ -68,7 +68,7 @@ class AuthenticatorDialogTest : public DialogBrowserTest {
           AuthenticatorRequestDialogModel::AuthenticatorReference>(
           "authenticator" /* authenticator_id */,
           base::string16() /* authenticator_display_name */,
-          AuthenticatorTransport::kInternal);
+          AuthenticatorTransport::kInternal, false /* is_in_pairing_mode */);
       model->SetSelectedAuthenticatorForTesting(test_authenticator_.get());
       model->SetCurrentStep(
           AuthenticatorRequestDialogModel::Step::kBlePinEntry);

chrome/browser/webauthn/authenticator_request_dialog_model.cc

@@ -73,10 +73,12 @@ base::Optional<device::FidoTransportProtocol> SelectMostLikelyTransport(
 AuthenticatorRequestDialogModel::AuthenticatorReference::AuthenticatorReference(
     base::StringPiece authenticator_id,
     base::StringPiece16 authenticator_display_name,
-    device::FidoTransportProtocol transport)
+    device::FidoTransportProtocol transport,
+    bool is_in_pairing_mode)
     : authenticator_id(authenticator_id),
       authenticator_display_name(authenticator_display_name),
-      transport(transport) {}
+      transport(transport),
+      is_in_pairing_mode(is_in_pairing_mode) {}
 AuthenticatorRequestDialogModel::AuthenticatorReference::AuthenticatorReference(
     AuthenticatorReference&& data) = default;
 AuthenticatorRequestDialogModel::AuthenticatorReference&
@@ -342,6 +344,17 @@ void AuthenticatorRequestDialogModel::UpdateAuthenticatorReferenceId(
     it->authenticator_id = std::move(new_authenticator_id);
 }
 
+void AuthenticatorRequestDialogModel::UpdateAuthenticatorReferencePairingMode(
+    base::StringPiece authenticator_id) {
+  auto it =
+      std::find_if(saved_authenticators_.begin(), saved_authenticators_.end(),
+                   [authenticator_id](const auto& authenticator) {
+                     return authenticator.authenticator_id == authenticator_id;
+                   });
+  if (it != saved_authenticators_.end())
+    it->is_in_pairing_mode = true;
+}
+
 void AuthenticatorRequestDialogModel::SetSelectedAuthenticatorForTesting(
     AuthenticatorReference* authenticator) {
   selected_authenticator_ = authenticator;

chrome/browser/webauthn/authenticator_request_dialog_model.h

@@ -80,7 +80,8 @@ class AuthenticatorRequestDialogModel {
   struct AuthenticatorReference {
     AuthenticatorReference(base::StringPiece device_id,
                            base::StringPiece16 authenticator_display_name,
-                           device::FidoTransportProtocol transport);
+                           device::FidoTransportProtocol transport,
+                           bool is_in_pairing_mode);
     AuthenticatorReference(AuthenticatorReference&& data);
     AuthenticatorReference& operator=(AuthenticatorReference&& other);
 
@@ -89,6 +90,7 @@ class AuthenticatorRequestDialogModel {
     std::string authenticator_id;
     base::string16 authenticator_display_name;
     device::FidoTransportProtocol transport;
+    bool is_in_pairing_mode = false;
     bool dispatched = false;
   };
 
@@ -266,6 +268,9 @@ class AuthenticatorRequestDialogModel {
   void UpdateAuthenticatorReferenceId(base::StringPiece old_authenticator_id,
                                       std::string new_authenticator_id);
 
+  void UpdateAuthenticatorReferencePairingMode(
+      base::StringPiece authenticator_id);
+
   void SetSelectedAuthenticatorForTesting(
       AuthenticatorReference* authenticator);
 

chrome/browser/webauthn/authenticator_request_dialog_model_unittest.cc

@@ -476,7 +476,7 @@ TEST_F(AuthenticatorRequestDialogModelTest,
       AuthenticatorRequestDialogModel::AuthenticatorReference(
           "authenticator" /* authenticator_id */,
           base::string16() /* authenticator_display_name */,
-          AuthenticatorTransport::kInternal));
+          AuthenticatorTransport::kInternal, false /* is_in_pairing_mode */));
 
   model.StartFlow(std::move(transports_info), base::nullopt);
   EXPECT_EQ(AuthenticatorRequestDialogModel::Step::kTransportSelection,

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -299,7 +299,7 @@ void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorAdded(
 
   weak_dialog_model_->saved_authenticators().emplace_back(
       authenticator.GetId(), authenticator.GetDisplayName(),
-      authenticator.AuthenticatorTransport());
+      authenticator.AuthenticatorTransport(), authenticator.IsInPairingMode());
 }
 
 void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorRemoved(
@@ -327,6 +327,14 @@ void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorIdChanged(
       old_authenticator_id, std::move(new_authenticator_id));
 }
 
+void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorPairingModeChanged(
+    base::StringPiece authenticator_id) {
+  if (!weak_dialog_model_)
+    return;
+
+  weak_dialog_model_->UpdateAuthenticatorReferencePairingMode(authenticator_id);
+}
+
 void ChromeAuthenticatorRequestDelegate::BluetoothAdapterPowerChanged(
     bool is_powered_on) {
   if (!weak_dialog_model_)

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -95,6 +95,8 @@ class ChromeAuthenticatorRequestDelegate
   void FidoAuthenticatorRemoved(base::StringPiece authenticator_id) override;
   void FidoAuthenticatorIdChanged(base::StringPiece old_authenticator_id,
                                   std::string new_authenticator_id) override;
+  void FidoAuthenticatorPairingModeChanged(
+      base::StringPiece authenticator_id) override;
   void BluetoothAdapterPowerChanged(bool is_powered_on) override;
 
   // AuthenticatorRequestDialogModel::Observer:

content/public/browser/authenticator_request_client_delegate.cc

@@ -71,4 +71,7 @@ void AuthenticatorRequestClientDelegate::FidoAuthenticatorIdChanged(
     base::StringPiece old_authenticator_id,
     std::string new_authenticator_id) {}
 
+void AuthenticatorRequestClientDelegate::FidoAuthenticatorPairingModeChanged(
+    base::StringPiece authenticator_id) {}
+
 }  // namespace content

content/public/browser/authenticator_request_client_delegate.h

@@ -118,6 +118,8 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   void FidoAuthenticatorRemoved(base::StringPiece device_id) override;
   void FidoAuthenticatorIdChanged(base::StringPiece old_authenticator_id,
                                   std::string new_authenticator_id) override;
+  void FidoAuthenticatorPairingModeChanged(
+      base::StringPiece authenticator_id) override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AuthenticatorRequestClientDelegate);

device/fido/ble/fido_ble_device.h

@@ -48,7 +48,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleDevice : public FidoDevice {
 
   // Returns whether or not the underlying BLE device is currently in pairing
   // mode by investigating the advertisement payload.
-  bool IsInPairingMode() const;
+  bool IsInPairingMode() const override;
 
   FidoBleConnection::ReadCallback GetReadCallbackForTesting();
 

device/fido/ble/fido_ble_discovery.cc

@@ -68,13 +68,27 @@ void FidoBleDiscovery::DeviceAdded(BluetoothAdapter* adapter,
 
 void FidoBleDiscovery::DeviceChanged(BluetoothAdapter* adapter,
                                      BluetoothDevice* device) {
-  if (!CheckForExcludedDeviceAndCacheAddress(device) &&
-      base::ContainsKey(device->GetUUIDs(), FidoServiceUUID()) &&
-      !GetDevice(FidoBleDevice::GetId(device->GetAddress()))) {
+  if (CheckForExcludedDeviceAndCacheAddress(device) ||
+      !base::ContainsKey(device->GetUUIDs(), FidoServiceUUID())) {
+    return;
+  }
+
+  const auto device_id = FidoBleDevice::GetId(device->GetAddress());
+  auto* fido_device = GetDevice(device_id);
+  if (!fido_device) {
     VLOG(2) << "Discovered U2F service on existing BLE device: "
             << device->GetAddress();
     AddDevice(std::make_unique<FidoBleDevice>(adapter, device->GetAddress()));
+    return;
   }
+
+  // Our model of FIDO BLE security key assumes that if BLE device is in pairing
+  // mode long enough time without pairing attempt, the device stops advertising
+  // and BluetoothAdapter::DeviceRemoved() is invoked instead of returning back
+  // to regular "non-pairing" mode. As so, we only notify observer when
+  // |fido_device| goes into pairing mode.
+  if (observer() && fido_device->IsInPairingMode())
+    observer()->AuthenticatorPairingModeChanged(this, device_id);
 }
 
 void FidoBleDiscovery::DeviceRemoved(BluetoothAdapter* adapter,

device/fido/ble/fido_ble_discovery_unittest.cc

@@ -31,9 +31,18 @@
 #include "device/bluetooth/test/bluetooth_test_bluez.h"
 #endif
 
+namespace device {
+
+namespace {
+
 using ::testing::_;
+using TestMockDevice = ::testing::NiceMock<MockBluetoothDevice>;
 
-namespace device {
+constexpr char kDeviceName[] = "device_name";
+constexpr char kDeviceAddress[] = "device_address";
+constexpr char kDeviceChangedAddress[] = "device_changed_address";
+constexpr char kAuthenticatorId[] = "ble:device_address";
+constexpr char kAuthenticatorChangedId[] = "ble:device_changed_address";
 
 ACTION_P(ReturnFromAsyncCall, closure) {
   closure.Run();
@@ -43,6 +52,8 @@ MATCHER_P(IdMatches, id, "") {
   return arg->GetId() == std::string("ble:") + id;
 }
 
+}  // namespace
+
 TEST_F(BluetoothTest, FidoBleDiscoveryNotifyObserverWhenAdapterNotPresent) {
   FidoBleDiscovery discovery;
   MockFidoDiscoveryObserver observer;
@@ -241,12 +252,6 @@ TEST_F(BluetoothTest, FidoBleDiscoveryRejectsCableDevice) {
 
 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;
@@ -287,4 +292,42 @@ TEST_F(BluetoothTest, DiscoveryDoesNotAddDuplicateDeviceOnAddressChanged) {
   EXPECT_TRUE(discovery.GetDevice(kAuthenticatorChangedId));
 }
 
+TEST_F(BluetoothTest, DiscoveryNotifiesObserverWhenDeviceInPairingMode) {
+  FidoBleDiscovery discovery;
+  MockFidoDiscoveryObserver observer;
+  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)}));
+  EXPECT_CALL(*mock_adapter, GetDevice(kDeviceAddress))
+      .WillRepeatedly(::testing::Return(mock_device.get()));
+
+  BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter.get());
+  const auto device_id = FidoBleDevice::GetId(kDeviceAddress);
+  discovery.Start();
+
+  ::testing::InSequence sequence;
+  EXPECT_CALL(observer,
+              AuthenticatorAdded(&discovery, IdMatches(kDeviceAddress)));
+  mock_adapter->NotifyDeviceChanged(mock_device.get());
+
+  EXPECT_CALL(observer, AuthenticatorPairingModeChanged(&discovery, _));
+  // Update device advertisement data so that it represents BLE device in
+  // pairing mode.
+  mock_device->UpdateAdvertisementData(
+      0 /* rssi */, 1 << kLeLimitedDiscoverableModeBit,
+      std::vector<BluetoothUUID>{BluetoothUUID(kFidoServiceUUID)},
+      base::nullopt /* tx_power */, BluetoothDevice::ServiceDataMap(),
+      BluetoothDevice::ManufacturerDataMap());
+  mock_adapter->NotifyDeviceChanged(mock_device.get());
+}
+
 }  // namespace device

device/fido/ble_adapter_manager_unittest.cc

@@ -47,6 +47,7 @@ class MockTransportAvailabilityObserver
   MOCK_METHOD2(FidoAuthenticatorIdChanged,
                void(base::StringPiece old_authenticator_id,
                     std::string new_authenticator_id));
+  MOCK_METHOD1(FidoAuthenticatorPairingModeChanged, void(base::StringPiece));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockTransportAvailabilityObserver);

device/fido/fido_authenticator.h

@@ -52,6 +52,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
   virtual base::string16 GetDisplayName() const = 0;
   virtual const AuthenticatorSupportedOptions& Options() const = 0;
   virtual FidoTransportProtocol AuthenticatorTransport() const = 0;
+  virtual bool IsInPairingMode() const = 0;
   virtual base::WeakPtr<FidoAuthenticator> GetWeakPtr() = 0;
 
  private:

device/fido/fido_device.cc

@@ -23,6 +23,10 @@ base::string16 FidoDevice::GetDisplayName() const {
   return base::string16(id.begin(), id.end());
 }
 
+bool FidoDevice::IsInPairingMode() const {
+  return false;
+}
+
 void FidoDevice::DiscoverSupportedProtocolAndDeviceInfo(
     base::OnceClosure done) {
   if (base::FeatureList::IsEnabled(kNewCtap2Device)) {

device/fido/fido_device.h

@@ -62,6 +62,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDevice {
   virtual std::string GetId() const = 0;
   virtual base::string16 GetDisplayName() const;
   virtual FidoTransportProtocol DeviceTransport() const = 0;
+  virtual bool IsInPairingMode() const;
   virtual base::WeakPtr<FidoDevice> GetWeakPtr() = 0;
 
   // Sends a speculative AuthenticatorGetInfo request to determine whether the

device/fido/fido_device_authenticator.cc

@@ -83,6 +83,10 @@ FidoTransportProtocol FidoDeviceAuthenticator::AuthenticatorTransport() const {
   return device_->DeviceTransport();
 }
 
+bool FidoDeviceAuthenticator::IsInPairingMode() const {
+  return device_->IsInPairingMode();
+}
+
 void FidoDeviceAuthenticator::SetTaskForTesting(
     std::unique_ptr<FidoTask> task) {
   task_ = std::move(task);

device/fido/fido_device_authenticator.h

@@ -43,6 +43,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   base::string16 GetDisplayName() const override;
   const AuthenticatorSupportedOptions& Options() const override;
   FidoTransportProtocol AuthenticatorTransport() const override;
+  bool IsInPairingMode() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 
   FidoDevice* GetDeviceForTesting() { return device_; }

device/fido/fido_discovery_base.h

@@ -40,6 +40,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryBase {
     virtual void AuthenticatorIdChanged(FidoDiscoveryBase* discovery,
                                         const std::string& previous_id,
                                         std::string new_id) = 0;
+
+    // Invoked when connected Bluetooth device advertises that it is in pairing
+    // mode.
+    virtual void AuthenticatorPairingModeChanged(
+        FidoDiscoveryBase* discovery,
+        const std::string& device_id) = 0;
   };
 
   // Start authenticator discovery. The Observer must have been set before this

device/fido/fido_request_handler_base.cc

@@ -236,6 +236,18 @@ void FidoRequestHandlerBase::AuthenticatorIdChanged(
     observer_->FidoAuthenticatorIdChanged(previous_id, std::move(new_id));
 }
 
+void FidoRequestHandlerBase::AuthenticatorPairingModeChanged(
+    FidoDiscoveryBase* discovery,
+    const std::string& device_id) {
+  DCHECK_EQ(FidoTransportProtocol::kBluetoothLowEnergy, discovery->transport());
+  auto it = active_authenticators_.find(device_id);
+  if (it == active_authenticators_.end())
+    return;
+
+  if (observer_)
+    observer_->FidoAuthenticatorPairingModeChanged(device_id);
+}
+
 void FidoRequestHandlerBase::AddAuthenticator(
     FidoAuthenticator* authenticator) {
   DCHECK(authenticator &&

device/fido/fido_request_handler_base.h

@@ -116,6 +116,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
     virtual void FidoAuthenticatorIdChanged(
         base::StringPiece old_authenticator_id,
         std::string new_authenticator_id) = 0;
+    virtual void FidoAuthenticatorPairingModeChanged(
+        base::StringPiece authenticator_id) = 0;
   };
 
   // TODO(https://crbug.com/769631): Remove the dependency on Connector once
@@ -202,6 +204,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   void AuthenticatorIdChanged(FidoDiscoveryBase* discovery,
                               const std::string& previous_id,
                               std::string new_id) final;
+  void AuthenticatorPairingModeChanged(FidoDiscoveryBase* discovery,
+                                       const std::string& device_id) final;
 
   void AddAuthenticator(FidoAuthenticator* authenticator);
   void NotifyObserverTransportAvailability();

device/fido/fido_request_handler_unittest.cc

@@ -104,6 +104,8 @@ class TestTransportAvailabilityObserver
     authenticator_id_change_notification_receiver_.callback().Run(
         std::move(new_authenticator_id));
   }
+  void FidoAuthenticatorPairingModeChanged(
+      base::StringPiece authenticator_id) override {}
 
  private:
   TransportAvailabilityNotificationReceiver

device/fido/mac/authenticator.h

@@ -60,6 +60,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
   base::string16 GetDisplayName() const override;
   const AuthenticatorSupportedOptions& Options() const override;
   FidoTransportProtocol AuthenticatorTransport() const override;
+  bool IsInPairingMode() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 
  private:

device/fido/mac/authenticator.mm

@@ -161,6 +161,10 @@ const AuthenticatorSupportedOptions& TouchIdAuthenticator::Options() const {
   return options;
 }
 
+bool TouchIdAuthenticator::IsInPairingMode() const {
+  return false;
+}
+
 base::WeakPtr<FidoAuthenticator> TouchIdAuthenticator::GetWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }

device/fido/mock_fido_discovery_observer.h

@@ -29,6 +29,8 @@ class MockFidoDiscoveryObserver : public FidoDiscoveryBase::Observer {
                void(FidoDiscoveryBase*, FidoAuthenticator*));
   MOCK_METHOD3(AuthenticatorIdChanged,
                void(FidoDiscoveryBase*, const std::string&, std::string));
+  MOCK_METHOD2(AuthenticatorPairingModeChanged,
+               void(FidoDiscoveryBase*, const std::string&));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockFidoDiscoveryObserver);