Plumb `interesting` failure reasons to embedder.

Add AuthenticatorRequestClientDelegate::DidFailWithInterestingReason
that informs the embedder when the request being serviced by
AuthenticatorImpl fails because of either:
 -- timeout,
 -- the key already being registered for MakeCredential,
 -- the key not being registered for GetAssertion.

Bug: 866601
Change-Id: Id018033c0b306a14ebf33d307ab8c69f7d8ad96d
Reviewed-on: https://chromium-review.googlesource.com/1195490Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587306}

content/browser/webauth/authenticator_impl.cc

@@ -655,6 +655,10 @@ void AuthenticatorImpl::OnRegisterResponse(
       // Duplicate registration: the new credential would be created on an
       // authenticator that already contains one of the credentials in
       // |exclude_credentials|.
+      DCHECK(request_delegate_);
+      request_delegate_->DidFailWithInterestingReason(
+          AuthenticatorRequestClientDelegate::InterestingFailureReason::
+              kKeyAlreadyRegistered);
       InvokeCallbackAndCleanup(
           std::move(make_credential_response_callback_),
           blink::mojom::AuthenticatorStatus::CREDENTIAL_EXCLUDED, nullptr,
@@ -768,6 +772,10 @@ void AuthenticatorImpl::OnSignResponse(
   switch (status_code) {
     case device::FidoReturnCode::kUserConsentButCredentialNotRecognized:
       // No authenticators contained the credential.
+      DCHECK(request_delegate_);
+      request_delegate_->DidFailWithInterestingReason(
+          AuthenticatorRequestClientDelegate::InterestingFailureReason::
+              kKeyNotRegistered);
       InvokeCallbackAndCleanup(
           std::move(get_assertion_response_callback_),
           blink::mojom::AuthenticatorStatus::CREDENTIAL_NOT_RECOGNIZED,
@@ -808,9 +816,7 @@ void AuthenticatorImpl::OnSignResponse(
   NOTREACHED();
 }
 
-void AuthenticatorImpl::OnTimeout() {
-  // TODO(crbug.com/814418): Add layout tests to verify timeouts are
-  // indistinguishable from NOT_ALLOWED_ERROR cases.
+void AuthenticatorImpl::FailWithNotAllowedErrorAndCleanup() {
   DCHECK(make_credential_response_callback_ ||
          get_assertion_response_callback_);
   if (make_credential_response_callback_) {
@@ -825,14 +831,22 @@ void AuthenticatorImpl::OnTimeout() {
   }
 }
 
+void AuthenticatorImpl::OnTimeout() {
+  DCHECK(request_delegate_);
+  request_delegate_->DidFailWithInterestingReason(
+      AuthenticatorRequestClientDelegate::InterestingFailureReason::kTimeout);
+
+  // TODO(crbug.com/814418): Add layout tests to verify timeouts are
+  // indistinguishable from NOT_ALLOWED_ERROR cases.
+  FailWithNotAllowedErrorAndCleanup();
+}
+
 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();
+  FailWithNotAllowedErrorAndCleanup();
 }
 
 void AuthenticatorImpl::InvokeCallbackAndCleanup(

content/browser/webauth/authenticator_impl.h

@@ -140,6 +140,8 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
       base::Optional<device::AuthenticatorGetAssertionResponse> response_data,
       device::FidoTransportProtocol transport_used);
 
+  void FailWithNotAllowedErrorAndCleanup();
+
   // Runs when timer expires and cancels all issued requests to a U2fDevice.
   void OnTimeout();
   // Runs when the user cancels WebAuthN request via UI dialog.

content/browser/webauth/authenticator_impl_unittest.cc

@@ -73,6 +73,11 @@ using cbor::CBORReader;
 
 namespace {
 
+using InterestingFailureReason =
+    ::content::AuthenticatorRequestClientDelegate::InterestingFailureReason;
+using FailureReasonCallbackReceiver =
+    ::device::test::TestCallbackReceiver<InterestingFailureReason>;
+
 typedef struct {
   const char* origin;
   // Either a relying party ID or a U2F AppID.
@@ -1814,15 +1819,26 @@ TEST_F(AuthenticatorContentBrowserClientTest, IsUVPAAFalse) {
 class MockAuthenticatorRequestDelegateObserver
     : public TestAuthenticatorRequestDelegate {
  public:
-  MockAuthenticatorRequestDelegateObserver()
+  using InterestingFailureReasonCallback =
+      base::OnceCallback<void(InterestingFailureReason)>;
+
+  MockAuthenticatorRequestDelegateObserver(
+      InterestingFailureReasonCallback failure_reasons_callback =
+          base::DoNothing())
       : TestAuthenticatorRequestDelegate(
             nullptr /* render_frame_host */,
             base::DoNothing() /* did_start_request_callback */,
             IndividualAttestation::NOT_REQUESTED,
             AttestationConsent::DENIED,
-            true /* is_focused */) {}
+            true /* is_focused */),
+        failure_reasons_callback_(std::move(failure_reasons_callback)) {}
   ~MockAuthenticatorRequestDelegateObserver() override = default;
 
+  void DidFailWithInterestingReason(InterestingFailureReason reason) override {
+    ASSERT_TRUE(failure_reasons_callback_);
+    std::move(failure_reasons_callback_).Run(reason);
+  }
+
   MOCK_METHOD1(
       OnTransportAvailabilityEnumerated,
       void(device::FidoRequestHandlerBase::TransportAvailabilityInfo data));
@@ -1832,6 +1848,8 @@ class MockAuthenticatorRequestDelegateObserver
   MOCK_METHOD1(FidoAuthenticatorRemoved, void(base::StringPiece));
 
  private:
+  InterestingFailureReasonCallback failure_reasons_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(MockAuthenticatorRequestDelegateObserver);
 };
 
@@ -1959,4 +1977,94 @@ TEST_F(AuthenticatorImplRequestDelegateTest,
   ble_device_lost_done.Run();
 }
 
+TEST_F(AuthenticatorImplRequestDelegateTest, FailureReasonForTimeout) {
+  SimulateNavigation(GURL(kTestOrigin1));
+
+  FailureReasonCallbackReceiver failure_reason_receiver;
+  auto mock_delegate = std::make_unique<
+      ::testing::NiceMock<MockAuthenticatorRequestDelegateObserver>>(
+      failure_reason_receiver.callback());
+  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
+      base::Time::Now(), base::TimeTicks::Now());
+  auto authenticator = ConstructFakeAuthenticatorWithTimer(
+      std::move(mock_delegate), task_runner);
+
+  TestGetAssertionCallback callback_receiver;
+  authenticator->GetAssertion(GetTestPublicKeyCredentialRequestOptions(),
+                              callback_receiver.callback());
+
+  base::RunLoop().RunUntilIdle();
+  task_runner->FastForwardBy(base::TimeDelta::FromMinutes(1));
+
+  callback_receiver.WaitForCallback();
+  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, callback_receiver.status());
+
+  ASSERT_TRUE(failure_reason_receiver.was_called());
+  EXPECT_EQ(content::AuthenticatorRequestClientDelegate::
+                InterestingFailureReason::kTimeout,
+            std::get<0>(*failure_reason_receiver.result()));
+}
+
+TEST_F(AuthenticatorImplRequestDelegateTest,
+       FailureReasonForDuplicateRegistration) {
+  device::test::ScopedVirtualFidoDevice scoped_virtual_device;
+  SimulateNavigation(GURL(kTestOrigin1));
+
+  FailureReasonCallbackReceiver failure_reason_receiver;
+  auto mock_delegate = std::make_unique<
+      ::testing::NiceMock<MockAuthenticatorRequestDelegateObserver>>(
+      failure_reason_receiver.callback());
+  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
+      base::Time::Now(), base::TimeTicks::Now());
+  auto authenticator = ConstructFakeAuthenticatorWithTimer(
+      std::move(mock_delegate), task_runner);
+
+  PublicKeyCredentialCreationOptionsPtr options =
+      GetTestPublicKeyCredentialCreationOptions();
+  options->exclude_credentials = GetTestAllowCredentials();
+  ASSERT_TRUE(scoped_virtual_device.mutable_state()->InjectRegistration(
+      options->exclude_credentials[0]->id, kTestRelyingPartyId));
+
+  TestMakeCredentialCallback callback_receiver;
+  authenticator->MakeCredential(std::move(options),
+                                callback_receiver.callback());
+
+  callback_receiver.WaitForCallback();
+  EXPECT_EQ(AuthenticatorStatus::CREDENTIAL_EXCLUDED,
+            callback_receiver.status());
+
+  ASSERT_TRUE(failure_reason_receiver.was_called());
+  EXPECT_EQ(content::AuthenticatorRequestClientDelegate::
+                InterestingFailureReason::kKeyAlreadyRegistered,
+            std::get<0>(*failure_reason_receiver.result()));
+}
+
+TEST_F(AuthenticatorImplRequestDelegateTest,
+       FailureReasonForMissingRegistration) {
+  device::test::ScopedVirtualFidoDevice scoped_virtual_device;
+  SimulateNavigation(GURL(kTestOrigin1));
+
+  FailureReasonCallbackReceiver failure_reason_receiver;
+  auto mock_delegate = std::make_unique<
+      ::testing::NiceMock<MockAuthenticatorRequestDelegateObserver>>(
+      failure_reason_receiver.callback());
+  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
+      base::Time::Now(), base::TimeTicks::Now());
+  auto authenticator = ConstructFakeAuthenticatorWithTimer(
+      std::move(mock_delegate), task_runner);
+
+  TestGetAssertionCallback callback_receiver;
+  authenticator->GetAssertion(GetTestPublicKeyCredentialRequestOptions(),
+                              callback_receiver.callback());
+
+  callback_receiver.WaitForCallback();
+  EXPECT_EQ(AuthenticatorStatus::CREDENTIAL_NOT_RECOGNIZED,
+            callback_receiver.status());
+
+  ASSERT_TRUE(failure_reason_receiver.was_called());
+  EXPECT_EQ(content::AuthenticatorRequestClientDelegate::
+                InterestingFailureReason::kKeyNotRegistered,
+            std::get<0>(*failure_reason_receiver.result()));
+}
+
 }  // namespace content

content/public/browser/authenticator_request_client_delegate.cc

@@ -16,6 +16,9 @@ AuthenticatorRequestClientDelegate::AuthenticatorRequestClientDelegate() =
 AuthenticatorRequestClientDelegate::~AuthenticatorRequestClientDelegate() =
     default;
 
+void AuthenticatorRequestClientDelegate::DidFailWithInterestingReason(
+    InterestingFailureReason reason) {}
+
 void AuthenticatorRequestClientDelegate::RegisterActionCallbacks(
     base::OnceClosure cancel_callback,
     device::FidoRequestHandlerBase::RequestCallback request_callback) {}

content/public/browser/authenticator_request_client_delegate.h

@@ -29,9 +29,21 @@ namespace content {
 class CONTENT_EXPORT AuthenticatorRequestClientDelegate
     : public device::FidoRequestHandlerBase::TransportAvailabilityObserver {
  public:
+  // Failure reasons that might be of interest to the user, so the embedder may
+  // decide to inform the user.
+  enum class InterestingFailureReason {
+    kTimeout,
+    kKeyNotRegistered,
+    kKeyAlreadyRegistered,
+  };
+
   AuthenticatorRequestClientDelegate();
   ~AuthenticatorRequestClientDelegate() override;
 
+  // Called when the request fails for the given |reason|, just before this
+  // delegate is destroyed.
+  virtual void DidFailWithInterestingReason(InterestingFailureReason reason);
+
   // Supplies callbacks that the embedder can invoke to initiate certain
   // actions, namely: initiate BLE pairing process, cancel WebAuthN request, and
   // dispatch request to connected authenticators.