Remove request logic from U2fDevice interface

Move all encoding/decoding/request specific logic in U2fDevice interface
to U2fRequest interface.

Bug: 810229
Change-Id: I62c721fcfe5be2c3efc1bfd87920ef020ad60198
Reviewed-on: https://chromium-review.googlesource.com/927105
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543394}

device/fido/fido_constants.cc

@@ -43,4 +43,7 @@ const size_t kU2fParameterLength = 32;
 
 const std::array<uint8_t, 2> kLegacyVersionSuffix = {0x00, 0x00};
 
+const std::array<uint8_t, 6> kU2fVersionResponse = {'U', '2', 'F',
+                                                    '_', 'V', '2'};
+
 }  // namespace device

device/fido/fido_constants.h

@@ -225,6 +225,11 @@ COMPONENT_EXPORT(DEVICE_FIDO) extern const size_t kU2fParameterLength;
 COMPONENT_EXPORT(DEVICE_FIDO)
 extern const std::array<uint8_t, 2> kLegacyVersionSuffix;
 
+// Expected response data for version request from U2F device.
+// https://fidoalliance.org/specs/fido-u2f-v1.2-ps-20170411/fido-u2f-raw-message-formats-v1.2-ps-20170411.html#getversion-request-and-response---u2f_version
+COMPONENT_EXPORT(DEVICE_FIDO)
+extern const std::array<uint8_t, 6> kU2fVersionResponse;
+
 }  // namespace device
 
 #endif  // DEVICE_FIDO_FIDO_CONSTANTS_H_

device/fido/mock_u2f_device.cc

@@ -7,12 +7,12 @@
 #include <utility>
 
 #include "components/apdu/apdu_response.h"
