device/fido: move protocol detection into FidoDiscovery

This moves the AuthenticatorGetInfo request to determine whether an
authenticator supports the kCtap ProtocolVersion out of FidoTask and
into FidoDiscovery. As a result, no half-initialized FidoDevice
instances are passed into the FidoRequestHandler and FidoTask
subclasses.

This is going to make it easier to centralize checking of certain
request preconditions, like AuthenticatorSelectionCriteria e.g..
Currently checking for those is handled within FidoTask (after
completion of the AuthenticatorGetInfo dance), and therefore
TouchIdAuthenticator evades it. Determining FidoDevice capabilities
upfront allows us to move these checks out of FidoTask such that
TouchIdAuthenticator can be covered by them.

Bug: 678128
Change-Id: Ie0f9101eeca5ee6d0fbf2d7f8885d9a1dc46d032
Reviewed-on: https://chromium-review.googlesource.com/1135120
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575466}

device/fido/fake_fido_discovery_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
+#include "device/fido/fido_test_data.h"
 #include "device/fido/mock_fido_device.h"
 #include "device/fido/mock_fido_discovery_observer.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -117,8 +118,15 @@ TEST_F(FakeFidoDiscoveryTest, AddDevice) {
 
   auto device0 = std::make_unique<MockFidoDevice>();
   EXPECT_CALL(*device0, GetId()).WillOnce(::testing::Return("device0"));
-  EXPECT_CALL(observer, DeviceAdded(&discovery, ::testing::_));
+  device0->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
+  base::RunLoop device0_done;
+  EXPECT_CALL(observer, DeviceAdded(&discovery, ::testing::_))
+      .WillOnce(testing::InvokeWithoutArgs(
+          [&device0_done]() { device0_done.Quit(); }));
   discovery.AddDevice(std::move(device0));
+  device0_done.Run();
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
@@ -127,8 +135,15 @@ TEST_F(FakeFidoDiscoveryTest, AddDevice) {
 
   auto device1 = std::make_unique<MockFidoDevice>();
   EXPECT_CALL(*device1, GetId()).WillOnce(::testing::Return("device1"));
-  EXPECT_CALL(observer, DeviceAdded(&discovery, ::testing::_));
+  device1->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
+  base::RunLoop device1_done;
+  EXPECT_CALL(observer, DeviceAdded(&discovery, ::testing::_))
+      .WillOnce(testing::InvokeWithoutArgs(
+          [&device1_done]() { device1_done.Quit(); }));
   discovery.AddDevice(std::move(device1));
+  device1_done.Run();
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 }
 

device/fido/fido_device.cc

@@ -6,11 +6,51 @@
 
 #include <utility>
 
+#include "base/bind.h"
+#include "base/stl_util.h"
+#include "device/base/features.h"
+#include "device/fido/ctap_empty_authenticator_request.h"
+#include "device/fido/device_response_converter.h"
+#include "device/fido/fido_constants.h"
+
 namespace device {
 
 FidoDevice::FidoDevice() = default;
 FidoDevice::~FidoDevice() = default;
 
+void FidoDevice::DiscoverSupportedProtocolAndDeviceInfo(
+    base::OnceClosure done) {
+  if (base::FeatureList::IsEnabled(kNewCtap2Device)) {
+    // Set the protocol version to CTAP2 for the purpose of sending the GetInfo
+    // request. The correct value will be set in the callback based on the
+    // device response.
+    supported_protocol_ = ProtocolVersion::kCtap;
+    DeviceTransact(AuthenticatorGetInfoRequest().Serialize(),
+                   base::BindOnce(&FidoDevice::OnDeviceInfoReceived,
+                                  GetWeakPtr(), std::move(done)));
+  } else {
+    supported_protocol_ = ProtocolVersion::kU2f;
+    std::move(done).Run();
+  }
+}
+
+void FidoDevice::OnDeviceInfoReceived(
+    base::OnceClosure done,
+    base::Optional<std::vector<uint8_t>> response) {
+  state_ = FidoDevice::State::kReady;
+
+  base::Optional<AuthenticatorGetInfoResponse> get_info_response =
+      response ? ReadCTAPGetInfoResponse(*response) : base::nullopt;
+  if (!get_info_response || !base::ContainsKey(get_info_response->versions(),
+                                               ProtocolVersion::kCtap)) {
+    supported_protocol_ = ProtocolVersion::kU2f;
+  } else {
+    supported_protocol_ = ProtocolVersion::kCtap;
+    device_info_ = std::move(*get_info_response);
+  }
+  std::move(done).Run();
+}
+
 void FidoDevice::SetDeviceInfo(AuthenticatorGetInfoResponse device_info) {
   device_info_ = std::move(device_info);
 }

device/fido/fido_device.h

@@ -21,6 +21,11 @@
 namespace device {
 
 // Device abstraction for an individual CTAP1.0/CTAP2.0 device.
+//
+// Devices are instantiated with an unknown protocol version. Users should call
+// |DiscoverSupportedProtocolAndDeviceInfo| to determine a device's
+// capabilities and initialize the instance accordingly. Instances returned by
+// |FidoDiscovery| are already fully initialized.
 class COMPONENT_EXPORT(DEVICE_FIDO) FidoDevice {
  public:
   using WinkCallback = base::OnceClosure;
@@ -41,11 +46,16 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDevice {
   virtual void Cancel() = 0;
   virtual std::string GetId() const = 0;
 
-  void SetDeviceInfo(AuthenticatorGetInfoResponse device_info);
+  // Sends a speculative AuthenticatorGetInfo request to determine whether the
+  // device supports the CTAP2 protocol, and initializes supported_protocol_
+  // and device_info_ according to the result (unless the
+  // device::kNewCtap2Device feature is off, in which case U2F is assumed).
+  void DiscoverSupportedProtocolAndDeviceInfo(base::OnceClosure done);
+
+  // TODO(martinkr): Rename to "SetSupportedProtocolForTesting".
   void set_supported_protocol(ProtocolVersion supported_protocol) {
     supported_protocol_ = supported_protocol;
   }
-  void set_state(State state) { state_ = state; }
 
   ProtocolVersion supported_protocol() const { return supported_protocol_; }
   const base::Optional<AuthenticatorGetInfoResponse>& device_info() const {
@@ -56,6 +66,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDevice {
  protected:
   virtual base::WeakPtr<FidoDevice> GetWeakPtr() = 0;
 
+  void OnDeviceInfoReceived(base::OnceClosure done,
+                            base::Optional<std::vector<uint8_t>> response);
+  void SetDeviceInfo(AuthenticatorGetInfoResponse device_info);
+
   State state_ = State::kInit;
   ProtocolVersion supported_protocol_ = ProtocolVersion::kUnknown;
   base::Optional<AuthenticatorGetInfoResponse> device_info_;

device/fido/fido_discovery.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/bind.h"
 #include "build/build_config.h"
 #include "device/fido/fido_ble_discovery.h"
 #include "device/fido/fido_device.h"
@@ -67,7 +68,7 @@ std::unique_ptr<FidoDiscovery> FidoDiscovery::Create(
 }
 
 FidoDiscovery::FidoDiscovery(FidoTransportProtocol transport)
-    : transport_(transport) {}
+    : transport_(transport), weak_factory_(this) {}
 
 FidoDiscovery::~FidoDiscovery() = default;
 
@@ -132,9 +133,15 @@ const FidoDevice* FidoDiscovery::GetDevice(base::StringPiece device_id) const {
 bool FidoDiscovery::AddDevice(std::unique_ptr<FidoDevice> device) {
   std::string device_id = device->GetId();
   const auto result = devices_.emplace(std::move(device_id), std::move(device));
-  if (result.second)
-    NotifyDeviceAdded(result.first->second.get());
-  return result.second;
+  if (!result.second) {
+    return false;  // Duplicate device id.
+  }
+  FidoDevice* device_ptr = result.first->second.get();
+  // Determine the device protocol version before notifying observers.
+  device_ptr->DiscoverSupportedProtocolAndDeviceInfo(
+      base::BindOnce(&FidoDiscovery::NotifyDeviceAdded,
+                     weak_factory_.GetWeakPtr(), device_ptr));
+  return true;
 }
 
 bool FidoDiscovery::RemoveDevice(base::StringPiece device_id) {

device/fido/fido_discovery.h

@@ -49,6 +49,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
     // before the client of FidoDiscovery calls FidoDiscovery::Start(). However,
     // for devices already known to the system at that point, DeviceAdded()
     // might already be called to reported already known devices.
+    //
+    // The supplied FidoDevice instance is guaranteed to have its protocol
+    // version initialized. I.e., FidoDiscovery calls
+    // FidoDevice::DiscoverSupportedProtocolAndDeviceInfo() before notifying
+    // the Observer.
     virtual void DeviceAdded(FidoDiscovery* discovery, FidoDevice* device) = 0;
     virtual void DeviceRemoved(FidoDiscovery* discovery,
                                FidoDevice* device) = 0;
@@ -112,6 +117,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
 
   const FidoTransportProtocol transport_;
   State state_ = State::kIdle;
+  base::WeakPtrFactory<FidoDiscovery> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FidoDiscovery);
 };

device/fido/fido_discovery_unittest.cc

@@ -7,6 +7,9 @@
 #include <utility>
 
 #include "base/macros.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "device/fido/fido_test_data.h"
 #include "device/fido/mock_fido_device.h"
 #include "device/fido/mock_fido_discovery_observer.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -90,6 +93,7 @@ TEST(FidoDiscoveryTest, TestNotificationsOnFailedStart) {
 }
 
 TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   ConcreteFidoDiscovery discovery(FidoTransportProtocol::kBluetoothLowEnergy);
   MockFidoDiscoveryObserver observer;
   discovery.set_observer(&observer);
@@ -98,18 +102,37 @@ TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
   // Expect successful insertion.
   auto device0 = std::make_unique<MockFidoDevice>();
   auto* device0_raw = device0.get();
-  EXPECT_CALL(observer, DeviceAdded(&discovery, device0_raw));
+  device0->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
+  base::RunLoop device0_done;
+  EXPECT_CALL(observer, DeviceAdded(&discovery, device0_raw))
+      .WillOnce(testing::InvokeWithoutArgs(
+          [&device0_done]() { device0_done.Quit(); }));
   EXPECT_CALL(*device0, GetId()).WillOnce(Return("device0"));
   EXPECT_TRUE(discovery.AddDevice(std::move(device0)));
+  device0_done.Run();
   ::testing::Mock::VerifyAndClearExpectations(&observer);
+  // Device should have been initialized as a CTAP device.
+  EXPECT_EQ(ProtocolVersion::kCtap, device0_raw->supported_protocol());
+  EXPECT_TRUE(device0_raw->device_info());
 
-  // // Expect successful insertion.
+  // Expect successful insertion.
+  base::RunLoop device1_done;
   auto device1 = std::make_unique<MockFidoDevice>();
   auto* device1_raw = device1.get();
-  EXPECT_CALL(observer, DeviceAdded(&discovery, device1_raw));
+  device1->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo, base::nullopt);
+  EXPECT_CALL(observer, DeviceAdded(&discovery, device1_raw))
+      .WillOnce(testing::InvokeWithoutArgs(
+          [&device1_done]() { device1_done.Quit(); }));
   EXPECT_CALL(*device1, GetId()).WillOnce(Return("device1"));
   EXPECT_TRUE(discovery.AddDevice(std::move(device1)));
+  device1_done.Run();
   ::testing::Mock::VerifyAndClearExpectations(&observer);
+  // Device should have been initialized as a U2F device.
+  EXPECT_EQ(ProtocolVersion::kU2f, device1_raw->supported_protocol());
+  EXPECT_FALSE(device1_raw->device_info());
 
   // Inserting a device with an already present id should be prevented.
   auto device1_dup = std::make_unique<MockFidoDevice>();

device/fido/fido_request_handler_base.cc

@@ -92,10 +92,6 @@ void FidoRequestHandlerBase::DiscoveryStarted(FidoDiscovery* discovery,
 void FidoRequestHandlerBase::DeviceAdded(FidoDiscovery* discovery,
                                          FidoDevice* device) {
   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);
   AddAuthenticator(CreateAuthenticatorFromDevice(device));
 }
 

device/fido/fido_request_handler_unittest.cc

@@ -180,6 +180,8 @@ TEST_F(FidoRequestHandlerTest, TestSingleDeviceSuccess) {
   discovery()->WaitForCallToStartAndSimulateSuccess();
 
   auto device = std::make_unique<MockFidoDevice>();
+  device->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo, base::nullopt);
   EXPECT_CALL(*device, GetId()).WillRepeatedly(testing::Return("device0"));
   // Device returns success response.
   device->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
@@ -200,12 +202,16 @@ TEST_F(FidoRequestHandlerTest, TestAuthenticatorHandlerReset) {
   discovery()->WaitForCallToStartAndSimulateSuccess();
 
   auto device0 = std::make_unique<MockFidoDevice>();
-  device0->set_supported_protocol(ProtocolVersion::kCtap);
+  device0->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device0, GetId()).WillRepeatedly(testing::Return("device0"));
   device0->ExpectRequestAndDoNotRespond(std::vector<uint8_t>());
   EXPECT_CALL(*device0, Cancel());
   auto device1 = std::make_unique<MockFidoDevice>();
-  device1->set_supported_protocol(ProtocolVersion::kCtap);
+  device1->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device1, GetId()).WillRepeatedly(testing::Return("device1"));
   device1->ExpectRequestAndDoNotRespond(std::vector<uint8_t>());
   EXPECT_CALL(*device1, Cancel());
@@ -224,7 +230,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleDevices) {
 
   // Represents a connected device that hangs without a response.
   auto device0 = std::make_unique<MockFidoDevice>();
-  device0->set_supported_protocol(ProtocolVersion::kCtap);
+  device0->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device0, GetId()).WillRepeatedly(testing::Return("device0"));
   // Device is unresponsive and cancel command is invoked afterwards.
   device0->ExpectRequestAndDoNotRespond(std::vector<uint8_t>());
@@ -232,7 +240,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleDevices) {
 
   // Represents a connected device that response successfully.
   auto device1 = std::make_unique<MockFidoDevice>();
-  device1->set_supported_protocol(ProtocolVersion::kCtap);
+  device1->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device1, GetId()).WillRepeatedly(testing::Return("device1"));
   device1->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeSuccessDeviceResponse());
@@ -255,7 +265,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleSuccessResponses) {
   // Represents a connected device that responds successfully after small time
   // delay.
   auto device0 = std::make_unique<MockFidoDevice>();
-  device0->set_supported_protocol(ProtocolVersion::kCtap);
+  device0->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device0, GetId()).WillRepeatedly(testing::Return("device0"));
   device0->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeSuccessDeviceResponse(),
@@ -264,7 +276,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleSuccessResponses) {
   // Represents a device that returns a success response after a longer time
   // delay.
   auto device1 = std::make_unique<MockFidoDevice>();
-  device1->set_supported_protocol(ProtocolVersion::kCtap);
+  device1->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device1, GetId()).WillRepeatedly(testing::Return("device1"));
   device1->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeSuccessDeviceResponse(),
@@ -294,6 +308,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) {
   // Represents a connected device that immediately responds with a processing
   // error.
   auto device0 = std::make_unique<MockFidoDevice>();
+  device0->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device0, GetId()).WillRepeatedly(testing::Return("device0"));
   device0->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeDeviceProcesssingError());
@@ -301,6 +318,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) {
   // Represents a device that returns an UP verified failure response after a
   // small time delay.
   auto device1 = std::make_unique<MockFidoDevice>();
+  device1->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device1, GetId()).WillRepeatedly(testing::Return("device1"));
   device1->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeUserPresenceVerifiedError(),
@@ -309,7 +329,9 @@ TEST_F(FidoRequestHandlerTest, TestRequestWithMultipleFailureResponses) {
   // Represents a device that returns an UP verified failure response after a
   // big time delay.
   auto device2 = std::make_unique<MockFidoDevice>();
-  device2->set_supported_protocol(ProtocolVersion::kCtap);
+  device2->ExpectCtap2CommandAndRespondWith(
+      CtapRequestCommand::kAuthenticatorGetInfo,
+      test_data::kTestAuthenticatorGetInfoResponse);
   EXPECT_CALL(*device2, GetId()).WillRepeatedly(testing::Return("device2"));
   device2->ExpectRequestAndRespondWith(std::vector<uint8_t>(),
                                        CreateFakeDeviceProcesssingError(),
@@ -333,7 +355,7 @@ TEST_F(FidoRequestHandlerTest, TestPlatformAuthenticatorCallback) {
   // A platform authenticator usually wouldn't usually use a FidoDevice, but
   // that's not the point of the test here. The test is only trying to ensure
   // the authenticator gets injected and used.
-  auto device = std::make_unique<MockFidoDevice>();
+  auto device = MockFidoDevice::MakeCtap();
   EXPECT_CALL(*device, GetId()).WillRepeatedly(testing::Return("device0"));
   // Device returns success response.
   device->ExpectRequestAndRespondWith(std::vector<uint8_t>(),

device/fido/fido_task.cc

@@ -9,14 +9,16 @@
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
-#include "device/fido/ctap_empty_authenticator_request.h"
-#include "device/fido/device_response_converter.h"
 #include "device/fido/fido_constants.h"
 
 namespace device {
 
 FidoTask::FidoTask(FidoDevice* device) : device_(device), weak_factory_(this) {
   DCHECK(device_);
+  DCHECK((device_->supported_protocol() == ProtocolVersion::kCtap &&
+          device_->device_info()) ||
+         device_->supported_protocol() == ProtocolVersion::kU2f)
+      << "FidoDevice protocol version initialized incorrectly";
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&FidoTask::StartTask, weak_factory_.GetWeakPtr()));
@@ -31,35 +33,4 @@ void FidoTask::CancelTask() {
   device()->Cancel();
 }
 
-void FidoTask::GetAuthenticatorInfo(base::OnceClosure ctap_callback,
-                                    base::OnceClosure u2f_callback) {
-  // When AuthenticatorInfo command is sent to authenticators, we first assume
-  // that the connected device is a CTAP2 device.
-  device_->set_supported_protocol(ProtocolVersion::kCtap);
-  device()->DeviceTransact(
-      AuthenticatorGetInfoRequest().Serialize(),
-      base::BindOnce(&FidoTask::OnAuthenticatorInfoReceived,
-                     weak_factory_.GetWeakPtr(), std::move(ctap_callback),
-                     std::move(u2f_callback)));
-}
-
-void FidoTask::OnAuthenticatorInfoReceived(
-    base::OnceClosure ctap_callback,
-    base::OnceClosure u2f_callback,
-    base::Optional<std::vector<uint8_t>> response) {
-  device()->set_state(FidoDevice::State::kReady);
-
-  base::Optional<AuthenticatorGetInfoResponse> get_info_response;
-  if (!response || !(get_info_response = ReadCTAPGetInfoResponse(*response)) ||
-      !base::ContainsKey(get_info_response->versions(),
-                         ProtocolVersion::kCtap)) {
-    device()->set_supported_protocol(ProtocolVersion::kU2f);
-    std::move(u2f_callback).Run();
-    return;
-  }
-
-  device()->SetDeviceInfo(std::move(*get_info_response));
-  std::move(ctap_callback).Run();
-}
-
 }  // namespace device

device/fido/fido_task.h

@@ -13,7 +13,6 @@
 #include "base/component_export.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/optional.h"
 #include "device/fido/fido_device.h"
 
 namespace device {
@@ -42,27 +41,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoTask {
   // Asynchronously initiates CTAP request operation for a single device.
   virtual void StartTask() = 0;
 
-  // Invokes the AuthenticatorGetInfo method on |device_|. If successful and a
-  // well formed response is received, then |device_| is deemed to support CTAP
-  // protocol and |ctap_callback| is invoked, which sends CBOR encoded command
-  // to the authenticator. For all failure cases, |device_| is assumed to
-  // support the U2F protocol as FidoDiscovery selects only devices that support
-  // either the U2F or CTAP protocols during discovery. Therefore |u2f_callback|
-  // is invoked, which sends APDU encoded request to authenticator.
-  void GetAuthenticatorInfo(base::OnceClosure ctap_callback,
-                            base::OnceClosure u2f_callback);
-
   FidoDevice* device() const {
     DCHECK(device_);
     return device_;
   }
 
  private:
-  void OnAuthenticatorInfoReceived(
-      base::OnceClosure ctap_callback,
-      base::OnceClosure u2f_callback,
-      base::Optional<std::vector<uint8_t>> response);
-
   FidoDevice* const device_;
   base::WeakPtrFactory<FidoTask> weak_factory_;
 

device/fido/get_assertion_task.cc

@@ -43,11 +43,9 @@ GetAssertionTask::GetAssertionTask(FidoDevice* device,
 GetAssertionTask::~GetAssertionTask() = default;
 
 void GetAssertionTask::StartTask() {
-  if (base::FeatureList::IsEnabled(kNewCtap2Device)) {
-    GetAuthenticatorInfo(
-        base::BindOnce(&GetAssertionTask::GetAssertion,
-                       weak_factory_.GetWeakPtr()),
-        base::BindOnce(&GetAssertionTask::U2fSign, weak_factory_.GetWeakPtr()));
+  if (base::FeatureList::IsEnabled(kNewCtap2Device) &&
+      device()->supported_protocol() == ProtocolVersion::kCtap) {
+    GetAssertion();
   } else {
     U2fSign();
   }

device/fido/make_credential_task.cc

@@ -31,11 +31,9 @@ MakeCredentialTask::MakeCredentialTask(
 MakeCredentialTask::~MakeCredentialTask() = default;
 
 void MakeCredentialTask::StartTask() {
-  if (base::FeatureList::IsEnabled(kNewCtap2Device)) {
-    GetAuthenticatorInfo(base::BindOnce(&MakeCredentialTask::MakeCredential,
-                                        weak_factory_.GetWeakPtr()),
-                         base::BindOnce(&MakeCredentialTask::U2fRegister,
-                                        weak_factory_.GetWeakPtr()));
+  if (base::FeatureList::IsEnabled(kNewCtap2Device) &&
+      device()->supported_protocol() == ProtocolVersion::kCtap) {
+    MakeCredential();
   } else {
     U2fRegister();
   }

device/fido/make_credential_task_unittest.cc

@@ -14,6 +14,7 @@
 #include "device/base/features.h"
 #include "device/fido/authenticator_make_credential_response.h"
 #include "device/fido/ctap_make_credential_request.h"
+#include "device/fido/device_response_converter.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/fido_parsing_utils.h"
 #include "device/fido/fido_test_data.h"
@@ -35,13 +36,9 @@ using TestMakeCredentialTaskCallback =
         CtapDeviceResponseCode,
         base::Optional<AuthenticatorMakeCredentialResponse>>;
 
-}  // namespace
-
 class FidoMakeCredentialTaskTest : public testing::Test {
  public:
-  FidoMakeCredentialTaskTest() {
-    scoped_feature_list_.emplace();
-  }
+  FidoMakeCredentialTaskTest() { scoped_feature_list_.emplace(); }
 
   std::unique_ptr<MakeCredentialTask> CreateMakeCredentialTask(
       FidoDevice* device) {
@@ -89,11 +86,7 @@ class FidoMakeCredentialTaskTest : public testing::Test {
 };
 
 TEST_F(FidoMakeCredentialTaskTest, MakeCredentialSuccess) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo,
-      test_data::kTestAuthenticatorGetInfoResponse);
+  auto device = MockFidoDevice::MakeCtap();
   device->ExpectCtap2CommandAndRespondWith(
       CtapRequestCommand::kAuthenticatorMakeCredential,
       test_data::kTestMakeCredentialResponse);
@@ -110,6 +103,9 @@ TEST_F(FidoMakeCredentialTaskTest, MakeCredentialSuccess) {
 
 TEST_F(FidoMakeCredentialTaskTest, TestRegisterSuccessWithFake) {
   auto device = std::make_unique<VirtualCtap2Device>();
+  test::TestCallbackReceiver<> done_init;
+  device->DiscoverSupportedProtocolAndDeviceInfo(done_init.callback());
+  done_init.WaitForCallback();
   const auto task = CreateMakeCredentialTask(device.get());
   make_credential_callback_receiver().WaitForCallback();
 
@@ -124,11 +120,7 @@ TEST_F(FidoMakeCredentialTaskTest, TestRegisterSuccessWithFake) {
 }
 
 TEST_F(FidoMakeCredentialTaskTest, MakeCredentialWithIncorrectRpIdHash) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo,
-      test_data::kTestAuthenticatorGetInfoResponse);
+  auto device = MockFidoDevice::MakeCtap();
   device->ExpectCtap2CommandAndRespondWith(
       CtapRequestCommand::kAuthenticatorMakeCredential,
       test_data::kTestMakeCredentialResponseWithIncorrectRpIdHash);
@@ -141,10 +133,7 @@ TEST_F(FidoMakeCredentialTaskTest, MakeCredentialWithIncorrectRpIdHash) {
 }
 
 TEST_F(FidoMakeCredentialTaskTest, FallbackToU2fRegisterSuccess) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo, base::nullopt);
+  auto device = MockFidoDevice::MakeU2f();
   device->ExpectRequestAndRespondWith(
       test_data::kU2fRegisterCommandApdu,
       test_data::kApduEncodedNoErrorRegisterResponse);
@@ -159,7 +148,7 @@ TEST_F(FidoMakeCredentialTaskTest, FallbackToU2fRegisterSuccess) {
 
 TEST_F(FidoMakeCredentialTaskTest, TestDefaultU2fRegisterOperationWithoutFlag) {
   RemoveCtapFlag();
-  auto device = std::make_unique<MockFidoDevice>();
+  auto device = MockFidoDevice::MakeU2f();
   device->ExpectRequestAndRespondWith(
       test_data::kU2fRegisterCommandApdu,
       test_data::kApduEncodedNoErrorRegisterResponse);
@@ -172,11 +161,7 @@ TEST_F(FidoMakeCredentialTaskTest, TestDefaultU2fRegisterOperationWithoutFlag) {
 }
 
 TEST_F(FidoMakeCredentialTaskTest, U2fRegisterWithUserVerificationRequired) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo, base::nullopt);
-
+  auto device = MockFidoDevice::MakeU2f();
   const auto task = CreateMakeCredentialTaskWithAuthenticatorSelectionCriteria(
       device.get(),
       AuthenticatorSelectionCriteria(
@@ -191,11 +176,7 @@ TEST_F(FidoMakeCredentialTaskTest, U2fRegisterWithUserVerificationRequired) {
 }
 
 TEST_F(FidoMakeCredentialTaskTest, U2fRegisterWithPlatformDeviceRequirement) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo, base::nullopt);
-
+  auto device = MockFidoDevice::MakeU2f();
   const auto task = CreateMakeCredentialTaskWithAuthenticatorSelectionCriteria(
       device.get(),
       AuthenticatorSelectionCriteria(
@@ -210,11 +191,7 @@ TEST_F(FidoMakeCredentialTaskTest, U2fRegisterWithPlatformDeviceRequirement) {
 }
 
 TEST_F(FidoMakeCredentialTaskTest, U2fRegisterWithResidentKeyRequirement) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo, base::nullopt);
-
+  auto device = MockFidoDevice::MakeU2f();
   const auto task = CreateMakeCredentialTaskWithAuthenticatorSelectionCriteria(
       device.get(),
       AuthenticatorSelectionCriteria(
@@ -230,12 +207,8 @@ TEST_F(FidoMakeCredentialTaskTest, U2fRegisterWithResidentKeyRequirement) {
 
 TEST_F(FidoMakeCredentialTaskTest,
        UserVerificationAuthenticatorSelectionCriteria) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo,
-      test_data::kTestGetInfoResponseWithoutUvSupport);
-
+  auto device = MockFidoDevice::MakeCtap(*ReadCTAPGetInfoResponse(
+      test_data::kTestGetInfoResponseWithoutUvSupport));
   const auto task = CreateMakeCredentialTaskWithAuthenticatorSelectionCriteria(
       device.get(),
       AuthenticatorSelectionCriteria(
@@ -255,12 +228,8 @@ TEST_F(FidoMakeCredentialTaskTest,
 
 TEST_F(FidoMakeCredentialTaskTest,
        PlatformDeviceAuthenticatorSelectionCriteria) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo,
-      test_data::kTestGetInfoResponseCrossPlatformDevice);
-
+  auto device = MockFidoDevice::MakeCtap(*ReadCTAPGetInfoResponse(
+      test_data::kTestGetInfoResponseCrossPlatformDevice));
   const auto task = CreateMakeCredentialTaskWithAuthenticatorSelectionCriteria(
       device.get(),
       AuthenticatorSelectionCriteria(
@@ -277,12 +246,8 @@ TEST_F(FidoMakeCredentialTaskTest,
 }
 
 TEST_F(FidoMakeCredentialTaskTest, ResidentKeyAuthenticatorSelectionCriteria) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo,
-      test_data::kTestGetInfoResponseWithoutResidentKeySupport);
-
+  auto device = MockFidoDevice::MakeCtap(*ReadCTAPGetInfoResponse(
+      test_data::kTestGetInfoResponseWithoutResidentKeySupport));
   const auto task = CreateMakeCredentialTaskWithAuthenticatorSelectionCriteria(
       device.get(),
       AuthenticatorSelectionCriteria(
@@ -299,11 +264,7 @@ TEST_F(FidoMakeCredentialTaskTest, ResidentKeyAuthenticatorSelectionCriteria) {
 }
 
 TEST_F(FidoMakeCredentialTaskTest, SatisfyAllAuthenticatorSelectionCriteria) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo,
-      test_data::kTestAuthenticatorGetInfoResponse);
+  auto device = MockFidoDevice::MakeCtap();
   device->ExpectCtap2CommandAndRespondWith(
       CtapRequestCommand::kAuthenticatorMakeCredential,
       test_data::kTestMakeCredentialResponse);
@@ -330,12 +291,8 @@ TEST_F(FidoMakeCredentialTaskTest, SatisfyAllAuthenticatorSelectionCriteria) {
 }
 
 TEST_F(FidoMakeCredentialTaskTest, IncompatibleUserVerificationSetting) {
-  auto device = std::make_unique<MockFidoDevice>();
-
-  device->ExpectCtap2CommandAndRespondWith(
-      CtapRequestCommand::kAuthenticatorGetInfo,
-      test_data::kTestGetInfoResponseWithoutUvSupport);
-
+  auto device = MockFidoDevice::MakeCtap(*ReadCTAPGetInfoResponse(
+      test_data::kTestGetInfoResponseWithoutUvSupport));
   const auto task = CreateMakeCredentialTaskWithAuthenticatorSelectionCriteria(
       device.get(),
       AuthenticatorSelectionCriteria(
@@ -349,4 +306,5 @@ TEST_F(FidoMakeCredentialTaskTest, IncompatibleUserVerificationSetting) {
   EXPECT_FALSE(make_credential_callback_receiver().value());
 }
 
+}  // namespace
 }  // namespace device

device/fido/mock_fido_device.cc

@@ -10,18 +10,49 @@
 #include "base/location.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/apdu/apdu_response.h"
+#include "device/fido/device_response_converter.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/fido_parsing_utils.h"
 #include "device/fido/fido_test_data.h"
 
 namespace device {
 
+namespace {
+AuthenticatorGetInfoResponse DefaultAuthenticatorInfo() {
+  return *ReadCTAPGetInfoResponse(test_data::kTestAuthenticatorGetInfoResponse);
+}
+}  // namespace
+
+// static
+std::unique_ptr<MockFidoDevice> MockFidoDevice::MakeU2f() {
+  return std::make_unique<MockFidoDevice>(ProtocolVersion::kU2f, base::nullopt);
+}
+
+// static
+std::unique_ptr<MockFidoDevice> MockFidoDevice::MakeCtap(
+    base::Optional<AuthenticatorGetInfoResponse> device_info) {
+  if (!device_info) {
+    device_info = DefaultAuthenticatorInfo();
+  }
+  return std::make_unique<MockFidoDevice>(ProtocolVersion::kCtap,
+                                          std::move(*device_info));
+}
+
 // Matcher to compare the fist byte of the incoming requests.
 MATCHER_P(IsCtap2Command, expected_command, "") {
   return !arg.empty() && arg[0] == base::strict_cast<uint8_t>(expected_command);
 }
 
 MockFidoDevice::MockFidoDevice() : weak_factory_(this) {}
+MockFidoDevice::MockFidoDevice(
+    ProtocolVersion protocol_version,
+    base::Optional<AuthenticatorGetInfoResponse> device_info)
+    : MockFidoDevice() {
+  set_supported_protocol(protocol_version);
+  if (device_info) {
+    SetDeviceInfo(std::move(*device_info));
+  }
+}
 MockFidoDevice::~MockFidoDevice() = default;
 
 void MockFidoDevice::TryWink(WinkCallback cb) {

device/fido/mock_fido_device.h

@@ -23,7 +23,13 @@ namespace device {
 
 class MockFidoDevice : public FidoDevice {
  public:
+  static std::unique_ptr<MockFidoDevice> MakeU2f();
+  static std::unique_ptr<MockFidoDevice> MakeCtap(
+      base::Optional<AuthenticatorGetInfoResponse> device_info = base::nullopt);
+
   MockFidoDevice();
+  MockFidoDevice(ProtocolVersion protocol_version,
+                 base::Optional<AuthenticatorGetInfoResponse> device_info);
   ~MockFidoDevice() override;
 
   // TODO(crbug.com/729950): Remove these workarounds once support for move-only