Fix Cancel operation on FidoHidDevice

Current Cancel operation on FidoHidDevice does not conform to behavior
defined by the CTAP spec. More specifically, CTAP spec mandates that once
Cancel operation is invoked, all pending transactions must be deleted
and cancel command must be sent before any remaining transactions. This
is not how current FidoHidDevice works. Currently FidoHidDevice will
simply add Cancel command packet into a queue and wait for prior
transaction to finish before sending Cancel command to the device.

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

device/fido/fido_hid_device.cc

@@ -15,10 +15,6 @@
 
 namespace device {
 
-namespace switches {
-static constexpr char kEnableU2fHidTest[] = "enable-u2f-hid-tests";
-}  // namespace switches
-
 namespace {
 // U2F devices only provide a single report so specify a report ID of 0 here.
 static constexpr uint8_t kReportId = 0x00;
@@ -43,7 +39,12 @@ void FidoHidDevice::Cancel() {
   if (state_ != State::kBusy && state_ != State::kReady)
     return;
 
-  Transition(std::vector<uint8_t>(), base::DoNothing());
+  // Delete any remaining pending requests on this Channel ID.
+  pending_transactions_ = {};
+  WriteMessage(
+      FidoHidMessage::Create(channel_id_, FidoHidDeviceCommand::kCancel,
+                             std::vector<uint8_t>()),
+      false /* response_expected */, base::DoNothing());
 }
 
 void FidoHidDevice::Transition(std::vector<uint8_t> command,
@@ -69,18 +70,6 @@ void FidoHidDevice::Transition(std::vector<uint8_t> command,
       state_ = State::kBusy;
       ArmTimeout(repeating_callback);
 
-      // If cancel command has been received, send HID_CANCEL with no-op
-      // callback.
-      // TODO(hongjunchoi): Re-factor cancel logic and consolidate it with
-      // FidoBleDevice::Cancel().
-      if (command.empty()) {
-        WriteMessage(
-            FidoHidMessage::Create(channel_id_, FidoHidDeviceCommand::kCancel,
-                                   std::move(command)),
-            false, base::DoNothing());
-        return;
-      }
-
       // Write message to the device.
       const auto command_type = supported_protocol() == ProtocolVersion::kCtap
                                     ? FidoHidDeviceCommand::kCbor
@@ -394,12 +383,6 @@ std::string FidoHidDevice::GetIdForDevice(
   return "hid:" + device_info.guid;
 }
 
-// static
-bool FidoHidDevice::IsTestEnabled() {
-  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-  return command_line->HasSwitch(switches::kEnableU2fHidTest);
-}
-
 base::WeakPtr<FidoDevice> FidoHidDevice::GetWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }

device/fido/fido_hid_device.h

@@ -5,7 +5,6 @@
 #ifndef DEVICE_FIDO_FIDO_HID_DEVICE_H_
 #define DEVICE_FIDO_FIDO_HID_DEVICE_H_
 
-#include <queue>
 #include <string>
 #include <utility>
 #include <vector>
@@ -13,6 +12,7 @@
 #include "base/callback.h"
 #include "base/cancelable_callback.h"
 #include "base/component_export.h"
+#include "base/containers/queue.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "components/apdu/apdu_command.h"
@@ -45,13 +45,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidDevice : public FidoDevice {
   // 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 HID hardware.
-  static bool IsTestEnabled();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(FidoHidDeviceTest, TestConnectionFailure);
   FRIEND_TEST_ALL_PREFIXES(FidoHidDeviceTest, TestDeviceError);
   FRIEND_TEST_ALL_PREFIXES(FidoHidDeviceTest, TestRetryChannelAllocation);
+  FRIEND_TEST_ALL_PREFIXES(FidoHidDeviceTest, TestCancel);
 
   static constexpr uint8_t kWinkCapability = 0x01;
   static constexpr uint8_t kLockCapability = 0x02;
@@ -105,7 +104,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidDevice : public FidoDevice {
   uint8_t capabilities_ = 0;
 
   base::CancelableOnceClosure timeout_callback_;
-  std::queue<std::pair<std::vector<uint8_t>, DeviceCallback>>
+  base::queue<std::pair<std::vector<uint8_t>, DeviceCallback>>
       pending_transactions_;
 
   // All the FidoHidDevice instances are owned by U2fRequest. So it is safe to

device/fido/fido_hid_device_unittest.cc

@@ -492,7 +492,11 @@ TEST_F(FidoHidDeviceTest, TestCancel) {
   device->set_supported_protocol(ProtocolVersion::kCtap);
   TestDeviceCallbackReceiver cb;
   device->DeviceTransact(GetMockDeviceRequest(), cb.callback());
-  device->Cancel();
+  auto delay_before_cancel = base::TimeDelta::FromSeconds(1);
+  auto cancel_callback = base::BindOnce(&FidoHidDevice::Cancel,
+                                        device->weak_factory_.GetWeakPtr());
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, std::move(cancel_callback), delay_before_cancel);
   scoped_task_environment_.FastForwardUntilNoTasksRemain();
 }