Add Cancel logic to WebAuthN UI

Add logic to cancel ongoing WebAuthN request when user clicks "cancel"
button from any UI modal or "back" button from the initial welcome
screen.

Bug: 847985
TBR: jam@chromium.org
Change-Id: Ia9a014cf78583c2512ceaebf822758aa0dc65d99
Reviewed-on: https://chromium-review.googlesource.com/1173489
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582886}

chrome/browser/webauthn/authenticator_request_dialog_model.cc

@@ -73,11 +73,17 @@ void AuthenticatorRequestDialogModel::TryUsbDevice() {
   DCHECK_EQ(current_step(), Step::kUsbInsertAndActivateOnRegister);
 }
 
-void AuthenticatorRequestDialogModel::Cancel() {}
+void AuthenticatorRequestDialogModel::Cancel() {
+  for (auto& observer : observers_)
+    observer.OnCancelRequest();
+}
 
 void AuthenticatorRequestDialogModel::Back() {
-  // For now, return to the initial step all the time.
+  if (current_step() == Step::kInitial) {
+    Cancel();
+  } else {
     SetCurrentStep(Step::kInitial);
+  }
 }
 
 void AuthenticatorRequestDialogModel::AddObserver(Observer* observer) {

chrome/browser/webauthn/authenticator_request_dialog_model.h

@@ -71,6 +71,10 @@ class AuthenticatorRequestDialogModel {
     // Called when the UX flow has navigated to a different step, so the UI
     // should update.
     virtual void OnStepTransition() {}
+
+    // Called when the user cancelled WebAuthN request by clicking the
+    // "cancel" button or the back arrow in the UI dialog.
+    virtual void OnCancelRequest() {}
   };
 
   AuthenticatorRequestDialogModel();

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -123,6 +123,14 @@ ChromeAuthenticatorRequestDelegate::~ChromeAuthenticatorRequestDelegate() {
   }
 }
 
+base::Optional<device::FidoTransportProtocol>
+ChromeAuthenticatorRequestDelegate::GetLastTransportUsed() const {
+  PrefService* prefs =
+      Profile::FromBrowserContext(browser_context())->GetPrefs();
+  return device::ConvertToFidoTransportProtocol(
+      prefs->GetString(kWebAuthnLastTransportUsedPrefName));
+}
+
 base::WeakPtr<ChromeAuthenticatorRequestDelegate>
 ChromeAuthenticatorRequestDelegate::AsWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
@@ -134,7 +142,8 @@ content::BrowserContext* ChromeAuthenticatorRequestDelegate::browser_context()
       ->GetBrowserContext();
 }
 
