fido: reenable Touch ID for kAny attachment if transport selection UI…

… is enabled We previously disabled the MakeCredential operation for Touch ID if AuthenticatorAttachment was set to kAny in order to avoid having the fingerprint dialog compete with external authenticators. Since MakeCredential now always triggers the transport selection UI, we can now undo this limitation if the UI is active. Bug: 873710 Change-Id: I68ad9f0e7d5c3bdeda84652f893dc5df2743254f Reviewed-on: https://chromium-review.googlesource.com/1180541 Commit-Queue: Martin Kreichgauer <martinkr@google.com> Reviewed-by: Balazs Engedy <engedy@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#585639}
marcosholgado · Aug 23, 2018 · 23355d5 · 23355d5
1 parent bea7e78
commit 23355d5
Show file tree

Hide file tree

Showing 11 changed files with 128 additions and 59 deletions.
diff --git a/chrome/browser/webauthn/chrome_authenticator_request_delegate.cc b/chrome/browser/webauthn/chrome_authenticator_request_delegate.cc
@@ -63,17 +63,6 @@ bool IsWebAuthnUiEnabled() {
       switches::kWebAuthenticationUI);
 }
 
-bool ShouldDispatchRequestImmediately(
-    const device::FidoAuthenticator& authenticator) {
-  // TODO(hongjunchoi): Change this so that requests for BLE authenticators are
-  // not dispatched immediately if WebAuthN UI is enabled.
-  if (!IsWebAuthnUiEnabled())
-    return true;
-
-  return authenticator.AuthenticatorTransport() !=
-         device::FidoTransportProtocol::kInternal;
-}
-
 }  // namespace
 
 #if defined(OS_MACOSX)
@@ -267,12 +256,19 @@ void ChromeAuthenticatorRequestDelegate::OnTransportAvailabilityEnumerated(
 #endif
 }
 
