Make WebAuthN UI appear after device/fido signals

Make WebAuthN UI modals appear after all fields of
TransportAvailabilityInfo have been set by FidoRequestHandlerBase. Also,
Add rp_id to TransportAvailabilityInfo struct as rp id is shown in the
UI dialogs.

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

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -149,26 +149,11 @@ content::BrowserContext* ChromeAuthenticatorRequestDelegate::browser_context()
       ->GetBrowserContext();
 }
 
-void ChromeAuthenticatorRequestDelegate::DidStartRequest(
+void ChromeAuthenticatorRequestDelegate::RegisterActionCallbacks(
     base::OnceClosure cancel_callback,
     device::FidoRequestHandlerBase::RequestCallback request_callback) {
-#if !defined(OS_ANDROID)
   request_callback_ = request_callback;
-
-  if (!IsWebAuthnUiEnabled())
-    return;
-
-  auto dialog_model = std::make_unique<AuthenticatorRequestDialogModel>();
-  weak_dialog_model_ = dialog_model.get();
-  SetInitialUiModelBasedOnPreviouslyUsedTransport(weak_dialog_model_,
-                                                  GetLastTransportUsed());
   cancel_callback_ = std::move(cancel_callback);
-  weak_dialog_model_->AddObserver(this);
-
-  ShowAuthenticatorRequestDialog(
-      content::WebContents::FromRenderFrameHost(render_frame_host()),
-      std::move(dialog_model));
-#endif
 }
 
 bool ChromeAuthenticatorRequestDelegate::ShouldPermitIndividualAttestation(
@@ -276,6 +261,22 @@ void ChromeAuthenticatorRequestDelegate::UpdateLastTransportUsed(
                    device::ToString(transport));
 }
 
+void ChromeAuthenticatorRequestDelegate::OnTransportAvailabilityEnumerated(
+    device::FidoRequestHandlerBase::TransportAvailabilityInfo data) {
+#if !defined(OS_ANDROID)
+  if (!IsWebAuthnUiEnabled())
+    return;
+
+  weak_dialog_model_ = transient_dialog_model_holder_.get();
+  SetInitialUiModelBasedOnPreviouslyUsedTransport(weak_dialog_model_,
+                                                  GetLastTransportUsed());
+  weak_dialog_model_->AddObserver(this);
+  ShowAuthenticatorRequestDialog(
+      content::WebContents::FromRenderFrameHost(render_frame_host()),
+      std::move(transient_dialog_model_holder_));
+#endif
+}
+
 void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorAdded(
     const device::FidoAuthenticator& authenticator,
     bool* hold_off_request) {

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_WEBAUTHN_CHROME_AUTHENTICATOR_REQUEST_DELEGATE_H_
 #define CHROME_BROWSER_WEBAUTHN_CHROME_AUTHENTICATOR_REQUEST_DELEGATE_H_
 
+#include <memory>
 #include <string>
 
 #include "base/callback.h"
@@ -63,9 +64,9 @@ class ChromeAuthenticatorRequestDelegate
   content::BrowserContext* browser_context() const;
 
   // content::AuthenticatorRequestClientDelegate:
-  void DidStartRequest(base::OnceClosure cancel_callback,
-                       device::FidoRequestHandlerBase::RequestCallback
-                           request_callback) override;
+  void RegisterActionCallbacks(base::OnceClosure cancel_callback,
+                               device::FidoRequestHandlerBase::RequestCallback
+                                   request_callback) override;
   bool ShouldPermitIndividualAttestation(
       const std::string& relying_party_id) override;
   void ShouldReturnAttestation(
@@ -76,6 +77,8 @@ class ChromeAuthenticatorRequestDelegate
       device::FidoTransportProtocol transport) override;
 
   // device::FidoRequestHandlerBase::TransportAvailabilityObserver:
+  void OnTransportAvailabilityEnumerated(
+      device::FidoRequestHandlerBase::TransportAvailabilityInfo data) override;
   void FidoAuthenticatorAdded(const device::FidoAuthenticator& authenticator,
                               bool* hold_off_request) override;
   void FidoAuthenticatorRemoved(base::StringPiece device_id) override;
@@ -86,6 +89,13 @@ class ChromeAuthenticatorRequestDelegate
 
   content::RenderFrameHost* const render_frame_host_;
   AuthenticatorRequestDialogModel* weak_dialog_model_ = nullptr;
+  // Holds ownership of AuthenticatorRequestDialogModel until
+  // OnTransportAvailabilityEnumerated() is invoked, at which point the
+  // ownership of the model is transferred to AuthenticatorRequestDialogView and
+  // |this| instead holds weak pointer of the model via above
+  // |weak_dialog_model_|.
+  std::unique_ptr<AuthenticatorRequestDialogModel>
+      transient_dialog_model_holder_;
   base::OnceClosure cancel_callback_;
   device::FidoRequestHandlerBase::RequestCallback request_callback_;
 

content/browser/webauth/authenticator_impl.cc

@@ -494,7 +494,7 @@ void AuthenticatorImpl::MakeCredential(
       base::BindOnce(&AuthenticatorImpl::CreatePlatformAuthenticatorIfAvailable,
                      base::Unretained(this)));
 
-  request_delegate_->DidStartRequest(
+  request_delegate_->RegisterActionCallbacks(
       base::BindOnce(&AuthenticatorImpl::Cancel,
                      weak_factory_.GetWeakPtr()) /* cancel_callback */,
       base::BindRepeating(
@@ -577,7 +577,7 @@ void AuthenticatorImpl::GetAssertion(
       base::BindOnce(&AuthenticatorImpl::CreatePlatformAuthenticatorIfAvailable,
                      base::Unretained(this)));
 
-  request_delegate_->DidStartRequest(
+  request_delegate_->RegisterActionCallbacks(
       base::BindOnce(&AuthenticatorImpl::Cancel,
                      weak_factory_.GetWeakPtr()) /* cancel_callback */,
       base::BindRepeating(

content/browser/webauth/authenticator_impl_unittest.cc

@@ -1104,22 +1104,25 @@ enum class AttestationType {
 class TestAuthenticatorRequestDelegate
     : public AuthenticatorRequestClientDelegate {
  public:
-  TestAuthenticatorRequestDelegate(RenderFrameHost* render_frame_host,
-                                   base::OnceClosure did_start_request_callback,
-                                   IndividualAttestation individual_attestation,
-                                   AttestationConsent attestation_consent,
-                                   bool is_focused)
-      : did_start_request_callback_(std::move(did_start_request_callback)),
+  TestAuthenticatorRequestDelegate(
+      RenderFrameHost* render_frame_host,
+      base::OnceClosure action_callbacks_registered_callback,
+      IndividualAttestation individual_attestation,
+      AttestationConsent attestation_consent,
+      bool is_focused)
+      : action_callbacks_registered_callback_(
+            std::move(action_callbacks_registered_callback)),
         individual_attestation_(individual_attestation),
         attestation_consent_(attestation_consent),
         is_focused_(is_focused) {}
   ~TestAuthenticatorRequestDelegate() 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();
+  void RegisterActionCallbacks(base::OnceClosure cancel_callback,
+                               device::FidoRequestHandlerBase::RequestCallback
+                                   request_callback) override {
+    ASSERT_TRUE(action_callbacks_registered_callback_)
+        << "RegisterActionCallbacks called twice.";
+    std::move(action_callbacks_registered_callback_).Run();
   }
 
   bool ShouldPermitIndividualAttestation(
@@ -1136,7 +1139,7 @@ class TestAuthenticatorRequestDelegate
 
   bool IsFocused() override { return is_focused_; }
 
-  base::OnceClosure did_start_request_callback_;
+  base::OnceClosure action_callbacks_registered_callback_;
   const IndividualAttestation individual_attestation_;
   const AttestationConsent attestation_consent_;
   const bool is_focused_;
@@ -1154,8 +1157,9 @@ class TestAuthenticatorContentBrowserClient : public ContentBrowserClient {
       return nullptr;
     return std::make_unique<TestAuthenticatorRequestDelegate>(
         render_frame_host,
-        request_started_callback ? std::move(request_started_callback)
-                                 : base::DoNothing(),
+        action_callbacks_registered_callback
+            ? std::move(action_callbacks_registered_callback)
+            : base::DoNothing(),
         individual_attestation, attestation_consent, is_focused);
   }
 
@@ -1169,7 +1173,7 @@ class TestAuthenticatorContentBrowserClient : public ContentBrowserClient {
 
   // If set, this closure will be called when the subsequently constructed
   // delegate is informed that the request has started.
-  base::OnceClosure request_started_callback;
+  base::OnceClosure action_callbacks_registered_callback;
 
   IndividualAttestation individual_attestation =
       IndividualAttestation::NOT_REQUESTED;
@@ -1567,7 +1571,8 @@ TEST_F(AuthenticatorContentBrowserClientTest,
       GetTestPublicKeyCredentialCreationOptions();
 
   TestRequestStartedCallback request_started;
-  test_client_.request_started_callback = request_started.callback();
+  test_client_.action_callbacks_registered_callback =
+      request_started.callback();
   authenticator->MakeCredential(std::move(options), base::DoNothing());
   request_started.WaitForCallback();
 }
@@ -1582,7 +1587,8 @@ TEST_F(AuthenticatorContentBrowserClientTest,
       GetTestPublicKeyCredentialRequestOptions();
 
   TestRequestStartedCallback request_started;
-  test_client_.request_started_callback = request_started.callback();
+  test_client_.action_callbacks_registered_callback =
+      request_started.callback();
   authenticator->GetAssertion(std::move(options), base::DoNothing());
   request_started.WaitForCallback();
 }
@@ -1603,7 +1609,8 @@ TEST_F(AuthenticatorContentBrowserClientTest, Unfocused) {
 
     TestMakeCredentialCallback cb;
     TestRequestStartedCallback request_started;
-    test_client_.request_started_callback = request_started.callback();
+    test_client_.action_callbacks_registered_callback =
+        request_started.callback();
 
     authenticator->MakeCredential(std::move(options), cb.callback());
     cb.WaitForCallback();
@@ -1629,7 +1636,8 @@ TEST_F(AuthenticatorContentBrowserClientTest, Unfocused) {
 
     TestGetAssertionCallback cb;
     TestRequestStartedCallback request_started;
-    test_client_.request_started_callback = request_started.callback();
+    test_client_.action_callbacks_registered_callback =
+        request_started.callback();
 
     authenticator->GetAssertion(std::move(options), cb.callback());
     cb.WaitForCallback();

content/public/browser/authenticator_request_client_delegate.cc

@@ -16,7 +16,7 @@ AuthenticatorRequestClientDelegate::AuthenticatorRequestClientDelegate() =
 AuthenticatorRequestClientDelegate::~AuthenticatorRequestClientDelegate() =
     default;
 
-void AuthenticatorRequestClientDelegate::DidStartRequest(
+void AuthenticatorRequestClientDelegate::RegisterActionCallbacks(
     base::OnceClosure cancel_callback,
     device::FidoRequestHandlerBase::RequestCallback request_callback) {}
 

content/public/browser/authenticator_request_client_delegate.h

@@ -32,8 +32,10 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   AuthenticatorRequestClientDelegate();
   ~AuthenticatorRequestClientDelegate() override;
 
-  // Notifies the delegate that the request is actually starting.
-  virtual void DidStartRequest(
+  // Supplies callbacks that the embedder can invoke to initiate certain
+  // actions, namely: initiate BLE pairing process, cancel WebAuthN request, and
+  // dispatch request to connected authenticators.
+  virtual void RegisterActionCallbacks(
       base::OnceClosure cancel_callback,
       device::FidoRequestHandlerBase::RequestCallback request_callback);
 

device/fido/fido_request_handler_base.cc

@@ -101,8 +101,9 @@ FidoRequestHandlerBase::FidoRequestHandlerBase(
   transport_availability_info_.available_transports = available_transports;
   notify_observer_callback_ = base::BarrierClosure(
       transport_info_callback_count,
-      base::BindOnce(&FidoRequestHandlerBase::NotifyObserverUiData,
-                     weak_factory_.GetWeakPtr()));
+      base::BindOnce(
+          &FidoRequestHandlerBase::NotifyObserverTransportAvailability,
+          weak_factory_.GetWeakPtr()));
 }
 
 FidoRequestHandlerBase::~FidoRequestHandlerBase() = default;
@@ -243,9 +244,8 @@ void FidoRequestHandlerBase::MaybeAddPlatformAuthenticator() {
   notify_observer_callback_.Run();
 }
 
-void FidoRequestHandlerBase::NotifyObserverUiData() {
+void FidoRequestHandlerBase::NotifyObserverTransportAvailability() {
   DCHECK(observer_);
-
   observer_->OnTransportAvailabilityEnumerated(transport_availability_info_);
 }
 

device/fido/fido_request_handler_base.h

@@ -61,6 +61,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
         const TransportAvailabilityInfo& other);
     ~TransportAvailabilityInfo();
 
+    // TODO(hongjunchoi): Factor |rp_id| and |request_type| from
+    // TransportAvailabilityInfo.
+    // See: https://crbug.com/875011
+    std::string rp_id;
     RequestType request_type = RequestType::kMakeCredential;
 
     // The intersection of transports supported by the client and allowed by the
@@ -155,7 +159,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
 
   void AddAuthenticator(std::unique_ptr<FidoAuthenticator> authenticator);
   void MaybeAddPlatformAuthenticator();
-  void NotifyObserverUiData();
+  void NotifyObserverTransportAvailability();
 
   AuthenticatorMap active_authenticators_;
   std::vector<std::unique_ptr<FidoDiscovery>> discoveries_;

device/fido/get_assertion_request_handler.cc

@@ -172,6 +172,7 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
           std::move(add_platform_authenticator)),
       request_(std::move(request)),
       weak_factory_(this) {
+  transport_availability_info().rp_id = request.rp_id();
   transport_availability_info().request_type =
       FidoRequestHandlerBase::RequestType::kGetAssertion;
 

device/fido/make_credential_request_handler.cc

@@ -123,6 +123,7 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
       authenticator_selection_criteria_(
           std::move(authenticator_selection_criteria)),
       weak_factory_(this) {
+  transport_availability_info().rp_id = request.rp().rp_id();
   transport_availability_info().request_type =
       FidoRequestHandlerBase::RequestType::kMakeCredential;
   Start();