Revert "[webauthn] Don't terminate operations early that return NotAllowedError."

This reverts commit fefcfdfc.

Reason for revert:

WinMSVC64(dbg) bot error:
https://ci.chromium.org/buildbot/chromium.win/WinMSVC64%20(dbg)/2965

c:\b\c\b\win\src\content\browser\webauth\authenticator_impl.cc(393) : error C2220: warning treated as error - no 'object' file generated
c:\b\c\b\win\src\content\browser\webauth\authenticator_impl.cc(393) : warning C4702: unreachable code





Original change's description:
> [webauthn] Don't terminate operations early that return NotAllowedError.
> 
> NotAllowedError responses should not be returned until after the
> timer has expired in order to prevent a leak of possibly-identifying
> user information.
> 
> See https://w3c.github.io/webauthn/#createCredential step 21.
> 
> Bug: 809104
> Change-Id: I3655f4c9cac29dc29c68c234c2e0b8ea00f1c5c2
> Reviewed-on: https://chromium-review.googlesource.com/912149
> Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
> Reviewed-by: Balazs Engedy <engedy@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#537229}

TBR=engedy@chromium.org,kpaulhamus@chromium.org

Change-Id: I022d8586c26a9201c5d80a025b2a954cca3fe5d4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 809104
Reviewed-on: https://chromium-review.googlesource.com/923223Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537232}

content/browser/webauth/authenticator_impl.cc

