webauthn: prevent UI from dispatching to the same authenticator multiple times

Using the transport switch drop-down menu, it is possible to cycle away
from and then back to the Touch ID sheet, which currently causes the
request to be dispatched onto the TouchIdAuthenticator each time the
sheet is shown. This sort of works in the production build (it restarts
the request, which will make the Touch ID dialog disappear and then
reappear). In debug, however, it crashes on a DCHECK.

We haven't defined whether a FidoAuthenticator should be able to handle
multiple invocations of MakeCredential/GetAssertion over the lifetime of
the instance. Hence, the UI should stop doing this.

Bug: 678128,847985
Change-Id: Id008de98bc8aa6477e9863ca2e22cd136316b423
Reviewed-on: https://chromium-review.googlesource.com/1196427
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587575}

chrome/browser/webauthn/authenticator_request_dialog_model.cc

@@ -186,27 +186,19 @@ void AuthenticatorRequestDialogModel::StartTouchIdFlow() {
 
   SetCurrentStep(Step::kTouchId);
 
-  if (!request_callback_)
-    return;
-
-  auto touch_id_authenticator =
+  auto touch_id_authenticator_it =
       std::find_if(saved_authenticators_.begin(), saved_authenticators_.end(),
                    [](const auto& authenticator) {
                      return authenticator.transport ==
                             device::FidoTransportProtocol::kInternal;
                    });
 
-  if (touch_id_authenticator == saved_authenticators_.end())
+  if (touch_id_authenticator_it == saved_authenticators_.end())
     return;
 
   static base::TimeDelta kTouchIdDispatchDelay =
       base::TimeDelta::FromMilliseconds(1250);
-
-  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(request_callback_,
-                     touch_id_authenticator->authenticator_id),
-      kTouchIdDispatchDelay);
+  DispatchRequestAsync(&*touch_id_authenticator_it, kTouchIdDispatchDelay);
 }
 
 void AuthenticatorRequestDialogModel::Cancel() {
@@ -263,7 +255,7 @@ void AuthenticatorRequestDialogModel::OnBluetoothPoweredStateChanged(
 }
 
 void AuthenticatorRequestDialogModel::SetRequestCallback(
-    device::FidoRequestHandlerBase::RequestCallback request_callback) {
+    RequestCallback request_callback) {
   request_callback_ = request_callback;
 }
 
@@ -271,3 +263,21 @@ void AuthenticatorRequestDialogModel::SetBluetoothAdapterPowerOnCallback(
     base::RepeatingClosure bluetooth_adapter_power_on_callback) {
   bluetooth_adapter_power_on_callback_ = bluetooth_adapter_power_on_callback;
 }
+
+void AuthenticatorRequestDialogModel::DispatchRequestAsync(
+    AuthenticatorReference* authenticator,
+    base::TimeDelta delay) {
+  if (!request_callback_)
+    return;
+
+  // Dispatching to the same authenticator twice may result in unexpected
+  // behavior.
+  if (authenticator->dispatched)
+    return;
+
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(request_callback_, authenticator->authenticator_id),
+      delay);
+  authenticator->dispatched = true;
+}

chrome/browser/webauthn/authenticator_request_dialog_model.h

