win: don't show an error dialog after cancelling out of Win WebAuthn UI

This fixes a regression that caused cancelling out of the native Windows
WebAuthn UI would show an error dialog, when the request should have
just been ended right away.

The error dialog in question contained a "retry" button, which was non-
functional because it just returned the UI to an empty transport
selection UI. Conceptually, we don't want to offer the "retry" button on
Windows, because the Windows UI brings its own retry behavior for
certain cases (e.g. user touched the wrong security key). Hence, this
change also extends the retry logic on
ChromeAuthenticatorRequestDelegate to hide retry buttons after
dispatching a request to the Windows API.

Bug: 985597
Change-Id: Ie02721068c847156222e81f193fd446929ca996f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1710741
Commit-Queue: Martin Kreichgauer <martinkr@chromium.org>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680277}

chrome/browser/ui/webauthn/sheet_models.cc

@@ -296,7 +296,7 @@ base::string16 AuthenticatorNotRegisteredErrorModel::GetCancelButtonLabel()
 }
 
 bool AuthenticatorNotRegisteredErrorModel::IsAcceptButtonVisible() const {
-  return true;
+  return dialog_model()->request_may_start_over();
 }
 
 bool AuthenticatorNotRegisteredErrorModel::IsAcceptButtonEnabled() const {
@@ -341,7 +341,7 @@ base::string16 AuthenticatorAlreadyRegisteredErrorModel::GetCancelButtonLabel()
 }
 
 bool AuthenticatorAlreadyRegisteredErrorModel::IsAcceptButtonVisible() const {
-  return true;
+  return dialog_model()->request_may_start_over();
 }
 
 bool AuthenticatorAlreadyRegisteredErrorModel::IsAcceptButtonEnabled() const {
@@ -376,9 +376,14 @@ void AuthenticatorAlreadyRegisteredErrorModel::OnAccept() {
 
 // AuthenticatorInternalUnrecognizedErrorSheetModel
 // -----------------------------------
+bool AuthenticatorInternalUnrecognizedErrorSheetModel::IsBackButtonVisible()
+    const {
+  return dialog_model()->request_may_start_over();
+}
+
 bool AuthenticatorInternalUnrecognizedErrorSheetModel::IsAcceptButtonVisible()
     const {
-  return true;
+  return dialog_model()->request_may_start_over();
 }
 
 bool AuthenticatorInternalUnrecognizedErrorSheetModel::IsAcceptButtonEnabled()

chrome/browser/ui/webauthn/sheet_models.h

@@ -191,6 +191,7 @@ class AuthenticatorInternalUnrecognizedErrorSheetModel
 
  private:
   // AuthenticatorSheetModelBase:
+  bool IsBackButtonVisible() const override;
   bool IsAcceptButtonVisible() const override;
   bool IsAcceptButtonEnabled() const override;
   base::string16 GetAcceptButtonLabel() const override;

chrome/browser/webauthn/authenticator_request_dialog_model.cc

@@ -131,6 +131,10 @@ void AuthenticatorRequestDialogModel::StartFlow(
 }
 
 void AuthenticatorRequestDialogModel::StartOver() {
+  if (!request_may_start_over_) {
+    NOTREACHED();
+    return;
+  }
   ephemeral_state_.Reset();
 
   for (auto& observer : observers_)
@@ -215,8 +219,13 @@ void AuthenticatorRequestDialogModel::
     return;
   }
 
-  // There is no AuthenticatorReference for the Windows authenticator,
-  // hence directly call DispatchRequestAsyncInternal here.
+  // The StartOver() logic does not work in combination with the Windows API.
+  // Therefore do not show a retry button on any error sheet shown after the
+  // Windows API call returns.
+  request_may_start_over_ = false;
+
+  // There is no AuthenticatorReference for the Windows authenticator, hence
+  // directly call DispatchRequestAsyncInternal here.
   DispatchRequestAsyncInternal(
       transport_availability()->win_native_api_authenticator_id);
 

chrome/browser/webauthn/authenticator_request_dialog_model.h

@@ -421,6 +421,8 @@ class AuthenticatorRequestDialogModel {
 
   const std::string& relying_party_id() const { return relying_party_id_; }
 
+  bool request_may_start_over() const { return request_may_start_over_; }
+
  private:
   // Contains the state that will be reset when calling StartOver(). StartOver()
   // might be called at an arbitrary point of execution.
@@ -494,6 +496,11 @@ class AuthenticatorRequestDialogModel {
 
   bool incognito_mode_ = false;
 
+  // request_may_start_over_ indicates whether a button to retry the request
+  // should be included on the dialog sheet shown when encountering certain
+  // errors.
+  bool request_may_start_over_ = true;
+
   base::WeakPtrFactory<AuthenticatorRequestDialogModel> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(AuthenticatorRequestDialogModel);

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -125,12 +125,24 @@ content::BrowserContext* ChromeAuthenticatorRequestDelegate::browser_context()
 }
 
 bool ChromeAuthenticatorRequestDelegate::DoesBlockRequestOnFailure(
+    const ::device::FidoAuthenticator* authenticator,
     InterestingFailureReason reason) {
   if (!IsWebAuthnUIEnabled())
     return false;
   if (!weak_dialog_model_)
     return false;
 
+  DCHECK(authenticator || reason == InterestingFailureReason::kTimeout);
+
+#if defined(OS_WIN)
+  if (authenticator && authenticator->IsWinNativeApiAuthenticator()) {
+    // Do not display a Chrome error dialog if the user cancels out of the
+    // Windows UI. No other errors are reachable.
+    DCHECK(reason == InterestingFailureReason::kUserConsentDenied);
+    return false;
+  }
+#endif  // defined(OS_WIN)
+
   switch (reason) {
     case InterestingFailureReason::kTimeout:
       weak_dialog_model_->OnRequestTimeout();
@@ -230,7 +242,6 @@ void ChromeAuthenticatorRequestDelegate::ShouldReturnAttestation(
     std::move(callback).Run(true);
     return;
   }
-
 #endif  // defined(OS_WIN)
 
   weak_dialog_model_->RequestAttestationPermission(std::move(callback));

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -61,7 +61,9 @@ class ChromeAuthenticatorRequestDelegate
   AuthenticatorRequestDialogModel* WeakDialogModelForTesting() const;
 
   // content::AuthenticatorRequestClientDelegate:
-  bool DoesBlockRequestOnFailure(InterestingFailureReason reason) override;
+  bool DoesBlockRequestOnFailure(
+      const ::device::FidoAuthenticator* authenticator,
+      InterestingFailureReason reason) override;
   void RegisterActionCallbacks(
       base::OnceClosure cancel_callback,
       base::Closure start_over_callback,

chrome/browser/webauthn/chrome_authenticator_request_delegate_unittest.cc

@@ -10,6 +10,7 @@
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/test/web_contents_tester.h"
+#include "device/fido/fido_device_authenticator.h"
 #include "device/fido/test_callback_receiver.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -116,14 +117,14 @@ TEST_F(ChromeAuthenticatorRequestDelegateTest,
 #endif
 
 #if defined(OS_WIN)
+// Tests that ShouldReturnAttestation() returns with true if |authenticator|
+// is the Windows native WebAuthn API with WEBAUTHN_API_VERSION_2 or higher,
+// where Windows prompts for attestation in its own native UI.
+//
+// Ideally, this would also test the inverse case, i.e. that with
+// WEBAUTHN_API_VERSION_1 Chrome's own attestation prompt is shown. However,
+// there seems to be no good way to test AuthenticatorRequestDialogModel UI.
 TEST_F(ChromeAuthenticatorRequestDelegateTest, ShouldPromptForAttestationWin) {
-  // Test that ShouldReturnAttestation() returns with true if |authenticator|
-  // is the Windows native WebAuthn API with WEBAUTHN_API_VERSION_2 or higher,
-  // where Windows prompts for attestation in its own native UI.
-  //
-  // Ideally, this would also test the inverse case, i.e. that with
-  // WEBAUTHN_API_VERSION_1 Chrome's own attestation prompt is shown. However,
-  // there seems to be no good way to test AuthenticatorRequestDialogModel UI.
   ::device::ScopedFakeWinWebAuthnApi win_webauthn_api;
   win_webauthn_api.set_version(WEBAUTHN_API_VERSION_2);
   ::device::WinWebAuthnApiAuthenticator authenticator(
@@ -136,4 +137,29 @@ TEST_F(ChromeAuthenticatorRequestDelegateTest, ShouldPromptForAttestationWin) {
   cb.WaitForCallback();
   EXPECT_EQ(cb.value(), true);
 }
+
+// Ensures that DoesBlockOnRequestFailure() returns false if |authenticator|
+// is the Windows native WebAuthn API because Chrome's request dialog UI
+// should not show an error sheet after the user cancels out of the native
+// Windows UI.
+TEST_F(ChromeAuthenticatorRequestDelegateTest, DoesBlockRequestOnFailure) {
+  ::device::ScopedFakeWinWebAuthnApi win_webauthn_api;
+  ::device::WinWebAuthnApiAuthenticator win_authenticator(
+      /*current_window=*/nullptr);
+  ::device::FidoDeviceAuthenticator device_authenticator(/*device=*/nullptr);
+
+  for (const bool use_win_api : {false, true}) {
+    SCOPED_TRACE(::testing::Message() << "use_win_api=" << use_win_api);
+
+    ChromeAuthenticatorRequestDelegate delegate(main_rfh(), kRelyingPartyID);
+    EXPECT_EQ(delegate.DoesBlockRequestOnFailure(
+                  use_win_api ? static_cast<::device::FidoAuthenticator*>(
+                                    &win_authenticator)
+                              : static_cast<::device::FidoAuthenticator*>(
+                                    &device_authenticator),
+                  ChromeAuthenticatorRequestDelegate::InterestingFailureReason::
+                      kUserConsentDenied),
+              !use_win_api);
+  }
+}
 #endif  // defined(OS_WIN)

content/browser/webauth/authenticator_common.cc

@@ -1040,8 +1040,8 @@ void AuthenticatorCommon::OnRegisterResponse(
       // authenticator that already contains one of the credentials in
       // |exclude_credentials|.
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kKeyAlreadyRegistered);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kKeyAlreadyRegistered);
       return;
     case device::FidoReturnCode::kAuthenticatorResponseInvalid:
       // The response from the authenticator was corrupted.
@@ -1056,31 +1056,34 @@ void AuthenticatorCommon::OnRegisterResponse(
       return;
     case device::FidoReturnCode::kUserConsentDenied:
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kUserConsentDenied);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kUserConsentDenied);
       return;
     case device::FidoReturnCode::kSoftPINBlock:
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kSoftPINBlock);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kSoftPINBlock);
       return;
     case device::FidoReturnCode::kHardPINBlock:
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kHardPINBlock);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kHardPINBlock);
       return;
     case device::FidoReturnCode::kAuthenticatorRemovedDuringPINEntry:
       SignalFailureToRequestDelegate(
+          authenticator,
           AuthenticatorRequestClientDelegate::InterestingFailureReason::
               kAuthenticatorRemovedDuringPINEntry);
       return;
     case device::FidoReturnCode::kAuthenticatorMissingResidentKeys:
       SignalFailureToRequestDelegate(
+          authenticator,
           AuthenticatorRequestClientDelegate::InterestingFailureReason::
               kAuthenticatorMissingResidentKeys);
       return;
     case device::FidoReturnCode::kAuthenticatorMissingUserVerification:
       SignalFailureToRequestDelegate(
+          authenticator,
           AuthenticatorRequestClientDelegate::InterestingFailureReason::
               kAuthenticatorMissingUserVerification);
       return;
@@ -1101,8 +1104,8 @@ void AuthenticatorCommon::OnRegisterResponse(
       return;
     case device::FidoReturnCode::kStorageFull:
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kStorageFull);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kStorageFull);
       return;
     case device::FidoReturnCode::kSuccess:
       DCHECK(response_data.has_value());
@@ -1249,8 +1252,8 @@ void AuthenticatorCommon::OnSignResponse(
   switch (status_code) {
     case device::FidoReturnCode::kUserConsentButCredentialNotRecognized:
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kKeyNotRegistered);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kKeyNotRegistered);
       return;
     case device::FidoReturnCode::kAuthenticatorResponseInvalid:
       // The response from the authenticator was corrupted.
@@ -1264,31 +1267,34 @@ void AuthenticatorCommon::OnSignResponse(
       return;
     case device::FidoReturnCode::kUserConsentDenied:
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kUserConsentDenied);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kUserConsentDenied);
       return;
     case device::FidoReturnCode::kSoftPINBlock:
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kSoftPINBlock);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kSoftPINBlock);
       return;
     case device::FidoReturnCode::kHardPINBlock:
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kHardPINBlock);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kHardPINBlock);
       return;
     case device::FidoReturnCode::kAuthenticatorRemovedDuringPINEntry:
       SignalFailureToRequestDelegate(
+          authenticator,
           AuthenticatorRequestClientDelegate::InterestingFailureReason::
               kAuthenticatorRemovedDuringPINEntry);
       return;
     case device::FidoReturnCode::kAuthenticatorMissingResidentKeys:
       SignalFailureToRequestDelegate(
+          authenticator,
           AuthenticatorRequestClientDelegate::InterestingFailureReason::
               kAuthenticatorMissingResidentKeys);
       return;
     case device::FidoReturnCode::kAuthenticatorMissingUserVerification:
       SignalFailureToRequestDelegate(
+          authenticator,
           AuthenticatorRequestClientDelegate::InterestingFailureReason::
               kAuthenticatorMissingUserVerification);
       return;
