device/fido: don't use the caBLE transport as a signal.

Previously, having the caBLE transport in the list of transports was
special and indicated that a caBLE discovery should be created. Also,
unlike all over discoveries, the caBLE discovery was created in the
GetAssertion request handler, not the base class.

This change makes the caBLE transport no longer special because, in the
future, we'll want to create a caBLE discovery without the extension. It
also regularises the handling of the caBLE transport to match all other
transports. As a consequence, the caBLE discovery data becomes the
property of the FidoDiscoveryFactory and two tests become inapplicable
at the level at which they were being tested.

Change-Id: Ibcd758fa9304a7cb8a868b1bcf2c692c15f72221
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1756951
Auto-Submit: Adam Langley <agl@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691798}

content/browser/webauth/authenticator_common.cc

@@ -589,6 +589,11 @@ void AuthenticatorCommon::StartGetAssertionRequest() {
   if (!discovery_factory)
     discovery_factory = request_delegate_->GetDiscoveryFactory();
 
+  if (ctap_get_assertion_request_->cable_extension) {
+    discovery_factory->set_cable_data(
+        *ctap_get_assertion_request_->cable_extension);
+  }
+
   request_ = std::make_unique<device::GetAssertionRequestHandler>(
       connector_, discovery_factory, GetTransports(caller_origin_, transports_),
       *ctap_get_assertion_request_,

content/browser/webauth/authenticator_impl_unittest.cc

@@ -1689,6 +1689,7 @@ class TestAuthenticatorRequestDelegate
       override {
     ASSERT_TRUE(action_callbacks_registered_callback_)
         << "RegisterActionCallbacks called twice.";
+    cancel_callback_.emplace(std::move(cancel_callback));
     std::move(action_callbacks_registered_callback_).Run();
   }
 
@@ -1711,7 +1712,19 @@ class TestAuthenticatorRequestDelegate
 
   bool IsFocused() override { return is_focused_; }
 
+  void OnTransportAvailabilityEnumerated(
+      device::FidoRequestHandlerBase::TransportAvailabilityInfo transport_info)
+      override {
+    // Simulate the behaviour of Chrome's |AuthenticatorRequestDialogModel|
+    // which shows a specific error when no transports are available and lets
+    // the user cancel the request.
+    if (transport_info.available_transports.empty()) {
+      std::move(*cancel_callback_).Run();
+    }
+  }
+
   base::OnceClosure action_callbacks_registered_callback_;
+  base::Optional<base::OnceClosure> cancel_callback_;
   const IndividualAttestation individual_attestation_;
   const AttestationConsent attestation_consent_;
   const bool is_focused_;
@@ -2430,6 +2443,35 @@ TEST_F(AuthenticatorContentBrowserClientTest,
   EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
 }
 
+TEST_F(AuthenticatorContentBrowserClientTest,
+       CableCredentialWithoutCableExtension) {
+  // Exercise the case where a credential is marked as "cable" but no caBLE
+  // extension is provided. The AuthenticatorRequestClientDelegate should see no
+  // transports, which triggers it to cancel the request. (Outside of a testing
+  // environment, Chrome's AuthenticatorRequestClientDelegate will show an
+  // informative error and wait for the user to cancel the request.)
+  EnableFeature(features::kWebAuthCable);
+  TestServiceManagerContext service_manager_context;
+  SimulateNavigation(GURL(kTestOrigin1));
+
+  PublicKeyCredentialRequestOptionsPtr options =
+      GetTestPublicKeyCredentialRequestOptions();
+  std::vector<uint8_t> id(32u, 1u);
+  base::flat_set<device::FidoTransportProtocol> transports{
+      device::FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy};
+  options->allow_credentials.clear();
+  options->allow_credentials.emplace_back(device::CredentialType::kPublicKey,
+                                          std::move(id), std::move(transports));
+
+  TestGetAssertionCallback callback_receiver;
+  mojo::Remote<blink::mojom::Authenticator> authenticator =
+      ConnectToAuthenticator();
+  authenticator->GetAssertion(std::move(options), callback_receiver.callback());
+  callback_receiver.WaitForCallback();
+
+  EXPECT_EQ(AuthenticatorStatus::NOT_ALLOWED_ERROR, callback_receiver.status());
+}
+
 class MockAuthenticatorRequestDelegateObserver
     : public TestAuthenticatorRequestDelegate {
  public:

content/browser/webauth/virtual_fido_discovery_factory.cc

@@ -123,13 +123,6 @@ VirtualFidoDiscoveryFactory::Create(device::FidoTransportProtocol transport,
   return discovery;
 }
 
-std::unique_ptr<::device::FidoDiscoveryBase>
-VirtualFidoDiscoveryFactory::CreateCable(
-    std::vector<device::CableDiscoveryData> cable_data) {
-  return Create(device::FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy,
-                nullptr);
-}
-
 void VirtualFidoDiscoveryFactory::CreateAuthenticator(
     blink::test::mojom::VirtualAuthenticatorOptionsPtr options,
     CreateAuthenticatorCallback callback) {

content/browser/webauth/virtual_fido_discovery_factory.h

@@ -70,8 +70,6 @@ class CONTENT_EXPORT VirtualFidoDiscoveryFactory
   std::unique_ptr<::device::FidoDiscoveryBase> Create(
       device::FidoTransportProtocol transport,
       ::service_manager::Connector* connector) override;
-  std::unique_ptr<::device::FidoDiscoveryBase> CreateCable(
-      std::vector<device::CableDiscoveryData> cable_data) override;
 
  protected:
   // blink::test::mojom::VirtualAuthenticatorManager:

device/fido/fake_fido_discovery.cc

@@ -98,8 +98,7 @@ std::unique_ptr<FidoDiscoveryBase> FakeFidoDiscoveryFactory::Create(
     case FidoTransportProtocol::kBluetoothLowEnergy:
       return std::move(next_ble_discovery_);
     case FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy:
-      NOTREACHED() << "CaBLE should be handled by CreateCable().";
-      return nullptr;
+      return std::move(next_cable_discovery_);
     case FidoTransportProtocol::kInternal:
       return std::move(next_platform_discovery_);
   }
@@ -107,11 +106,6 @@ std::unique_ptr<FidoDiscoveryBase> FakeFidoDiscoveryFactory::Create(
   return nullptr;
 }
 
-std::unique_ptr<FidoDiscoveryBase> FakeFidoDiscoveryFactory::CreateCable(
-    std::vector<CableDiscoveryData> cable_data) {
-  return std::move(next_cable_discovery_);
-}
-
 }  // namespace test
 
 }  // namespace device

device/fido/fake_fido_discovery.h

@@ -126,8 +126,6 @@ class FakeFidoDiscoveryFactory : public device::FidoDiscoveryFactory {
   std::unique_ptr<FidoDiscoveryBase> Create(
       FidoTransportProtocol transport,
       ::service_manager::Connector* connector) override;
-  std::unique_ptr<FidoDiscoveryBase> CreateCable(
-      std::vector<CableDiscoveryData> cable_data) override;
 
  private:
   std::unique_ptr<FakeFidoDiscovery> next_hid_discovery_;

device/fido/fido_discovery_factory.cc

@@ -55,8 +55,9 @@ std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create(
     case FidoTransportProtocol::kBluetoothLowEnergy:
       return std::make_unique<FidoBleDiscovery>();
     case FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy:
-      NOTREACHED() << "Cable discovery is constructed using the dedicated "
-                      "factory method.";
+      if (cable_data_) {
+        return std::make_unique<FidoCableDiscovery>(*cable_data_);
+      }
       return nullptr;
     case FidoTransportProtocol::kNearFieldCommunication:
       // TODO(https://crbug.com/825949): Add NFC support.
@@ -75,9 +76,9 @@ std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create(
   return nullptr;
 }
 
-std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::CreateCable(
+void FidoDiscoveryFactory::set_cable_data(
     std::vector<CableDiscoveryData> cable_data) {
-  return std::make_unique<FidoCableDiscovery>(std::move(cable_data));
+  cable_data_.emplace(std::move(cable_data));
 }
 
 #if defined(OS_WIN)

device/fido/fido_discovery_factory.h

@@ -34,17 +34,16 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory {
   FidoDiscoveryFactory();
   virtual ~FidoDiscoveryFactory();
 
-  // Instantiates a FidoDiscoveryBase for all protocols except caBLE and
-  // internal/platform.
+  // Instantiates a FidoDiscoveryBase for the given transport.
   //
   // FidoTransportProtocol::kUsbHumanInterfaceDevice requires specifying a
   // valid |connector| on Desktop, and is not valid on Android.
   virtual std::unique_ptr<FidoDiscoveryBase> Create(
       FidoTransportProtocol transport,
       ::service_manager::Connector* connector);
-  // Instantiates a FidoDiscovery for caBLE.
-  virtual std::unique_ptr<FidoDiscoveryBase> CreateCable(
-      std::vector<CableDiscoveryData> cable_data);
+
+  // set_cable_data configures caBLE obtained via a WebAuthn extension.
+  void set_cable_data(std::vector<CableDiscoveryData> cable_data);
 
 #if defined(OS_MACOSX)
   // Configures the Touch ID authenticator. Set to base::nullopt to disable it.
@@ -64,6 +63,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory {
 #if defined(OS_MACOSX)
   base::Optional<fido::mac::AuthenticatorConfig> mac_touch_id_config_;
 #endif  // defined(OS_MACOSX)
+  base::Optional<std::vector<CableDiscoveryData>> cable_data_;
 };
 
 }  // namespace device

device/fido/fido_request_handler_base.cc

@@ -81,35 +81,27 @@ void FidoRequestHandlerBase::InitDiscoveries(
 
   transport_availability_info_.available_transports = available_transports;
   for (const auto transport : available_transports) {
-    // Construction of CaBleDiscovery is handled by the implementing class as it
-    // requires an extension passed on from the relying party.
-    if (transport == FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy)
-      continue;
-
     std::unique_ptr<FidoDiscoveryBase> discovery =
         fido_discovery_factory_->Create(transport, connector_);
-    if (transport == FidoTransportProtocol::kInternal) {
-      if (discovery) {
-        ++transport_info_callback_count;
-      } else {
-        // The platform authenticator is not configured for this request.
-        transport_availability_info_.available_transports.erase(
-            FidoTransportProtocol::kInternal);
-      }
-    }
     if (discovery == nullptr) {
       // This can occur in tests when a ScopedVirtualU2fDevice is in effect and
-      // HID transports are not configured.
+      // HID transports are not configured or when caBLE discovery data isn't
+      // available.
+      transport_availability_info_.available_transports.erase(transport);
       continue;
     }
 
+    if (transport == FidoTransportProtocol::kInternal) {
+      ++transport_info_callback_count;
+    }
+
     discovery->set_observer(this);
     discoveries_.push_back(std::move(discovery));
   }
 
-  if (base::Contains(available_transports,
+  if (base::Contains(transport_availability_info_.available_transports,
                      FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy) ||
-      base::Contains(available_transports,
+      base::Contains(transport_availability_info_.available_transports,
                      FidoTransportProtocol::kBluetoothLowEnergy)) {
     ++transport_info_callback_count;
     base::SequencedTaskRunnerHandle::Get()->PostTask(
@@ -157,9 +149,10 @@ void FidoRequestHandlerBase::InitDiscoveriesWin(
   // caBLE). Otherwise, do not instantiate any other transports.
   base::flat_set<FidoTransportProtocol> other_transports = {};
   if (base::Contains(available_transports,
-                     FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy))
+                     FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy)) {
     other_transports = {
         FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy};
+  }
 
   InitDiscoveries(other_transports);
 }

device/fido/get_assertion_handler_unittest.cc

@@ -466,41 +466,6 @@ TEST_F(FidoGetAssertionHandlerTest,
                               FidoTransportProtocol::kNearFieldCommunication});
 }
 
-TEST_F(FidoGetAssertionHandlerTest,
-       CableDisabledIfAllowCredentialsListUndefinedButCableExtensionMissing) {
-  CtapGetAssertionRequest request(test_data::kRelyingPartyId,
-                                  test_data::kClientDataJson);
-  ASSERT_FALSE(!!request.cable_extension);
-  EXPECT_CALL(*mock_adapter_, IsPresent()).WillOnce(::testing::Return(true));
-  auto request_handler =
-      CreateGetAssertionHandlerWithRequest(std::move(request));
-  ExpectAllowedTransportsForRequestAre(
-      request_handler.get(), {FidoTransportProtocol::kBluetoothLowEnergy,
-                              FidoTransportProtocol::kUsbHumanInterfaceDevice,
-                              FidoTransportProtocol::kNearFieldCommunication,
-                              FidoTransportProtocol::kInternal});
-}
-
-TEST_F(FidoGetAssertionHandlerTest,
-       CableDisabledIfExplicitlyAllowedButCableExtensionMissing) {
-  CtapGetAssertionRequest request(test_data::kRelyingPartyId,
-                                  test_data::kClientDataJson);
-  ASSERT_FALSE(!!request.cable_extension);
-  request.allow_list = {
-      PublicKeyCredentialDescriptor(
-          CredentialType::kPublicKey,
-          fido_parsing_utils::Materialize(
-              test_data::kTestGetAssertionCredentialId),
-          {FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy,
-           FidoTransportProtocol::kUsbHumanInterfaceDevice}),
-  };
-
-  auto request_handler =
-      CreateGetAssertionHandlerWithRequest(std::move(request));
-  ExpectAllowedTransportsForRequestAre(
-      request_handler.get(), {FidoTransportProtocol::kUsbHumanInterfaceDevice});
-}
-
 TEST_F(FidoGetAssertionHandlerTest, SupportedTransportsAreOnlyBleAndNfc) {
   const base::flat_set<FidoTransportProtocol> kBleAndNfc = {
       FidoTransportProtocol::kBluetoothLowEnergy,

device/fido/get_assertion_request_handler.cc

@@ -147,14 +147,6 @@ base::flat_set<FidoTransportProtocol> GetTransportsAllowedByRP(
   return transports;
 }
 
-base::flat_set<FidoTransportProtocol> GetTransportsAllowedAndConfiguredByRP(
-    const CtapGetAssertionRequest& request) {
-  auto transports = GetTransportsAllowedByRP(request);
-  if (!request.cable_extension)
-    transports.erase(FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy);
-  return transports;
-}
-
 void ReportGetAssertionRequestTransport(FidoAuthenticator* authenticator) {
   if (authenticator->AuthenticatorTransport()) {
     base::UmaHistogramEnumeration(
@@ -184,7 +176,7 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
           fido_discovery_factory,
           base::STLSetIntersection<base::flat_set<FidoTransportProtocol>>(
               supported_transports,
-              GetTransportsAllowedAndConfiguredByRP(request)),
+              GetTransportsAllowedByRP(request)),
           std::move(completion_callback)),
       request_(std::move(request)) {
   transport_availability_info().request_type =
@@ -194,15 +186,6 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
   transport_availability_info().cable_pairing_data_supplied =
       static_cast<bool>(request_.cable_extension);
 
-  if (base::Contains(transport_availability_info().available_transports,
-                     FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy)) {
-    DCHECK(request_.cable_extension);
-    auto discovery =
-        fido_discovery_factory_->CreateCable(*request_.cable_extension);
-    discovery->set_observer(this);
-    discoveries().push_back(std::move(discovery));
-  }
-
   if (request_.allow_list.empty()) {
     // Resident credential requests always involve user verification.
     request_.user_verification = UserVerificationRequirement::kRequired;

device/fido/make_credential_request_handler.cc

@@ -115,16 +115,16 @@ base::flat_set<FidoTransportProtocol> GetTransportsAllowedByRP(
     case AuthenticatorAttachment::kPlatform:
       return {FidoTransportProtocol::kInternal};
     case AuthenticatorAttachment::kCrossPlatform:
-      // Cloud-assisted BLE is not yet supported for MakeCredential requests.
       return {FidoTransportProtocol::kUsbHumanInterfaceDevice,
               FidoTransportProtocol::kBluetoothLowEnergy,
-              FidoTransportProtocol::kNearFieldCommunication};
+              FidoTransportProtocol::kNearFieldCommunication,
+              FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy};
     case AuthenticatorAttachment::kAny:
-      // Cloud-assisted BLE is not yet supported for MakeCredential requests.
       return {FidoTransportProtocol::kInternal,
               FidoTransportProtocol::kNearFieldCommunication,
               FidoTransportProtocol::kUsbHumanInterfaceDevice,
-              FidoTransportProtocol::kBluetoothLowEnergy};
+              FidoTransportProtocol::kBluetoothLowEnergy,
+              FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy};
   }
 
   NOTREACHED();

device/fido/virtual_fido_device_factory.cc

@@ -91,11 +91,5 @@ std::unique_ptr<FidoDiscoveryBase> VirtualFidoDeviceFactory::Create(
       transport_, state_, supported_protocol_, ctap2_config_);
 }
 
-std::unique_ptr<FidoDiscoveryBase> VirtualFidoDeviceFactory::CreateCable(
-    std::vector<CableDiscoveryData> cable_data) {
-  return Create(FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy,
-                nullptr);
-}
-
 }  // namespace test
 }  // namespace device

device/fido/virtual_fido_device_factory.h

@@ -42,9 +42,6 @@ class VirtualFidoDeviceFactory : public device::FidoDiscoveryFactory {
   std::unique_ptr<FidoDiscoveryBase> Create(
       FidoTransportProtocol transport,
       ::service_manager::Connector* connector) override;
-  // Instantiates a FidoDiscovery for caBLE.
-  std::unique_ptr<FidoDiscoveryBase> CreateCable(
-      std::vector<CableDiscoveryData> cable_data) override;
 
  private:
   ProtocolVersion supported_protocol_ = ProtocolVersion::kU2f;