fido: invoke Windows API authenticator for all transports

Rather than only forwarding USB requests to the Windows WebAuthn API,
we are going to forward all requests. This consequently renames the
FidoAuthenticator subclass to WinWebAuthnApiAuthenticator. The
corresponding FidoDiscovery is instantiated for all requests where the
WebAuthn API is available (i.e., the feature is flag enabled, and the
DLL can be loaded and is at least version 1). When it is indeed
available, all other discoveries are disabled and the UI is suppressed
via a newly introduced option in the TransportAvailabilityInfo struct
relayed to embedders via the TransportAvailabilityObserver interface.

Note this temporarily breaks caBLE whenever the Windows API is
flag-enabled and available. (But the OS would presumably block the
device communication in that case anyway.)

Bug: 898718
Change-Id: If2bddac4bac519d1aa5aa9cb5d8fdc326297de73
Reviewed-on: https://chromium-review.googlesource.com/c/1330897
Commit-Queue: Martin Kreichgauer <martinkr@chromium.org>
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608127}

chrome/browser/webauthn/authenticator_request_dialog_model.cc

@@ -88,6 +88,7 @@ void AuthenticatorRequestDialogModel::StartFlow(
     TransportAvailabilityInfo transport_availability,
     base::Optional<device::FidoTransportProtocol> last_used_transport) {
   DCHECK_EQ(current_step(), Step::kNotStarted);
+  DCHECK(!transport_availability.disable_embedder_ui);
 
   transport_availability_ = std::move(transport_availability);
   last_used_transport_ = last_used_transport;

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -62,10 +62,6 @@ bool IsWebauthnRPIDListedInEnterprisePolicy(
 #endif
 }
 
-bool IsWebAuthnUiEnabled() {
-  return base::FeatureList::IsEnabled(features::kWebAuthenticationUI);
-}
-
 }  // namespace
 
 #if defined(OS_MACOSX)
@@ -267,6 +263,11 @@ void ChromeAuthenticatorRequestDelegate::UpdateLastTransportUsed(
 void ChromeAuthenticatorRequestDelegate::OnTransportAvailabilityEnumerated(
     device::FidoRequestHandlerBase::TransportAvailabilityInfo data) {
 #if !defined(OS_ANDROID)
+  if (data.disable_embedder_ui) {
+    disable_ui_ = true;
+    return;
+  }
+
   if (!IsWebAuthnUiEnabled())
     return;
 
@@ -282,11 +283,14 @@ void ChromeAuthenticatorRequestDelegate::OnTransportAvailabilityEnumerated(
 
 bool ChromeAuthenticatorRequestDelegate::EmbedderControlsAuthenticatorDispatch(
     const device::FidoAuthenticator& authenticator) {
-  // TODO(hongjunchoi): Change this so that requests for BLE authenticators are
-  // not dispatched immediately if WebAuthN UI is enabled.
+  // TODO(hongjunchoi): Change this so that requests for BLE authenticators
+  // are not dispatched immediately if WebAuthN UI is enabled.
   if (!IsWebAuthnUiEnabled())
     return false;
 
+  // On macOS, a native dialog is shown for the Touch ID authenticator
+  // immediately after dispatch to that authenticator. This dialog must not
+  // be triggered before Chrome's WebAuthn UI has advanced accordingly.
   return authenticator.AuthenticatorTransport() &&
          *authenticator.AuthenticatorTransport() ==
              device::FidoTransportProtocol::kInternal;
@@ -384,3 +388,10 @@ ChromeAuthenticatorRequestDelegate::GetPreviouslyPairedFidoBleDeviceAddresses()
       Profile::FromBrowserContext(browser_context())->GetPrefs();
   return prefs->GetList(kWebAuthnBlePairedMacAddressesPrefName);
 }
+
+bool ChromeAuthenticatorRequestDelegate::IsWebAuthnUiEnabled() const {
+  // UI can be disabled via flag or by the request handler for certain
+  // requests (e.g. on Windows, where the native API renders its own UI).
+  return base::FeatureList::IsEnabled(features::kWebAuthenticationUI) &&
+         !disable_ui_;
+}

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -106,6 +106,7 @@ class ChromeAuthenticatorRequestDelegate
   void AddFidoBleDeviceToPairedList(std::string device_address);
   base::Optional<device::FidoTransportProtocol> GetLastTransportUsed() const;
   const base::ListValue* GetPreviouslyPairedFidoBleDeviceAddresses() const;
+  bool IsWebAuthnUiEnabled() const;
 
   content::RenderFrameHost* const render_frame_host_;
   AuthenticatorRequestDialogModel* weak_dialog_model_ = nullptr;
@@ -119,6 +120,11 @@ class ChromeAuthenticatorRequestDelegate
   base::OnceClosure cancel_callback_;
   device::FidoRequestHandlerBase::RequestCallback request_callback_;
 
+  // If in the TransportAvailabilityInfo reported by the request handler,
+  // disable_embedder_ui is set, this will be set to true. No UI must be
+  // rendered and all request handler callbacks will be ignored.
+  bool disable_ui_ = false;
+
   base::WeakPtrFactory<ChromeAuthenticatorRequestDelegate> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeAuthenticatorRequestDelegate);

device/fido/fido_discovery_factory.cc

@@ -33,19 +33,6 @@ std::unique_ptr<FidoDiscoveryBase> CreateUsbFidoDiscovery(
   return nullptr;
 #else
 
-#if defined(OS_WIN)
-  // On platforms where the Windows webauthn.dll is present, access to USB
-  // devices is blocked and we use a special authenticator that forwards
-  // requests to the Windows WebAuthn API instead.
-  if (base::FeatureList::IsEnabled(device::kWebAuthUseNativeWinApi) &&
-      WinWebAuthnApi::GetDefault()->IsAvailable()) {
-    return std::make_unique<WinNativeCrossPlatformAuthenticatorDiscovery>(
-        WinWebAuthnApi::GetDefault(),
-        // TODO(martinkr): Inject the window from which the request originated.
-        GetForegroundWindow());
-  }
-#endif  // defined(OS_WIN)
-
   DCHECK(connector);
   return std::make_unique<FidoHidDiscovery>(connector);
 #endif  // !defined(OS_ANDROID)
@@ -74,13 +61,13 @@ std::unique_ptr<FidoDiscoveryBase> CreateFidoDiscoveryImpl(
   return nullptr;
 }
 
-}  // namespace
-
 std::unique_ptr<FidoDiscoveryBase> CreateCableDiscoveryImpl(
     std::vector<CableDiscoveryData> cable_data) {
   return std::make_unique<FidoCableDiscovery>(std::move(cable_data));
 }
 
+}  // namespace
+
 // static
 FidoDiscoveryFactory::FactoryFuncPtr FidoDiscoveryFactory::g_factory_func_ =
     &CreateFidoDiscoveryImpl;
@@ -102,6 +89,26 @@ std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::CreateCable(
   return (*g_cable_factory_func_)(std::move(cable_data));
 }
 
+//  static
+#if defined(OS_WIN)
+std::unique_ptr<FidoDiscoveryBase>
+FidoDiscoveryFactory::MaybeCreateWinWebAuthnApiDiscovery() {
+  if (!base::FeatureList::IsEnabled(device::kWebAuthUseNativeWinApi) ||
+      !WinWebAuthnApi::GetDefault()->IsAvailable()) {
+    return nullptr;
+  }
+  return std::make_unique<WinWebAuthnApiAuthenticatorDiscovery>(
+      WinWebAuthnApi::GetDefault(),
+      // TODO(martinkr): Inject the window from which the request
+      // originated. Windows uses this parameter to center the
+      // dialog over the parent. The dialog should be centered
+      // over the originating Chrome Window; the foreground window
+      // may have changed to something else since the request was
+      // issued.
+      GetForegroundWindow());
+}
+#endif  // defined(OS_WIN)
+
 // ScopedFidoDiscoveryFactory -------------------------------------------------
 
 namespace internal {

device/fido/fido_discovery_factory.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/component_export.h"
+#include "build/build_config.h"
 #include "device/fido/cable/cable_discovery_data.h"
 #include "device/fido/fido_device_discovery.h"
 #include "device/fido/fido_discovery_base.h"
@@ -39,6 +40,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory {
   // Instantiates a FidoDiscovery for caBLE.
   static std::unique_ptr<FidoDiscoveryBase> CreateCable(
       std::vector<CableDiscoveryData> cable_data);
+#if defined(OS_WIN)
+  // Instantiates a FidoDiscovery for the native Windows WebAuthn
+  // API where available. Returns nullptr otherwise.
+  static std::unique_ptr<FidoDiscoveryBase>
+  MaybeCreateWinWebAuthnApiDiscovery();
+#endif  // defined(OS_WIN)
 
  private:
   friend class internal::ScopedFidoDiscoveryFactory;

device/fido/fido_request_handler_base.cc

@@ -58,7 +58,16 @@ FidoRequestHandlerBase::TransportAvailabilityObserver::
 FidoRequestHandlerBase::FidoRequestHandlerBase(
     service_manager::Connector* connector,
     const base::flat_set<FidoTransportProtocol>& available_transports)
-    : weak_factory_(this) {
+    : connector_(connector), weak_factory_(this) {
+#if defined(OS_WIN)
+  InitDiscoveriesWin(available_transports);
+#else
+  InitDiscoveries(available_transports);
+#endif  // !defined(OS_WIN)
+}
+
+void FidoRequestHandlerBase::InitDiscoveries(
+    const base::flat_set<FidoTransportProtocol>& available_transports) {
   // The number of times |notify_observer_callback_| needs to be invoked before
   // Observer::OnTransportAvailabilityEnumerated is dispatched. Essentially this
   // is used to wait until all the parts of |transport_availability_info_| are
@@ -85,7 +94,7 @@ FidoRequestHandlerBase::FidoRequestHandlerBase(
       continue;
     }
 
-    auto discovery = FidoDiscoveryFactory::Create(transport, connector);
+    auto discovery = FidoDiscoveryFactory::Create(transport, connector_);
     if (discovery == nullptr) {
       // This can occur in tests when a ScopedVirtualU2fDevice is in effect and
       // HID transports are not configured.
@@ -116,6 +125,55 @@ FidoRequestHandlerBase::FidoRequestHandlerBase(
           weak_factory_.GetWeakPtr()));
 }
 
+#if defined(OS_WIN)
+void FidoRequestHandlerBase::InitDiscoveriesWin(
+    const base::flat_set<FidoTransportProtocol>& available_transports) {
+  // Try to instantiate the discovery for proxying requests to the native
+  // Windows WebAuthn API; or fall back to using the regular device transport
+  // discoveries if the API is unavailable.
+  auto discovery = FidoDiscoveryFactory::MaybeCreateWinWebAuthnApiDiscovery();
+  if (!discovery) {
+    InitDiscoveries(available_transports);
+    return;
+  }
+
+  // The Windows WebAuthn API is available. On this platform, communicating
+  // with authenticator devices directly is blocked by the OS, so we need to go
+  // through the native API instead. No device discoveries may be instantiated.
+  //
+  // The Windows API supports USB, NFC, BLE and platform authenticators, but
+  // not caBLE.  Communicating with caBLE devices directly is subject to the
+  // same block by the OS, so this platform is without caBLE support for now.
+  //
+  // TODO(martinkr): Re-enable the caBLE discovery once caBLE has moved to a
+  // different UUID. See crbug.com/905111.
+
+  discovery->set_observer(this);
+  discoveries_.push_back(std::move(discovery));
+
+  // Tell the embedder to not render a UI and ignore all future callbacks. Also
+  // don't report any available transports; the embedder is not supposed to use
+  // this information anyway.
+  transport_availability_info_.disable_embedder_ui = true;
+  transport_availability_info_.available_transports = {};
+
+  // The number of times |notify_observer_callback_| needs to be invoked before
+  // Observer::OnTransportAvailabilityEnumerated is dispatched. Essentially this
+  // is used to wait until all the parts of |transport_availability_info_| are
+  // filled out. In the case of Windows, there are no transport discoveries to
+  // wait for, so the |notify_observer_callback_| is only invoked in:
+  //   1) SetPlatformAuthenticatorOrMarkUnavailable().
+  //   2) set_observer().
+  constexpr size_t transport_info_callback_count = 2u;
+
+  notify_observer_callback_ = base::BarrierClosure(
+      transport_info_callback_count,
+      base::BindOnce(
+          &FidoRequestHandlerBase::NotifyObserverTransportAvailability,
+          weak_factory_.GetWeakPtr()));
+}
+#endif  // defined(OS_WIN)
+
 FidoRequestHandlerBase::~FidoRequestHandlerBase() = default;
 
 void FidoRequestHandlerBase::StartAuthenticatorRequest(

device/fido/fido_request_handler_base.h

@@ -19,6 +19,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string_piece_forward.h"
+#include "build/build_config.h"
 #include "device/fido/fido_device_authenticator.h"
 #include "device/fido/fido_discovery_base.h"
 #include "device/fido/fido_transport_protocol.h"
@@ -87,6 +88,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
     bool has_recognized_mac_touch_id_credential = false;
     bool is_ble_powered = false;
     bool can_power_on_ble_adapter = false;
+
+    // If true, dispatch of the request cannot be controlled by
+    // the embedder. The embedder must not display a UI for this
+    // request and must ignore all subsequent invocations of the
+    // TransportAvailabilityObserver interface methods.
+    bool disable_embedder_ui = false;
   };
 
   class COMPONENT_EXPORT(DEVICE_FIDO) TransportAvailabilityObserver {
@@ -201,6 +208,13 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
  private:
   friend class FidoRequestHandlerTest;
 
+  void InitDiscoveries(
+      const base::flat_set<FidoTransportProtocol>& available_transports);
+#if defined(OS_WIN)
+  void InitDiscoveriesWin(
+      const base::flat_set<FidoTransportProtocol>& available_transports);
+#endif
+
   // FidoDiscoveryBase::Observer
   void AuthenticatorAdded(FidoDiscoveryBase* discovery,
                           FidoAuthenticator* authenticator) final;
@@ -232,6 +246,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   // TODO(martinkr): Inject platform authenticators through a new
   // FidoDiscoveryBase specialization and hold ownership there.
   std::unique_ptr<FidoAuthenticator> platform_authenticator_;
+  service_manager::Connector* const connector_;
 
   base::WeakPtrFactory<FidoRequestHandlerBase> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(FidoRequestHandlerBase);

device/fido/get_assertion_handler_unittest.cc

@@ -31,6 +31,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_WIN)
+#include "device/fido/win/authenticator.h"
 #include "device/fido/win/fake_webauthn_api.h"
 #endif  // defined(OS_WIN)
 
@@ -753,10 +754,13 @@ TEST_F(GetAssertionRequestHandlerWinTest, TestWinUsbDiscovery) {
     if (test.enable_feature_flag)
       scoped_feature_list.InitAndEnableFeature(kWebAuthUseNativeWinApi);
 
+    // Simulate a connected HID device.
+    ScopedFakeHidManager fake_hid_manager;
+    fake_hid_manager.AddFidoHidDevice("guid");
+
     TestGetAssertionRequestCallback cb;
-    ScopedFakeHidManager fake_hid_manager_;
     auto handler = std::make_unique<GetAssertionRequestHandler>(
-        fake_hid_manager_.service_manager_connector(),
+        fake_hid_manager.service_manager_connector(),
         base::flat_set<FidoTransportProtocol>(
             {FidoTransportProtocol::kUsbHumanInterfaceDevice}),
         CtapGetAssertionRequest(test_data::kRelyingPartyId,
@@ -765,14 +769,11 @@ TEST_F(GetAssertionRequestHandlerWinTest, TestWinUsbDiscovery) {
         cb.callback());
     scoped_task_environment_.RunUntilIdle();
 
-    fake_hid_manager_.AddFidoHidDevice("guid");
-    scoped_task_environment_.RunUntilIdle();
-
     EXPECT_EQ(1u, handler->AuthenticatorsForTesting().size());
     // Crudely distinguish authenticator type by FidoAuthenticator::GetId.
     EXPECT_EQ(test.expect_device_type == DeviceType::kHid
                   ? "hid:guid"
-                  : "WinNativeCrossPlatformAuthenticator",
+                  : WinWebAuthnApiAuthenticator::kAuthenticatorId,
               handler->AuthenticatorsForTesting().begin()->second->GetId());
   }
 }