+#include "device/fido/fido_constants.h"
 #include "device/fido/u2f_response_test_data.h"
 
 namespace device {
 
 MockU2fDevice::MockU2fDevice() : weak_factory_(this) {}
-
 MockU2fDevice::~MockU2fDevice() = default;
 
 void MockU2fDevice::TryWink(WinkCallback cb) {
@@ -21,56 +21,70 @@ void MockU2fDevice::TryWink(WinkCallback cb) {
 
 void MockU2fDevice::DeviceTransact(std::vector<uint8_t> command,
                                    DeviceCallback cb) {
-  DeviceTransactPtr(std::move(command), cb);
+  DeviceTransactPtr(command, cb);
 }
 
 // static
-void MockU2fDevice::NotSatisfied(const std::vector<uint8_t>& cmd,
+void MockU2fDevice::NotSatisfied(const std::vector<uint8_t>& command,
                                  DeviceCallback& cb) {
-  std::move(cb).Run(
-      true, apdu::ApduResponse(
-                std::vector<uint8_t>(),
-                apdu::ApduResponse::Status::SW_CONDITIONS_NOT_SATISFIED));
+  std::move(cb).Run(apdu::ApduResponse(
+                        std::vector<uint8_t>(),
+                        apdu::ApduResponse::Status::SW_CONDITIONS_NOT_SATISFIED)
+                        .GetEncodedResponse());
 }
 
 // static
-void MockU2fDevice::WrongData(const std::vector<uint8_t>& cmd,
+void MockU2fDevice::WrongData(const std::vector<uint8_t>& command,
                               DeviceCallback& cb) {
   std::move(cb).Run(
-      true, apdu::ApduResponse(std::vector<uint8_t>(),
-                               apdu::ApduResponse::Status::SW_WRONG_DATA));
+      apdu::ApduResponse(std::vector<uint8_t>(),
+                         apdu::ApduResponse::Status::SW_WRONG_DATA)
+          .GetEncodedResponse());
 }
 
 // static
-void MockU2fDevice::NoErrorSign(const std::vector<uint8_t>& cmd,
+void MockU2fDevice::NoErrorSign(const std::vector<uint8_t>& command,
                                 DeviceCallback& cb) {
   std::move(cb).Run(
-      true, apdu::ApduResponse(std::vector<uint8_t>(
-                                   std::begin(test_data::kTestU2fSignResponse),
-                                   std::end(test_data::kTestU2fSignResponse)),
-                               apdu::ApduResponse::Status::SW_NO_ERROR));
+      apdu::ApduResponse(
+          std::vector<uint8_t>(std::begin(test_data::kTestU2fSignResponse),
+                               std::end(test_data::kTestU2fSignResponse)),
+          apdu::ApduResponse::Status::SW_NO_ERROR)
+          .GetEncodedResponse());
 }
 
 // static
-void MockU2fDevice::NoErrorRegister(const std::vector<uint8_t>& cmd,
+void MockU2fDevice::NoErrorRegister(const std::vector<uint8_t>& command,
                                     DeviceCallback& cb) {
   std::move(cb).Run(
-      true,
       apdu::ApduResponse(
           std::vector<uint8_t>(std::begin(test_data::kTestU2fRegisterResponse),
                                std::end(test_data::kTestU2fRegisterResponse)),
-          apdu::ApduResponse::Status::SW_NO_ERROR));
+          apdu::ApduResponse::Status::SW_NO_ERROR)
+          .GetEncodedResponse());
+}
+
+// static
+void MockU2fDevice::NoErrorVersion(const std::vector<uint8_t>& command,
+                                   DeviceCallback& cb) {
+  std::move(cb).Run(
+      apdu::ApduResponse(std::vector<uint8_t>(kU2fVersionResponse.cbegin(),
+                                              kU2fVersionResponse.cend()),
+                         apdu::ApduResponse::Status::SW_NO_ERROR)
+          .GetEncodedResponse());
 }
 
 // static
-void MockU2fDevice::SignWithCorruptedResponse(const std::vector<uint8_t>& cmd,
-                                              DeviceCallback& cb) {
+void MockU2fDevice::SignWithCorruptedResponse(
+    const std::vector<uint8_t>& command,
+    DeviceCallback& cb) {
   std::move(cb).Run(
-      true, apdu::ApduResponse(
-                std::vector<uint8_t>(
-                    std::begin(test_data::kTestCorruptedU2fSignResponse),
-                    std::end(test_data::kTestCorruptedU2fSignResponse)),
-                apdu::ApduResponse::Status::SW_NO_ERROR));
+      apdu::ApduResponse(
+          std::vector<uint8_t>(
+              std::begin(test_data::kTestCorruptedU2fSignResponse),
+              std::end(test_data::kTestCorruptedU2fSignResponse)),
+          apdu::ApduResponse::Status::SW_NO_ERROR)
+          .GetEncodedResponse());
 }
 
 // static

device/fido/mock_u2f_device.h

@@ -33,11 +33,10 @@ class MockU2fDevice : public U2fDevice {
   // TODO(crbug.com/729950): Remove these workarounds once support for move-only
   // types is added to GMock.
   MOCK_METHOD2(DeviceTransactPtr,
-               void(std::vector<uint8_t> command, DeviceCallback& cb));
+               void(const std::vector<uint8_t>& command, DeviceCallback& cb));
   void DeviceTransact(std::vector<uint8_t> command, DeviceCallback cb) override;
+
   base::WeakPtr<U2fDevice> GetWeakPtr() override;
-  static void TransactNoError(const std::vector<uint8_t>& command,
-                              DeviceCallback cb);
   static void NotSatisfied(const std::vector<uint8_t>& command,
                            DeviceCallback& cb);
   static void WrongData(const std::vector<uint8_t>& command,
@@ -46,6 +45,8 @@ class MockU2fDevice : public U2fDevice {
                           DeviceCallback& cb);
   static void NoErrorRegister(const std::vector<uint8_t>& command,
                               DeviceCallback& cb);
+  static void NoErrorVersion(const std::vector<uint8_t>& command,
+                             DeviceCallback& cb);
   static void SignWithCorruptedResponse(const std::vector<uint8_t>& command,
                                         DeviceCallback& cb);
   static void WinkDoNothing(WinkCallback& cb);

device/fido/u2f_ble_device.cc

@@ -36,14 +36,13 @@ void U2fBleDevice::Connect() {
 }
 
 void U2fBleDevice::SendPing(std::vector<uint8_t> data,
-                            MessageCallback callback) {
+                            DeviceCallback callback) {
   pending_frames_.emplace(
       U2fBleFrame(U2fCommandType::CMD_PING, std::move(data)),
       base::BindOnce(
-          [](MessageCallback callback, base::Optional<U2fBleFrame> frame) {
-            std::move(callback).Run(
-                frame ? U2fReturnCode::SUCCESS : U2fReturnCode::FAILURE,
-                frame ? frame->data() : std::vector<uint8_t>());
+          [](DeviceCallback callback, base::Optional<U2fBleFrame> frame) {
+            std::move(callback).Run(frame ? base::make_optional(frame->data())
+                                          : base::nullopt);
           },
           std::move(callback)));
   Transition();
@@ -80,10 +79,8 @@ void U2fBleDevice::DeviceTransact(std::vector<uint8_t> command,
       U2fBleFrame(U2fCommandType::CMD_MSG, std::move(command)),
       base::BindOnce(
           [](DeviceCallback callback, base::Optional<U2fBleFrame> frame) {
-            std::move(callback).Run(
-                frame.has_value(),
-                frame ? apdu::ApduResponse::CreateFromMessage(frame->data())
-                      : base::nullopt);
+            std::move(callback).Run(frame ? base::make_optional(frame->data())
+                                          : base::nullopt);
           },
           std::move(callback)));
   Transition();

device/fido/u2f_ble_device.h

@@ -33,7 +33,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fBleDevice : public U2fDevice {
   ~U2fBleDevice() override;
 
   void Connect();
-  void SendPing(std::vector<uint8_t> data, MessageCallback callback);
+  void SendPing(std::vector<uint8_t> data, DeviceCallback callback);
   static std::string GetId(base::StringPiece address);
 
   // U2fDevice:

device/fido/u2f_ble_device_unittest.cc

@@ -18,9 +18,8 @@ namespace {
 using ::testing::_;
 using ::testing::Invoke;
 using ::testing::Test;
-using TestMessageCallbackReceiver =
-    test::StatusAndValueCallbackReceiver<U2fReturnCode,
-                                         const std::vector<uint8_t>&>;
+using TestDeviceCallbackReceiver =
+    test::TestCallbackReceiver<base::Optional<std::vector<uint8_t>>>;
 
 }  // namespace
 
@@ -73,12 +72,11 @@ TEST_F(U2fBleDeviceTest, SendPingTest_Failure_Callback) {
       .WillOnce(Invoke(
           [this](const auto& data, auto* cb) { std::move(*cb).Run(false); }));
 
-  TestMessageCallbackReceiver callback_receiver;
+  TestDeviceCallbackReceiver callback_receiver;
   device()->SendPing({'T', 'E', 'S', 'T'}, callback_receiver.callback());
 
   callback_receiver.WaitForCallback();
-  EXPECT_EQ(U2fReturnCode::FAILURE, callback_receiver.status());
-  EXPECT_EQ(std::vector<uint8_t>(), callback_receiver.value());
+  EXPECT_FALSE(std::get<0>(*callback_receiver.result()));
 }
 
 TEST_F(U2fBleDeviceTest, SendPingTest_Failure_Timeout) {
@@ -89,12 +87,11 @@ TEST_F(U2fBleDeviceTest, SendPingTest_Failure_Timeout) {
         scoped_task_environment_.FastForwardBy(U2fDevice::kDeviceTimeout);
       }));
 
-  TestMessageCallbackReceiver callback_receiver;
+  TestDeviceCallbackReceiver callback_receiver;
   device()->SendPing({'T', 'E', 'S', 'T'}, callback_receiver.callback());
 
   callback_receiver.WaitForCallback();
-  EXPECT_EQ(U2fReturnCode::FAILURE, callback_receiver.status());
-  EXPECT_EQ(std::vector<uint8_t>(), callback_receiver.value());
+  EXPECT_FALSE(std::get<0>(*callback_receiver.result()));
 }
 
 TEST_F(U2fBleDeviceTest, SendPingTest) {
@@ -110,12 +107,13 @@ TEST_F(U2fBleDeviceTest, SendPingTest) {
         std::move(*cb).Run(true);
       }));
 
-  TestMessageCallbackReceiver callback_receiver;
+  TestDeviceCallbackReceiver callback_receiver;
   device()->SendPing(ping_data, callback_receiver.callback());
 
   callback_receiver.WaitForCallback();
-  EXPECT_EQ(U2fReturnCode::SUCCESS, callback_receiver.status());
-  EXPECT_EQ(ping_data, callback_receiver.value());
+  const auto& result = std::get<0>(*callback_receiver.result());
+  ASSERT_TRUE(result);
+  EXPECT_EQ(ping_data, *result);
 }
 
 TEST_F(U2fBleDeviceTest, StaticGetIdTest) {

device/fido/u2f_device.cc

@@ -4,120 +4,11 @@
 
 #include "device/fido/u2f_device.h"
 
-#include <utility>
-
-#include "base/bind.h"
-#include "components/apdu/apdu_command.h"
-#include "device/fido/u2f_request.h"
-
 namespace device {
 
 constexpr base::TimeDelta U2fDevice::kDeviceTimeout;
 
 U2fDevice::U2fDevice() = default;
-
 U2fDevice::~U2fDevice() = default;
 
-void U2fDevice::Register(base::Optional<std::vector<uint8_t>> register_cmd,
-                         MessageCallback callback) {
-  if (!register_cmd) {
-    std::move(callback).Run(U2fReturnCode::INVALID_PARAMS,
-                            std::vector<uint8_t>());
-    return;
-  }
-  DeviceTransact(std::move(*register_cmd),
-                 base::BindOnce(&U2fDevice::OnRegisterComplete, GetWeakPtr(),
-                                std::move(callback)));
-}
-
-void U2fDevice::Sign(base::Optional<std::vector<uint8_t>> sign_cmd,
-                     MessageCallback callback) {
-  if (!sign_cmd) {
-    std::move(callback).Run(U2fReturnCode::INVALID_PARAMS,
-                            std::vector<uint8_t>());
-    return;
-  }
-  DeviceTransact(std::move(*sign_cmd),
-                 base::BindOnce(&U2fDevice::OnSignComplete, GetWeakPtr(),
-                                std::move(callback)));
-}
-
-void U2fDevice::Version(VersionCallback callback) {
-  DeviceTransact(U2fRequest::GetU2fVersionApduCommand(),
-                 base::BindOnce(&U2fDevice::OnVersionComplete, GetWeakPtr(),
-                                std::move(callback), false /* legacy */));
-}
-
-void U2fDevice::OnRegisterComplete(
-    MessageCallback callback,
-    bool success,
-    base::Optional<apdu::ApduResponse> register_response) {
-  if (!success || !register_response) {
-    std::move(callback).Run(U2fReturnCode::FAILURE, std::vector<uint8_t>());
-    return;
-  }
-  switch (register_response->status()) {
-    case apdu::ApduResponse::Status::SW_CONDITIONS_NOT_SATISFIED:
-      std::move(callback).Run(U2fReturnCode::CONDITIONS_NOT_SATISFIED,
-                              std::vector<uint8_t>());
-      break;
-    case apdu::ApduResponse::Status::SW_NO_ERROR:
-      std::move(callback).Run(U2fReturnCode::SUCCESS,
-                              register_response->data());
-      break;
-    case apdu::ApduResponse::Status::SW_WRONG_DATA:
-      std::move(callback).Run(U2fReturnCode::INVALID_PARAMS,
-                              std::vector<uint8_t>());
-      break;
-    default:
-      std::move(callback).Run(U2fReturnCode::FAILURE, std::vector<uint8_t>());
-      break;
-  }
-}
-
-void U2fDevice::OnSignComplete(
-    MessageCallback callback,
-    bool success,
-    base::Optional<apdu::ApduResponse> sign_response) {
-  if (!success || !sign_response) {
-    std::move(callback).Run(U2fReturnCode::FAILURE, std::vector<uint8_t>());
-    return;
-  }
-  switch (sign_response->status()) {
-    case apdu::ApduResponse::Status::SW_CONDITIONS_NOT_SATISFIED:
-      std::move(callback).Run(U2fReturnCode::CONDITIONS_NOT_SATISFIED,
-                              std::vector<uint8_t>());
-      break;
-    case apdu::ApduResponse::Status::SW_NO_ERROR:
-      std::move(callback).Run(U2fReturnCode::SUCCESS, sign_response->data());
-      break;
-    case apdu::ApduResponse::Status::SW_WRONG_DATA:
-    case apdu::ApduResponse::Status::SW_WRONG_LENGTH:
-    default:
-      std::move(callback).Run(U2fReturnCode::INVALID_PARAMS,
-                              std::vector<uint8_t>());
-      break;
-  }
-}
-
-void U2fDevice::OnVersionComplete(
-    VersionCallback callback,
-    bool legacy,
-    bool success,
-    base::Optional<apdu::ApduResponse> version_response) {
-  if (success && version_response &&
-      version_response->status() == apdu::ApduResponse::Status::SW_NO_ERROR &&
-      version_response->data() ==
-          std::vector<uint8_t>({'U', '2', 'F', '_', 'V', '2'})) {
-    std::move(callback).Run(success, ProtocolVersion::U2F_V2);
-  } else if (!legacy) {
-    // Standard GetVersion failed, attempt legacy GetVersion command.
-    DeviceTransact(U2fRequest::GetU2fVersionApduCommand(true /* legacy */),
-                   base::BindOnce(&U2fDevice::OnVersionComplete, GetWeakPtr(),
-                                  std::move(callback), true /* legacy */));
-  } else {
-    std::move(callback).Run(success, ProtocolVersion::UNKNOWN);
-  }
-}
-
 }  // namespace device

device/fido/u2f_device.h

@@ -5,7 +5,8 @@
 #ifndef DEVICE_FIDO_U2F_DEVICE_H_
 #define DEVICE_FIDO_U2F_DEVICE_H_
 
-#include <memory>
+#include <stdint.h>
+
 #include <string>
 #include <vector>
 
@@ -14,8 +15,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
-#include "components/apdu/apdu_response.h"
-#include "device/fido/u2f_return_code.h"
+#include "base/time/time.h"
 
 namespace device {
 
@@ -28,51 +28,24 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fDevice {
     UNKNOWN,
   };
 
-  using MessageCallback =
-      base::OnceCallback<void(U2fReturnCode, const std::vector<uint8_t>&)>;
-  using VersionCallback =
-      base::OnceCallback<void(bool success, ProtocolVersion version)>;
+  using WinkCallback = base::OnceClosure;
+
   using DeviceCallback =
-      base::OnceCallback<void(bool success,
-                              base::Optional<apdu::ApduResponse> response)>;
-  using WinkCallback = base::OnceCallback<void()>;
+      base::OnceCallback<void(base::Optional<std::vector<uint8_t>> response)>;
 
   static constexpr auto kDeviceTimeout = base::TimeDelta::FromSeconds(3);
 
   U2fDevice();
   virtual ~U2fDevice();
-
-  // TODO(hongjunchoi): https://crbug.com/810229 Move all encoding logic from
-  // U2fDevice to U2fRequest.
-  // Raw messages parameters are defined by the specification at
-  // https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-u2f-raw-message-formats.html
-  void Register(base::Optional<std::vector<uint8_t>> register_cmd,
-                MessageCallback callback);
-  void Sign(base::Optional<std::vector<uint8_t>> sign_cmd,
-            MessageCallback callback);
-  void Version(VersionCallback callback);
-
-  virtual void TryWink(WinkCallback callback) = 0;
-  virtual std::string GetId() const = 0;
-
- protected:
   // Pure virtual function defined by each device type, implementing
   // the device communication transaction.
   virtual void DeviceTransact(std::vector<uint8_t> command,
                               DeviceCallback callback) = 0;
-  virtual base::WeakPtr<U2fDevice> GetWeakPtr() = 0;
+  virtual void TryWink(WinkCallback callback) = 0;
+  virtual std::string GetId() const = 0;
 
- private:
-  void OnRegisterComplete(MessageCallback callback,
-                          bool success,
-                          base::Optional<apdu::ApduResponse> register_response);
-  void OnSignComplete(MessageCallback callback,
-                      bool success,
-                      base::Optional<apdu::ApduResponse> sign_response);
-  void OnVersionComplete(VersionCallback callback,
-                         bool legacy,
-                         bool success,
-                         base::Optional<apdu::ApduResponse> version_response);
+ protected:
+  virtual base::WeakPtr<U2fDevice> GetWeakPtr() = 0;
 
   DISALLOW_COPY_AND_ASSIGN(U2fDevice);
 };

device/fido/u2f_hid_device.cc

@@ -60,7 +60,7 @@ void U2fHidDevice::Transition(std::vector<uint8_t> command,
     case State::IDLE: {
       state_ = State::BUSY;
       ArmTimeout(repeating_callback);
-      // Write message to the device
+      // Write message to the device.
       WriteMessage(
           FidoHidMessage::Create(channel_id_, CtapHidDeviceCommand::kCtapHidMsg,
                                  std::move(command)),
@@ -75,15 +75,14 @@ void U2fHidDevice::Transition(std::vector<uint8_t> command,
     case State::DEVICE_ERROR:
     default:
       base::WeakPtr<U2fHidDevice> self = weak_factory_.GetWeakPtr();
-      repeating_callback.Run(false, base::nullopt);
-
+      repeating_callback.Run(base::nullopt);
       // Executing callbacks may free |this|. Check |self| first.
       while (self && !pending_transactions_.empty()) {
-        // Respond to any pending requests
+        // Respond to any pending requests.
         DeviceCallback pending_cb =
             std::move(pending_transactions_.front().second);
         pending_transactions_.pop();
-        std::move(pending_cb).Run(false, base::nullopt);
+        std::move(pending_cb).Run(base::nullopt);
       }
       break;
   }
@@ -112,7 +111,7 @@ void U2fHidDevice::OnConnect(std::vector<uint8_t> command,
 
 void U2fHidDevice::AllocateChannel(std::vector<uint8_t> command,
                                    DeviceCallback callback) {
-  // Send random nonce to device to verify received message
+  // Send random nonce to device to verify received message.
   std::vector<uint8_t> nonce(8);
   crypto::RandBytes(nonce.data(), nonce.size());
   WriteMessage(FidoHidMessage::Create(
@@ -228,7 +227,7 @@ void U2fHidDevice::OnRead(U2fHidMessageCallback callback,
     return;
   }
 
-  // Received a message from a different channel, so try again
+  // Received a message from a different channel, so try again.
   if (channel_id_ != read_message->channel_id()) {
     connection_->Read(base::BindOnce(&U2fHidDevice::OnRead,
                                      weak_factory_.GetWeakPtr(),
@@ -241,7 +240,7 @@ void U2fHidDevice::OnRead(U2fHidMessageCallback callback,
     return;
   }
 
-  // Continue reading additional packets
+  // Continue reading additional packets.
   connection_->Read(base::BindOnce(
       &U2fHidDevice::OnReadContinuation, weak_factory_.GetWeakPtr(),
       std::move(read_message), std::move(callback)));
@@ -281,18 +280,15 @@ void U2fHidDevice::MessageReceived(DeviceCallback callback,
     return;
   }
 
-  auto response =
-      message
-          ? apdu::ApduResponse::CreateFromMessage(message->GetMessagePayload())
-          : base::nullopt;
-
   state_ = State::IDLE;
   base::WeakPtr<U2fHidDevice> self = weak_factory_.GetWeakPtr();
-  std::move(callback).Run(success, std::move(response));
+  std::move(callback).Run(
+      (success && message) ? base::make_optional(message->GetMessagePayload())
+                           : base::nullopt);
 
   // Executing |callback| may have freed |this|. Check |self| first.
   if (self && !pending_transactions_.empty()) {
-    // If any transactions were queued, process the first one
+    // If any transactions were queued, process the first one.
     auto pending_cmd = std::move(pending_transactions_.front().first);
     auto pending_cb = std::move(pending_transactions_.front().second);
     pending_transactions_.pop();
@@ -301,7 +297,7 @@ void U2fHidDevice::MessageReceived(DeviceCallback callback,
 }
 
 void U2fHidDevice::TryWink(WinkCallback callback) {
-  // Only try to wink if device claims support
+  // Only try to wink if device claims support.
   if (!(capabilities_ & kWinkCapability) || state_ != State::IDLE) {
     std::move(callback).Run();
     return;
@@ -326,7 +322,7 @@ void U2fHidDevice::ArmTimeout(DeviceCallback callback) {
   timeout_callback_.Reset(base::BindOnce(&U2fHidDevice::OnTimeout,
                                          weak_factory_.GetWeakPtr(),
                                          std::move(callback)));
-  // Setup timeout task for 3 seconds
+  // Setup timeout task for 3 seconds.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, timeout_callback_.callback(), kDeviceTimeout);
 }

device/fido/u2f_hid_device.h

@@ -11,9 +11,12 @@
 #include <utility>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/cancelable_callback.h"
 #include "base/component_export.h"
 #include "base/macros.h"
+#include "components/apdu/apdu_command.h"
+#include "components/apdu/apdu_response.h"
 #include "device/fido/u2f_device.h"
 #include "services/device/public/mojom/hid.mojom.h"
 
@@ -27,22 +30,24 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fHidDevice : public U2fDevice {
                device::mojom::HidManager* hid_manager);
   ~U2fHidDevice() final;
 
-  // Send a U2f command to this device
+  // Send a command to this device.
   void DeviceTransact(std::vector<uint8_t> command,
                       DeviceCallback callback) final;
-  // Send a wink command if supported
+  // Send a wink command if supported.
   void TryWink(WinkCallback callback) final;
-  // Use a string identifier to compare to other devices
+  // Use a string identifier to compare to other devices.
   std::string GetId() const final;
-  // Get a string identifier for a given device info
+
+  // Get a string identifier for a given device info.
   static std::string GetIdForDevice(
       const device::mojom::HidDeviceInfo& device_info);
-  // Command line flag to enable tests on actual U2f HID hardware
+  // Command line flag to enable tests on actual HID hardware.
   static bool IsTestEnabled();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(U2fHidDeviceTest, TestConnectionFailure);
   FRIEND_TEST_ALL_PREFIXES(U2fHidDeviceTest, TestDeviceError);
+  FRIEND_TEST_ALL_PREFIXES(U2fHidDeviceTest, TestRetryChannelAllocation);
 
   static constexpr uint8_t kWinkCapability = 0x01;
   static constexpr uint8_t kLockCapability = 0x02;
@@ -55,12 +60,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fHidDevice : public U2fDevice {
       base::OnceCallback<void(bool, std::unique_ptr<FidoHidMessage>)>;
   using ConnectCallback = device::mojom::HidManager::ConnectCallback;
 
-  // Open a connection to this device
+  // Open a connection to this device.
   void Connect(ConnectCallback callback);
   void OnConnect(std::vector<uint8_t> command,
                  DeviceCallback callback,
                  device::mojom::HidConnectionPtr connection);
-  // Ask device to allocate a unique channel id for this connection
+  // Ask device to allocate a unique channel id for this connection.
   void AllocateChannel(std::vector<uint8_t> command, DeviceCallback callback);
   void OnAllocateChannel(std::vector<uint8_t> nonce,
                          std::vector<uint8_t> command,
@@ -68,7 +73,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fHidDevice : public U2fDevice {
                          bool success,
                          std::unique_ptr<FidoHidMessage> message);
   void Transition(std::vector<uint8_t> command, DeviceCallback callback);
-  // Write all message packets to device, and read response if expected
+  // Write all message packets to device, and read response if expected.
   void WriteMessage(std::unique_ptr<FidoHidMessage> message,
                     bool response_expected,
                     U2fHidMessageCallback callback);
@@ -76,7 +81,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fHidDevice : public U2fDevice {
                      bool response_expected,
                      U2fHidMessageCallback callback,
                      bool success);
-  // Read all response message packets from device
+  // Read all response message packets from device.
   void ReadMessage(U2fHidMessageCallback callback);
   void MessageReceived(DeviceCallback callback,
                        bool success,
@@ -95,8 +100,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fHidDevice : public U2fDevice {
               std::unique_ptr<FidoHidMessage> response);
   void ArmTimeout(DeviceCallback callback);
   void OnTimeout(DeviceCallback callback);
-  void OnDeviceTransact(bool success,
-                        base::Optional<apdu::ApduResponse> response);
   base::WeakPtr<U2fDevice> GetWeakPtr() override;
 
   uint32_t channel_id_ = kBroadcastChannel;

device/fido/u2f_hid_device_unittest.cc

@@ -9,13 +9,11 @@
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/test/scoped_mock_time_message_loop_task_runner.h"
 #include "base/test/scoped_task_environment.h"
 #include "components/apdu/apdu_command.h"
 #include "components/apdu/apdu_response.h"
 #include "device/fido/fake_hid_impl_for_testing.h"
 #include "device/fido/test_callback_receiver.h"
-#include "device/fido/u2f_command_type.h"
 #include "device/fido/u2f_hid_device.h"
 #include "device/fido/u2f_request.h"
 #include "mojo/public/cpp/bindings/binding.h"
@@ -58,7 +56,7 @@ std::vector<uint8_t> CreateMockHidInitResponse(
     std::vector<uint8_t> nonce,
     std::vector<uint8_t> channel_id) {
   // 4 bytes of broadcast channel identifier(ffffffff), followed by
-  // HID_INIT command(86) and 2 byte payload length(11)
+  // HID_INIT command(86) and 2 byte payload length(11).
   return MakePacket("ffffffff860011" + HexEncode(nonce) +
                     HexEncode(channel_id));
 }
@@ -71,12 +69,12 @@ std::vector<uint8_t> CreateMockVersionResponse(
   return MakePacket(HexEncode(channel_id) + "8300085532465f56329000");
 }
 
-// Returns a failure mock response to version request with given channel id.
-std::vector<uint8_t> CreateFailureMockVersionResponse(
-    std::vector<uint8_t> channel_id) {
-  // HID_MSG command(83), followed by payload length(0002), followed by
-  // an invalid class response code (6E00).
-  return MakePacket(HexEncode(channel_id) + "8300026E00");
+// Returns U2F_V2 version response formatted in APDU response encoding.
+std::vector<uint8_t> GetValidU2fVersionResponse() {
+  return apdu::ApduResponse(std::vector<uint8_t>(kU2fVersionResponse.begin(),
+                                                 kU2fVersionResponse.end()),
+                            apdu::ApduResponse::Status::SW_NO_ERROR)
+      .GetEncodedResponse();
 }
 
 device::mojom::HidDeviceInfoPtr TestHidDevice() {
@@ -122,10 +120,8 @@ class U2fDeviceEnumerateCallbackReceiver
   DISALLOW_COPY_AND_ASSIGN(U2fDeviceEnumerateCallbackReceiver);
 };
 
-using TestVersionCallbackReceiver =
-    test::StatusAndValueCallbackReceiver<bool, U2fDevice::ProtocolVersion>;
-using TestDeviceCallbackReceiver = ::device::test::
-    StatusAndValueCallbackReceiver<bool, base::Optional<apdu::ApduResponse>>;
+using TestDeviceCallbackReceiver =
+    ::device::test::TestCallbackReceiver<base::Optional<std::vector<uint8_t>>>;
 
 }  // namespace
 
@@ -146,43 +142,6 @@ class U2fHidDeviceTest : public ::testing::Test {
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 };
 
-TEST_F(U2fHidDeviceTest, TestHidDeviceVersion) {
-  if (!U2fHidDevice::IsTestEnabled())
-    return;
-
-  U2fDeviceEnumerateCallbackReceiver receiver(hid_manager_.get());
-  hid_manager_->GetDevices(receiver.callback());
-  receiver.WaitForCallback();
-
-  for (auto& device : receiver.TakeReturnedDevicesFiltered()) {
-    TestVersionCallbackReceiver vc;
-    device->Version(vc.callback());
-    vc.WaitForCallback();
-    EXPECT_EQ(U2fDevice::ProtocolVersion::U2F_V2, vc.value());
-  }
-}
-
-TEST_F(U2fHidDeviceTest, TestMultipleRequests) {
-  if (!U2fHidDevice::IsTestEnabled())
-    return;
-
-  U2fDeviceEnumerateCallbackReceiver receiver(hid_manager_.get());
-  hid_manager_->GetDevices(receiver.callback());
-  receiver.WaitForCallback();
-
-  for (auto& device : receiver.TakeReturnedDevicesFiltered()) {
-    TestVersionCallbackReceiver vc;
-    TestVersionCallbackReceiver vc2;
-    // Call version twice to check message queueing.
-    device->Version(vc.callback());
-    device->Version(vc2.callback());
-    vc.WaitForCallback();
-    EXPECT_EQ(U2fDevice::ProtocolVersion::U2F_V2, vc.value());
-    vc2.WaitForCallback();
-    EXPECT_EQ(U2fDevice::ProtocolVersion::U2F_V2, vc2.value());
-  }
-}
-
 TEST_F(U2fHidDeviceTest, TestConnectionFailure) {
   // Setup and enumerate mock device.
   U2fDeviceEnumerateCallbackReceiver receiver(hid_manager_.get());
@@ -214,9 +173,10 @@ TEST_F(U2fHidDeviceTest, TestConnectionFailure) {
                          receiver_3.callback());
 
   EXPECT_EQ(U2fHidDevice::State::DEVICE_ERROR, device->state_);
-  EXPECT_EQ(base::nullopt, receiver_1.value());
-  EXPECT_EQ(base::nullopt, receiver_2.value());
-  EXPECT_EQ(base::nullopt, receiver_3.value());
+
+  EXPECT_FALSE(std::get<0>(*receiver_1.result()));
+  EXPECT_FALSE(std::get<0>(*receiver_2.result()));
+  EXPECT_FALSE(std::get<0>(*receiver_3.result()));
 }
 
 TEST_F(U2fHidDeviceTest, TestDeviceError) {
@@ -241,7 +201,7 @@ TEST_F(U2fHidDeviceTest, TestDeviceError) {
   TestDeviceCallbackReceiver receiver_0;
   device->DeviceTransact(U2fRequest::GetU2fVersionApduCommand(),
                          receiver_0.callback());
-  EXPECT_EQ(base::nullopt, receiver_0.value());
+  EXPECT_FALSE(std::get<0>(*receiver_0.result()));
   EXPECT_EQ(U2fHidDevice::State::DEVICE_ERROR, device->state_);
 
   // Add pending transactions manually and ensure they are processed.
@@ -258,84 +218,9 @@ TEST_F(U2fHidDeviceTest, TestDeviceError) {
   FakeHidConnection::mock_connection_error_ = false;
 
   EXPECT_EQ(U2fHidDevice::State::DEVICE_ERROR, device->state_);
-  EXPECT_EQ(base::nullopt, receiver_1.value());
-  EXPECT_EQ(base::nullopt, receiver_2.value());
-  EXPECT_EQ(base::nullopt, receiver_3.value());
-}
-
-TEST_F(U2fHidDeviceTest, TestLegacyVersion) {
-  const std::vector<uint8_t> kChannelId = {0x01, 0x02, 0x03, 0x04};
-
-  auto hid_device = TestHidDevice();
-
-  // Replace device HID connection with custom client connection bound to mock
-  // server-side mojo connection.
-  device::mojom::HidConnectionPtr connection_client;
-  MockHidConnection mock_connection(
-      hid_device.Clone(), mojo::MakeRequest(&connection_client), kChannelId);
-
-  // Delegate custom functions to be invoked for mock hid connection.
-  EXPECT_CALL(mock_connection, WritePtr(_, _, _))
-      // HID_INIT request to authenticator for channel allocation.
-      .WillOnce(WithArgs<1, 2>(
-          Invoke([&](const std::vector<uint8_t>& buffer,
-                     device::mojom::HidConnection::WriteCallback* cb) {
-            mock_connection.SetNonce(base::make_span(buffer).subspan(7, 8));
-            std::move(*cb).Run(true);
-          })))
-
-      // HID_MSG request to authenticator for version request.
-      .WillOnce(WithArgs<2>(
-          Invoke([](device::mojom::HidConnection::WriteCallback* cb) {
-            std::move(*cb).Run(true);
-          })))
-
-      .WillOnce(WithArgs<2>(
-          Invoke([](device::mojom::HidConnection::WriteCallback* cb) {
-            std::move(*cb).Run(true);
-          })));
-
-  EXPECT_CALL(mock_connection, ReadPtr(_))
-      // Response to HID_INIT request with correct nonce.
-      .WillOnce(WithArg<0>(Invoke(
-          [&mock_connection](device::mojom::HidConnection::ReadCallback* cb) {
-            std::move(*cb).Run(true, 0,
-                               CreateMockHidInitResponse(
-                                   mock_connection.nonce(),
-                                   mock_connection.connection_channel_id()));
-          })))
-      // Invalid version response from the authenticator.
-      .WillOnce(WithArg<0>(Invoke(
-          [&mock_connection](device::mojom::HidConnection::ReadCallback* cb) {
-            std::move(*cb).Run(true, 0,
-                               CreateFailureMockVersionResponse(
-                                   mock_connection.connection_channel_id()));
-          })))
-      // Legacy version response from the authenticator.
-      .WillOnce(WithArg<0>(Invoke(
-          [&mock_connection](device::mojom::HidConnection::ReadCallback* cb) {
-            std::move(*cb).Run(true, 0,
-                               CreateMockVersionResponse(
-                                   mock_connection.connection_channel_id()));
-          })));
-
-  // Add device and set mock connection to fake hid manager.
-  fake_hid_manager_->AddDeviceAndSetConnection(std::move(hid_device),
-                                               std::move(connection_client));
-
-  U2fDeviceEnumerateCallbackReceiver receiver(hid_manager_.get());
-  hid_manager_->GetDevices(receiver.callback());
-  receiver.WaitForCallback();
-
-  std::vector<std::unique_ptr<U2fHidDevice>> u2f_devices =
-      receiver.TakeReturnedDevicesFiltered();
-
-  ASSERT_EQ(1u, u2f_devices.size());
-  auto& device = u2f_devices.front();
-  TestVersionCallbackReceiver vc;
-  device->Version(vc.callback());
-  vc.WaitForCallback();
-  EXPECT_EQ(U2fDevice::ProtocolVersion::U2F_V2, vc.value());
+  EXPECT_FALSE(std::get<0>(*receiver_1.result()));
+  EXPECT_FALSE(std::get<0>(*receiver_2.result()));
+  EXPECT_FALSE(std::get<0>(*receiver_3.result()));
 }
 
 TEST_F(U2fHidDeviceTest, TestRetryChannelAllocation) {
@@ -407,10 +292,15 @@ TEST_F(U2fHidDeviceTest, TestRetryChannelAllocation) {
 
   ASSERT_EQ(1u, u2f_devices.size());
   auto& device = u2f_devices.front();
-  TestVersionCallbackReceiver vc;
-  device->Version(vc.callback());
-  vc.WaitForCallback();
-  EXPECT_EQ(U2fDevice::ProtocolVersion::U2F_V2, vc.value());
+
+  TestDeviceCallbackReceiver cb;
+  device->DeviceTransact(U2fRequest::GetU2fVersionApduCommand(false),
+                         cb.callback());
+  cb.WaitForCallback();
+
+  const auto& result = std::get<0>(*cb.result());
+  ASSERT_TRUE(result);
+  EXPECT_THAT(*result, testing::ElementsAreArray(GetValidU2fVersionResponse()));
 }
 
 }  // namespace device

device/fido/u2f_register.cc

@@ -7,6 +7,8 @@
 #include <utility>
 
 #include "base/stl_util.h"
+#include "components/apdu/apdu_command.h"
+#include "components/apdu/apdu_response.h"
 #include "device/fido/register_response_data.h"
 #include "services/service_manager/public/cpp/connector.h"
 
@@ -51,12 +53,13 @@ void U2fRegister::TryDevice() {
   DCHECK(current_device_);
   if (!registered_keys_.empty() && !CheckedForDuplicateRegistration()) {
     auto it = registered_keys_.cbegin();
-    current_device_->Sign(GetU2fSignApduCommand(application_parameter_, *it,
-                                                true /* check_only */),
-                          base::BindOnce(&U2fRegister::OnTryCheckRegistration,
-                                         weak_factory_.GetWeakPtr(), it));
+    InitiateDeviceTransaction(
+        GetU2fSignApduCommand(application_parameter_, *it,
+                              true /* check_only */),
+        base::BindOnce(&U2fRegister::OnTryCheckRegistration,
+                       weak_factory_.GetWeakPtr(), it));
   } else {
-    current_device_->Register(
+    InitiateDeviceTransaction(
         GetU2fRegisterApduCommand(individual_attestation_ok_),
         base::BindOnce(&U2fRegister::OnTryDevice, weak_factory_.GetWeakPtr(),
                        false /* is_duplicate_registration */));
@@ -65,24 +68,30 @@ void U2fRegister::TryDevice() {
 
 void U2fRegister::OnTryCheckRegistration(
     std::vector<std::vector<uint8_t>>::const_iterator it,
-    U2fReturnCode return_code,
-    const std::vector<uint8_t>& response_data) {
+    base::Optional<std::vector<uint8_t>> response) {
+  const auto apdu_response =
+      response ? apdu::ApduResponse::CreateFromMessage(std::move(*response))
+               : base::nullopt;
+  auto return_code = apdu_response ? apdu_response->status()
+                                   : apdu::ApduResponse::Status::SW_WRONG_DATA;
+
   switch (return_code) {
-    case U2fReturnCode::SUCCESS:
-    case U2fReturnCode::CONDITIONS_NOT_SATISFIED:
+    case apdu::ApduResponse::Status::SW_NO_ERROR:
+    case apdu::ApduResponse::Status::SW_CONDITIONS_NOT_SATISFIED: {
       // Duplicate registration found. Call bogus registration to check for
       // user presence (touch) and terminate the registration process.
-      current_device_->Register(
+      InitiateDeviceTransaction(
           U2fRequest::GetBogusRegisterCommand(),
           base::BindOnce(&U2fRegister::OnTryDevice, weak_factory_.GetWeakPtr(),
                          true /* is_duplicate_registration */));
       break;
+    }
 
-    case U2fReturnCode::INVALID_PARAMS:
+    case apdu::ApduResponse::Status::SW_WRONG_DATA:
       // Continue to iterate through the provided key handles in the exclude
       // list and check for already registered keys.
       if (++it != registered_keys_.end()) {
-        current_device_->Sign(
+        InitiateDeviceTransaction(
             GetU2fSignApduCommand(application_parameter_, *it,
                                   true /* check_only */),
             base::BindOnce(&U2fRegister::OnTryCheckRegistration,
@@ -122,10 +131,14 @@ bool U2fRegister::CheckedForDuplicateRegistration() {
 }
 
 void U2fRegister::OnTryDevice(bool is_duplicate_registration,
-                              U2fReturnCode return_code,
-                              const std::vector<uint8_t>& response_data) {
+                              base::Optional<std::vector<uint8_t>> response) {
+  const auto apdu_response =
+      response ? apdu::ApduResponse::CreateFromMessage(std::move(*response))
+               : base::nullopt;
+  auto return_code = apdu_response ? apdu_response->status()
+                                   : apdu::ApduResponse::Status::SW_WRONG_DATA;
   switch (return_code) {
-    case U2fReturnCode::SUCCESS: {
+    case apdu::ApduResponse::Status::SW_NO_ERROR: {
       state_ = State::COMPLETE;
       if (is_duplicate_registration) {
         std::move(completion_callback_)
@@ -133,7 +146,7 @@ void U2fRegister::OnTryDevice(bool is_duplicate_registration,
         break;
       }
       auto response = RegisterResponseData::CreateFromU2fRegisterResponse(
-          application_parameter_, std::move(response_data));
+          application_parameter_, apdu_response->data());
       if (!response) {
         // The response data was corrupted / didn't parse properly.
         std::move(completion_callback_)
@@ -144,7 +157,7 @@ void U2fRegister::OnTryDevice(bool is_duplicate_registration,
           .Run(U2fReturnCode::SUCCESS, std::move(response));
       break;
     }
-    case U2fReturnCode::CONDITIONS_NOT_SATISFIED:
+    case apdu::ApduResponse::Status::SW_CONDITIONS_NOT_SATISFIED:
       // Waiting for user touch, move on and try this device later.
       state_ = State::IDLE;
       Transition();

device/fido/u2f_register.h

@@ -15,6 +15,7 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "device/fido/u2f_request.h"
+#include "device/fido/u2f_return_code.h"
 #include "device/fido/u2f_transport_protocol.h"
 
 namespace service_manager {
@@ -50,20 +51,16 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fRegister : public U2fRequest {
   ~U2fRegister() override;
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(U2fRegisterTest, TestCreateU2fRegisterCommand);
-
   void TryDevice() override;
   void OnTryDevice(bool is_duplicate_registration,
-                   U2fReturnCode return_code,
-                   const std::vector<uint8_t>& response_data);
+                   base::Optional<std::vector<uint8_t>> response);
 
   // Callback function called when non-empty exclude list was provided. This
   // function iterates through all key handles in |registered_keys_| for all
   // devices and checks for already registered keys.
   void OnTryCheckRegistration(
       std::vector<std::vector<uint8_t>>::const_iterator it,
-      U2fReturnCode return_code,
-      const std::vector<uint8_t>& response_data);
+      base::Optional<std::vector<uint8_t>> response);
   // Function handling registration flow after all devices were checked for
   // already registered keys.
   void CompleteNewDeviceRegistration();

device/fido/u2f_register_unittest.cc

@@ -439,6 +439,7 @@ TEST_F(U2fRegisterTest, TestRegisterSuccess) {
 
   register_callback_receiver().WaitForCallback();
   EXPECT_EQ(U2fReturnCode::SUCCESS, register_callback_receiver().status());
+  ASSERT_TRUE(register_callback_receiver().value());
   EXPECT_EQ(GetTestCredentialRawIdBytes(),
             register_callback_receiver().value()->raw_id());
 }
@@ -455,6 +456,7 @@ TEST_F(U2fRegisterTest, TestRegisterSuccessWithFake) {
   EXPECT_EQ(U2fReturnCode::SUCCESS, register_callback_receiver().status());
 
   // We don't verify the response from the fake, but do a quick sanity check.
+  ASSERT_TRUE(register_callback_receiver().value());
   EXPECT_EQ(32ul, register_callback_receiver().value()->raw_id().size());
 }
 
@@ -476,6 +478,7 @@ TEST_F(U2fRegisterTest, TestDelayedSuccess) {
 
   register_callback_receiver().WaitForCallback();
   EXPECT_EQ(U2fReturnCode::SUCCESS, register_callback_receiver().status());
+  ASSERT_TRUE(register_callback_receiver().value());
   EXPECT_EQ(GetTestCredentialRawIdBytes(),
             register_callback_receiver().value()->raw_id());
 }
@@ -506,6 +509,7 @@ TEST_F(U2fRegisterTest, TestMultipleDevices) {
 
   register_callback_receiver().WaitForCallback();
   EXPECT_EQ(U2fReturnCode::SUCCESS, register_callback_receiver().status());
+  ASSERT_TRUE(register_callback_receiver().value());
   EXPECT_EQ(GetTestCredentialRawIdBytes(),
             register_callback_receiver().value()->raw_id());
 }
@@ -544,6 +548,7 @@ TEST_F(U2fRegisterTest, TestSingleDeviceRegistrationWithExclusionList) {
 
   register_callback_receiver().WaitForCallback();
   EXPECT_EQ(U2fReturnCode::SUCCESS, register_callback_receiver().status());
+  ASSERT_TRUE(register_callback_receiver().value());
   EXPECT_EQ(GetTestCredentialRawIdBytes(),
             register_callback_receiver().value()->raw_id());
 }
@@ -596,6 +601,7 @@ TEST_F(U2fRegisterTest, TestMultipleDeviceRegistrationWithExclusionList) {
 
   register_callback_receiver().WaitForCallback();
   EXPECT_EQ(U2fReturnCode::SUCCESS, register_callback_receiver().status());
+  ASSERT_TRUE(register_callback_receiver().value());
   EXPECT_EQ(GetTestCredentialRawIdBytes(),
             register_callback_receiver().value()->raw_id());
 }
@@ -637,7 +643,7 @@ TEST_F(U2fRegisterTest, TestSingleDeviceRegistrationWithDuplicateHandle) {
   register_callback_receiver().WaitForCallback();
   EXPECT_EQ(U2fReturnCode::CONDITIONS_NOT_SATISFIED,
             register_callback_receiver().status());
-  EXPECT_EQ(base::nullopt, register_callback_receiver().value());
+  EXPECT_FALSE(register_callback_receiver().value());
 }
 
 // Tests a scenario where one (device1) of the two devices connected has created
@@ -689,7 +695,7 @@ TEST_F(U2fRegisterTest, TestMultipleDeviceRegistrationWithDuplicateHandle) {
   register_callback_receiver().WaitForCallback();
   EXPECT_EQ(U2fReturnCode::CONDITIONS_NOT_SATISFIED,
             register_callback_receiver().status());
-  EXPECT_EQ(base::nullopt, register_callback_receiver().value());
+  EXPECT_FALSE(register_callback_receiver().value());
 }
 
 // These test the parsing of the U2F raw bytes of the registration response.
@@ -825,6 +831,7 @@ TEST_F(U2fRegisterTest, TestIndividualAttestation) {
 
     cb.WaitForCallback();
     EXPECT_EQ(U2fReturnCode::SUCCESS, cb.status());
+    ASSERT_TRUE(cb.value());
     EXPECT_EQ(GetTestCredentialRawIdBytes(), cb.value()->raw_id());
   }
 }

device/fido/u2f_request.cc

@@ -13,6 +13,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "components/apdu/apdu_command.h"
+#include "components/apdu/apdu_response.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 namespace device {
@@ -119,7 +120,7 @@ void U2fRequest::Transition() {
     case State::IDLE:
       IterateDevice();
       if (!current_device_) {
-        // No devices available
+        // No devices available.
         state_ = State::OFF;
         break;
       }
@@ -136,6 +137,41 @@ void U2fRequest::Transition() {
   }
 }
 
+void U2fRequest::InitiateDeviceTransaction(
+    base::Optional<std::vector<uint8_t>> cmd,
+    U2fDevice::DeviceCallback callback) {
+  if (!cmd) {
+    std::move(callback).Run(base::nullopt);
+    return;
+  }
+  current_device_->DeviceTransact(std::move(*cmd), std::move(callback));
+}
+
+void U2fRequest::OnDeviceVersionRequest(
+    VersionCallback callback,
+    base::WeakPtr<U2fDevice> device,
+    bool legacy,
+    base::Optional<std::vector<uint8_t>> response) {
+  const auto apdu_response =
+      response ? apdu::ApduResponse::CreateFromMessage(std::move(*response))
+               : base::nullopt;
+  if (apdu_response &&
+      apdu_response->status() == apdu::ApduResponse::Status::SW_NO_ERROR &&
+      std::equal(apdu_response->data().cbegin(), apdu_response->data().cend(),
+                 kU2fVersionResponse.cbegin(), kU2fVersionResponse.cend())) {
+    std::move(callback).Run(U2fDevice::ProtocolVersion::U2F_V2);
+  } else if (!legacy) {
+    // Standard GetVersion failed, attempt legacy GetVersion command.
+    device->DeviceTransact(
+        GetU2fVersionApduCommand(true),
+        base::BindOnce(&U2fRequest::OnDeviceVersionRequest,
+                       weak_factory_.GetWeakPtr(), std::move(callback), device,
+                       true /* legacy */));
+  } else {
+    std::move(callback).Run(U2fDevice::ProtocolVersion::UNKNOWN);
+  }
+}
+
 void U2fRequest::DiscoveryStarted(U2fDiscovery* discovery, bool success) {
   if (success) {
     // The discovery might know about devices that have already been added to
@@ -215,7 +251,7 @@ void U2fRequest::IterateDevice() {
     devices_.pop_front();
   } else if (attempted_devices_.size() > 0) {
     devices_ = std::move(attempted_devices_);
-    // After trying every device, wait 200ms before trying again
+    // After trying every device, wait 200ms before trying again.
     delay_callback_.Reset(
         base::Bind(&U2fRequest::OnWaitComplete, weak_factory_.GetWeakPtr()));
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

device/fido/u2f_request.h

@@ -5,6 +5,8 @@
 #ifndef DEVICE_FIDO_U2F_REQUEST_H_
 #define DEVICE_FIDO_U2F_REQUEST_H_
 
+#include <stdint.h>
+
 #include <list>
 #include <memory>
 #include <string>
@@ -28,6 +30,9 @@ namespace device {
 
 class COMPONENT_EXPORT(DEVICE_FIDO) U2fRequest : public U2fDiscovery::Observer {
  public:
+  using VersionCallback =
+      base::OnceCallback<void(U2fDevice::ProtocolVersion version)>;
+
   // U2fRequest will create a discovery instance and register itself as an
   // observer for each passed in transport protocol.
   // TODO(https://crbug.com/769631): Remove the dependency on Connector once U2F
@@ -68,6 +73,17 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fRequest : public U2fDiscovery::Observer {
 
   void Transition();
 
+  // Starts sign, register, and version request transaction on
+  // |current_device_|.
+  void InitiateDeviceTransaction(base::Optional<std::vector<uint8_t>> cmd,
+                                 U2fDevice::DeviceCallback callback);
+  // Callback function to U2F version request. If non-legacy version request
+  // fails, retry with legacy version request.
+  void OnDeviceVersionRequest(VersionCallback callback,
+                              base::WeakPtr<U2fDevice> device,
+                              bool legacy,
+                              base::Optional<std::vector<uint8_t>> response);
+
   virtual void TryDevice() = 0;
 
   // Hold handles to the devices known to the system. Known devices are
@@ -94,6 +110,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fRequest : public U2fDiscovery::Observer {
   FRIEND_TEST_ALL_PREFIXES(U2fRequestTest, TestMultipleDiscoveries);
   FRIEND_TEST_ALL_PREFIXES(U2fRequestTest, TestSlowDiscovery);
   FRIEND_TEST_ALL_PREFIXES(U2fRequestTest, TestMultipleDiscoveriesWithFailures);
+  FRIEND_TEST_ALL_PREFIXES(U2fRequestTest, TestLegacyVersionRequest);
 
   // U2fDiscovery::Observer
   void DiscoveryStarted(U2fDiscovery* discovery, bool success) override;

device/fido/u2f_request_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/test/scoped_task_environment.h"
 #include "device/fido/fake_u2f_discovery.h"
 #include "device/fido/mock_u2f_device.h"
+#include "device/fido/test_callback_receiver.h"
 #include "device/fido/u2f_request.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -34,6 +35,9 @@ class FakeU2fRequest : public U2fRequest {
   }
 };
 
+using TestVersionCallback =
+    ::device::test::TestCallbackReceiver<U2fDevice::ProtocolVersion>;
+
 }  // namespace
 
 class U2fRequestTest : public ::testing::Test {
@@ -46,10 +50,15 @@ class U2fRequestTest : public ::testing::Test {
     return discovery_factory_;
   }
 
+  TestVersionCallback& version_callback_receiver() {
+    return version_callback_receiver_;
+  }
+
  private:
   base::test::ScopedTaskEnvironment scoped_task_environment_{
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME};
   test::ScopedFakeU2fDiscoveryFactory discovery_factory_;
+  TestVersionCallback version_callback_receiver_;
 };
 
 TEST_F(U2fRequestTest, TestIterateDevice) {
@@ -336,4 +345,34 @@ TEST_F(U2fRequestTest, TestEncodeVersionRequest) {
               ::testing::ElementsAreArray(kEncodedU2fLegacyVersionRequest));
 }
 
+// Test a scenario when version request is sent to legacy U2F token.
+// After non-legacy version requests fails, legacy version request should be
+// sent to device as a retry.
+TEST_F(U2fRequestTest, TestLegacyVersionRequest) {
+  auto* discovery = discovery_factory().ForgeNextHidDiscovery();
+  FakeU2fRequest request({U2fTransportProtocol::kUsbHumanInterfaceDevice});
+  request.Start();
+
+  auto device0 = std::make_unique<MockU2fDevice>();
+  EXPECT_CALL(*device0, GetId()).WillRepeatedly(::testing::Return("device0"));
+  EXPECT_CALL(*device0,
+              DeviceTransactPtr(U2fRequest::GetU2fVersionApduCommand(true), _))
+      // Success response for legacy version request after retry.
+      .WillOnce(testing::Invoke(MockU2fDevice::NoErrorVersion));
+
+  auto* device_ptr = device0.get();
+  discovery->AddDevice(std::move(device0));
+
+  // Represents version callback received from legacy U2F token on initial
+  // version request. Device responses with invalid protocol version (in this
+  // case, empty byte array). Retry version request with legacy bit is expected
+  // to be issued afterwards.
+  request.OnDeviceVersionRequest(version_callback_receiver().callback(),
+                                 device_ptr->GetWeakPtr(), false /* legacy */,
+                                 std::vector<uint8_t>());
+
+  EXPECT_EQ(U2fDevice::ProtocolVersion::U2F_V2,
+            std::get<0>(*version_callback_receiver().result()));
+}
+
 }  // namespace device

device/fido/u2f_sign.cc

@@ -6,6 +6,8 @@
 
 #include <utility>
 
+#include "components/apdu/apdu_command.h"
+#include "components/apdu/apdu_response.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 namespace device {
@@ -65,7 +67,7 @@ void U2fSign::TryDevice() {
   // https://fidoalliance.org/specs/fido-v2.0-ps-20170927/fido-client-to-authenticator-protocol-v2.0-ps-20170927.html
   if (registered_keys_.size() == 0) {
     // Send registration (Fake enroll) if no keys were provided.
-    current_device_->Register(
+    InitiateDeviceTransaction(
         U2fRequest::GetBogusRegisterCommand(),
         base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(),
                        registered_keys_.cend(),
@@ -74,18 +76,26 @@ void U2fSign::TryDevice() {
   }
   // Try signing current device with the first registered key.
   auto it = registered_keys_.cbegin();
-  current_device_->Sign(
+  InitiateDeviceTransaction(
       GetU2fSignApduCommand(application_parameter_, *it),
-      base::Bind(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(), it,
-                 ApplicationParameterType::kPrimary));
+      base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(), it,
+                     ApplicationParameterType::kPrimary));
 }
 
 void U2fSign::OnTryDevice(std::vector<std::vector<uint8_t>>::const_iterator it,
                           ApplicationParameterType application_parameter_type,
-                          U2fReturnCode return_code,
-                          const std::vector<uint8_t>& response_data) {
+                          base::Optional<std::vector<uint8_t>> response) {
+  const auto apdu_response =
+      response ? apdu::ApduResponse::CreateFromMessage(std::move(*response))
+               : base::nullopt;
+  auto return_code = apdu_response ? apdu_response->status()
+                                   : apdu::ApduResponse::Status::SW_WRONG_DATA;
+  auto response_data = return_code == apdu::ApduResponse::Status::SW_WRONG_DATA
+                           ? std::vector<uint8_t>()
+                           : apdu_response->data();
+
   switch (return_code) {
-    case U2fReturnCode::SUCCESS: {
+    case apdu::ApduResponse::Status::SW_NO_ERROR: {
       state_ = State::COMPLETE;
       if (it == registered_keys_.cend()) {
         // This was a response to a fake enrollment. Return an empty key handle.
@@ -108,33 +118,34 @@ void U2fSign::OnTryDevice(std::vector<std::vector<uint8_t>>::const_iterator it,
       }
       break;
     }
-    case U2fReturnCode::CONDITIONS_NOT_SATISFIED: {
+    case apdu::ApduResponse::Status::SW_CONDITIONS_NOT_SATISFIED: {
       // Key handle is accepted by this device, but waiting on user touch. Move
       // on and try this device again later.
       state_ = State::IDLE;
       Transition();
       break;
     }
-    case U2fReturnCode::INVALID_PARAMS: {
+    case apdu::ApduResponse::Status::SW_WRONG_DATA:
+    case apdu::ApduResponse::Status::SW_WRONG_LENGTH: {
       if (application_parameter_type == ApplicationParameterType::kPrimary &&
           alt_application_parameter_) {
         // |application_parameter_| failed, but there is also
         // |alt_application_parameter_| to try.
-        current_device_->Sign(
+        InitiateDeviceTransaction(
             GetU2fSignApduCommand(*alt_application_parameter_, *it),
             base::Bind(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(), it,
                        ApplicationParameterType::kAlternative));
       } else if (++it != registered_keys_.end()) {
         // Key is not for this device. Try signing with the next key.
-        current_device_->Sign(
+        InitiateDeviceTransaction(
             GetU2fSignApduCommand(application_parameter_, *it),
             base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(),
                            it, ApplicationParameterType::kPrimary));
       } else {
         // No provided key was accepted by this device. Send registration
         // (Fake enroll) request to device.
-        current_device_->Register(
-            GetBogusRegisterCommand(),
+        InitiateDeviceTransaction(
+            U2fRequest::GetBogusRegisterCommand(),
             base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(),
                            registered_keys_.cend(),
                            ApplicationParameterType::kPrimary));

device/fido/u2f_sign.h

@@ -15,6 +15,7 @@
 #include "base/optional.h"
 #include "device/fido/sign_response_data.h"
 #include "device/fido/u2f_request.h"
+#include "device/fido/u2f_return_code.h"
 #include "device/fido/u2f_transport_protocol.h"
 
 namespace service_manager {
@@ -48,8 +49,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fSign : public U2fRequest {
   ~U2fSign() override;
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(U2fSignTest, TestCreateSignApduCommand);
-
   // Enumerates the two types of |application_parameter| values used: the
   // "primary" value is the hash of the relying party ID[1] and is always
   // provided. The "alternative" value is the hash of a U2F AppID, specified in
@@ -66,8 +65,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fSign : public U2fRequest {
   void TryDevice() override;
   void OnTryDevice(std::vector<std::vector<uint8_t>>::const_iterator it,
                    ApplicationParameterType application_parameter_type,
-                   U2fReturnCode return_code,
-                   const std::vector<uint8_t>& response_data);
+                   base::Optional<std::vector<uint8_t>> response);
 
   base::Optional<std::vector<uint8_t>> alt_application_parameter_;
   SignResponseCallback completion_callback_;

device/fido/virtual_u2f_device.cc

@@ -145,9 +145,9 @@ void VirtualU2fDevice::DeviceTransact(std::vector<uint8_t> command,
       break;
     default:
       std::move(cb).Run(
-          true,
           apdu::ApduResponse(std::vector<uint8_t>(),
-                             apdu::ApduResponse::Status::SW_INS_NOT_SUPPORTED));
+                             apdu::ApduResponse::Status::SW_INS_NOT_SUPPORTED)
+              .GetEncodedResponse());
   }
 }
 
@@ -162,8 +162,9 @@ void VirtualU2fDevice::DoRegister(uint8_t ins,
                                   DeviceCallback cb) {
   if (data.size() != 64) {
     std::move(cb).Run(
-        true, apdu::ApduResponse(std::vector<uint8_t>(),
-                                 apdu::ApduResponse::Status::SW_WRONG_LENGTH));
+        apdu::ApduResponse(std::vector<uint8_t>(),
+                           apdu::ApduResponse::Status::SW_WRONG_LENGTH)
+            .GetEncodedResponse());
     return;
   }
 
@@ -223,9 +224,9 @@ void VirtualU2fDevice::DoRegister(uint8_t ins,
                   std::vector<uint8_t>(app_id_hash.begin(), app_id_hash.end()),
                   1);
 
-  std::move(cb).Run(
-      true, apdu::ApduResponse(std::move(response),
-                               apdu::ApduResponse::Status::SW_NO_ERROR));
+  std::move(cb).Run(apdu::ApduResponse(std::move(response),
+                                       apdu::ApduResponse::Status::SW_NO_ERROR)
+                        .GetEncodedResponse());
 }
 
 void VirtualU2fDevice::DoSign(uint8_t ins,
@@ -237,8 +238,9 @@ void VirtualU2fDevice::DoSign(uint8_t ins,
         p1 == kP1IndividualAttestation) ||
       p2 != 0) {
     std::move(cb).Run(
-        true, apdu::ApduResponse(std::vector<uint8_t>(),
-                                 apdu::ApduResponse::Status::SW_WRONG_DATA));
+        apdu::ApduResponse(std::vector<uint8_t>(),
+                           apdu::ApduResponse::Status::SW_WRONG_DATA)
+            .GetEncodedResponse());
     return;
   }
 
@@ -249,14 +251,16 @@ void VirtualU2fDevice::DoSign(uint8_t ins,
     // Our own keyhandles are always 32 bytes long, if the request has something
     // else then we already know it is not ours.
     std::move(cb).Run(
-        true, apdu::ApduResponse(std::vector<uint8_t>(),
-                                 apdu::ApduResponse::Status::SW_WRONG_DATA));
+        apdu::ApduResponse(std::vector<uint8_t>(),
+                           apdu::ApduResponse::Status::SW_WRONG_DATA)
+            .GetEncodedResponse());
     return;
   }
   if (data.size() != 32 + 32 + 1 + key_handle_length) {
     std::move(cb).Run(
-        true, apdu::ApduResponse(std::vector<uint8_t>(),
-                                 apdu::ApduResponse::Status::SW_WRONG_LENGTH));
+        apdu::ApduResponse(std::vector<uint8_t>(),
+                           apdu::ApduResponse::Status::SW_WRONG_LENGTH)
+            .GetEncodedResponse());
     return;
   }
   auto key_handle = data.last(key_handle_length);
@@ -267,8 +271,9 @@ void VirtualU2fDevice::DoSign(uint8_t ins,
 
   if (it == registrations_.end()) {
     std::move(cb).Run(
-        true, apdu::ApduResponse(std::vector<uint8_t>(),
-                                 apdu::ApduResponse::Status::SW_WRONG_DATA));
+        apdu::ApduResponse(std::vector<uint8_t>(),
+                           apdu::ApduResponse::Status::SW_WRONG_DATA)
+            .GetEncodedResponse());
     return;
   }
 
@@ -278,8 +283,9 @@ void VirtualU2fDevice::DoSign(uint8_t ins,
     // It's important this error looks identical to the previous one, as
     // tokens should not reveal the existence of keyHandles to unrelated appIds.
     std::move(cb).Run(
-        true, apdu::ApduResponse(std::vector<uint8_t>(),
-                                 apdu::ApduResponse::Status::SW_WRONG_DATA));
+        apdu::ApduResponse(std::vector<uint8_t>(),
+                           apdu::ApduResponse::Status::SW_WRONG_DATA)
+            .GetEncodedResponse());
     return;
   }
 
@@ -311,9 +317,9 @@ void VirtualU2fDevice::DoSign(uint8_t ins,
   // Add signature for full response.
   AppendTo(&response, sig);
 
-  std::move(cb).Run(
-      true, apdu::ApduResponse(std::move(response),
-                               apdu::ApduResponse::Status::SW_NO_ERROR));
+  std::move(cb).Run(apdu::ApduResponse(std::move(response),
+                                       apdu::ApduResponse::Status::SW_NO_ERROR)
+                        .GetEncodedResponse());
 }
 
 }  // namespace device