//device/fido: add FidoAuthenticator interface

This adds the FidoAuthenticator interface, which defines the basic
methods from the WebAuthn authenticator model (MakeCredential and
GetAssertion).

FidoRequestHandler is changed to hold a map of FidoAuthenticator
instances, rather than a map of FidoTasks. FidoTask and FidoDevice are
moved behind a concrete FidoAuthenticator subclass called
FidoDeviceAuthenticator (but remain unchanged otherwise). This allows
for FidoAuthenticators that are backed by platform APIs rather than a
detachable physical  device.

This is a purely structural change. No functional changes intended.

Change-Id: Id158db5801ac4556066a1a88136d7a720b137e1b
Reviewed-on: https://chromium-review.googlesource.com/1045892
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557311}

device/fido/BUILD.gn

@@ -40,6 +40,7 @@ component("fido") {
     "ec_public_key.h",
     "fido_attestation_statement.cc",
     "fido_attestation_statement.h",
+    "fido_authenticator.h",
     "fido_ble_connection.cc",
     "fido_ble_connection.h",
     "fido_ble_device.cc",
@@ -56,6 +57,8 @@ component("fido") {
     "fido_constants.h",
     "fido_device.cc",
     "fido_device.h",
+    "fido_device_authenticator.cc",
+    "fido_device_authenticator.h",
     "fido_discovery.cc",
     "fido_discovery.h",
     "fido_hid_message.cc",

device/fido/fido_authenticator.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DEVICE_FIDO_FIDO_AUTHENTICATOR_H_
+#define DEVICE_FIDO_FIDO_AUTHENTICATOR_H_
+
+#include <string>
+
+#include "base/callback_forward.h"
+#include "base/component_export.h"
+#include "base/macros.h"
+#include "base/optional.h"
+#include "device/fido/authenticator_get_assertion_response.h"
+#include "device/fido/authenticator_make_credential_response.h"
+
+namespace device {
+
+class AuthenticatorSelectionCriteria;
+class CtapGetAssertionRequest;
+class CtapMakeCredentialRequest;
+
+// FidoAuthenticator is an authenticator from the WebAuthn Authenticator model
+// (https://www.w3.org/TR/webauthn/#sctn-authenticator-model). It may be a
+// physical device, or a built-in (platform) authenticator.
+class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
+ public:
+  using MakeCredentialCallback = base::OnceCallback<void(
+      CtapDeviceResponseCode,
+      base::Optional<AuthenticatorMakeCredentialResponse>)>;
+  using GetAssertionCallback = base::OnceCallback<void(
+      CtapDeviceResponseCode,
+      base::Optional<AuthenticatorGetAssertionResponse>)>;
+
+  FidoAuthenticator() = default;
+  virtual ~FidoAuthenticator() = default;
+
+  virtual void MakeCredential(
+      AuthenticatorSelectionCriteria authenticator_selection_criteria,
+      CtapMakeCredentialRequest request,
+      MakeCredentialCallback callback) = 0;
+  virtual void GetAssertion(CtapGetAssertionRequest request,
+                            GetAssertionCallback callback) = 0;
+  virtual void Cancel() = 0;
+  virtual std::string GetId() const = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FidoAuthenticator);
+};
+
+}  // namespace device
+
+#endif  // DEVICE_FIDO_FIDO_AUTHENTICATOR_H_

device/fido/fido_device_authenticator.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "device/fido/fido_device_authenticator.h"
+
+#include <utility>
+
+#include "device/fido/authenticator_selection_criteria.h"
+#include "device/fido/ctap_get_assertion_request.h"
+#include "device/fido/ctap_make_credential_request.h"
+#include "device/fido/fido_device.h"
+#include "device/fido/get_assertion_task.h"
+#include "device/fido/make_credential_task.h"
+
+namespace device {
+
+FidoDeviceAuthenticator::FidoDeviceAuthenticator(FidoDevice* device)
+    : device_(device) {}
+FidoDeviceAuthenticator::~FidoDeviceAuthenticator() = default;
+
+void FidoDeviceAuthenticator::MakeCredential(
+    AuthenticatorSelectionCriteria authenticator_selection_criteria,
+    CtapMakeCredentialRequest request,
+    MakeCredentialCallback callback) {
+  DCHECK(!task_);
+  // TODO(martinkr): Change FidoTasks to take all request parameters by const
+  // reference, so we can avoid copying these from the RequestHandler.
+  task_ = std::make_unique<MakeCredentialTask>(
+      device_, std::move(request), std::move(authenticator_selection_criteria),
+      std::move(callback));
+}
+
+void FidoDeviceAuthenticator::GetAssertion(CtapGetAssertionRequest request,
+                                           GetAssertionCallback callback) {
+  task_ = std::make_unique<GetAssertionTask>(device_, std::move(request),
+                                             std::move(callback));
+}
+
+void FidoDeviceAuthenticator::Cancel() {
+  if (!task_)
+    return;
+
+  task_->CancelTask();
+}
+
+std::string FidoDeviceAuthenticator::GetId() const {
+  return device_->GetId();
+}
+
+void FidoDeviceAuthenticator::SetTaskForTesting(
+    std::unique_ptr<FidoTask> task) {
+  task_ = std::move(task);
+}
+
+}  // namespace device

device/fido/fido_device_authenticator.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DEVICE_FIDO_FIDO_DEVICE_AUTHENTICATOR_H_
+#define DEVICE_FIDO_FIDO_DEVICE_AUTHENTICATOR_H_
+
+#include <string>
+#include <vector>
+
+#include "base/component_export.h"
+#include "base/macros.h"
+#include "base/optional.h"
+#include "device/fido/fido_authenticator.h"
+
+namespace device {
+
+class AuthenticatorSelectionCriteria;
+class CtapGetAssertionRequest;
+class CtapMakeCredentialRequest;
+class FidoDevice;
+class FidoTask;
+
+// Adaptor class from a |FidoDevice| to the |FidoAuthenticator| interface.
+// Responsible for translating WebAuthn-level requests into serializations that
+// can be passed to the device for transport.
+class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
+    : public FidoAuthenticator {
+ public:
+  FidoDeviceAuthenticator(FidoDevice* device);
+  ~FidoDeviceAuthenticator() override;
+
+  // FidoAuthenticator:
+  void MakeCredential(
+      AuthenticatorSelectionCriteria authenticator_selection_criteria,
+      CtapMakeCredentialRequest request,
+      MakeCredentialCallback callback) override;
+  void GetAssertion(CtapGetAssertionRequest request,
+                    GetAssertionCallback callback) override;
+  void Cancel() override;
+  std::string GetId() const override;
+
+ protected:
+  void OnCtapMakeCredentialResponseReceived(
+      MakeCredentialCallback callback,
+      base::Optional<std::vector<uint8_t>> response_data);
+  void OnCtapGetAssertionResponseReceived(
+      GetAssertionCallback callback,
+      base::Optional<std::vector<uint8_t>> response_data);
+
+  FidoDevice* device() { return device_; }
+  void SetTaskForTesting(std::unique_ptr<FidoTask> task);
+
+ private:
+  FidoDevice* const device_;
+  std::unique_ptr<FidoTask> task_;
+
+  DISALLOW_COPY_AND_ASSIGN(FidoDeviceAuthenticator);
+};
+
+}  // namespace device
+
+#endif  // DEVICE_FIDO_FIDO_DEVICE_AUTHENTICATOR_H_

device/fido/fido_request_handler.h

@@ -13,6 +13,7 @@
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "device/fido/fido_authenticator.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/fido_device.h"
 #include "device/fido/fido_transport_protocol.h"
@@ -41,11 +42,11 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
   bool is_complete() const { return completion_callback_.is_null(); }
 
  protected:
-  // Converts device response code received from CTAP1/CTAP2 device into
+  // Converts authenticator response code received from CTAP1/CTAP2 device into
   // FidoReturnCode and passes response data to webauth::mojom::Authenticator.
-  void OnDeviceResponse(FidoDevice* device,
-                        CtapDeviceResponseCode device_response_code,
-                        base::Optional<Response> response_data) {
+  void OnAuthenticatorResponse(FidoAuthenticator* authenticator,
+                               CtapDeviceResponseCode device_response_code,
+                               base::Optional<Response> response_data) {
     if (is_complete()) {
       DVLOG(2)
           << "Response from authenticator received after request is complete.";
@@ -55,17 +56,17 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
     const auto return_code = ConvertDeviceResponseCodeToFidoReturnCode(
         device_response_code, response_data.has_value());
 
-    // Any device response codes that do not result from user consent
-    // imply that the device should be dropped and that other on-going
+    // Any authenticator response codes that do not result from user consent
+    // imply that the authenticator should be dropped and that other on-going
     // requests should continue until timeout is reached.
     if (!return_code) {
-      ongoing_tasks().erase(device->GetId());
+      active_authenticators().erase(authenticator->GetId());
       return;
     }
 
     // Once response has been passed to the relying party, cancel all other on
     // going requests.
-    CancelOngoingTasks(device->GetId());
+    CancelOngoingTasks(authenticator->GetId());
     std::move(completion_callback_).Run(*return_code, std::move(response_data));
   }
 
@@ -81,7 +82,7 @@ class FidoRequestHandler : public FidoRequestHandlerBase {
                    : FidoReturnCode::kAuthenticatorResponseInvalid;
 
       // These errors are only returned after the user interacted with the
-      // device.
+      // authenticator.
       case CtapDeviceResponseCode::kCtap2ErrCredentialExcluded:
         return FidoReturnCode::kUserConsentButCredentialExcluded;
       case CtapDeviceResponseCode::kCtap2ErrNoCredentials:

device/fido/fido_request_handler_base.cc

@@ -32,13 +32,13 @@ FidoRequestHandlerBase::~FidoRequestHandlerBase() = default;
 
 void FidoRequestHandlerBase::CancelOngoingTasks(
     base::StringPiece exclude_device_id) {
-  for (auto task_it = ongoing_tasks_.begin();
-       task_it != ongoing_tasks_.end();) {
+  for (auto task_it = active_authenticators_.begin();
+       task_it != active_authenticators_.end();) {
     DCHECK(!task_it->first.empty());
     if (task_it->first != exclude_device_id) {
       DCHECK(task_it->second);
-      task_it->second->CancelTask();
-      task_it = ongoing_tasks_.erase(task_it);
+      task_it->second->Cancel();
+      task_it = active_authenticators_.erase(task_it);
     } else {
       ++task_it;
     }
@@ -56,12 +56,21 @@ void FidoRequestHandlerBase::DiscoveryStarted(FidoDiscovery* discovery,
 
 void FidoRequestHandlerBase::DeviceAdded(FidoDiscovery* discovery,
                                          FidoDevice* device) {
-  DCHECK(!base::ContainsKey(ongoing_tasks(), device->GetId()));
+  DCHECK(!base::ContainsKey(active_authenticators(), device->GetId()));
   // All devices are initially assumed to support CTAP protocol and thus
   // AuthenticatorGetInfo command is sent to all connected devices. If device
   // errors out, then it is assumed to support U2F protocol.
   device->set_supported_protocol(ProtocolVersion::kCtap);
-  ongoing_tasks_.emplace(device->GetId(), CreateTaskForNewDevice(device));
+  auto* authenticator =
+      active_authenticators_
+          .emplace(device->GetId(), CreateAuthenticatorFromDevice(device))
+          .first->second.get();
+  DispatchRequest(authenticator);
+}
+
+std::unique_ptr<FidoDeviceAuthenticator>
+FidoRequestHandlerBase::CreateAuthenticatorFromDevice(FidoDevice* device) {
+  return std::make_unique<FidoDeviceAuthenticator>(device);
 }
 
 void FidoRequestHandlerBase::DeviceRemoved(FidoDiscovery* discovery,
@@ -71,7 +80,7 @@ void FidoRequestHandlerBase::DeviceRemoved(FidoDiscovery* discovery,
   // 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().
-  ongoing_tasks_.erase(device->GetId());
+  active_authenticators_.erase(device->GetId());
 }
 
 }  // namespace device

device/fido/fido_request_handler_base.h

@@ -15,6 +15,7 @@
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/strings/string_piece_forward.h"
+#include "device/fido/fido_device_authenticator.h"
 #include "device/fido/fido_discovery.h"
 #include "device/fido/fido_transport_protocol.h"
 
@@ -24,6 +25,7 @@ class Connector;
 
 namespace device {
 
+class FidoAuthenticator;
 class FidoDevice;
 class FidoTask;
 
@@ -35,7 +37,8 @@ class FidoTask;
 class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
     : public FidoDiscovery::Observer {
  public:
-  using TaskMap = std::map<std::string, std::unique_ptr<FidoTask>, std::less<>>;
+  using AuthenticatorMap =
+      std::map<std::string, std::unique_ptr<FidoAuthenticator>, std::less<>>;
 
   FidoRequestHandlerBase(
       service_manager::Connector* connector,
@@ -55,10 +58,18 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   void CancelOngoingTasks(base::StringPiece exclude_device_id = nullptr);
 
  protected:
-  virtual std::unique_ptr<FidoTask> CreateTaskForNewDevice(FidoDevice*) = 0;
+  // Subclasses implement this method to dispatch their request onto the given
+  // FidoAuthenticator. The FidoAuthenticator is owned by this
+  // FidoRequestHandler and stored in active_authenticators().
+  virtual void DispatchRequest(FidoAuthenticator*) = 0;
+
   void Start();
 
-  TaskMap& ongoing_tasks() { return ongoing_tasks_; }
+  // Testing seam to allow unit tests to inject a fake authenticator.
+  virtual std::unique_ptr<FidoDeviceAuthenticator>
+  CreateAuthenticatorFromDevice(FidoDevice* device);
+
+  AuthenticatorMap& active_authenticators() { return active_authenticators_; }
 
  private:
   // FidoDiscovery::Observer
@@ -66,7 +77,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   void DeviceAdded(FidoDiscovery* discovery, FidoDevice* device) final;
   void DeviceRemoved(FidoDiscovery* discovery, FidoDevice* device) final;
 
-  TaskMap ongoing_tasks_;
+  AuthenticatorMap active_authenticators_;
   std::vector<std::unique_ptr<FidoDiscovery>> discoveries_;
 
   DISALLOW_COPY_AND_ASSIGN(FidoRequestHandlerBase);

device/fido/fido_request_handler_unittest.cc

@@ -6,6 +6,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/test/scoped_task_environment.h"
 #include "device/fido/fake_fido_discovery.h"
@@ -83,6 +84,16 @@ class FakeFidoTask : public FidoTask {
   base::WeakPtrFactory<FakeFidoTask> weak_factory_;
 };
 
+class FakeFidoAuthenticator : public FidoDeviceAuthenticator {
+ public:
+  FakeFidoAuthenticator(FidoDevice* device) : FidoDeviceAuthenticator(device) {}
+
+  void RunFakeTask(FakeTaskCallback callback) {
+    SetTaskForTesting(
+        std::make_unique<FakeFidoTask>(device(), std::move(callback)));
+  }
+};
+
 class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> {
  public:
   FakeFidoRequestHandler(const base::flat_set<FidoTransportProtocol>& protocols,
@@ -95,15 +106,19 @@ class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> {
   }
   ~FakeFidoRequestHandler() override = default;
 
-  std::unique_ptr<FidoTask> CreateTaskForNewDevice(
+  void DispatchRequest(FidoAuthenticator* authenticator) override {
+    static_cast<FakeFidoAuthenticator*>(authenticator)
+        ->RunFakeTask(
+            base::BindOnce(&FakeFidoRequestHandler::OnAuthenticatorResponse,
+                           weak_factory_.GetWeakPtr(), authenticator));
+  }
+
+  std::unique_ptr<FidoDeviceAuthenticator> CreateAuthenticatorFromDevice(
       FidoDevice* device) override {
-    return std::make_unique<FakeFidoTask>(
-        device, base::BindOnce(&FakeFidoRequestHandler::OnDeviceResponse,
-                               weak_factory_.GetWeakPtr(), device));
+    return std::make_unique<FakeFidoAuthenticator>(device);
   }
 
  private:
-  FakeHandlerCallback callback_;
   base::WeakPtrFactory<FakeFidoRequestHandler> weak_factory_;
 };
 

device/fido/get_assertion_request_handler.cc

@@ -8,7 +8,7 @@
 
 #include "base/bind.h"
 #include "device/fido/authenticator_get_assertion_response.h"
-#include "device/fido/fido_device.h"
+#include "device/fido/fido_authenticator.h"
 #include "device/fido/get_assertion_task.h"
 
 namespace device {
@@ -26,12 +26,12 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
 
 GetAssertionRequestHandler::~GetAssertionRequestHandler() = default;
 
-std::unique_ptr<FidoTask> GetAssertionRequestHandler::CreateTaskForNewDevice(
-    FidoDevice* device) {
-  return std::make_unique<GetAssertionTask>(
-      device, request_,
-      base::BindOnce(&GetAssertionRequestHandler::OnDeviceResponse,
-                     weak_factory_.GetWeakPtr(), device));
+void GetAssertionRequestHandler::DispatchRequest(
+    FidoAuthenticator* authenticator) {
+  authenticator->GetAssertion(
+      request_,
+      base::BindOnce(&GetAssertionRequestHandler::OnAuthenticatorResponse,
+                     weak_factory_.GetWeakPtr(), authenticator));
 }
 
 }  // namespace device

device/fido/get_assertion_request_handler.h

@@ -22,8 +22,7 @@ class Connector;
 
 namespace device {
 
-class FidoDevice;
-class FidoTask;
+class FidoAuthenticator;
 class AuthenticatorGetAssertionResponse;
 
 using SignResponseCallback =
@@ -42,7 +41,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
 
  private:
   // FidoRequestHandlerBase:
-  std::unique_ptr<FidoTask> CreateTaskForNewDevice(FidoDevice* device) override;
+  void DispatchRequest(FidoAuthenticator* authenticator) override;
 
   CtapGetAssertionRequest request_;
   base::WeakPtrFactory<GetAssertionRequestHandler> weak_factory_;

device/fido/make_credential_request_handler.cc

@@ -8,7 +8,7 @@
 
 #include "base/bind.h"
 #include "device/fido/authenticator_make_credential_response.h"
-#include "device/fido/fido_device.h"
+#include "device/fido/fido_authenticator.h"
 #include "device/fido/make_credential_task.h"
 #include "services/service_manager/public/cpp/connector.h"
 
@@ -30,12 +30,12 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
 
 MakeCredentialRequestHandler::~MakeCredentialRequestHandler() = default;
 
-std::unique_ptr<FidoTask> MakeCredentialRequestHandler::CreateTaskForNewDevice(
-    FidoDevice* device) {
-  return std::make_unique<MakeCredentialTask>(
-      device, request_parameter_, authenticator_selection_criteria_,
-      base::BindOnce(&MakeCredentialRequestHandler::OnDeviceResponse,
-                     weak_factory_.GetWeakPtr(), device));
+void MakeCredentialRequestHandler::DispatchRequest(
+    FidoAuthenticator* authenticator) {
+  return authenticator->MakeCredential(
+      authenticator_selection_criteria_, request_parameter_,
+      base::BindOnce(&MakeCredentialRequestHandler::OnAuthenticatorResponse,
+                     weak_factory_.GetWeakPtr(), authenticator));
 }
 
 }  // namespace device

device/fido/make_credential_request_handler.h

@@ -24,8 +24,7 @@ class Connector;
 
 namespace device {
 
-class FidoDevice;
-class FidoTask;
+class FidoAuthenticator;
 class AuthenticatorMakeCredentialResponse;
 
 using RegisterResponseCallback = base::OnceCallback<
@@ -44,7 +43,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
 
  private:
   // FidoRequestHandlerBase:
-  std::unique_ptr<FidoTask> CreateTaskForNewDevice(FidoDevice* device) final;
+  void DispatchRequest(FidoAuthenticator* authenticator) final;
 
   CtapMakeCredentialRequest request_parameter_;
   AuthenticatorSelectionCriteria authenticator_selection_criteria_;