@@ -1308,8 +1314,8 @@ void AuthenticatorCommon::OnSignResponse(
     case device::FidoReturnCode::kStorageFull:
       NOTREACHED() << "Should not be possible for assertions.";
       SignalFailureToRequestDelegate(
-          AuthenticatorRequestClientDelegate::InterestingFailureReason::
-              kStorageFull);
+          authenticator, AuthenticatorRequestClientDelegate::
+                             InterestingFailureReason::kStorageFull);
       return;
     case device::FidoReturnCode::kSuccess:
       DCHECK(response_data.has_value());
@@ -1355,6 +1361,7 @@ void AuthenticatorCommon::OnAccountSelected(
 }
 
 void AuthenticatorCommon::SignalFailureToRequestDelegate(
+    const ::device::FidoAuthenticator* authenticator,
     AuthenticatorRequestClientDelegate::InterestingFailureReason reason) {
   blink::mojom::AuthenticatorStatus status =
       blink::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR;
@@ -1405,7 +1412,7 @@ void AuthenticatorCommon::SignalFailureToRequestDelegate(
 
   // If WebAuthnUi is enabled, this error blocks until after receiving user
   // acknowledgement. Otherwise, the error is returned right away.
-  if (request_delegate_->DoesBlockRequestOnFailure(reason)) {
+  if (request_delegate_->DoesBlockRequestOnFailure(authenticator, reason)) {
     // Cancel pending authenticator requests before the error dialog is shown.
     request_->CancelActiveAuthenticators();
     return;
@@ -1424,6 +1431,7 @@ void AuthenticatorCommon::OnTimeout() {
   }
 
   SignalFailureToRequestDelegate(
+      /*authenticator=*/nullptr,
       AuthenticatorRequestClientDelegate::InterestingFailureReason::kTimeout);
 }
 

content/browser/webauth/authenticator_common.h

@@ -176,6 +176,7 @@ class CONTENT_EXPORT AuthenticatorCommon {
   // acknowledgement before returning the error, and handles the error
   // appropriately.
   void SignalFailureToRequestDelegate(
+      const ::device::FidoAuthenticator* authenticator,
       AuthenticatorRequestClientDelegate::InterestingFailureReason reason);
 
   void InvokeCallbackAndCleanup(

content/browser/webauth/authenticator_impl_unittest.cc

@@ -2390,7 +2390,9 @@ class MockAuthenticatorRequestDelegateObserver
         failure_reasons_callback_(std::move(failure_reasons_callback)) {}
   ~MockAuthenticatorRequestDelegateObserver() override = default;
 
-  bool DoesBlockRequestOnFailure(InterestingFailureReason reason) override {
+  bool DoesBlockRequestOnFailure(
+      const ::device::FidoAuthenticator* authenticator,
+      InterestingFailureReason reason) override {
     CHECK(failure_reasons_callback_);
     std::move(failure_reasons_callback_).Run(reason);
     return false;
@@ -2897,10 +2899,12 @@ class PINTestAuthenticatorRequestDelegate
 
   void FinishCollectPIN() override {}
 
-  bool DoesBlockRequestOnFailure(InterestingFailureReason reason) override {
+  bool DoesBlockRequestOnFailure(
+      const ::device::FidoAuthenticator* authenticator,
+      InterestingFailureReason reason) override {
     *failure_reason_ = reason;
     return AuthenticatorRequestClientDelegate::DoesBlockRequestOnFailure(
-        reason);
+        authenticator, reason);
   }
 
  private:
@@ -3515,10 +3519,12 @@ class ResidentKeyTestAuthenticatorRequestDelegate
     *might_create_resident_credential_ = v;
   }
 
-  bool DoesBlockRequestOnFailure(InterestingFailureReason reason) override {
+  bool DoesBlockRequestOnFailure(
+      const ::device::FidoAuthenticator* authenticator,
+      InterestingFailureReason reason) override {
     *failure_reason_ = reason;
     return AuthenticatorRequestClientDelegate::DoesBlockRequestOnFailure(
-        reason);
+        authenticator, reason);
   }
 
  private:

content/public/browser/authenticator_request_client_delegate.cc

@@ -17,6 +17,7 @@ AuthenticatorRequestClientDelegate::~AuthenticatorRequestClientDelegate() =
     default;
 
 bool AuthenticatorRequestClientDelegate::DoesBlockRequestOnFailure(
+    const ::device::FidoAuthenticator* authenticator,
     InterestingFailureReason reason) {
   return false;
 }

content/public/browser/authenticator_request_client_delegate.h

@@ -54,14 +54,19 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   AuthenticatorRequestClientDelegate();
   ~AuthenticatorRequestClientDelegate() override;
 
-  // Called when the request fails for the given |reason|. Embedders may return
-  // true if they want AuthenticatorImpl to hold off from resolving the WebAuthn
-  // request with an error, e.g. because they want the user to dismiss an error
-  // dialog first. In this case, embedders *must* eventually invoke the
-  // FidoRequestHandlerBase::CancelCallback in order to resolve the request.
-  // Returning false causes AuthenticatorImpl to resolve the request with the
-  // error right away.
-  virtual bool DoesBlockRequestOnFailure(InterestingFailureReason reason);
+  // Called when the request fails for the given |reason|.  |authenticator|
+  // points to the FidoAuthenticator used in the request that resulted in the
+  // error. It may be nullptr if |reason| is kTimeout.
+  //
+  // Embedders may return true if they want AuthenticatorImpl to hold off from
+  // resolving the WebAuthn request with an error, e.g. because they want the
+  // user to dismiss an error dialog first. In this case, embedders *must*
+  // eventually invoke the FidoRequestHandlerBase::CancelCallback in order to
+  // resolve the request. Returning false causes AuthenticatorImpl to resolve
+  // the request with the error right away.
+  virtual bool DoesBlockRequestOnFailure(
+      const ::device::FidoAuthenticator* authenticator,
+      InterestingFailureReason reason);
 
   // Supplies callbacks that the embedder can invoke to initiate certain
   // actions, namely: cancel the request, start the request over, initiate BLE