[fido] Introduce a list of abandoned devices in U2fRequest

This change adds a list of abandoned devices to U2fRequest. This list
gets populated, when the request gives up on a device due to an error.
Devices in this list will not be considered when IterateDevice() is
called.

Bug: NONE
Change-Id: I6760b956f2fefa31631379c181224b888ee32902
Reviewed-on: https://chromium-review.googlesource.com/1001897
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549450}

device/fido/u2f_register.cc

@@ -109,9 +109,7 @@ void U2fRegister::OnTryCheckRegistration(
       break;
     default:
       // Some sort of failure occurred. Abandon this device and move on.
-      state_ = State::IDLE;
-      current_device_ = nullptr;
-      Transition();
+      AbandonCurrentDeviceAndTransition();
       break;
   }
 }
@@ -164,10 +162,8 @@ void U2fRegister::OnTryDevice(bool is_duplicate_registration,
       Transition();
       break;
     default:
-      state_ = State::IDLE;
       // An error has occurred, quit trying this device.
-      current_device_ = nullptr;
-      Transition();
+      AbandonCurrentDeviceAndTransition();
       break;
   }
 }

device/fido/u2f_request.cc

@@ -176,6 +176,13 @@ void U2fRequest::OnDeviceVersionRequest(
   }
 }
 
+void U2fRequest::AbandonCurrentDeviceAndTransition() {
+  DCHECK_NE(nullptr, current_device_);
+  abandoned_devices_.emplace_back(std::exchange(current_device_, nullptr));
+  state_ = State::IDLE;
+  Transition();
+}
+
 void U2fRequest::DiscoveryStarted(FidoDiscovery* discovery, bool success) {
 #if DCHECK_IS_ON()
   if (success) {
@@ -189,6 +196,8 @@ void U2fRequest::DiscoveryStarted(FidoDiscovery* discovery, bool success) {
       device_ids_known_to_request.insert(current_device_->GetId());
     for (const auto* device : devices_)
       device_ids_known_to_request.insert(device->GetId());
+    for (const auto* device : abandoned_devices_)
+      device_ids_known_to_request.insert(device->GetId());
 
     std::set<std::string> device_ids_from_newly_started_discovery;
     for (const auto* device : discovery->GetDevices())
@@ -234,6 +243,7 @@ void U2fRequest::DeviceRemoved(FidoDiscovery* discovery, FidoDevice* device) {
   // Remove the device if it exists in either device list
   devices_.remove_if(device_id_eq);
   attempted_devices_.remove_if(device_id_eq);
+  abandoned_devices_.remove_if(device_id_eq);
 }
 
 void U2fRequest::IterateDevice() {

device/fido/u2f_request.h

@@ -92,17 +92,25 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fRequest
 
   virtual void TryDevice() = 0;
 
+  // Moves |current_device_| to the list of |abandoned_devices_| and iterates
+  // |current_device_|. Expects |current_device_| to be valid prior to calling
+  // this method.
+  void AbandonCurrentDeviceAndTransition();
+
   // Hold handles to the devices known to the system. Known devices are
-  // partitioned into three parts:
-  // [attempted_devices_), current_device_, [devices_)
+  // partitioned into four parts:
+  // [attempted_devices_), current_device_, [devices_), [abandoned_devices_)
   // During device iteration the |current_device_| gets pushed to
   // |attempted_devices_|, and, if possible, the first element of |devices_|
   // gets popped and becomes the new |current_device_|. Once all |devices_| are
   // exhausted, |attempted_devices_| get moved into |devices_| and
-  // |current_device_| is reset.
+  // |current_device_| is reset. |abandoned_devices_| contains a list of devices
+  // that have been tried in the past, but were abandoned because of an error.
+  // Devices in this list won't be tried again.
   FidoDevice* current_device_ = nullptr;
   std::list<FidoDevice*> devices_;
   std::list<FidoDevice*> attempted_devices_;
+  std::list<FidoDevice*> abandoned_devices_;
   State state_;
   std::vector<std::unique_ptr<FidoDiscovery>> discoveries_;
   std::vector<uint8_t> application_parameter_;
@@ -111,6 +119,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fRequest
 
  private:
   FRIEND_TEST_ALL_PREFIXES(U2fRequestTest, TestIterateDevice);
+  FRIEND_TEST_ALL_PREFIXES(U2fRequestTest,
+                           TestAbandonCurrentDeviceAndTransition);
   FRIEND_TEST_ALL_PREFIXES(U2fRequestTest, TestBasicMachine);
   FRIEND_TEST_ALL_PREFIXES(U2fRequestTest, TestAlreadyPresentDevice);
   FRIEND_TEST_ALL_PREFIXES(U2fRequestTest, TestMultipleDiscoveries);

device/fido/u2f_request_unittest.cc

@@ -106,6 +106,36 @@ TEST_F(U2fRequestTest, TestIterateDevice) {
   EXPECT_EQ(static_cast<size_t>(0), request.attempted_devices_.size());
 }
 
+TEST_F(U2fRequestTest, TestAbandonCurrentDeviceAndTransition) {
+  auto* discovery = discovery_factory().ForgeNextHidDiscovery();
+
+  FakeU2fRequest request({U2fTransportProtocol::kUsbHumanInterfaceDevice});
+  request.Start();
+
+  auto device = std::make_unique<MockFidoDevice>();
+  EXPECT_CALL(*device, GetId()).WillRepeatedly(::testing::Return("device"));
+
+  discovery->AddDevice(std::move(device));
+
+  // Move device to current.
+  request.IterateDevice();
+  EXPECT_NE(nullptr, request.current_device_);
+
+  // Abandon device.
+  request.AbandonCurrentDeviceAndTransition();
+  EXPECT_EQ(nullptr, request.current_device_);
+  EXPECT_EQ(1u, request.abandoned_devices_.size());
+
+  // Iterating through the device list should not change the state.
+  request.IterateDevice();
+  EXPECT_EQ(nullptr, request.current_device_);
+  EXPECT_EQ(1u, request.abandoned_devices_.size());
+
+  // Removing the device from the discovery should clear it from the list.
+  discovery->RemoveDevice("device");
+  EXPECT_TRUE(request.abandoned_devices_.empty());
+}
+
 TEST_F(U2fRequestTest, TestBasicMachine) {
   auto* discovery = discovery_factory().ForgeNextHidDiscovery();
   FakeU2fRequest request({U2fTransportProtocol::kUsbHumanInterfaceDevice});

device/fido/u2f_sign.cc

@@ -138,9 +138,7 @@ void U2fSign::OnTryDevice(std::vector<std::vector<uint8_t>>::const_iterator it,
                        ApplicationParameterType::kAlternative));
       } else if (it == registered_keys_.cend()) {
         // The fake enrollment errored out. Move on to the next device.
-        state_ = State::IDLE;
-        current_device_ = nullptr;
-        Transition();
+        AbandonCurrentDeviceAndTransition();
       } else if (++it != registered_keys_.end()) {
         // Key is not for this device. Try signing with the next key.
         InitiateDeviceTransaction(
@@ -160,9 +158,7 @@ void U2fSign::OnTryDevice(std::vector<std::vector<uint8_t>>::const_iterator it,
     }
     default:
       // Some sort of failure occured. Abandon this device and move on.
-      state_ = State::IDLE;
-      current_device_ = nullptr;
-      Transition();
+      AbandonCurrentDeviceAndTransition();
       break;
   }
 }