Add request dispatching logic to AuthenticatorDelegate

In order to prevent MacTouchId and BLE system UI dialog from appearing
prior to user explictly selecting which transport type to use,
ChromeAuthenticatorRequestDelegate should be able to "lazily" dispatch
WebAuthN request to FidoAuthenticators. Add logic trigger request from
embedder layer.

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

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -63,6 +63,13 @@ bool IsWebAuthnUiEnabled() {
       switches::kWebAuthenticationUI);
 }
 
+bool ShouldDispatchRequestImmediately(
+    const device::FidoAuthenticator& authenticator) {
+  // TODO(hongjunchoi): Change this so that requests for BLE and platform
+  // authenticators are not dispatched immediately if WebAuthN UI is enabled.
+  return true;
+}
+
 #if !defined(OS_ANDROID)
 void SetInitialUiModelBasedOnPreviouslyUsedTransport(
     AuthenticatorRequestDialogModel* model,
@@ -143,8 +150,11 @@ content::BrowserContext* ChromeAuthenticatorRequestDelegate::browser_context()
 }
 
 void ChromeAuthenticatorRequestDelegate::DidStartRequest(
-    base::OnceClosure cancel_callback) {
+    base::OnceClosure cancel_callback,
+    device::FidoRequestHandlerBase::RequestCallback request_callback) {
 #if !defined(OS_ANDROID)
+  request_callback_ = request_callback;
+
   if (!IsWebAuthnUiEnabled())
     return;
 
@@ -267,7 +277,11 @@ void ChromeAuthenticatorRequestDelegate::UpdateLastTransportUsed(
 }
 
 void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorAdded(
-    const device::FidoAuthenticator& authenticator) {
+    const device::FidoAuthenticator& authenticator,
+    bool* hold_off_request) {
+  if (!ShouldDispatchRequestImmediately(authenticator))
+    *hold_off_request = true;
+
   if (!IsWebAuthnUiEnabled())
     return;
 

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -63,7 +63,9 @@ class ChromeAuthenticatorRequestDelegate
   content::BrowserContext* browser_context() const;
 
   // content::AuthenticatorRequestClientDelegate:
-  void DidStartRequest(base::OnceClosure cancel_callback) override;
+  void DidStartRequest(base::OnceClosure cancel_callback,
+                       device::FidoRequestHandlerBase::RequestCallback
+                           request_callback) override;
   bool ShouldPermitIndividualAttestation(
       const std::string& relying_party_id) override;
   void ShouldReturnAttestation(
@@ -74,8 +76,8 @@ class ChromeAuthenticatorRequestDelegate
       device::FidoTransportProtocol transport) override;
 
   // device::FidoRequestHandlerBase::TransportAvailabilityObserver:
-  void FidoAuthenticatorAdded(
-      const device::FidoAuthenticator& authenticator) override;
+  void FidoAuthenticatorAdded(const device::FidoAuthenticator& authenticator,
+                              bool* hold_off_request) override;
   void FidoAuthenticatorRemoved(base::StringPiece device_id) override;
 
   // AuthenticatorRequestDialogModel::Observer:
@@ -85,6 +87,7 @@ class ChromeAuthenticatorRequestDelegate
   content::RenderFrameHost* const render_frame_host_;
   AuthenticatorRequestDialogModel* weak_dialog_model_ = nullptr;
   base::OnceClosure cancel_callback_;
+  device::FidoRequestHandlerBase::RequestCallback request_callback_;
 
   base::WeakPtrFactory<ChromeAuthenticatorRequestDelegate> weak_ptr_factory_;
 

content/browser/webauth/authenticator_impl.cc

@@ -452,9 +452,6 @@ void AuthenticatorImpl::MakeCredential(
 
   DCHECK(make_credential_response_callback_.is_null());
   make_credential_response_callback_ = std::move(callback);
-  request_delegate_->DidStartRequest(
-      base::BindOnce(&AuthenticatorImpl::Cancel,
-                     weak_factory_.GetWeakPtr()) /* cancel_callback */);
 
   timer_->Start(
       FROM_HERE, options->adjusted_timeout,
@@ -496,6 +493,13 @@ void AuthenticatorImpl::MakeCredential(
                      weak_factory_.GetWeakPtr()),
       base::BindOnce(&AuthenticatorImpl::CreatePlatformAuthenticatorIfAvailable,
                      base::Unretained(this)));
+
+  request_delegate_->DidStartRequest(
+      base::BindOnce(&AuthenticatorImpl::Cancel,
+                     weak_factory_.GetWeakPtr()) /* cancel_callback */,
+      base::BindRepeating(
+          &device::FidoRequestHandlerBase::StartAuthenticatorRequest,
+          request_->GetWeakPtr()) /* request_callback */);
   request_->set_observer(request_delegate_.get());
 }
 
@@ -549,9 +553,6 @@ void AuthenticatorImpl::GetAssertion(
 
   DCHECK(get_assertion_response_callback_.is_null());
   get_assertion_response_callback_ = std::move(callback);
-  request_delegate_->DidStartRequest(
-      base::BindOnce(&AuthenticatorImpl::Cancel,
-                     weak_factory_.GetWeakPtr()) /* cancel_callback */);
 
   timer_->Start(
       FROM_HERE, options->adjusted_timeout,
@@ -575,6 +576,13 @@ void AuthenticatorImpl::GetAssertion(
                      weak_factory_.GetWeakPtr()),
       base::BindOnce(&AuthenticatorImpl::CreatePlatformAuthenticatorIfAvailable,
                      base::Unretained(this)));
+
+  request_delegate_->DidStartRequest(
+      base::BindOnce(&AuthenticatorImpl::Cancel,
+                     weak_factory_.GetWeakPtr()) /* cancel_callback */,
+      base::BindRepeating(
+          &device::FidoRequestHandlerBase::StartAuthenticatorRequest,
+          request_->GetWeakPtr()) /* request_callback */);
   request_->set_observer(request_delegate_.get());
 }
 

content/browser/webauth/authenticator_impl_unittest.cc

@@ -1115,7 +1115,9 @@ class TestAuthenticatorRequestDelegate
         is_focused_(is_focused) {}
   ~TestAuthenticatorRequestDelegate() override {}
 
-  void DidStartRequest(base::OnceClosure cancel_callback) override {
+  void DidStartRequest(base::OnceClosure cancel_callback,
+                       device::FidoRequestHandlerBase::RequestCallback
+                           request_callback) override {
     ASSERT_TRUE(did_start_request_callback_) << "DidStartRequest called twice.";
     std::move(did_start_request_callback_).Run();
   }
@@ -1750,7 +1752,8 @@ class MockAuthenticatorRequestDelegateObserver
   MOCK_METHOD1(
       OnTransportAvailabilityEnumerated,
       void(device::FidoRequestHandlerBase::TransportAvailabilityInfo data));
-  MOCK_METHOD1(FidoAuthenticatorAdded, void(const device::FidoAuthenticator&));
+  MOCK_METHOD2(FidoAuthenticatorAdded,
+               void(const device::FidoAuthenticator&, bool*));
   MOCK_METHOD1(FidoAuthenticatorRemoved, void(base::StringPiece));
 
  private:
@@ -1863,7 +1866,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(); }));
 

