diff --git a/chrome/browser/webauthn/chrome_authenticator_request_delegate.cc b/chrome/browser/webauthn/chrome_authenticator_request_delegate.cc index 8850cf639fc541..dfafcd720e1cc6 100644 --- 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 index 83bff00d5269a8..e8f9126860ede3 100644 --- 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 index 6222b2290f8a56..d55fd0dbd5ec40 100644 --- 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 index 8275010ffbf5ec..caf59614b260fd 100644 --- 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 index 0fc72787e5fb9a..df7164c04381da 100644 --- 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 index 7886f009ad4901..ebe07db7e34c36 100644 --- 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 index fa1f960a6f9618..f6d5967287f56b 100644 --- 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 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 index 94948f9a1ea4a9..099070f779dd8a 100644 --- 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 index 0ffd560ffac24f..ae20083efbe820 100644 --- 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,6 +268,11 @@ 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( @@ -280,6 +280,13 @@ TEST_F(FidoMakeCredentialHandlerTest, AnyAttachment) { 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 index 6586589da57058..6149654bdaf17d 100644 --- 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 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 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 index d27637d5321b83..a5e799f0f7fa50 100644 --- 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 platform_authenticator_info) + override; + private: // FidoRequestHandlerBase: - void DispatchRequest(FidoAuthenticator* authenticator) final; + void DispatchRequest(FidoAuthenticator* authenticator) override; void HandleResponse( FidoAuthenticator* authenticator,