Decouple creating AuthenticatorRequestClientDelegate from showing UI.

This allows AuthenticatorImpl to first create the delegate and ask it if the
WebContents has focus, and only then triggering the request dialog (which may
steal the focus), so as to avoid race conditions around focus checking at
request start.

The CL also fixes fake FidoDiscoveries to use weak pointers so as to support
being immediately destroyed after the discovery is started.

Bug: 849323, 851593
Change-Id: Iae4ac25dc39b527e5481836e4522a6f4da7e31ba
Reviewed-on: https://chromium-review.googlesource.com/1095619
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566750}

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -54,20 +54,7 @@ bool IsWebauthnRPIDListedInEnterprisePolicy(
 
 ChromeAuthenticatorRequestDelegate::ChromeAuthenticatorRequestDelegate(
     content::RenderFrameHost* render_frame_host)
-    : render_frame_host_(render_frame_host), weak_ptr_factory_(this) {
-#if !defined(OS_ANDROID)
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kWebAuthenticationUI)) {
-    return;
-  }
-  auto dialog_model = std::make_unique<AuthenticatorRequestDialogModel>();
-  weak_dialog_model_ = dialog_model.get();
-  weak_dialog_model_->AddObserver(this);
-  ShowAuthenticatorRequestDialog(
-      content::WebContents::FromRenderFrameHost(render_frame_host),
-      std::move(dialog_model));
-#endif
-}
+    : render_frame_host_(render_frame_host), weak_ptr_factory_(this) {}
 
 ChromeAuthenticatorRequestDelegate::~ChromeAuthenticatorRequestDelegate() {
   // Currently, completion of the request is indicated by //content destroying
@@ -88,6 +75,21 @@ ChromeAuthenticatorRequestDelegate::AsWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
 }
 
+void ChromeAuthenticatorRequestDelegate::DidStartRequest() {
+#if !defined(OS_ANDROID)
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kWebAuthenticationUI)) {
+    return;
+  }
+  auto dialog_model = std::make_unique<AuthenticatorRequestDialogModel>();
+  weak_dialog_model_ = dialog_model.get();
+  weak_dialog_model_->AddObserver(this);
+  ShowAuthenticatorRequestDialog(
+      content::WebContents::FromRenderFrameHost(render_frame_host()),
+      std::move(dialog_model));
+#endif
+}
+
 bool ChromeAuthenticatorRequestDelegate::ShouldPermitIndividualAttestation(
     const std::string& relying_party_id) {
   // If the RP ID is listed in the policy, signal that individual attestation is

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -32,6 +32,7 @@ class ChromeAuthenticatorRequestDelegate
   }
 
   // content::AuthenticatorRequestClientDelegate:
