webauth: move isUVPAA() implementation out of the //chrome layer

The IsUserVerifyingPlatformAuthenticator() WebAuthn API call needlessly
required the content embedder to provide their own implementation in an
AuthenticatorRequestClientDelegate specialization. This change moves the
ChromeAuthenticatorRequestDelegate implementation into the content layer
(AuthenticatorCommon). The implementation is general enough to be useful
for other embedders and it doesn't depend on any state internal to the
delegate.

Concretly, IsUVPAA()
- on Windows returns the corresponding value from the native WebAuthn
API;
- on macOS reflects TouchIdAuthenticator availability as long as the
embedder provides an implementation of GetTouchIdAuthenticatorConfig.

Also collapse methods in AuthenticatorCommon that determine which
transports to instantiate discoveries for, and also add a safeguard
against instantiating platform discoveries in contexts where IsUVPAA()
returns false.

Change-Id: I8eb68ca6f8cd17fae61d9d0294b872ef0cfb3465
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2083936
Commit-Queue: Martin Kreichgauer <martinkr@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748544}

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -41,7 +41,6 @@
 
 #if defined(OS_WIN)
 #include "device/fido/win/authenticator.h"
-#include "device/fido/win/webauthn_api.h"
 #endif
 
 namespace {
@@ -420,33 +419,11 @@ bool ChromeAuthenticatorRequestDelegate::IsWebAuthnUIEnabled() {
   return !disable_ui_;
 }
 
-bool ChromeAuthenticatorRequestDelegate::
-    IsUserVerifyingPlatformAuthenticatorAvailable() {
-#if defined(OS_MACOSX)
-  // Touch ID is available in Incognito, but not in Guest mode.
-  if (Profile::FromBrowserContext(browser_context())->IsGuestSession())
-    return false;
-
-  return device::fido::mac::TouchIdAuthenticator::IsAvailable(
-      TouchIdAuthenticatorConfigForProfile(
-          Profile::FromBrowserContext(browser_context())));
-#elif defined(OS_WIN)
-  if (browser_context()->IsOffTheRecord())
-    return false;
-
-  return base::FeatureList::IsEnabled(device::kWebAuthUseNativeWinApi) &&
-         device::WinWebAuthnApiAuthenticator::
-             IsUserVerifyingPlatformAuthenticatorAvailable(
-                 GetDiscoveryFactory()->win_webauthn_api());
-#else
-  return false;
-#endif  // defined(OS_MACOSX) || defined(OS_WIN)
-}
-
 #if defined(OS_MACOSX)
 base::Optional<ChromeAuthenticatorRequestDelegate::TouchIdAuthenticatorConfig>
 ChromeAuthenticatorRequestDelegate::GetTouchIdAuthenticatorConfig() {
-  if (!IsUserVerifyingPlatformAuthenticatorAvailable())
+  // Touch ID is available in Incognito but not Guest windows.
+  if (Profile::FromBrowserContext(browser_context())->IsGuestSession())
     return base::nullopt;
 
   return TouchIdAuthenticatorConfigForProfile(

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -95,7 +95,6 @@ class ChromeAuthenticatorRequestDelegate
       device::FidoTransportProtocol transport) override;
   void DisableUI() override;
   bool IsWebAuthnUIEnabled() override;
-  bool IsUserVerifyingPlatformAuthenticatorAvailable() override;
 
   // device::FidoRequestHandlerBase::Observer:
   void OnTransportAvailabilityEnumerated(

chrome/browser/webauthn/chrome_authenticator_request_delegate_unittest.cc

@@ -24,17 +24,10 @@
 
 #if defined(OS_MACOSX)
 #include "device/fido/mac/authenticator_config.h"
-#include "device/fido/mac/scoped_touch_id_test_environment.h"
 #endif  // defined(OS_MACOSX)
 
 class ChromeAuthenticatorRequestDelegateTest
-    : public ChromeRenderViewHostTestHarness {
- protected:
-#if defined(OS_MACOSX)
-  API_AVAILABLE(macos(10.12.2))
-  device::fido::mac::ScopedTouchIdTestEnvironment touch_id_test_environment_;
-#endif  // defined(OS_MACOSX)
-};
+    : public ChromeRenderViewHostTestHarness {};
 
 TEST_F(ChromeAuthenticatorRequestDelegateTest, TestTransportPrefType) {
   ChromeAuthenticatorRequestDelegate delegate(main_rfh());
@@ -128,49 +121,12 @@ TEST_F(ChromeAuthenticatorRequestDelegateTest,
     EXPECT_EQ(32u, TouchIdMetadataSecret(&delegate2).size());
   }
 }
-
-TEST_F(ChromeAuthenticatorRequestDelegateTest, IsUVPAA) {
-  if (__builtin_available(macOS 10.12.2, *)) {
-    for (const bool touch_id_available : {false, true}) {
-      SCOPED_TRACE(::testing::Message()
-                   << "touch_id_available=" << touch_id_available);
-      touch_id_test_environment_.SetTouchIdAvailable(touch_id_available);
-
-      std::unique_ptr<content::AuthenticatorRequestClientDelegate> delegate =
-          std::make_unique<ChromeAuthenticatorRequestDelegate>(main_rfh());
-      EXPECT_EQ(touch_id_available,
-                delegate->IsUserVerifyingPlatformAuthenticatorAvailable());
-    }
-  }
-}
-
 #endif  // defined(OS_MACOSX)
 
 #if defined(OS_WIN)
 
 static constexpr char kRelyingPartyID[] = "example.com";
 
-TEST_F(ChromeAuthenticatorRequestDelegateTest, WinIsUVPAA) {
-  auto delegate =
-      std::make_unique<ChromeAuthenticatorRequestDelegate>(main_rfh());
-  device::FakeWinWebAuthnApi win_webauthn_api;
-  delegate->GetDiscoveryFactory()->set_win_webauthn_api(&win_webauthn_api);
-
-  for (const bool enable_win_webauthn_api : {false, true}) {
-    SCOPED_TRACE(enable_win_webauthn_api ? "enable_win_webauthn_api"
-                                         : "!enable_win_webauthn_api");
-    for (const bool is_uvpaa : {false, true}) {
-      SCOPED_TRACE(is_uvpaa ? "is_uvpaa" : "!is_uvpaa");
-
-      win_webauthn_api.set_available(enable_win_webauthn_api);
-      win_webauthn_api.set_is_uvpaa(is_uvpaa);
-
-      EXPECT_EQ(enable_win_webauthn_api && is_uvpaa,
-                delegate->IsUserVerifyingPlatformAuthenticatorAvailable());
-    }
-  }
-}
-
 // Tests that ShouldReturnAttestation() returns with true if |authenticator|
 // is the Windows native WebAuthn API with WEBAUTHN_API_VERSION_2 or higher,
 // where Windows prompts for attestation in its own native UI.

content/browser/webauth/authenticator_common.cc

@@ -19,6 +19,7 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversion_utils.h"
 #include "base/timer/timer.h"
+#include "build/build_config.h"
 #include "content/browser/bad_message.h"
 #include "content/browser/webauth/authenticator_environment_impl.h"
 #include "content/browser/webauth/virtual_authenticator_request_delegate.h"
@@ -58,6 +59,15 @@
 #include "url/url_constants.h"
 #include "url/url_util.h"
 
+#if defined(OS_MACOSX)
+#include "device/fido/mac/authenticator.h"
+#include "device/fido/mac/credential_metadata.h"
+#endif
+
+#if defined(OS_WIN)
+#include "device/fido/win/authenticator.h"
+#endif
+
 namespace content {
 
 namespace client_data {
@@ -437,22 +447,88 @@ std::string ToJSONString(const std::string& in) {
   return ret;
 }
 
-base::flat_set<device::FidoTransportProtocol> GetTransportsEnabledByFlags(
-    FrameTreeNode* frame_tree_node) {
-  if (AuthenticatorEnvironmentImpl::GetInstance()->GetVirtualFactoryFor(
-          frame_tree_node)) {
+bool IsUserVerifyingPlatformAuthenticatorAvailableImpl(
+    AuthenticatorRequestClientDelegate* delegate,
+    device::FidoDiscoveryFactory* discovery_factory,
+    BrowserContext* browser_context) {
+  base::Optional<bool> is_uvpaa_override =
+      delegate->IsUserVerifyingPlatformAuthenticatorAvailableOverride();
+  if (is_uvpaa_override) {
+    return *is_uvpaa_override;
+  }
+
+#if defined(OS_MACOSX)
+  const base::Optional<device::fido::mac::AuthenticatorConfig> config =
+      delegate->GetTouchIdAuthenticatorConfig();
+  if (!config) {
+    return false;
+  }
+  return device::fido::mac::TouchIdAuthenticator::IsAvailable(*config);
+#elif defined(OS_WIN)
+  if (browser_context->IsOffTheRecord()) {
+    return false;
+  }
+  return base::FeatureList::IsEnabled(device::kWebAuthUseNativeWinApi) &&
+         device::WinWebAuthnApiAuthenticator::
+             IsUserVerifyingPlatformAuthenticatorAvailable(
+                 discovery_factory->win_webauthn_api());
+#else
+  return false;
+#endif
+}
+
+// GetAvailableTransports returns the set of transports that should be passed to
+// a FidoRequestHandler for the current request. This determines for which
+// transports the request handler will attempt to obtain FidoDiscovery
+// instances.
+base::flat_set<device::FidoTransportProtocol> GetAvailableTransports(
+    RenderFrameHost* render_frame_host,
+    AuthenticatorRequestClientDelegate* delegate,
+    const url::Origin& caller_origin) {
+  // U2F requests proxied from the cryptotoken extension are limited to USB
+  // devices.
+  if (WebAuthRequestSecurityChecker::OriginIsCryptoTokenExtension(
+          caller_origin)) {
+    return base::flat_set<device::FidoTransportProtocol>(
+        {device::FidoTransportProtocol::kUsbHumanInterfaceDevice});
+  }
+
+  // Try all transports if the FidoDiscoveryFactory has been injected in tests
+  // or via the testing API.
+  if (AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride(
+          static_cast<RenderFrameHostImpl*>(render_frame_host)
+              ->frame_tree_node())) {
     return device::GetAllTransportProtocols();
   }
+
   base::flat_set<device::FidoTransportProtocol> transports;
   transports.insert(device::FidoTransportProtocol::kUsbHumanInterfaceDevice);
+
+  device::FidoDiscoveryFactory* discovery_factory =
+      AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride(
+          static_cast<RenderFrameHostImpl*>(render_frame_host)
+              ->frame_tree_node());
+  if (!discovery_factory) {
+    discovery_factory = delegate->GetDiscoveryFactory();
+  }
+
+  // Don't instantiate a platform discovery in contexts where IsUVPAA() would
+  // return false. This avoids platform authenticators mistakenly being
+  // available when e.g. an embedder provided implementation of
+  // IsUserVerifyingPlatformAuthenticatorAvailableOverride() returned false.
+  if (IsUserVerifyingPlatformAuthenticatorAvailableImpl(
+          delegate, discovery_factory,
+          content::WebContents::FromRenderFrameHost(render_frame_host)
+              ->GetBrowserContext())) {
     transports.insert(device::FidoTransportProtocol::kInternal);
+  }
 
-  // TODO(crbug.com/885165): We should not directly access the BLE stack here.
-  // It is used by //device/fido, so its availability should be checked there.
-  if (!device::BluetoothAdapterFactory::Get().IsLowEnergySupported())
+  // FIXME(martinkr): Check whether this can be moved in front of the BLE
+  // adapter enumeration logic in FidoRequestHandlerBase.
+  if (!device::BluetoothAdapterFactory::Get().IsLowEnergySupported()) {
     return transports;
+  }
 
-  // caBLE is independent of the BLE transport.
   if (base::FeatureList::IsEnabled(features::kWebAuthCable) ||
       base::FeatureList::IsEnabled(device::kWebAuthPhoneSupport)) {
     transports.insert(
@@ -462,28 +538,12 @@ base::flat_set<device::FidoTransportProtocol> GetTransportsEnabledByFlags(
   return transports;
 }
 
-// Returns the transports to be used for a request made by |caller_origin|.
-base::flat_set<device::FidoTransportProtocol> GetTransports(
-    url::Origin caller_origin,
-    base::flat_set<device::FidoTransportProtocol> available_transports) {
-  // U2F requests proxied from the cryptotoken extension are limited to USB
-  // devices.
-  return WebAuthRequestSecurityChecker::OriginIsCryptoTokenExtension(
-             caller_origin)
-             ? base::flat_set<device::FidoTransportProtocol>(
-                   {device::FidoTransportProtocol::kUsbHumanInterfaceDevice})
-             : available_transports;
-}
-
 }  // namespace
 
 AuthenticatorCommon::AuthenticatorCommon(
     RenderFrameHost* render_frame_host,
     std::unique_ptr<base::OneShotTimer> timer)
     : render_frame_host_(render_frame_host),
-      transports_(GetTransportsEnabledByFlags(
-          static_cast<RenderFrameHostImpl*>(render_frame_host)
-              ->frame_tree_node())),
       security_checker_(static_cast<RenderFrameHostImpl*>(render_frame_host)
                             ->GetWebAuthRequestSecurityChecker()),
       timer_(std::move(timer)) {
@@ -541,7 +601,9 @@ void AuthenticatorCommon::StartMakeCredentialRequest(
   }
 
   request_ = std::make_unique<device::MakeCredentialRequestHandler>(
-      discovery_factory_, GetTransports(caller_origin_, transports_),
+      discovery_factory_,
+      GetAvailableTransports(render_frame_host_, request_delegate_.get(),
+                             caller_origin_),
       *ctap_make_credential_request_, *authenticator_selection_criteria_,
       allow_skipping_pin_touch,
       base::BindOnce(&AuthenticatorCommon::OnRegisterResponse,
@@ -606,7 +668,9 @@ void AuthenticatorCommon::StartGetAssertionRequest(
   }
 
   request_ = std::make_unique<device::GetAssertionRequestHandler>(
-      discovery_factory_, GetTransports(caller_origin_, transports_),
+      discovery_factory_,
+      GetAvailableTransports(render_frame_host_, request_delegate_.get(),
+                             caller_origin_),
       *ctap_get_assertion_request_, allow_skipping_pin_touch,
       base::BindOnce(&AuthenticatorCommon::OnSignResponse,
                      weak_factory_.GetWeakPtr()));
@@ -1025,19 +1089,23 @@ void AuthenticatorCommon::IsUserVerifyingPlatformAuthenticatorAvailable(
     blink::mojom::Authenticator::
         IsUserVerifyingPlatformAuthenticatorAvailableCallback callback) {
   // Use |request_delegate_| if a request is currently in progress; or create a
-  // temporary request delegate otherwise.
-  //
-  // Note that |CreateRequestDelegate| may return nullptr if there is an active
-  // |request_delegate_| already.
+  // temporary request delegate otherwise. Note that CreateRequestDelegate() may
+  // return nullptr if there is an active |request_delegate_| already.
   std::unique_ptr<AuthenticatorRequestClientDelegate> maybe_request_delegate =
       request_delegate_ ? nullptr : CreateRequestDelegate();
   AuthenticatorRequestClientDelegate* request_delegate_ptr =
       request_delegate_ ? request_delegate_.get()
                         : maybe_request_delegate.get();
+  device::FidoDiscoveryFactory* discovery_factory =
+      AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride(
+          static_cast<RenderFrameHostImpl*>(render_frame_host_)
+              ->frame_tree_node());
+  if (!discovery_factory) {
+    discovery_factory = request_delegate_ptr->GetDiscoveryFactory();
+  }
 
-  const bool result =
-      request_delegate_ptr->IsUserVerifyingPlatformAuthenticatorAvailable();
-
+  const bool result = IsUserVerifyingPlatformAuthenticatorAvailableImpl(
+      request_delegate_ptr, discovery_factory, browser_context());
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), result));
 }

content/browser/webauth/authenticator_common.h

@@ -89,15 +89,6 @@ class CONTENT_EXPORT AuthenticatorCommon {
 
   void DisableUI();
 
-  base::flat_set<device::FidoTransportProtocol> enabled_transports_for_testing()
-      const {
-    return transports_;
-  }
-  void set_transports_for_testing(
-      base::flat_set<device::FidoTransportProtocol> transports) {
-    transports_ = transports;
-  }
-
  protected:
   virtual std::unique_ptr<AuthenticatorRequestClientDelegate>
   CreateRequestDelegate();
@@ -190,7 +181,6 @@ class CONTENT_EXPORT AuthenticatorCommon {
   BrowserContext* browser_context() const;
 
   RenderFrameHost* const render_frame_host_;
-  base::flat_set<device::FidoTransportProtocol> transports_;
   device::FidoDiscoveryFactory* discovery_factory_ = nullptr;
   std::unique_ptr<device::FidoRequestHandlerBase> request_;
   blink::mojom::Authenticator::MakeCredentialCallback

content/browser/webauth/virtual_authenticator_request_delegate.cc

@@ -36,13 +36,13 @@ void VirtualAuthenticatorRequestDelegate::SelectAccount(
   std::move(callback).Run(std::move(responses[0]));
 }
 
-bool VirtualAuthenticatorRequestDelegate::
-    IsUserVerifyingPlatformAuthenticatorAvailable() {
+base::Optional<bool> VirtualAuthenticatorRequestDelegate::
+    IsUserVerifyingPlatformAuthenticatorAvailableOverride() {
   auto* virtual_discovery_factory =
       AuthenticatorEnvironmentImpl::GetInstance()->GetVirtualFactoryFor(
           frame_tree_node_);
   if (!virtual_discovery_factory) {
-    return false;
+    return base::nullopt;
   }
   const auto& authenticators = virtual_discovery_factory->GetAuthenticators();
   return std::any_of(authenticators.begin(), authenticators.end(),

content/browser/webauth/virtual_authenticator_request_delegate.h

@@ -27,7 +27,8 @@ class VirtualAuthenticatorRequestDelegate
       std::vector<device::AuthenticatorGetAssertionResponse> responses,
       base::OnceCallback<void(device::AuthenticatorGetAssertionResponse)>
           callback) override;
-  bool IsUserVerifyingPlatformAuthenticatorAvailable() override;
+  base::Optional<bool> IsUserVerifyingPlatformAuthenticatorAvailableOverride()
+      override;
 
  private:
   FrameTreeNode* const frame_tree_node_;

content/browser/webauth/webauth_browsertest.cc

@@ -1467,38 +1467,6 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
 }
 #endif
 
-// WebAuthBrowserBleDisabledTest
-// ----------------------------------------------
-
-// A test fixture that does not enable BLE discovery.
-class WebAuthBrowserBleDisabledTest : public WebAuthLocalClientBrowserTest {
- public:
-  WebAuthBrowserBleDisabledTest() {}
-
- protected:
-  device::test::FakeFidoDiscoveryFactory* discovery_factory;
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-  DISALLOW_COPY_AND_ASSIGN(WebAuthBrowserBleDisabledTest);
-};
-
-// Tests that the BLE discovery does not start when the WebAuthnBle feature
-// flag is disabled.
-IN_PROC_BROWSER_TEST_F(WebAuthBrowserBleDisabledTest, CheckBleDisabled) {
-  auto* fake_hid_discovery = discovery_factory_->ForgeNextHidDiscovery();
-  auto* fake_ble_discovery = discovery_factory_->ForgeNextBleDiscovery();
-
-  // Do something that will start discoveries.
-  TestCreateCallbackReceiver create_callback_receiver;
-  authenticator()->MakeCredential(BuildBasicCreateOptions(),
-                                  create_callback_receiver.callback());
-
-  fake_hid_discovery->WaitForCallToStart();
-  EXPECT_TRUE(fake_hid_discovery->is_start_requested());
-  EXPECT_FALSE(fake_ble_discovery->is_start_requested());
-}
-
 class WebAuthLocalClientBackForwardCacheBrowserTest
     : public WebAuthLocalClientBrowserTest {
  protected:

content/public/browser/authenticator_request_client_delegate.cc

@@ -100,9 +100,9 @@ AuthenticatorRequestClientDelegate::GetTouchIdAuthenticatorConfig() {
 }
 #endif  // defined(OS_MACOSX)
 
-bool AuthenticatorRequestClientDelegate::
-    IsUserVerifyingPlatformAuthenticatorAvailable() {
-  return false;
+base::Optional<bool> AuthenticatorRequestClientDelegate::
+    IsUserVerifyingPlatformAuthenticatorAvailableOverride() {
+  return base::nullopt;
 }
 
 device::FidoDiscoveryFactory*

content/public/browser/authenticator_request_client_delegate.h

@@ -198,15 +198,17 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   using TouchIdAuthenticatorConfig = device::fido::mac::AuthenticatorConfig;
 
   // Returns configuration data for the built-in Touch ID platform
-  // authenticator. May return nullopt if the authenticator is not used or not
-  // available.
+  // authenticator. May return nullopt if the authenticator is not available in
+  // the current context, in which case the Touch ID authenticator will be
+  // unavailable.
   virtual base::Optional<TouchIdAuthenticatorConfig>
   GetTouchIdAuthenticatorConfig();
 #endif  // defined(OS_MACOSX)
 
-  // Returns true if a user verifying platform authenticator is available and
-  // configured.
-  virtual bool IsUserVerifyingPlatformAuthenticatorAvailable();
+  // Returns a bool if the result of the isUserVerifyingPlatformAuthenticator
+  // API call should be overridden with that value, or base::nullopt otherwise.
+  virtual base::Optional<bool>
+  IsUserVerifyingPlatformAuthenticatorAvailableOverride();
 
   // Returns a FidoDiscoveryFactory that has been configured for the current
   // environment.