content/public/browser/authenticator_request_client_delegate.cc

@@ -17,7 +17,8 @@ AuthenticatorRequestClientDelegate::~AuthenticatorRequestClientDelegate() =
     default;
 
 void AuthenticatorRequestClientDelegate::DidStartRequest(
-    base::OnceClosure cancel_callback) {}
+    base::OnceClosure cancel_callback,
+    device::FidoRequestHandlerBase::RequestCallback request_callback) {}
 
 bool AuthenticatorRequestClientDelegate::ShouldPermitIndividualAttestation(
     const std::string& relying_party_id) {
@@ -51,7 +52,8 @@ void AuthenticatorRequestClientDelegate::BluetoothAdapterPowerChanged(
     bool is_powered_on) {}
 
 void AuthenticatorRequestClientDelegate::FidoAuthenticatorAdded(
-    const device::FidoAuthenticator& authenticator) {}
+    const device::FidoAuthenticator& authenticator,
+    bool* hold_off_request) {}
 
 void AuthenticatorRequestClientDelegate::FidoAuthenticatorRemoved(
     base::StringPiece device_id) {}

content/public/browser/authenticator_request_client_delegate.h

@@ -33,7 +33,9 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   ~AuthenticatorRequestClientDelegate() override;
 
   // Notifies the delegate that the request is actually starting.