@@ -24,6 +24,7 @@
 // order, to complete the authentication flow.
 class AuthenticatorRequestDialogModel {
  public:
+  using RequestCallback = device::FidoRequestHandlerBase::RequestCallback;
   using TransportAvailabilityInfo =
       device::FidoRequestHandlerBase::TransportAvailabilityInfo;
 
@@ -83,6 +84,7 @@ class AuthenticatorRequestDialogModel {
 
     std::string authenticator_id;
     device::FidoTransportProtocol transport;
+    bool dispatched = false;
   };
 
   // Implemented by the dialog to observe this model and show the UI panels
@@ -220,8 +222,7 @@ class AuthenticatorRequestDialogModel {
   // To be called when the Bluetooth adapter powered state changes.
   void OnBluetoothPoweredStateChanged(bool powered);
 
-  void SetRequestCallback(
-      device::FidoRequestHandlerBase::RequestCallback request_callback);
+  void SetRequestCallback(RequestCallback request_callback);
 
   void SetBluetoothAdapterPowerOnCallback(
       base::RepeatingClosure bluetooth_adapter_power_on_callback);
@@ -231,6 +232,9 @@ class AuthenticatorRequestDialogModel {
   }
 
  private:
+  void DispatchRequestAsync(AuthenticatorReference* authenticator,
+                            base::TimeDelta delay);
+
   // The current step of the request UX flow that is currently shown.
   Step current_step_ = Step::kNotStarted;
 
@@ -245,7 +249,7 @@ class AuthenticatorRequestDialogModel {
   // that the WebAuthN request for the corresponding authenticators can be
   // dispatched lazily after the user interacts with the UI element.
   std::vector<AuthenticatorReference> saved_authenticators_;
-  device::FidoRequestHandlerBase::RequestCallback request_callback_;
+  RequestCallback request_callback_;
   base::RepeatingClosure bluetooth_adapter_power_on_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(AuthenticatorRequestDialogModel);

chrome/browser/webauthn/authenticator_request_dialog_model_unittest.cc

@@ -6,9 +6,11 @@
 
 #include <utility>
 
+#include "base/callback_helpers.h"
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "base/test/scoped_task_environment.h"
 #include "chrome/browser/webauthn/transport_list_model.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -46,6 +48,10 @@ class AuthenticatorRequestDialogModelTest : public ::testing::Test {
   AuthenticatorRequestDialogModelTest() {}
   ~AuthenticatorRequestDialogModelTest() override {}
 
+ protected:
+  base::test::ScopedTaskEnvironment task_environment_{
+      base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME};
+
  private:
   DISALLOW_COPY_AND_ASSIGN(AuthenticatorRequestDialogModelTest);
 };
@@ -276,3 +282,46 @@ TEST_F(AuthenticatorRequestDialogModelTest, PostMortems) {
     EXPECT_CALL(mock_observer, OnModelDestroyed());
   }
 }
+
+TEST_F(AuthenticatorRequestDialogModelTest,
+       RequestCallbackOnlyCalledOncePerAuthenticator) {
+  ::device::FidoRequestHandlerBase::TransportAvailabilityInfo transports_info;
+  transports_info.request_type =
+      device::FidoRequestHandlerBase::RequestType::kMakeCredential;
+  transports_info.available_transports = {
+      AuthenticatorTransport::kInternal,
+      AuthenticatorTransport::kUsbHumanInterfaceDevice};
+
+  int num_called = 0;
+  AuthenticatorRequestDialogModel model;
+  model.SetRequestCallback(base::BindRepeating(
+      [](int* i, const std::string& authenticator_id) { ++(*i); },
+      &num_called));
+  model.saved_authenticators().emplace_back(
+      AuthenticatorRequestDialogModel::AuthenticatorReference(
+          "authenticator", AuthenticatorTransport::kInternal));
+
+  model.StartFlow(std::move(transports_info), base::nullopt);
+  EXPECT_EQ(AuthenticatorRequestDialogModel::Step::kTransportSelection,
+            model.current_step());
+  EXPECT_EQ(0, num_called);
+
+  // Simulate switching back and forth between transports. The request callback
+  // should only be invoked once (USB is not dispatched through the UI).
+  model.StartGuidedFlowForTransport(AuthenticatorTransport::kInternal);
+  EXPECT_EQ(AuthenticatorRequestDialogModel::Step::kTouchId,
+            model.current_step());
+  task_environment_.FastForwardUntilNoTasksRemain();
+  EXPECT_EQ(1, num_called);
+  model.StartGuidedFlowForTransport(
+      AuthenticatorTransport::kUsbHumanInterfaceDevice);
+  EXPECT_EQ(AuthenticatorRequestDialogModel::Step::kUsbInsertAndActivate,
+            model.current_step());
+  task_environment_.FastForwardUntilNoTasksRemain();
+  EXPECT_EQ(1, num_called);
+  model.StartGuidedFlowForTransport(AuthenticatorTransport::kInternal);
+  EXPECT_EQ(AuthenticatorRequestDialogModel::Step::kTouchId,
+            model.current_step());
+  task_environment_.FastForwardUntilNoTasksRemain();
+  EXPECT_EQ(1, num_called);
+}