@@ -180,8 +180,7 @@ void AuthenticatorImpl::MakeCredential(
     webauth::mojom::PublicKeyCredentialCreationOptionsPtr options,
     MakeCredentialCallback callback) {
   if (u2f_request_) {
-    InvokeCallbackAndCleanup(
-        std::move(callback),
+    std::move(callback).Run(
         webauth::mojom::AuthenticatorStatus::PENDING_REQUEST, nullptr);
     return;
   }
@@ -192,26 +191,23 @@ void AuthenticatorImpl::MakeCredential(
   if (!HasValidEffectiveDomain(caller_origin)) {
     bad_message::ReceivedBadMessage(render_frame_host_->GetProcess(),
                                     bad_message::AUTH_INVALID_EFFECTIVE_DOMAIN);
-    InvokeCallbackAndCleanup(
-        std::move(callback),
-        webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN, nullptr);
+    std::move(callback).Run(webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN,
+                            nullptr);
     return;
   }
 
   if (!IsRelyingPartyIdValid(relying_party_id_, caller_origin)) {
     bad_message::ReceivedBadMessage(render_frame_host_->GetProcess(),
                                     bad_message::AUTH_INVALID_RELYING_PARTY);
-    InvokeCallbackAndCleanup(
-        std::move(callback),
-        webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN, nullptr);
+    std::move(callback).Run(webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN,
+                            nullptr);
     return;
   }
 
   // Check that at least one of the cryptographic parameters is supported.
   // Only ES256 is currently supported by U2F_V2.
   if (!HasValidAlgorithm(options->public_key_parameters)) {
-    InvokeCallbackAndCleanup(
-        std::move(callback),
+    std::move(callback).Run(
         webauth::mojom::AuthenticatorStatus::NOT_SUPPORTED_ERROR, nullptr);
     return;
   }
@@ -272,8 +268,7 @@ void AuthenticatorImpl::GetAssertion(
     webauth::mojom::PublicKeyCredentialRequestOptionsPtr options,
     GetAssertionCallback callback) {
   if (u2f_request_) {
-    InvokeCallbackAndCleanup(
-        std::move(callback),
+    std::move(callback).Run(
         webauth::mojom::AuthenticatorStatus::PENDING_REQUEST, nullptr);
     return;
   }
@@ -283,18 +278,16 @@ void AuthenticatorImpl::GetAssertion(
   if (!HasValidEffectiveDomain(caller_origin)) {
     bad_message::ReceivedBadMessage(render_frame_host_->GetProcess(),
                                     bad_message::AUTH_INVALID_EFFECTIVE_DOMAIN);
-    InvokeCallbackAndCleanup(
-        std::move(callback),
-        webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN, nullptr);
+    std::move(callback).Run(webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN,
+                            nullptr);
     return;
   }
 
   if (!IsRelyingPartyIdValid(options->relying_party_id, caller_origin)) {
     bad_message::ReceivedBadMessage(render_frame_host_->GetProcess(),
                                     bad_message::AUTH_INVALID_RELYING_PARTY);
-    InvokeCallbackAndCleanup(
-        std::move(callback),
-        webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN, nullptr);
+    std::move(callback).Run(webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN,
+                            nullptr);
     return;
   }
 
@@ -329,20 +322,19 @@ void AuthenticatorImpl::GetAssertion(
 void AuthenticatorImpl::OnRegisterResponse(
     device::U2fReturnCode status_code,
     base::Optional<device::RegisterResponseData> response_data) {
+  timer_->Stop();
+
   switch (status_code) {
     case device::U2fReturnCode::CONDITIONS_NOT_SATISFIED:
       // Duplicate registration.
-      // NotAllowedError responses should not be returned until after the
-      // timer has expired in order to prevent a leak of possibly-identifying
-      // user information.
-      error_response_ = webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR;
-      return;
+      std::move(make_credential_response_callback_)
+          .Run(webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR, nullptr);
+      break;
     case device::U2fReturnCode::FAILURE:
     case device::U2fReturnCode::INVALID_PARAMS:
-      InvokeCallbackAndCleanup(
-          std::move(make_credential_response_callback_),
-          webauth::mojom::AuthenticatorStatus::UNKNOWN_ERROR, nullptr);
-      return;
+      std::move(make_credential_response_callback_)
+          .Run(webauth::mojom::AuthenticatorStatus::UNKNOWN_ERROR, nullptr);
+      break;
     case device::U2fReturnCode::SUCCESS:
       DCHECK(response_data.has_value());
 
@@ -360,14 +352,12 @@ void AuthenticatorImpl::OnRegisterResponse(
       }
 
       response_data->EraseAttestationStatement();
-      InvokeCallbackAndCleanup(
-          std::move(make_credential_response_callback_),
-          webauth::mojom::AuthenticatorStatus::SUCCESS,
+      std::move(make_credential_response_callback_)
+          .Run(webauth::mojom::AuthenticatorStatus::SUCCESS,
                CreateMakeCredentialResponse(std::move(client_data_),
                                             std::move(*response_data)));
-      return;
   }
-  NOTREACHED();
+  Cleanup();
 }
 
 void AuthenticatorImpl::OnRegisterResponseAttestationDecided(
@@ -377,78 +367,58 @@ void AuthenticatorImpl::OnRegisterResponseAttestationDecided(
          webauth::mojom::AttestationConveyancePreference::NONE);
 
   if (!attestation_permitted) {
-    // NotAllowedError responses should not be returned until after the
-    // timer has expired in order to prevent a leak of possibly-identifying
-    // user information.
-    error_response_ = webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR;
-    return;
+    std::move(make_credential_response_callback_)
+        .Run(webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR, nullptr);
   } else {
-    InvokeCallbackAndCleanup(
-        std::move(make_credential_response_callback_),
-        webauth::mojom::AuthenticatorStatus::SUCCESS,
+    std::move(make_credential_response_callback_)
+        .Run(webauth::mojom::AuthenticatorStatus::SUCCESS,
              CreateMakeCredentialResponse(std::move(client_data_),
                                           std::move(response_data)));
-    return;
   }
-  NOTREACHED();
+
+  Cleanup();
 }
 
 void AuthenticatorImpl::OnSignResponse(
     device::U2fReturnCode status_code,
     base::Optional<device::SignResponseData> response_data) {
+  timer_->Stop();
   switch (status_code) {
     case device::U2fReturnCode::CONDITIONS_NOT_SATISFIED:
       // No authenticators contained the credential.
-      error_response_ = webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR;
-      return;
+      std::move(get_assertion_response_callback_)
+          .Run(webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR, nullptr);
+      break;
     case device::U2fReturnCode::FAILURE:
     case device::U2fReturnCode::INVALID_PARAMS:
-      InvokeCallbackAndCleanup(
-          std::move(get_assertion_response_callback_),
-          webauth::mojom::AuthenticatorStatus::UNKNOWN_ERROR, nullptr);
-      return;
+      std::move(get_assertion_response_callback_)
+          .Run(webauth::mojom::AuthenticatorStatus::UNKNOWN_ERROR, nullptr);
+      break;
     case device::U2fReturnCode::SUCCESS:
       DCHECK(response_data.has_value());
-      InvokeCallbackAndCleanup(
-          std::move(get_assertion_response_callback_),
-          webauth::mojom::AuthenticatorStatus::SUCCESS,
+      std::move(get_assertion_response_callback_)
+          .Run(webauth::mojom::AuthenticatorStatus::SUCCESS,
                CreateGetAssertionResponse(std::move(client_data_),
                                           std::move(*response_data)));
-      return;
+      break;
   }
-  NOTREACHED();
+  Cleanup();
 }
 
 void AuthenticatorImpl::OnTimeout() {
   DCHECK(make_credential_response_callback_ ||
          get_assertion_response_callback_);
   if (make_credential_response_callback_) {
-    InvokeCallbackAndCleanup(std::move(make_credential_response_callback_),
-                             error_response_, nullptr);
+    std::move(make_credential_response_callback_)
+        .Run(webauth::mojom::AuthenticatorStatus::TIMED_OUT, nullptr);
   } else if (get_assertion_response_callback_) {
-    InvokeCallbackAndCleanup(std::move(get_assertion_response_callback_),
-                             error_response_, nullptr);
+    std::move(get_assertion_response_callback_)
+        .Run(webauth::mojom::AuthenticatorStatus::TIMED_OUT, nullptr);
   }
-}
-
-void AuthenticatorImpl::InvokeCallbackAndCleanup(
-    MakeCredentialCallback callback,
-    webauth::mojom::AuthenticatorStatus status,
-    webauth::mojom::MakeCredentialAuthenticatorResponsePtr response) {
-  std::move(callback).Run(status, std::move(response));
-  Cleanup();
-}
-
-void AuthenticatorImpl::InvokeCallbackAndCleanup(
-    GetAssertionCallback callback,
-    webauth::mojom::AuthenticatorStatus status,
-    webauth::mojom::GetAssertionAuthenticatorResponsePtr response) {
-  std::move(callback).Run(status, std::move(response));
   Cleanup();
 }
 
 void AuthenticatorImpl::Cleanup() {
-  timer_->Stop();
   u2f_request_.reset();
   make_credential_response_callback_.Reset();
   get_assertion_response_callback_.Reset();

content/browser/webauth/authenticator_impl.h

@@ -87,15 +87,6 @@ class CONTENT_EXPORT AuthenticatorImpl : public webauth::mojom::Authenticator {
 
   // Runs when timer expires and cancels all issued requests to a U2fDevice.
   void OnTimeout();
-
-  void InvokeCallbackAndCleanup(
-      MakeCredentialCallback callback,
-      webauth::mojom::AuthenticatorStatus status,
-      webauth::mojom::MakeCredentialAuthenticatorResponsePtr response);
-  void InvokeCallbackAndCleanup(
-      GetAssertionCallback callback,
-      webauth::mojom::AuthenticatorStatus status,
-      webauth::mojom::GetAssertionAuthenticatorResponsePtr response);
   void Cleanup();
 
   // Owns pipes to this Authenticator from |render_frame_host_|.
@@ -110,11 +101,6 @@ class CONTENT_EXPORT AuthenticatorImpl : public webauth::mojom::Authenticator {
   MakeCredentialCallback make_credential_response_callback_;
   GetAssertionCallback get_assertion_response_callback_;
 
-  // The response to return at timeout. The default may be overriden by
-  // NOT_ALLOWED_ERROR based on the response from the authenticator.
-  webauth::mojom::AuthenticatorStatus error_response_ =
-      webauth::mojom::AuthenticatorStatus::TIMED_OUT;
-
   // Holds the client data to be returned to the caller.
   CollectedClientData client_data_;
   webauth::mojom::AttestationConveyancePreference attestation_preference_;