-  virtual void DidStartRequest(base::OnceClosure cancel_callback);
+  virtual void DidStartRequest(
+      base::OnceClosure cancel_callback,
+      device::FidoRequestHandlerBase::RequestCallback request_callback);
 
   // Returns true if the given relying party ID is permitted to receive
   // individual attestation certificates. This:
@@ -91,8 +93,8 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   void OnTransportAvailabilityEnumerated(
       device::FidoRequestHandlerBase::TransportAvailabilityInfo data) override;
   void BluetoothAdapterPowerChanged(bool is_powered_on) override;
-  void FidoAuthenticatorAdded(
-      const device::FidoAuthenticator& authenticator) override;
+  void FidoAuthenticatorAdded(const device::FidoAuthenticator& authenticator,
+                              bool* hold_off_request) override;
   void FidoAuthenticatorRemoved(base::StringPiece device_id) override;
 
  private:

device/fido/fido_request_handler_base.cc

@@ -19,16 +19,6 @@
 
 namespace device {
 
-namespace {
-
-bool ShouldDeferRequestDispatchToUi(const FidoAuthenticator& authenticator) {
-  // TODO(hongjunchoi): Change this to be dependent on authenticator transport
-  // type once UI component is in place.
-  return false;
-}
-
-}  // namespace
-
 // FidoRequestHandlerBase::TransportAvailabilityInfo --------------------------
 
 FidoRequestHandlerBase::TransportAvailabilityInfo::TransportAvailabilityInfo() =
@@ -117,6 +107,15 @@ FidoRequestHandlerBase::FidoRequestHandlerBase(
 
 FidoRequestHandlerBase::~FidoRequestHandlerBase() = default;
 
+void FidoRequestHandlerBase::StartAuthenticatorRequest(
+    const std::string& authenticator_id) {
+  auto authenticator = active_authenticators_.find(authenticator_id);
+  if (authenticator == active_authenticators_.end())
+    return;
+
+  DispatchRequest(authenticator->second.get());
+}
+
 void FidoRequestHandlerBase::CancelOngoingTasks(
     base::StringPiece exclude_device_id) {
   for (auto task_it = active_authenticators_.begin();
@@ -132,11 +131,22 @@ void FidoRequestHandlerBase::CancelOngoingTasks(
   }
 }
 
+base::WeakPtr<FidoRequestHandlerBase> FidoRequestHandlerBase::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 void FidoRequestHandlerBase::Start() {
   for (const auto& discovery : discoveries_)
     discovery->Start();
 
-  MaybeAddPlatformAuthenticator();
+  // Post |MaybeAddPlatformAuthenticator| into its own task. This avoids
+  // FidoAuthenticatorAdded() to be called synchronously from the constructor of
+  // FidoRequestHandlerBase prior to any TransportAvailabilityObserver being
+  // set.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&FidoRequestHandlerBase::MaybeAddPlatformAuthenticator,
+                     weak_factory_.GetWeakPtr()));
 }
 
 void FidoRequestHandlerBase::DiscoveryStarted(FidoDiscovery* discovery,
@@ -195,16 +205,23 @@ void FidoRequestHandlerBase::AddAuthenticator(
   FidoAuthenticator* authenticator_ptr = authenticator.get();
   active_authenticators_.emplace(authenticator->GetId(),
                                  std::move(authenticator));
-  if (!ShouldDeferRequestDispatchToUi(*authenticator_ptr)) {
-    // Post |DispatchRequest| 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::DispatchRequest,
-                                  GetWeakPtr(), authenticator_ptr));
-  }
 
+  // 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);
+    observer_->FidoAuthenticatorAdded(*authenticator_ptr,
+                                      &should_delay_request);
+
+  if (should_delay_request)
+    return;
+
+  // Post |DispatchRequest| 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::DispatchRequest,
+                                GetWeakPtr(), authenticator_ptr));
 }
 
 void FidoRequestHandlerBase::MaybeAddPlatformAuthenticator() {

device/fido/fido_request_handler_base.h

@@ -45,6 +45,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
       std::map<std::string, std::unique_ptr<FidoAuthenticator>, std::less<>>;
   using AddPlatformAuthenticatorCallback =
       base::OnceCallback<std::unique_ptr<FidoAuthenticator>()>;
+  using RequestCallback = base::RepeatingCallback<void(const std::string&)>;
 
   enum class RequestType { kMakeCredential, kGetAssertion };
 
@@ -79,8 +80,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
     virtual void OnTransportAvailabilityEnumerated(
         TransportAvailabilityInfo data) = 0;
     virtual void BluetoothAdapterPowerChanged(bool is_powered_on) = 0;
-    virtual void FidoAuthenticatorAdded(
-        const FidoAuthenticator& authenticator) = 0;
+    virtual void FidoAuthenticatorAdded(const FidoAuthenticator& authenticator,
+                                        bool* hold_off_request) = 0;
     virtual void FidoAuthenticatorRemoved(base::StringPiece device_id) = 0;
   };
 
