fido: make CTAP2_ERR_OPERATION_DENIED cancel the request

This is a follow-up to crrev.com/c/1181863, which added OPERATION_DENIED
handling for Touch ID. In this change that behavior is extended to all
transports.

Bug: 875982
Change-Id: If10000f8333dccd4c2e83becf51e36baedbc2655
Reviewed-on: https://chromium-review.googlesource.com/1187683
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585823}

device/fido/fido_request_handler.h

@@ -58,9 +58,9 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
       return;
     }
 
-    const auto return_code = ConvertDeviceResponseCodeToFidoReturnCode(
-        device_response_code, response_data.has_value(),
-        authenticator->AuthenticatorTransport());
+    base::Optional<FidoReturnCode> return_code =
+        ConvertDeviceResponseCodeToFidoReturnCode(device_response_code,
+                                                  response_data.has_value());
 
     // Any authenticator response codes that do not result from user consent
     // imply that the authenticator should be dropped and that other on-going
@@ -82,8 +82,7 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
   static base::Optional<FidoReturnCode>
   ConvertDeviceResponseCodeToFidoReturnCode(
       CtapDeviceResponseCode device_response_code,
-      bool response_has_value,
-      FidoTransportProtocol transport) {
+      bool response_has_value) {
     switch (device_response_code) {
       case CtapDeviceResponseCode::kSuccess:
         return response_has_value
@@ -97,21 +96,11 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
       case CtapDeviceResponseCode::kCtap2ErrNoCredentials:
         return FidoReturnCode::kUserConsentButCredentialNotRecognized;
 
-      // The user explicitly denied the operation.
+      // The user explicitly denied the operation. Touch ID returns this error
+      // when the user cancels the macOS prompt. External authenticators may
+      // return it e.g. after the user fails fingerprint verification.
       case CtapDeviceResponseCode::kCtap2ErrOperationDenied:
-        // TODO(crbug/875982): Clarify if |CTAP2_ERR_OPERATION_DENIED| is "a
-        // status indicating that the user cancelled the operation" in the
-        // spirit of https://www.w3.org/TR/webauthn/, sections 5.1.3 and
-        // 5.1.4.. The CTAP2 spec also uses it for authenticator-chosen
-        // timeouts, so it is a little unclear.
-        //
-        // For Touch ID, we know it means that the operation failed during (or
-        // after) user verification, so we do the translation the internal
-        // transport only.
-        return transport == FidoTransportProtocol::kInternal
-                   ? base::make_optional<FidoReturnCode>(
-                         FidoReturnCode::kUserConsentDenied)
-                   : base::nullopt;
+        return FidoReturnCode::kUserConsentDenied;
 
       // This error is returned by some authenticators (e.g. the "Yubico FIDO
       // 2" CTAP2 USB keys) during GetAssertion **before the user interacted

device/fido/fido_request_handler_unittest.cc

@@ -431,10 +431,8 @@ TEST_F(FidoRequestHandlerTest,
   EXPECT_EQ(FidoReturnCode::kUserConsentDenied, callback().status());
 }
 
-// Like |TestRequestWithOperationDeniedErrorInternalTransport|, but the
-// CTAP2_ERR_OPERATION_DENIED error is returned by a device with
-// cross-platform transport. The operation should not be cancelled (see
-// https://crbug/875982).
+// Like |TestRequestWithOperationDeniedErrorInternalTransport|, but with a
+// cross-platform authenticator.
 TEST_F(FidoRequestHandlerTest,
        TestRequestWithOperationDeniedErrorCrossPlatform) {
   auto request_handler = CreateFakeHandler();
@@ -446,21 +444,17 @@ TEST_F(FidoRequestHandlerTest,
   device0->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeOperationDeniedError());
 
-  // Pending device will *NOT* be cancelled and can send a success reply
-  // eventually.
   auto device1 = MockFidoDevice::MakeCtapWithGetInfoExpectation();
-  device1->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
-                                       CreateFakeSuccessDeviceResponse(),
-                                       base::TimeDelta::FromMicroseconds(10));
+  device1->ExpectRequestAndDoNotRespond(std::vector<uint8_t>());
+  EXPECT_CALL(*device1, Cancel());
 
   discovery()->AddDevice(std::move(device0));
   discovery()->AddDevice(std::move(device1));
 
-  // The request will stay pending, the reply has not triggered the callback.
   scoped_task_environment_.FastForwardUntilNoTasksRemain();
   callback().WaitForCallback();
   EXPECT_TRUE(request_handler->is_complete());
-  EXPECT_EQ(FidoReturnCode::kSuccess, callback().status());
+  EXPECT_EQ(FidoReturnCode::kUserConsentDenied, callback().status());
 }
 
 // Requests should be dispatched to the authenticator passed to

device/fido/get_assertion_handler_unittest.cc

@@ -620,8 +620,7 @@ TEST_F(FidoGetAssertionHandlerTest,
 }
 
 // Like |TestRequestWithOperationDeniedErrorPlatform|, but with a
-// cross-platform device. The request should not complete after the
-// CTAP2_ERR_OPERATION_DENIED error (see https://crbug/875982).
+// cross-platform device.
 TEST_F(FidoGetAssertionHandlerTest,
        TestRequestWithOperationDeniedErrorCrossPlatform) {
   auto device = MockFidoDevice::MakeCtapWithGetInfoExpectation();
@@ -634,7 +633,9 @@ TEST_F(FidoGetAssertionHandlerTest,
   discovery()->AddDevice(std::move(device));
 
   scoped_task_environment_.FastForwardUntilNoTasksRemain();
-  EXPECT_FALSE(get_assertion_callback().was_called());
+  EXPECT_TRUE(get_assertion_callback().was_called());
+  EXPECT_EQ(FidoReturnCode::kUserConsentDenied,
+            get_assertion_callback().status());
 }
 
 }  // namespace device

device/fido/make_credential_handler_unittest.cc

@@ -562,8 +562,7 @@ TEST_F(FidoMakeCredentialHandlerTest,
 }
 
 // Like |TestRequestWithOperationDeniedErrorPlatform|, but with a
-// cross-platform device. The request should not complete after the
-// CTAP2_ERR_OPERATION_DENIED error (see https://crbug/875982).
+// cross-platform device.
 TEST_F(FidoMakeCredentialHandlerTest,
        TestRequestWithOperationDeniedErrorCrossPlatform) {
   auto device = MockFidoDevice::MakeCtapWithGetInfoExpectation();
@@ -582,7 +581,8 @@ TEST_F(FidoMakeCredentialHandlerTest,
   discovery()->AddDevice(std::move(device));
 
   scoped_task_environment_.FastForwardUntilNoTasksRemain();
-  EXPECT_FALSE(callback().was_called());
+  EXPECT_TRUE(callback().was_called());
+  EXPECT_EQ(FidoReturnCode::kUserConsentDenied, callback().status());
 }
 
 }  // namespace device