-void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorAdded(
-    const device::FidoAuthenticator& authenticator,
-    bool* hold_off_request) {
-  if (!ShouldDispatchRequestImmediately(authenticator))
-    *hold_off_request = true;
+bool ChromeAuthenticatorRequestDelegate::EmbedderControlsAuthenticatorDispatch(
+    const device::FidoAuthenticator& authenticator) {
+  // TODO(hongjunchoi): Change this so that requests for BLE authenticators are
+  // not dispatched immediately if WebAuthN UI is enabled.
+  if (!IsWebAuthnUiEnabled())
+    return false;
 
+  return authenticator.AuthenticatorTransport() ==
+         device::FidoTransportProtocol::kInternal;
+}
+
+void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorAdded(
+    const device::FidoAuthenticator& authenticator) {
   if (!IsWebAuthnUiEnabled())
     return;
 

diff --git a/chrome/browser/webauthn/chrome_authenticator_request_delegate.h b/chrome/browser/webauthn/chrome_authenticator_request_delegate.h
@@ -79,8 +79,10 @@ class ChromeAuthenticatorRequestDelegate
   // device::FidoRequestHandlerBase::TransportAvailabilityObserver:
   void OnTransportAvailabilityEnumerated(
       device::FidoRequestHandlerBase::TransportAvailabilityInfo data) override;
-  void FidoAuthenticatorAdded(const device::FidoAuthenticator& authenticator,
-                              bool* hold_off_request) override;
+  bool EmbedderControlsAuthenticatorDispatch(
+      const device::FidoAuthenticator& authenticator) override;
+  void FidoAuthenticatorAdded(
+      const device::FidoAuthenticator& authenticator) override;
   void FidoAuthenticatorRemoved(base::StringPiece authenticator_id) override;
 
   // AuthenticatorRequestDialogModel::Observer:

diff --git a/content/browser/webauth/authenticator_impl_unittest.cc b/content/browser/webauth/authenticator_impl_unittest.cc
@@ -1759,8 +1759,9 @@ class MockAuthenticatorRequestDelegateObserver
   MOCK_METHOD1(
       OnTransportAvailabilityEnumerated,
       void(device::FidoRequestHandlerBase::TransportAvailabilityInfo data));
-  MOCK_METHOD2(FidoAuthenticatorAdded,
-               void(const device::FidoAuthenticator&, bool*));
+  MOCK_METHOD1(EmbedderControlsAuthenticatorDispatch,
+               bool(const device::FidoAuthenticator&));
+  MOCK_METHOD1(FidoAuthenticatorAdded, void(const device::FidoAuthenticator&));
   MOCK_METHOD1(FidoAuthenticatorRemoved, void(base::StringPiece));
 
  private:
@@ -1873,7 +1874,7 @@ TEST_F(AuthenticatorImplRequestDelegateTest,
   EXPECT_CALL(*mock_delegate_ptr, OnTransportAvailabilityEnumerated(_));
 
   base::RunLoop ble_device_found_done;
-  EXPECT_CALL(*mock_delegate_ptr, FidoAuthenticatorAdded(_, _))
+  EXPECT_CALL(*mock_delegate_ptr, FidoAuthenticatorAdded(_))
       .WillOnce(testing::InvokeWithoutArgs(
           [&ble_device_found_done]() { ble_device_found_done.Quit(); }));
 

diff --git a/content/public/browser/authenticator_request_client_delegate.cc b/content/public/browser/authenticator_request_client_delegate.cc
@@ -48,12 +48,16 @@ void AuthenticatorRequestClientDelegate::UpdateLastTransportUsed(
 void AuthenticatorRequestClientDelegate::OnTransportAvailabilityEnumerated(
     device::FidoRequestHandlerBase::TransportAvailabilityInfo data) {}
 
+bool AuthenticatorRequestClientDelegate::EmbedderControlsAuthenticatorDispatch(
+    const device::FidoAuthenticator& authenticator) {
+  return false;
+}
+
 void AuthenticatorRequestClientDelegate::BluetoothAdapterPowerChanged(
     bool is_powered_on) {}
 
 void AuthenticatorRequestClientDelegate::FidoAuthenticatorAdded(
-    const device::FidoAuthenticator& authenticator,
-    bool* hold_off_request) {}
+    const device::FidoAuthenticator& authenticator) {}
 
 void AuthenticatorRequestClientDelegate::FidoAuthenticatorRemoved(
     base::StringPiece device_id) {}

diff --git a/content/public/browser/authenticator_request_client_delegate.h b/content/public/browser/authenticator_request_client_delegate.h
@@ -94,9 +94,20 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   // device::FidoRequestHandlerBase::TransportAvailabilityObserver:
   void OnTransportAvailabilityEnumerated(
       device::FidoRequestHandlerBase::TransportAvailabilityInfo data) override;
+  // If true, the request handler will defer dispatch of its request onto the
+  // given authenticator to the embedder. The embedder needs to call
+  // |StartAuthenticatorRequest| when it wants to initiate request dispatch.
+  //
+  // This method is invoked before |FidoAuthenticatorAdded|, and may be
+  // invoked multiple times for the same authenticator. Depending on the
+  // result, the request handler might decide not to make the authenticator
+  // available, in which case it never gets passed to
+  // |FidoAuthenticatorAdded|.
+  bool EmbedderControlsAuthenticatorDispatch(
+      const device::FidoAuthenticator& authenticator) override;
   void BluetoothAdapterPowerChanged(bool is_powered_on) override;
-  void FidoAuthenticatorAdded(const device::FidoAuthenticator& authenticator,
-                              bool* hold_off_request) override;
+  void FidoAuthenticatorAdded(
+      const device::FidoAuthenticator& authenticator) override;
   void FidoAuthenticatorRemoved(base::StringPiece device_id) override;
 
  private:

diff --git a/device/fido/fido_request_handler_base.cc b/device/fido/fido_request_handler_base.cc
@@ -206,22 +206,23 @@ void FidoRequestHandlerBase::AddAuthenticator(
   // If |observer_| exists, dispatching request to |authenticator_ptr| is
   // delegated to |observer_|. Else, dispatch request to |authenticator_ptr|
   // immediately.
-  bool should_delay_request = false;
-  if (observer_)
-    observer_->FidoAuthenticatorAdded(*authenticator_ptr,
-                                      &should_delay_request);
-
-  if (should_delay_request)
-    return;
+  bool embedder_controls_dispatch = false;
+  if (observer_) {
+    embedder_controls_dispatch =
+        observer_->EmbedderControlsAuthenticatorDispatch(*authenticator_ptr);
+    observer_->FidoAuthenticatorAdded(*authenticator_ptr);
+  }
 
-  // Post |InitializeAuthenticatorAndDispatchRequest| into its own task. This
-  // avoids hairpinning, even if the authenticator immediately invokes the
-  // request callback.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          &FidoRequestHandlerBase::InitializeAuthenticatorAndDispatchRequest,
-          GetWeakPtr(), authenticator_ptr));
+  if (!embedder_controls_dispatch) {
+    // Post |InitializeAuthenticatorAndDispatchRequest| into its own task. This
+    // avoids hairpinning, even if the authenticator immediately invokes the
+    // request callback.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &FidoRequestHandlerBase::InitializeAuthenticatorAndDispatchRequest,
+            GetWeakPtr(), authenticator_ptr));
+  }
 }
 
 void FidoRequestHandlerBase::SetPlatformAuthenticatorOrMarkUnavailable(

diff --git a/device/fido/fido_request_handler_base.h b/device/fido/fido_request_handler_base.h
@@ -92,9 +92,22 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
     // This method will not be invoked until the observer is set.
     virtual void OnTransportAvailabilityEnumerated(
         TransportAvailabilityInfo data) = 0;
+
+    // If true, the request handler will defer dispatch of its request onto the
+    // given authenticator to the embedder. The embedder needs to call
+    // |StartAuthenticatorRequest| when it wants to initiate request dispatch.
+    //
+    // This method is invoked before |FidoAuthenticatorAdded|, and may be
+    // invoked multiple times for the same authenticator. Depending on the
+    // result, the request handler might decide not to make the authenticator
+    // available, in which case it never gets passed to
+    // |FidoAuthenticatorAdded|.
+    virtual bool EmbedderControlsAuthenticatorDispatch(
+        const FidoAuthenticator& authenticator) = 0;
+
     virtual void BluetoothAdapterPowerChanged(bool is_powered_on) = 0;
-    virtual void FidoAuthenticatorAdded(const FidoAuthenticator& authenticator,
-                                        bool* hold_off_request) = 0;
+    virtual void FidoAuthenticatorAdded(
+        const FidoAuthenticator& authenticator) = 0;
     virtual void FidoAuthenticatorRemoved(base::StringPiece device_id) = 0;
   };
 