@@ -97,6 +98,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
       AddPlatformAuthenticatorCallback add_platform_authenticator);
   ~FidoRequestHandlerBase() override;
 
+  // Triggers DispatchRequest() if |active_authenticators_| hold
+  // 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_|.
@@ -109,6 +114,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   // https://w3c.github.io/webauthn/#iface-pkcredential
   void CancelOngoingTasks(base::StringPiece exclude_device_id = nullptr);
 
+  base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr();
+
   void set_observer(TransportAvailabilityObserver* observer) {
     DCHECK(!observer_) << "Only one observer is supported.";
     observer_ = observer;
@@ -122,8 +129,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   }
 
  protected:
-  virtual base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr() = 0;
-
   // Subclasses implement this method to dispatch their request onto the given
   // FidoAuthenticator. The FidoAuthenticator is owned by this
   // FidoRequestHandler and stored in active_authenticators().

device/fido/fido_request_handler_unittest.cc

@@ -72,8 +72,8 @@ class TestTransportAvailabilityObserver
   }
 
   void BluetoothAdapterPowerChanged(bool is_powered_on) override {}
-  void FidoAuthenticatorAdded(const FidoAuthenticator& authenticator) override {
-  }
+  void FidoAuthenticatorAdded(const FidoAuthenticator& authenticator,
+                              bool* hold_off_request) override {}
   void FidoAuthenticatorRemoved(base::StringPiece device_id) override {}
 
  private:
@@ -126,7 +126,8 @@ class FakeFidoTask : public FidoTask {
 
 class FakeFidoAuthenticator : public FidoDeviceAuthenticator {
  public:
-  FakeFidoAuthenticator(FidoDevice* device) : FidoDeviceAuthenticator(device) {}
+  explicit FakeFidoAuthenticator(FidoDevice* device)
+      : FidoDeviceAuthenticator(device) {}
 
   void RunFakeTask(FakeTaskCallback callback) {
     SetTaskForTesting(
@@ -150,10 +151,6 @@ class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> {
   }
   ~FakeFidoRequestHandler() override = default;
 
-  base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr() override {
-    return weak_factory_.GetWeakPtr();
-  }
-
   void DispatchRequest(FidoAuthenticator* authenticator) override {
     static_cast<FakeFidoAuthenticator*>(authenticator)
         ->RunFakeTask(

device/fido/get_assertion_request_handler.cc

@@ -190,10 +190,6 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
 
 GetAssertionRequestHandler::~GetAssertionRequestHandler() = default;
 
-base::WeakPtr<FidoRequestHandlerBase> GetAssertionRequestHandler::GetWeakPtr() {
-  return weak_factory_.GetWeakPtr();
-}
-
 void GetAssertionRequestHandler::DispatchRequest(
     FidoAuthenticator* authenticator) {
   // The user verification field of the request may be adjusted to the

device/fido/get_assertion_request_handler.h

@@ -48,7 +48,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
 
  private:
   // FidoRequestHandlerBase:
-  base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr() final;
   void DispatchRequest(FidoAuthenticator* authenticator) override;
 
   void HandleResponse(

device/fido/make_credential_request_handler.cc

@@ -130,11 +130,6 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
 
 MakeCredentialRequestHandler::~MakeCredentialRequestHandler() = default;
 
-base::WeakPtr<FidoRequestHandlerBase>
-MakeCredentialRequestHandler::GetWeakPtr() {
-  return weak_factory_.GetWeakPtr();
-}
-
 void MakeCredentialRequestHandler::DispatchRequest(
     FidoAuthenticator* authenticator) {
   // The user verification field of the request may be adjusted to the

device/fido/make_credential_request_handler.h

@@ -52,7 +52,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
 
  private:
   // FidoRequestHandlerBase:
-  base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr() final;
   void DispatchRequest(FidoAuthenticator* authenticator) final;
 
   void HandleResponse(