webauthn: consolidate three embedder functions into one.

All caBLE work can actually be done in by a single upcall from the
content/ layer rather than needing three functions.

BUG=1002262

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

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -306,29 +306,32 @@ bool ChromeAuthenticatorRequestDelegate::SupportsResidentKeys() {
   return true;
 }
 
-bool ChromeAuthenticatorRequestDelegate::ShouldPermitCableExtension(
-    const url::Origin& origin) {
-  // Because the future of the caBLE extension might be that we transition
-  // everything to QR-code or sync-based pairing, we don't want use of the
-  // extension to spread without consideration. Therefore it's limited to
-  // origins that are already depending on it and test sites.
-  if (origin.DomainIs("google.com")) {
-    return true;
+void ChromeAuthenticatorRequestDelegate::ConfigureCable(
+    const url::Origin& origin,
+    base::span<const device::CableDiscoveryData> pairings_from_extension) {
+  std::vector<device::CableDiscoveryData> pairings;
+  if (ShouldPermitCableExtension(origin)) {
+    pairings.insert(pairings.end(), pairings_from_extension.begin(),
+                    pairings_from_extension.end());
+  }
+  const bool cable_extension_provided = !pairings.empty();
+
+  base::Optional<device::QRGeneratorKey> qr_generator_key;
+  bool have_paired_phones = false;
+  if (base::FeatureList::IsEnabled(device::kWebAuthPhoneSupport)) {
+    qr_generator_key.emplace(device::CableDiscoveryData::NewQRKey());
+    auto paired_phones = GetCablePairings();
+    have_paired_phones = !paired_phones.empty();
+    pairings.insert(pairings.end(), paired_phones.begin(), paired_phones.end());
   }
 
-  const GURL test_site("https://webauthndemo.appspot.com");
-  DCHECK(test_site.is_valid());
-  return origin.IsSameOriginWith(url::Origin::Create(test_site));
-}
+  if (pairings.empty() && !qr_generator_key) {
+    return;
+  }
 
-bool ChromeAuthenticatorRequestDelegate::SetCableTransportInfo(
-    bool cable_extension_provided,
-    bool have_paired_phones,
-    base::Optional<device::QRGeneratorKey> qr_generator_key) {
-  weak_dialog_model_->set_cable_transport_info(cable_extension_provided,
-                                               have_paired_phones,
-                                               std::move(qr_generator_key));
-  return true;
+  weak_dialog_model_->set_cable_transport_info(
+      cable_extension_provided, have_paired_phones, qr_generator_key);
+  discovery_factory()->set_cable_data(std::move(pairings), qr_generator_key);
 }
 
 void ChromeAuthenticatorRequestDelegate::SelectAccount(
@@ -554,6 +557,29 @@ void ChromeAuthenticatorRequestDelegate::OnCancelRequest() {
   std::move(cancel_callback_).Run();
 }
 
+base::Optional<device::FidoTransportProtocol>
+ChromeAuthenticatorRequestDelegate::GetLastTransportUsed() const {
+  PrefService* prefs =
+      Profile::FromBrowserContext(browser_context())->GetPrefs();
+  return device::ConvertToFidoTransportProtocol(
+      prefs->GetString(kWebAuthnLastTransportUsedPrefName));
+}
+
+bool ChromeAuthenticatorRequestDelegate::ShouldPermitCableExtension(
+    const url::Origin& origin) {
+  // Because the future of the caBLE extension might be that we transition
+  // everything to QR-code or sync-based pairing, we don't want use of the
+  // extension to spread without consideration. Therefore it's limited to
+  // origins that are already depending on it and test sites.
+  if (origin.DomainIs("google.com")) {
+    return true;
+  }
+
+  const GURL test_site("https://webauthndemo.appspot.com");
+  DCHECK(test_site.is_valid());
+  return origin.IsSameOriginWith(url::Origin::Create(test_site));
+}
+
 std::vector<device::CableDiscoveryData>
 ChromeAuthenticatorRequestDelegate::GetCablePairings() {
   std::vector<device::CableDiscoveryData> ret;
@@ -592,14 +618,6 @@ ChromeAuthenticatorRequestDelegate::GetCablePairings() {
   return ret;
 }
 
-base::Optional<device::FidoTransportProtocol>
-ChromeAuthenticatorRequestDelegate::GetLastTransportUsed() const {
-  PrefService* prefs =
-      Profile::FromBrowserContext(browser_context())->GetPrefs();
-  return device::ConvertToFidoTransportProtocol(
-      prefs->GetString(kWebAuthnLastTransportUsedPrefName));
-}
-
 void ChromeAuthenticatorRequestDelegate::CustomizeDiscoveryFactory(
     device::FidoDiscoveryFactory* discovery_factory) {
   discovery_factory->set_cable_pairing_callback(base::BindRepeating(

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -79,12 +79,9 @@ class ChromeAuthenticatorRequestDelegate
       bool is_enterprise_attestation,
       base::OnceCallback<void(bool)> callback) override;
   bool SupportsResidentKeys() override;
-  bool ShouldPermitCableExtension(const url::Origin& origin) override;
-  bool SetCableTransportInfo(
-      bool cable_extension_provided,
-      bool has_paired_phones,
-      base::Optional<device::QRGeneratorKey> qr_generator_key) override;
-  std::vector<device::CableDiscoveryData> GetCablePairings() override;
+  void ConfigureCable(const url::Origin& origin,
+                      base::span<const device::CableDiscoveryData>
+                          pairings_from_extension) override;
   void SelectAccount(
       std::vector<device::AuthenticatorGetAssertionResponse> responses,
       base::OnceCallback<void(device::AuthenticatorGetAssertionResponse)>
@@ -136,6 +133,15 @@ class ChromeAuthenticatorRequestDelegate
   content::BrowserContext* browser_context() const;
 
   base::Optional<device::FidoTransportProtocol> GetLastTransportUsed() const;
+
+  // ShouldPermitCableExtension returns true if the given |origin| may set a
+  // caBLE extension. This extension contains website-chosen BLE pairing
+  // information that will be broadcast by the device.
+  bool ShouldPermitCableExtension(const url::Origin& origin);
+
+  // GetCablePairings returns any known caBLE pairing data.
+  virtual std::vector<device::CableDiscoveryData> GetCablePairings();
+
   void StoreNewCablePairingInPrefs(
       std::unique_ptr<device::CableDiscoveryData> discovery_data);
 

content/browser/webauth/authenticator_common.cc

@@ -565,20 +565,8 @@ void AuthenticatorCommon::StartMakeCredentialRequest(
   if (!discovery_factory)
     discovery_factory = request_delegate_->GetDiscoveryFactory();
 
-  if (base::FeatureList::IsEnabled(device::kWebAuthPhoneSupport)) {
-    std::vector<device::CableDiscoveryData> cable_pairings =
-        request_delegate_->GetCablePairings();
-    const bool have_paired_phones = !cable_pairings.empty();
-
-    device::QRGeneratorKey qr_generator_key(
-        device::CableDiscoveryData::NewQRKey());
-    if (request_delegate_->SetCableTransportInfo(
-            /*cable_extension_provided=*/false, have_paired_phones,
-            qr_generator_key)) {
-      discovery_factory->set_cable_data(cable_pairings,
-                                        std::move(qr_generator_key));
-    }
-  }
+  request_delegate_->ConfigureCable(
+      caller_origin_, base::span<const device::CableDiscoveryData>());
 
   make_credential_options_->allow_skipping_pin_touch = allow_skipping_pin_touch;
 
@@ -619,31 +607,12 @@ void AuthenticatorCommon::StartGetAssertionRequest(
   if (!discovery_factory)
     discovery_factory = request_delegate_->GetDiscoveryFactory();
 
-  std::vector<device::CableDiscoveryData> cable_pairings;
-  bool have_cable_extension = false;
+  base::span<const device::CableDiscoveryData> cable_pairings;
   if (ctap_get_assertion_request_->cable_extension &&
-      request_delegate_->ShouldPermitCableExtension(caller_origin_) &&
       IsFocused()) {
     cable_pairings = *ctap_get_assertion_request_->cable_extension;
-    have_cable_extension = !cable_pairings.empty();
-  }
-
-  base::Optional<device::QRGeneratorKey> qr_generator_key;
-  bool have_paired_phones = false;
-  if (base::FeatureList::IsEnabled(device::kWebAuthPhoneSupport)) {
-    qr_generator_key.emplace(device::CableDiscoveryData::NewQRKey());
-    auto paired_phones = request_delegate_->GetCablePairings();
-    have_paired_phones = !paired_phones.empty();
-    cable_pairings.insert(cable_pairings.end(), paired_phones.begin(),
-                          paired_phones.end());
-  }
-
-  if ((!cable_pairings.empty() || qr_generator_key.has_value()) &&
-      request_delegate_->SetCableTransportInfo(
-          have_cable_extension, have_paired_phones, qr_generator_key)) {
-    discovery_factory->set_cable_data(std::move(cable_pairings),
-                                      std::move(qr_generator_key));
   }
+  request_delegate_->ConfigureCable(caller_origin_, cable_pairings);
 
   request_ = std::make_unique<device::GetAssertionRequestHandler>(
       discovery_factory,

content/public/browser/authenticator_request_client_delegate.cc

@@ -64,22 +64,9 @@ bool AuthenticatorRequestClientDelegate::SupportsResidentKeys() {
 void AuthenticatorRequestClientDelegate::SetMightCreateResidentCredential(
     bool v) {}
 
-bool AuthenticatorRequestClientDelegate::ShouldPermitCableExtension(
-    const url::Origin& origin) {
-  return false;
-}
-
-bool AuthenticatorRequestClientDelegate::SetCableTransportInfo(
-    bool cable_extension_provided,
-    bool have_paired_phones,
-    base::Optional<device::QRGeneratorKey> qr_generator_key) {
-  return false;
-}
-
-std::vector<device::CableDiscoveryData>
-AuthenticatorRequestClientDelegate::GetCablePairings() {
-  return {};
-}
+void AuthenticatorRequestClientDelegate::ConfigureCable(
+    const url::Origin& origin,
+    base::span<const device::CableDiscoveryData> pairings_from_extension) {}
 
 void AuthenticatorRequestClientDelegate::SelectAccount(
     std::vector<device::AuthenticatorGetAssertionResponse> responses,
@@ -185,4 +172,9 @@ void AuthenticatorRequestClientDelegate::OnInternalUserVerificationLocked() {}
 void AuthenticatorRequestClientDelegate::CustomizeDiscoveryFactory(
     device::FidoDiscoveryFactory* discovery_factory) {}
 
+device::FidoDiscoveryFactory*
+AuthenticatorRequestClientDelegate::discovery_factory() {
+  return discovery_factory_.get();
+}
+
 }  // namespace content

content/public/browser/authenticator_request_client_delegate.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/callback_forward.h"
+#include "base/containers/span.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "build/build_config.h"
@@ -139,43 +140,15 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   // authenticator and thus has privacy implications.
   void SetMightCreateResidentCredential(bool v) override;
 
-  // ShouldPermitCableExtension returns true if the given |origin| may set a
-  // caBLE extension. This extension contains website-chosen BLE pairing
-  // information that will be broadcast by the device and so should not be
-  // accepted if the embedder UI does not indicate that this is happening.
-  virtual bool ShouldPermitCableExtension(const url::Origin& origin);
-
-  // SetCableTransportInfo configures the embedder for handling Cloud-assisted
-  // Bluetooth Low Energy transports (i.e. using a phone as an authenticator).
-  // The |cable_extension_provided| argument is true if the site provided
-  // explicit caBLE discovery information. This is a hint that the UI may wish
-  // to advance to directly to guiding the user to check their phone as the site
-  // is strongly indicating that it will work.
-  //
-  // have_paired_phones is true if a previous call to |GetCablePairings|
-  // returned one or more caBLE pairings.
-  //
-  // |qr_generator_key| is a random AES-256 key that can be used to
-  // encrypt a coarse timestamp with |CableDiscoveryData::DeriveQRKeyMaterial|.
-  // The UI may display a QR code with the resulting secret which, if
-  // decoded and transmitted over BLE by an authenticator, will be accepted for
-  // caBLE pairing.
-  //
-  // This function returns true if the embedder will provide UI support for
-  // caBLE. If it returns false, all caBLE will be disabled because BLE
-  // broadcasting should not occur without user notification and accepting QR
-  // handshakes is irrelevant if the UI is not displaying the QR codes.
-  virtual bool SetCableTransportInfo(
-      bool cable_extension_provided,
-      bool have_paired_phones,
-      base::Optional<device::QRGeneratorKey> qr_generator_key);
-
-  // GetCablePairings returns any known caBLE pairing data. For example, the
-  // embedder may know of pairings because it configured the
-  // |FidoDiscoveryFactory| (using |CustomizeDiscoveryFactory|) to make a
-  // callback when a phone offered long-term pairing data. Additionally, it may
-  // know of pairings via some cloud-based service or sync feature.
-  virtual std::vector<device::CableDiscoveryData> GetCablePairings();
+  // ConfigureCable optionally configures Cloud-assisted Bluetooth Low Energy
+  // transports. |origin| is the origin of the calling site and
+  // |pairings_from_extension| are caBLEv1 pairings that have been provided in
+  // an extension to the WebAuthn get() call. If the embedder wishes, it may use
+  // this to configure caBLE on the |FidoDiscoveryFactory| for use in this
+  // request.
+  virtual void ConfigureCable(
+      const url::Origin& origin,
+      base::span<const device::CableDiscoveryData> pairings_from_extension);
 
   // SelectAccount is called to allow the embedder to select between one or more
   // accounts. This is triggered when the web page requests an unspecified
@@ -262,6 +235,7 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   // |discovery_factory|.
   virtual void CustomizeDiscoveryFactory(
       device::FidoDiscoveryFactory* discovery_factory);
+  device::FidoDiscoveryFactory* discovery_factory();
 
  private:
 #if !defined(OS_ANDROID)