@@ -136,7 +149,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   // Set the platform authenticator for this request, if one is available.
   // |AuthenticatorImpl| must call this method after invoking |set_oberver| even
   // if no platform authenticator is available, in which case it passes nullptr.
-  void SetPlatformAuthenticatorOrMarkUnavailable(
+  virtual void SetPlatformAuthenticatorOrMarkUnavailable(
       base::Optional<PlatformAuthenticatorInfo> platform_authenticator_info);
 
   TransportAvailabilityInfo& transport_availability_info() {

diff --git a/device/fido/fido_request_handler_unittest.cc b/device/fido/fido_request_handler_unittest.cc
@@ -77,10 +77,14 @@ class TestTransportAvailabilityObserver
     transport_availability_notification_receiver_.callback().Run(
         std::move(data));
   }
+  bool EmbedderControlsAuthenticatorDispatch(
+      const FidoAuthenticator&) override {
+    return false;
+  }
 
   void BluetoothAdapterPowerChanged(bool is_powered_on) override {}
-  void FidoAuthenticatorAdded(const FidoAuthenticator& authenticator,
-                              bool* hold_off_request) override {}
+  void FidoAuthenticatorAdded(const FidoAuthenticator& authenticator) override {
+  }
   void FidoAuthenticatorRemoved(base::StringPiece device_id) override {}
 
  private:

diff --git a/device/fido/make_credential_handler_unittest.cc b/device/fido/make_credential_handler_unittest.cc
@@ -106,11 +106,6 @@ class FidoMakeCredentialHandlerTest : public ::testing::Test {
     if (!base::ContainsKey(transports, Transport::kNearFieldCommunication))
       EXPECT_FALSE(nfc_discovery()->is_start_requested());
 
-    // Even with FidoTransportProtocol::kInternal allowed, unless the platform
-    // authenticator factory returns a FidoAuthenticator instance (which it will
-    // not be default), the transport will be marked `unavailable`.
-    transports.erase(Transport::kInternal);
-
     EXPECT_THAT(
         request_handler->transport_availability_info().available_transports,
         ::testing::UnorderedElementsAreArray(transports));
@@ -273,13 +268,25 @@ TEST_F(FidoMakeCredentialHandlerTest, UserVerificationRequirementNotMet) {
 // TODO(crbug.com/873710): Platform authenticators are temporarily disabled if
 // AuthenticatorAttachment is unset (kAny).
 TEST_F(FidoMakeCredentialHandlerTest, AnyAttachment) {
+  auto platform_device = MockFidoDevice::MakeCtap(
+      ReadCTAPGetInfoResponse(test_data::kTestGetInfoResponsePlatformDevice));
+  platform_device->SetDeviceTransport(FidoTransportProtocol::kInternal);
+  set_mock_platform_device(std::move(platform_device));
+
   auto request_handler =
       CreateMakeCredentialHandlerWithAuthenticatorSelectionCriteria(
           AuthenticatorSelectionCriteria(
               AuthenticatorSelectionCriteria::AuthenticatorAttachment::kAny,
               false /* require_resident_key */,
               UserVerificationRequirement::kPreferred));
 
+  // MakeCredentialHandler will not dispatch the kAny request to the platform
+  // authenticator since the request does not get dispatched through UI. Despite
+  // setting a platform authenticator, the internal transport never becomes
+  // available.
+  scoped_task_environment_.FastForwardUntilNoTasksRemain();
+  EXPECT_FALSE(callback().was_called());
+
   ExpectAllowedTransportsForRequestAre(
       request_handler.get(), {FidoTransportProtocol::kBluetoothLowEnergy,
                               FidoTransportProtocol::kNearFieldCommunication,
@@ -302,6 +309,16 @@ TEST_F(FidoMakeCredentialHandlerTest, CrossPlatformAttachment) {
 }
 
 TEST_F(FidoMakeCredentialHandlerTest, PlatformAttachment) {
+  // Add a platform device to trigger the transport actually becoming available.
+  // We don't care about the result of the request.
+  auto platform_device = MockFidoDevice::MakeCtapWithGetInfoExpectation(
+      test_data::kTestGetInfoResponsePlatformDevice);
+  platform_device->SetDeviceTransport(FidoTransportProtocol::kInternal);
+  platform_device->ExpectCtap2CommandAndDoNotRespond(
+      CtapRequestCommand::kAuthenticatorMakeCredential);
+  EXPECT_CALL(*platform_device, Cancel());
+  set_mock_platform_device(std::move(platform_device));
+
   auto request_handler =
       CreateMakeCredentialHandlerWithAuthenticatorSelectionCriteria(
           AuthenticatorSelectionCriteria(

diff --git a/device/fido/make_credential_request_handler.cc b/device/fido/make_credential_request_handler.cc
@@ -35,11 +35,6 @@ bool CheckIfAuthenticatorSelectionCriteriaAreSatisfied(
        !options.is_platform_device()) ||
       (authenticator_selection_criteria.authenticator_attachement() ==
            AuthenticatorAttachment::kCrossPlatform &&
-       options.is_platform_device()) ||
-      // TODO(crbug.com/873710): Reenable platform authenticators for kAny,
-      // once Touch ID is integrated into the UI.
-      (authenticator_selection_criteria.authenticator_attachement() ==
-           AuthenticatorAttachment::kAny &&
        options.is_platform_device())) {
     return false;
   }
@@ -78,9 +73,8 @@ base::flat_set<FidoTransportProtocol> GetTransportsAllowedByRP(
               FidoTransportProtocol::kNearFieldCommunication,
               FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy};
     case AttachmentType::kAny:
-      // TODO(crbug.com/873710): Re-enable platform authenticators for kAny,
-      // once Touch ID is integrated into the UI.
-      return {FidoTransportProtocol::kNearFieldCommunication,
+      return {FidoTransportProtocol::kInternal,
+              FidoTransportProtocol::kNearFieldCommunication,
               FidoTransportProtocol::kUsbHumanInterfaceDevice,
               FidoTransportProtocol::kBluetoothLowEnergy,
               FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy};
@@ -153,4 +147,25 @@ void MakeCredentialRequestHandler::HandleResponse(
   OnAuthenticatorResponse(authenticator, response_code, std::move(response));
 }
 
+void MakeCredentialRequestHandler::SetPlatformAuthenticatorOrMarkUnavailable(
+    base::Optional<PlatformAuthenticatorInfo> platform_authenticator_info) {
+  if (platform_authenticator_info) {
+    // TODO(crbug.com/873710): In the case of a request with
+    // AuthenticatorAttachment::kAny and when there is no embedder-provided
+    // transport selection UI, disable the platform authenticator to avoid the
+    // Touch ID fingerprint prompt competing with external devices.
+    const bool has_transport_selection_ui =
+        observer() && observer()->EmbedderControlsAuthenticatorDispatch(
+                          *platform_authenticator_info->authenticator);
+    if (authenticator_selection_criteria_.authenticator_attachement() ==
+            AuthenticatorSelectionCriteria::AuthenticatorAttachment::kAny &&
+        !has_transport_selection_ui) {
+      platform_authenticator_info = base::nullopt;
+    }
+  }
+
+  FidoRequestHandlerBase::SetPlatformAuthenticatorOrMarkUnavailable(
+      std::move(platform_authenticator_info));
+}
+
 }  // namespace device
diff --git a/device/fido/make_credential_request_handler.h b/device/fido/make_credential_request_handler.h
@@ -43,9 +43,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
       RegisterResponseCallback completion_callback);
   ~MakeCredentialRequestHandler() override;
 
+  // FidoRequestHandlerBase:
+  void SetPlatformAuthenticatorOrMarkUnavailable(
+      base::Optional<PlatformAuthenticatorInfo> platform_authenticator_info)
+      override;
+
  private:
   // FidoRequestHandlerBase:
-  void DispatchRequest(FidoAuthenticator* authenticator) final;
+  void DispatchRequest(FidoAuthenticator* authenticator) override;
 
   void HandleResponse(
       FidoAuthenticator* authenticator,