+  void DidStartRequest() override;
   bool ShouldPermitIndividualAttestation(
       const std::string& relying_party_id) override;
   void ShouldReturnAttestation(

content/browser/webauth/authenticator_impl.cc

@@ -460,6 +460,7 @@ void AuthenticatorImpl::MakeCredential(
 
   DCHECK(make_credential_response_callback_.is_null());
   make_credential_response_callback_ = std::move(callback);
+  request_delegate_->DidStartRequest();
 
   timer_->Start(
       FROM_HERE, options->adjusted_timeout,
@@ -562,6 +563,7 @@ void AuthenticatorImpl::GetAssertion(
 
   DCHECK(get_assertion_response_callback_.is_null());
   get_assertion_response_callback_ = std::move(callback);
+  request_delegate_->DidStartRequest();
 
   timer_->Start(
       FROM_HERE, options->adjusted_timeout,

content/browser/webauth/authenticator_impl_unittest.cc

@@ -9,6 +9,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/bind_helpers.h"
 #include "base/json/json_parser.h"
 #include "base/json/json_writer.h"
 #include "base/run_loop.h"
@@ -198,6 +199,7 @@ using TestMakeCredentialCallback = device::test::StatusAndValueCallbackReceiver<
 using TestGetAssertionCallback = device::test::StatusAndValueCallbackReceiver<
     AuthenticatorStatus,
     GetAssertionAuthenticatorResponsePtr>;
+using TestRequestStartedCallback = device::test::TestCallbackReceiver<>;
 
 std::vector<uint8_t> GetTestChallengeBytes() {
   return std::vector<uint8_t>(std::begin(kTestChallengeBytes),
@@ -1045,14 +1047,21 @@ 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)
-      : individual_attestation_(individual_attestation),
+      : did_start_request_callback_(std::move(did_start_request_callback)),
+        individual_attestation_(individual_attestation),
         attestation_consent_(attestation_consent),
         is_focused_(is_focused) {}
   ~TestAuthenticatorRequestDelegate() override {}
 
+  void DidStartRequest() override {
+    ASSERT_TRUE(did_start_request_callback_) << "DidStartRequest called twice.";
+    std::move(did_start_request_callback_).Run();
+  }
+
   bool ShouldPermitIndividualAttestation(
       const std::string& relying_party_id) override {
     return individual_attestation_ == IndividualAttestation::REQUESTED;
@@ -1067,6 +1076,7 @@ class TestAuthenticatorRequestDelegate
 
   bool IsFocused() override { return is_focused_; }
 
+  base::OnceClosure did_start_request_callback_;
   const IndividualAttestation individual_attestation_;
   const AttestationConsent attestation_consent_;
   const bool is_focused_;
@@ -1083,10 +1093,16 @@ class TestAuthenticatorContentBrowserClient : public ContentBrowserClient {
     if (return_null_delegate)
       return nullptr;
     return std::make_unique<TestAuthenticatorRequestDelegate>(
-        render_frame_host, individual_attestation, attestation_consent,
-        is_focused);
+        render_frame_host,
+        request_started_callback ? std::move(request_started_callback)
+                                 : base::DoNothing(),
+        individual_attestation, attestation_consent, is_focused);
   }
 
+  // If set, this closure will be called when the subsequently constructed
+  // delegate is informed that the request has started.
+  base::OnceClosure request_started_callback;
+
   IndividualAttestation individual_attestation =
       IndividualAttestation::NOT_REQUESTED;
   AttestationConsent attestation_consent = AttestationConsent::DENIED;
@@ -1369,6 +1385,36 @@ TEST_F(AuthenticatorContentBrowserClientTest,
   RunTestCases(kTests);
 }
 
+TEST_F(AuthenticatorContentBrowserClientTest,
+       MakeCredentialRequestStartedCallback) {
+  TestServiceManagerContext smc;
+  NavigateAndCommit(GURL(kTestOrigin1));
+  AuthenticatorPtr authenticator = ConnectToAuthenticator();
+
+  PublicKeyCredentialCreationOptionsPtr options =
+      GetTestPublicKeyCredentialCreationOptions();
+
+  TestRequestStartedCallback request_started;
+  test_client_.request_started_callback = request_started.callback();
+  authenticator->MakeCredential(std::move(options), base::DoNothing());
+  request_started.WaitForCallback();
+}
+
+TEST_F(AuthenticatorContentBrowserClientTest,
+       GetAssertionRequestStartedCallback) {
+  TestServiceManagerContext smc;
+  NavigateAndCommit(GURL(kTestOrigin1));
+  AuthenticatorPtr authenticator = ConnectToAuthenticator();
+
+  PublicKeyCredentialRequestOptionsPtr options =
+      GetTestPublicKeyCredentialRequestOptions();
+
+  TestRequestStartedCallback request_started;
+  test_client_.request_started_callback = request_started.callback();
+  authenticator->GetAssertion(std::move(options), base::DoNothing());
+  request_started.WaitForCallback();
+}
+
 TEST_F(AuthenticatorContentBrowserClientTest, Unfocused) {
   // When the |ContentBrowserClient| considers the tab to be unfocused,
   // registration requests should fail with a |NOT_FOCUSED| error, but getting
@@ -1384,9 +1430,14 @@ TEST_F(AuthenticatorContentBrowserClientTest, Unfocused) {
     options->public_key_parameters = GetTestPublicKeyCredentialParameters(123);
 
     TestMakeCredentialCallback cb;
+    TestRequestStartedCallback request_started;
+    test_client_.request_started_callback = request_started.callback();
+
     authenticator->MakeCredential(std::move(options), cb.callback());
     cb.WaitForCallback();
+
     EXPECT_EQ(AuthenticatorStatus::NOT_FOCUSED, cb.status());
+    EXPECT_FALSE(request_started.was_called());
   }
 
   {
@@ -1403,10 +1454,14 @@ TEST_F(AuthenticatorContentBrowserClientTest, Unfocused) {
     options->allow_credentials.emplace_back(std::move(credential));
 
     TestGetAssertionCallback cb;
+    TestRequestStartedCallback request_started;
+    test_client_.request_started_callback = request_started.callback();
+
     authenticator->GetAssertion(std::move(options), cb.callback());
     cb.WaitForCallback();
 
     EXPECT_EQ(AuthenticatorStatus::SUCCESS, cb.status());
+    EXPECT_TRUE(request_started.was_called());
   }
 }
 

content/public/browser/authenticator_request_client_delegate.cc

@@ -13,6 +13,8 @@ AuthenticatorRequestClientDelegate::AuthenticatorRequestClientDelegate() =
 AuthenticatorRequestClientDelegate::~AuthenticatorRequestClientDelegate() =
     default;
 
+void AuthenticatorRequestClientDelegate::DidStartRequest() {}
+
 bool AuthenticatorRequestClientDelegate::ShouldPermitIndividualAttestation(
     const std::string& relying_party_id) {
   return false;

content/public/browser/authenticator_request_client_delegate.h

@@ -23,6 +23,9 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate {
   AuthenticatorRequestClientDelegate();
   virtual ~AuthenticatorRequestClientDelegate();
 
+  // Notifies the delegate that the request is actually starting.
+  virtual void DidStartRequest();
+
   // Returns true if the given relying party ID is permitted to receive
   // individual attestation certificates. This:
   //  a) triggers a signal to the security key that returning individual

device/fido/fake_fido_discovery.cc

@@ -43,7 +43,7 @@ void FakeFidoDiscovery::StartInternal() {
   if (mode_ == StartMode::kAutomatic) {
     base::SequencedTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(&FakeFidoDiscovery::SimulateStarted,
-                                  base::Unretained(this), true /* success */));
+                                  AsWeakPtr(), true /* success */));
   }
 }
 

device/fido/scoped_virtual_fido_device.cc

@@ -10,6 +10,7 @@
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "device/fido/virtual_u2f_device.h"
 
@@ -17,7 +18,9 @@ namespace device {
 namespace test {
 
 // A FidoDiscovery that always vends a single |VirtualFidoDevice|.
-class VirtualFidoDeviceDiscovery : public FidoDiscovery {
+class VirtualFidoDeviceDiscovery
+    : public FidoDiscovery,
+      public base::SupportsWeakPtr<VirtualFidoDeviceDiscovery> {
  public:
   explicit VirtualFidoDeviceDiscovery(
       scoped_refptr<VirtualFidoDevice::State> state)
@@ -32,7 +35,7 @@ class VirtualFidoDeviceDiscovery : public FidoDiscovery {
     base::SequencedTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::BindOnce(&VirtualFidoDeviceDiscovery::NotifyDiscoveryStarted,
-                       base::Unretained(this), true /* success */));
+                       AsWeakPtr(), true /* success */));
   }
 
  private: