fido: remove FidoRequestHandler::AuthenticatorState

A timer instantiated in the location where we want to measure request
duration is simpler than using timer for the entire lifetime of the
entire authenticator, and allows us to eliminate the additional type.

It's probably also more precise, because we previously included
GetInfo requests in the timer, which are irrelevant to the thing the
timer is actually supposed to measure.

Change-Id: I0669bcedc4dec517a6dc82922f836335cfc50855
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2204551
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773716}

device/fido/fido_request_handler_base.cc

@@ -13,7 +13,6 @@
 #include "base/strings/string_piece.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
-#include "base/timer/elapsed_timer.h"
 #include "build/build_config.h"
 #include "components/device_event_log/device_event_log.h"
 #include "device/fido/ble_adapter_manager.h"
@@ -24,23 +23,8 @@
 #include "device/fido/win/authenticator.h"
 #endif
 
-namespace {
-// Authenticators that return a response in less than this time are likely to
-// have done so without interaction from the user.
-static const base::TimeDelta kMinExpectedAuthenticatorResponseTime =
-    base::TimeDelta::FromMilliseconds(300);
-}  // namespace
-
 namespace device {
 
-// FidoRequestHandlerBase::AuthenticatorState ---------------------------------
-
-FidoRequestHandlerBase::AuthenticatorState::AuthenticatorState(
-    FidoAuthenticator* authenticator)
-    : authenticator(authenticator) {}
-
-FidoRequestHandlerBase::AuthenticatorState::~AuthenticatorState() = default;
-
 // FidoRequestHandlerBase::TransportAvailabilityInfo --------------------------
 
 FidoRequestHandlerBase::TransportAvailabilityInfo::TransportAvailabilityInfo() =
@@ -176,7 +160,7 @@ void FidoRequestHandlerBase::CancelActiveAuthenticators(
     DCHECK(!task_it->first.empty());
     if (task_it->first != exclude_device_id) {
       DCHECK(task_it->second);
-      task_it->second->authenticator->Cancel();
+      task_it->second->Cancel();
 
       // Note that the pointer being erased is non-owning. The actual
       // FidoAuthenticator instance is owned by its discovery (which in turn is
@@ -227,20 +211,6 @@ void FidoRequestHandlerBase::Start() {
     discovery->Start();
 }
 
-bool FidoRequestHandlerBase::AuthenticatorMayHaveReturnedImmediately(
-    const std::string& authenticator_id) {
-  auto it = active_authenticators_.find(authenticator_id);
-  if (it == active_authenticators_.end())
-    return false;
-
-  if (!it->second->timer)
-    return true;
-
-  FIDO_LOG(DEBUG) << "Authenticator returned in "
-                  << it->second->timer->Elapsed();
-  return it->second->timer->Elapsed() < kMinExpectedAuthenticatorResponseTime;
-}
-
 void FidoRequestHandlerBase::AuthenticatorRemoved(
     FidoDiscoveryBase* discovery,
     FidoAuthenticator* authenticator) {
@@ -249,11 +219,18 @@ void FidoRequestHandlerBase::AuthenticatorRemoved(
   // ongoing_tasks_.erase() will have no effect for the devices that have been
   // already removed due to processing error or due to invocation of
   // CancelOngoingTasks().
-  DCHECK(authenticator);
-  active_authenticators_.erase(authenticator->GetId());
-
-  if (observer_)
+  auto authenticator_it = active_authenticators_.find(authenticator->GetId());
+  if (authenticator_it == active_authenticators_.end()) {
+    NOTREACHED();
+    FIDO_LOG(ERROR) << "AuthenticatorRemoved() for unknown authenticator "
+                    << authenticator->GetId();
+    return;
+  }
+  DCHECK_EQ(authenticator_it->second, authenticator);
+  active_authenticators_.erase(authenticator_it);
+  if (observer_) {
     observer_->FidoAuthenticatorRemoved(authenticator->GetId());
+  }
 }
 
 void FidoRequestHandlerBase::DiscoveryStarted(
@@ -277,9 +254,8 @@ void FidoRequestHandlerBase::AuthenticatorAdded(
     FidoAuthenticator* authenticator) {
   DCHECK(!authenticator->GetId().empty());
   bool was_inserted;
-  std::tie(std::ignore, was_inserted) = active_authenticators_.insert(
-      {authenticator->GetId(),
-       std::make_unique<AuthenticatorState>(authenticator)});
+  std::tie(std::ignore, was_inserted) =
+      active_authenticators_.insert({authenticator->GetId(), authenticator});
   if (!was_inserted) {
     NOTREACHED();
     FIDO_LOG(ERROR) << "Authenticator with duplicate ID "
@@ -341,11 +317,10 @@ void FidoRequestHandlerBase::InitializeAuthenticatorAndDispatchRequest(
   if (authenticator_it == active_authenticators_.end()) {
     return;
   }
-  AuthenticatorState* authenticator_state = authenticator_it->second.get();
-  authenticator_state->timer = std::make_unique<base::ElapsedTimer>();
-  authenticator_state->authenticator->InitializeAuthenticator(base::BindOnce(
-      &FidoRequestHandlerBase::DispatchRequest, weak_factory_.GetWeakPtr(),
-      authenticator_state->authenticator));
+  FidoAuthenticator* authenticator = authenticator_it->second;
+  authenticator->InitializeAuthenticator(
+      base::BindOnce(&FidoRequestHandlerBase::DispatchRequest,
+                     weak_factory_.GetWeakPtr(), authenticator));
 }
 
 void FidoRequestHandlerBase::ConstructBleAdapterPowerManager() {
@@ -358,4 +333,7 @@ void FidoRequestHandlerBase::StopDiscoveries() {
   }
 }
 
+constexpr base::TimeDelta
+    FidoRequestHandlerBase::kMinExpectedAuthenticatorResponseTime;
+
 }  // namespace device

device/fido/fido_request_handler_base.h

@@ -25,10 +25,6 @@
 #include "device/fido/fido_discovery_base.h"
 #include "device/fido/fido_transport_protocol.h"
 
-namespace base {
-class ElapsedTimer;
-}
-
 namespace device {
 
 class BleAdapterManager;
@@ -52,16 +48,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
 
   enum class RequestType { kMakeCredential, kGetAssertion };
 
-  struct AuthenticatorState {
-    explicit AuthenticatorState(FidoAuthenticator* authenticator);
-    ~AuthenticatorState();
-
-    FidoAuthenticator* authenticator;
-    std::unique_ptr<base::ElapsedTimer> timer;
-  };
-
   using AuthenticatorMap =
-      std::map<std::string, std::unique_ptr<AuthenticatorState>, std::less<>>;
+      std::map<std::string, FidoAuthenticator*, std::less<>>;
 
   // Encapsulates data required to initiate WebAuthN UX dialog. Once all
   // components of TransportAvailabilityInfo is set,
@@ -240,6 +228,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   void StopDiscoveries();
 
  protected:
+  // Authenticators that return a response in less than this time are likely to
+  // have done so without interaction from the user.
+  static constexpr base::TimeDelta kMinExpectedAuthenticatorResponseTime =
+      base::TimeDelta::FromMilliseconds(300);
+
   // Subclasses implement this method to dispatch their request onto the given
   // FidoAuthenticator. The FidoAuthenticator is owned by this
   // FidoRequestHandler and stored in active_authenticators().
@@ -247,13 +240,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
 
   void Start();
 
-  // Returns |true| if a short enough time has elapsed since the request was
-  // dispatched that an authenticator may be suspected to have returned a
-  // response without user interaction.
-  // Must be called after |DispatchRequest| is called.
-  bool AuthenticatorMayHaveReturnedImmediately(
-      const std::string& authenticator_id);
-
   AuthenticatorMap& active_authenticators() { return active_authenticators_; }
   std::vector<std::unique_ptr<FidoDiscoveryBase>>& discoveries() {
     return discoveries_;

device/fido/get_assertion_handler_unittest.cc

@@ -805,7 +805,7 @@ TEST(GetAssertionRequestHandlerWinTest, TestWinUsbDiscovery) {
     EXPECT_EQ(handler->AuthenticatorsForTesting().size(), 1u);
     EXPECT_EQ(handler->AuthenticatorsForTesting()
                   .begin()
-                  ->second->authenticator->IsWinNativeApiAuthenticator(),
+                  ->second->IsWinNativeApiAuthenticator(),
               enable_api);
   }
 }

device/fido/get_assertion_request_handler.cc

@@ -13,6 +13,7 @@
 #include "base/bind.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/stl_util.h"
+#include "base/timer/elapsed_timer.h"
 #include "build/build_config.h"
 #include "components/cbor/diagnostic_writer.h"
 #include "components/device_event_log/device_event_log.h"
@@ -364,7 +365,8 @@ void GetAssertionRequestHandler::DispatchRequest(
   authenticator->GetAssertion(
       std::move(request),
       base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
-                     weak_factory_.GetWeakPtr(), authenticator));
+                     weak_factory_.GetWeakPtr(), authenticator,
+                     base::ElapsedTimer()));
 }
 
 void GetAssertionRequestHandler::AuthenticatorAdded(
@@ -407,6 +409,7 @@ void GetAssertionRequestHandler::AuthenticatorRemoved(
 
 void GetAssertionRequestHandler::HandleResponse(
     FidoAuthenticator* authenticator,
+    base::ElapsedTimer request_timer,
     CtapDeviceResponseCode status,
     base::Optional<AuthenticatorGetAssertionResponse> response) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(my_sequence_checker_);
@@ -457,9 +460,12 @@ void GetAssertionRequestHandler::HandleResponse(
        status == CtapDeviceResponseCode::kCtap2ErrOperationDenied) &&
       authenticator->WillNeedPINToGetAssertion(request_, observer()) ==
           PINDisposition::kUsePINForFallback) {
-    // Some authenticators will return this error immediately without user
-    // interaction when internal UV is locked.
-    if (AuthenticatorMayHaveReturnedImmediately(authenticator->GetId())) {
+    // Authenticators without uvToken support will return this error immediately
+    // without user interaction when internal UV is locked.
+    const base::TimeDelta response_time = request_timer.Elapsed();
+    if (response_time < kMinExpectedAuthenticatorResponseTime) {
+      FIDO_LOG(DEBUG) << "Authenticator is probably locked, response_time="
+                      << response_time;
       authenticator->GetTouch(base::BindOnce(
           &GetAssertionRequestHandler::StartPINFallbackForInternalUv,
           weak_factory_.GetWeakPtr(), authenticator));
@@ -801,7 +807,8 @@ void GetAssertionRequestHandler::DispatchRequestWithToken(
   authenticator_->GetAssertion(
       std::move(request),
       base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
-                     weak_factory_.GetWeakPtr(), authenticator_));
+                     weak_factory_.GetWeakPtr(), authenticator_,
+                     base::ElapsedTimer()));
 }
 
 }  // namespace device

device/fido/get_assertion_request_handler.h

@@ -19,6 +19,10 @@
 #include "device/fido/fido_request_handler_base.h"
 #include "device/fido/fido_transport_protocol.h"
 
+namespace base {
+class ElapsedTimer;
+}
+
 namespace device {
 
 class FidoAuthenticator;
@@ -86,6 +90,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
 
   void HandleResponse(
       FidoAuthenticator* authenticator,
+      base::ElapsedTimer request_timer,
       CtapDeviceResponseCode response_code,
       base::Optional<AuthenticatorGetAssertionResponse> response);
   void HandleNextResponse(

device/fido/make_credential_request_handler.cc

@@ -430,7 +430,8 @@ void MakeCredentialRequestHandler::DispatchRequest(
   authenticator->MakeCredential(
       std::move(request),
       base::BindOnce(&MakeCredentialRequestHandler::HandleResponse,
-                     weak_factory_.GetWeakPtr(), authenticator));
+                     weak_factory_.GetWeakPtr(), authenticator,
+                     base::ElapsedTimer()));
 }
 
 void MakeCredentialRequestHandler::AuthenticatorRemoved(
@@ -454,6 +455,7 @@ void MakeCredentialRequestHandler::AuthenticatorRemoved(
 
 void MakeCredentialRequestHandler::HandleResponse(
     FidoAuthenticator* authenticator,
+    base::ElapsedTimer request_timer,
     CtapDeviceResponseCode status,
     base::Optional<AuthenticatorMakeCredentialResponse> response) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(my_sequence_checker_);
@@ -501,9 +503,12 @@ void MakeCredentialRequestHandler::HandleResponse(
        status == CtapDeviceResponseCode::kCtap2ErrPinRequired) &&
       authenticator->WillNeedPINToMakeCredential(request_, observer()) ==
           MakeCredentialPINDisposition::kUsePINForFallback) {
-    // Some authenticators will return this error immediately without user
-    // interaction when internal UV is locked.
-    if (AuthenticatorMayHaveReturnedImmediately(authenticator->GetId())) {
+    // Authenticators without uvToken support will return this error immediately
+    // without user interaction when internal UV is locked.
+    const base::TimeDelta response_time = request_timer.Elapsed();
+    if (response_time < kMinExpectedAuthenticatorResponseTime) {
+      FIDO_LOG(DEBUG) << "Authenticator is probably locked, response_time="
+                      << response_time;
       authenticator->GetTouch(base::BindOnce(
           &MakeCredentialRequestHandler::StartPINFallbackForInternalUv,
           weak_factory_.GetWeakPtr(), authenticator));
@@ -885,7 +890,8 @@ void MakeCredentialRequestHandler::DispatchRequestWithToken(
   authenticator_->MakeCredential(
       std::move(request),
       base::BindOnce(&MakeCredentialRequestHandler::HandleResponse,
-                     weak_factory_.GetWeakPtr(), authenticator_));
+                     weak_factory_.GetWeakPtr(), authenticator_,
+                     base::ElapsedTimer()));
 }
 
 void MakeCredentialRequestHandler::SpecializeRequestForAuthenticator(

device/fido/make_credential_request_handler.h

@@ -26,6 +26,10 @@
 #include "device/fido/fido_transport_protocol.h"
 #include "device/fido/pin.h"
 
+namespace base {
+class ElapsedTimer;
+}
+
 namespace device {
 
 class FidoAuthenticator;
@@ -121,6 +125,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
 
   void HandleResponse(
       FidoAuthenticator* authenticator,
+      base::ElapsedTimer request_timer,
       CtapDeviceResponseCode response_code,
       base::Optional<AuthenticatorMakeCredentialResponse> response);
   void CollectPINThenSendRequest(FidoAuthenticator* authenticator);