-void ChromeAuthenticatorRequestDelegate::DidStartRequest() {
+void ChromeAuthenticatorRequestDelegate::DidStartRequest(
+    base::OnceClosure cancel_callback) {
 #if !defined(OS_ANDROID)
   if (!IsWebAuthnUiEnabled())
     return;
@@ -143,6 +152,7 @@ void ChromeAuthenticatorRequestDelegate::DidStartRequest() {
   weak_dialog_model_ = dialog_model.get();
   SetInitialUiModelBasedOnPreviouslyUsedTransport(weak_dialog_model_,
                                                   GetLastTransportUsed());
+  cancel_callback_ = std::move(cancel_callback);
   weak_dialog_model_->AddObserver(this);
 
   ShowAuthenticatorRequestDialog(
@@ -248,12 +258,12 @@ ChromeAuthenticatorRequestDelegate::GetTouchIdAuthenticatorConfig() const {
 }
 #endif
 
-base::Optional<device::FidoTransportProtocol>
-ChromeAuthenticatorRequestDelegate::GetLastTransportUsed() const {
+void ChromeAuthenticatorRequestDelegate::UpdateLastTransportUsed(
+    device::FidoTransportProtocol transport) {
   PrefService* prefs =
       Profile::FromBrowserContext(browser_context())->GetPrefs();
-  return device::ConvertToFidoTransportProtocol(
-      prefs->GetString(kWebAuthnLastTransportUsedPrefName));
+  prefs->SetString(kWebAuthnLastTransportUsedPrefName,
+                   device::ToString(transport));
 }
 
 void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorAdded(
@@ -281,15 +291,14 @@ void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorRemoved(
       saved_authenticators.end());
 }
 
-void ChromeAuthenticatorRequestDelegate::UpdateLastTransportUsed(
-    device::FidoTransportProtocol transport) {
-  PrefService* prefs =
-      Profile::FromBrowserContext(browser_context())->GetPrefs();
-  prefs->SetString(kWebAuthnLastTransportUsedPrefName,
-                   device::ToString(transport));
-}
-
 void ChromeAuthenticatorRequestDelegate::OnModelDestroyed() {
   DCHECK(weak_dialog_model_);
   weak_dialog_model_ = nullptr;
 }
+
+void ChromeAuthenticatorRequestDelegate::OnCancelRequest() {
+  // |cancel_callback_| must be invoked at most once as invocation of
+  // |cancel_callback_| will destroy |this|.
+  DCHECK(cancel_callback_);
+  std::move(cancel_callback_).Run();
+}

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
@@ -62,7 +63,7 @@ class ChromeAuthenticatorRequestDelegate
   content::BrowserContext* browser_context() const;
 
   // content::AuthenticatorRequestClientDelegate:
-  void DidStartRequest() override;
+  void DidStartRequest(base::OnceClosure cancel_callback) override;
   bool ShouldPermitIndividualAttestation(
       const std::string& relying_party_id) override;
   void ShouldReturnAttestation(
@@ -79,9 +80,11 @@ class ChromeAuthenticatorRequestDelegate
 
   // AuthenticatorRequestDialogModel::Observer:
   void OnModelDestroyed() override;
+  void OnCancelRequest() override;
 
   content::RenderFrameHost* const render_frame_host_;
   AuthenticatorRequestDialogModel* weak_dialog_model_ = nullptr;
+  base::OnceClosure cancel_callback_;
 
   base::WeakPtrFactory<ChromeAuthenticatorRequestDelegate> weak_ptr_factory_;
 

content/browser/webauth/authenticator_impl.cc

@@ -473,7 +473,9 @@ void AuthenticatorImpl::MakeCredential(
 
   DCHECK(make_credential_response_callback_.is_null());
   make_credential_response_callback_ = std::move(callback);
-  request_delegate_->DidStartRequest();
+  request_delegate_->DidStartRequest(
+      base::BindOnce(&AuthenticatorImpl::Cancel,
+                     weak_factory_.GetWeakPtr()) /* cancel_callback */);
 
   timer_->Start(
       FROM_HERE, options->adjusted_timeout,
@@ -569,7 +571,9 @@ void AuthenticatorImpl::GetAssertion(
 
   DCHECK(get_assertion_response_callback_.is_null());
   get_assertion_response_callback_ = std::move(callback);
-  request_delegate_->DidStartRequest();
+  request_delegate_->DidStartRequest(
+      base::BindOnce(&AuthenticatorImpl::Cancel,
+                     weak_factory_.GetWeakPtr()) /* cancel_callback */);
 
   timer_->Start(
       FROM_HERE, options->adjusted_timeout,
@@ -805,6 +809,16 @@ void AuthenticatorImpl::OnTimeout() {
   }
 }
 
+void AuthenticatorImpl::Cancel() {
+  // If response callback is invoked already, then ignore cancel request.
+  if (!make_credential_response_callback_ && !get_assertion_response_callback_)
+    return;
+
+  // Response from user cancellation is indistinguishable from error due to
+  // timeout.
+  OnTimeout();
+}
+
 void AuthenticatorImpl::InvokeCallbackAndCleanup(
     MakeCredentialCallback callback,
     blink::mojom::AuthenticatorStatus status,

content/browser/webauth/authenticator_impl.h

@@ -141,6 +141,8 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
 
   // Runs when timer expires and cancels all issued requests to a U2fDevice.
   void OnTimeout();
+  // Runs when the user cancels WebAuthN request via UI dialog.
+  void Cancel();
 
   void InvokeCallbackAndCleanup(
       MakeCredentialCallback callback,

content/browser/webauth/authenticator_impl_unittest.cc

@@ -1141,7 +1141,7 @@ class TestAuthenticatorRequestDelegate
         is_focused_(is_focused) {}
   ~TestAuthenticatorRequestDelegate() override {}
 
-  void DidStartRequest() override {
+  void DidStartRequest(base::OnceClosure cancel_callback) override {
     ASSERT_TRUE(did_start_request_callback_) << "DidStartRequest called twice.";
     std::move(did_start_request_callback_).Run();
   }

content/public/browser/authenticator_request_client_delegate.cc

@@ -16,7 +16,8 @@ AuthenticatorRequestClientDelegate::AuthenticatorRequestClientDelegate() =
 AuthenticatorRequestClientDelegate::~AuthenticatorRequestClientDelegate() =
     default;
 
-void AuthenticatorRequestClientDelegate::DidStartRequest() {}
+void AuthenticatorRequestClientDelegate::DidStartRequest(
+    base::OnceClosure cancel_callback) {}
 
 bool AuthenticatorRequestClientDelegate::ShouldPermitIndividualAttestation(
     const std::string& relying_party_id) {

content/public/browser/authenticator_request_client_delegate.h

@@ -33,7 +33,7 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   ~AuthenticatorRequestClientDelegate() override;
 
   // Notifies the delegate that the request is actually starting.
-  virtual void DidStartRequest();
+  virtual void DidStartRequest(base::OnceClosure cancel_callback);
 
   // Returns true if the given relying party ID is permitted to receive
   // individual attestation certificates. This: