fido: make transport fields optional

This changes the return values of
FidoAuthenticator::AuthenticatorTransport() and
AuthenticatorMakeCredentialResponse::transport_used() to
base::Optional<FidoTransportProtocol>.

This accommodates the Windows API integration use case where (a) the
FidoAuthenticator subclass cannot guarantee any particular transport
type and (b) the response we receive from the API contains no hints
about which transports were used.

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

chrome/browser/webauthn/authenticator_request_dialog_model.cc

@@ -349,9 +349,16 @@ void AuthenticatorRequestDialogModel::UpdateAuthenticatorReferenceId(
 
 void AuthenticatorRequestDialogModel::AddAuthenticator(
     const device::FidoAuthenticator& authenticator) {
+  if (!authenticator.AuthenticatorTransport()) {
+    // AuthenticatorTransport is nullopt if we cannot determine the transport
+    // upfront, i.e. for the Windows API authenticator. The UI will be
+    // suppressed for this authenticator, so we can simply ignore it here.
+    return;
+  }
   saved_authenticators_.AddAuthenticator(AuthenticatorReference(
       authenticator.GetId(), authenticator.GetDisplayName(),
-      authenticator.AuthenticatorTransport(), authenticator.IsInPairingMode()));
+      *authenticator.AuthenticatorTransport(),
+      authenticator.IsInPairingMode()));
 }
 
 void AuthenticatorRequestDialogModel::RemoveAuthenticator(

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -287,8 +287,9 @@ bool ChromeAuthenticatorRequestDelegate::EmbedderControlsAuthenticatorDispatch(
   if (!IsWebAuthnUiEnabled())
     return false;
 
-  return authenticator.AuthenticatorTransport() ==
-         device::FidoTransportProtocol::kInternal;
+  return authenticator.AuthenticatorTransport() &&
+         *authenticator.AuthenticatorTransport() ==
+             device::FidoTransportProtocol::kInternal;
 }
 
 void ChromeAuthenticatorRequestDelegate::FidoAuthenticatorAdded(

content/browser/webauth/authenticator_impl.cc

@@ -357,8 +357,10 @@ CreateMakeCredentialResponse(
 
   // The transport list must not contain duplicates but the order doesn't matter
   // because Blink will sort the resulting strings before returning them.
-  std::vector<device::FidoTransportProtocol> transports = {
-      response_data.transport_used()};
+  std::vector<device::FidoTransportProtocol> transports;
+  if (response_data.transport_used()) {
+    transports.push_back(*response_data.transport_used());
+  }
   // If the attestation certificate specifies that the token supports any other
   // transports, include them in the list.
   base::Optional<base::span<const uint8_t>> leaf_cert =
@@ -799,7 +801,7 @@ void AuthenticatorImpl::DidFinishNavigation(
 void AuthenticatorImpl::OnRegisterResponse(
     device::FidoReturnCode status_code,
     base::Optional<device::AuthenticatorMakeCredentialResponse> response_data,
-    device::FidoTransportProtocol transport_used) {
+    base::Optional<device::FidoTransportProtocol> transport_used) {
   if (!request_) {
     // Either the callback was called immediately and |request_| has not yet
     // been assigned (this is a bug), or a navigation caused the request to be
@@ -840,7 +842,9 @@ void AuthenticatorImpl::OnRegisterResponse(
       return;
     case device::FidoReturnCode::kSuccess:
       DCHECK(response_data.has_value());
-      request_delegate_->UpdateLastTransportUsed(transport_used);
+      if (transport_used) {
+        request_delegate_->UpdateLastTransportUsed(*transport_used);
+      }
 
       if (attestation_preference_ !=
           blink::mojom::AttestationConveyancePreference::NONE) {
@@ -859,7 +863,8 @@ void AuthenticatorImpl::OnRegisterResponse(
           AttestationErasureOption::kEraseAttestationAndAaguid;
       if (response_data->IsSelfAttestation()) {
         attestation_erasure = AttestationErasureOption::kIncludeAttestation;
-      } else if (transport_used == device::FidoTransportProtocol::kInternal) {
+      } else if (transport_used &&
+                 *transport_used == device::FidoTransportProtocol::kInternal) {
         // Contrary to what the WebAuthn spec says, for internal (platform)
         // authenticators we do not erase the AAGUID from authenticatorData,
         // even if requested attestationConveyancePreference is "none".
@@ -938,7 +943,7 @@ void AuthenticatorImpl::OnRegisterResponseAttestationDecided(
 void AuthenticatorImpl::OnSignResponse(
     device::FidoReturnCode status_code,
     base::Optional<device::AuthenticatorGetAssertionResponse> response_data,
-    device::FidoTransportProtocol transport_used) {
+    base::Optional<device::FidoTransportProtocol> transport_used) {
   if (!request_) {
     // Either the callback was called immediately and |request_| has not yet
     // been assigned (this is a bug), or a navigation caused the request to be
@@ -975,7 +980,9 @@ void AuthenticatorImpl::OnSignResponse(
       return;
     case device::FidoReturnCode::kSuccess:
       DCHECK(response_data.has_value());
-      request_delegate_->UpdateLastTransportUsed(transport_used);
+      if (transport_used) {
+        request_delegate_->UpdateLastTransportUsed(*transport_used);
+      }
 
       base::Optional<bool> echo_appid_extension;
       if (alternative_application_parameter_) {

content/browser/webauth/authenticator_impl.h

@@ -130,7 +130,7 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
   void OnRegisterResponse(
       device::FidoReturnCode status_code,
       base::Optional<device::AuthenticatorMakeCredentialResponse> response_data,
-      device::FidoTransportProtocol transport_used);
+      base::Optional<device::FidoTransportProtocol> transport_used);
 
   // Callback to complete the registration process once a decision about
   // whether or not to return attestation data has been made.
@@ -142,7 +142,7 @@ class CONTENT_EXPORT AuthenticatorImpl : public blink::mojom::Authenticator,
   void OnSignResponse(
       device::FidoReturnCode status_code,
       base::Optional<device::AuthenticatorGetAssertionResponse> response_data,
-      device::FidoTransportProtocol transport_used);
+      base::Optional<device::FidoTransportProtocol> transport_used);
 
   void FailWithNotAllowedErrorAndCleanup();
 

device/fido/authenticator_make_credential_response.cc

@@ -18,7 +18,7 @@ namespace device {
 // static
 base::Optional<AuthenticatorMakeCredentialResponse>
 AuthenticatorMakeCredentialResponse::CreateFromU2fRegisterResponse(
-    FidoTransportProtocol transport_used,
+    base::Optional<FidoTransportProtocol> transport_used,
     base::span<const uint8_t, kRpIdHashLength> relying_party_id_hash,
     base::span<const uint8_t> u2f_data) {
   auto public_key = ECPublicKey::ExtractFromU2fRegistrationResponse(
@@ -58,7 +58,7 @@ AuthenticatorMakeCredentialResponse::CreateFromU2fRegisterResponse(
 }
 
 AuthenticatorMakeCredentialResponse::AuthenticatorMakeCredentialResponse(
-    FidoTransportProtocol transport_used,
+    base::Optional<FidoTransportProtocol> transport_used,
     AttestationObject attestation_object)
     : ResponseData(attestation_object.GetCredentialId()),
       attestation_object_(std::move(attestation_object)),

device/fido/authenticator_make_credential_response.h

@@ -30,12 +30,13 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorMakeCredentialResponse
  public:
   static base::Optional<AuthenticatorMakeCredentialResponse>
   CreateFromU2fRegisterResponse(
-      FidoTransportProtocol transport_used,
+      base::Optional<FidoTransportProtocol> transport_used,
       base::span<const uint8_t, kRpIdHashLength> relying_party_id_hash,
       base::span<const uint8_t> u2f_data);
 
-  AuthenticatorMakeCredentialResponse(FidoTransportProtocol transport_used,
-                                      AttestationObject attestation_object);
+  AuthenticatorMakeCredentialResponse(
+      base::Optional<FidoTransportProtocol> transport_used,
+      AttestationObject attestation_object);
   AuthenticatorMakeCredentialResponse(
       AuthenticatorMakeCredentialResponse&& that);
   AuthenticatorMakeCredentialResponse& operator=(
@@ -66,13 +67,16 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorMakeCredentialResponse
     return attestation_object_;
   }
 
-  FidoTransportProtocol transport_used() const { return transport_used_; }
+  base::Optional<FidoTransportProtocol> transport_used() const {
+    return transport_used_;
+  }
 
  private:
   AttestationObject attestation_object_;
 
-  // Contains the transport used to register the credential in this case.
-  FidoTransportProtocol transport_used_;
+  // Contains the transport used to register the credential in this case. It is
+  // nullopt for cases where we cannot determine the transport (Windows).
+  base::Optional<FidoTransportProtocol> transport_used_;
 
   DISALLOW_COPY_AND_ASSIGN(AuthenticatorMakeCredentialResponse);
 };

device/fido/fido_authenticator.h

@@ -51,7 +51,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
   virtual base::string16 GetDisplayName() const = 0;
   virtual const base::Optional<AuthenticatorSupportedOptions>& Options()
       const = 0;
-  virtual FidoTransportProtocol AuthenticatorTransport() const = 0;
+  virtual base::Optional<FidoTransportProtocol> AuthenticatorTransport()
+      const = 0;
   virtual bool IsInPairingMode() const = 0;
   virtual base::WeakPtr<FidoAuthenticator> GetWeakPtr() = 0;
 

device/fido/fido_device_authenticator.cc

@@ -116,7 +116,8 @@ FidoDeviceAuthenticator::Options() const {
   return options_;
 }
 
-FidoTransportProtocol FidoDeviceAuthenticator::AuthenticatorTransport() const {
+base::Optional<FidoTransportProtocol>
+FidoDeviceAuthenticator::AuthenticatorTransport() const {
   return device_->DeviceTransport();
 }
 

device/fido/fido_device_authenticator.h

@@ -42,7 +42,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   std::string GetId() const override;
   base::string16 GetDisplayName() const override;
   const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
-  FidoTransportProtocol AuthenticatorTransport() const override;
+  base::Optional<FidoTransportProtocol> AuthenticatorTransport() const override;
   bool IsInPairingMode() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 

device/fido/fido_request_handler.h

@@ -25,10 +25,10 @@ namespace device {
 template <class Response>
 class FidoRequestHandler : public FidoRequestHandlerBase {
  public:
-  using CompletionCallback =
-      base::OnceCallback<void(FidoReturnCode status_code,
-                              base::Optional<Response> response_data,
-                              FidoTransportProtocol transport_used)>;
+  using CompletionCallback = base::OnceCallback<void(
+      FidoReturnCode status_code,
+      base::Optional<Response> response_data,
+      base::Optional<FidoTransportProtocol> transport_used)>;
 
   // The |available_transports| should be the intersection of transports
   // supported by the client and allowed by the relying party.

device/fido/fido_request_handler_base.cc

@@ -287,8 +287,9 @@ void FidoRequestHandlerBase::SetPlatformAuthenticatorOrMarkUnavailable(
                         FidoTransportProtocol::kInternal)) {
     DCHECK(platform_authenticator_info->authenticator);
     DCHECK(
-        (platform_authenticator_info->authenticator->AuthenticatorTransport() ==
-         FidoTransportProtocol::kInternal));
+        platform_authenticator_info->authenticator->AuthenticatorTransport() &&
+        *platform_authenticator_info->authenticator->AuthenticatorTransport() ==
+            FidoTransportProtocol::kInternal);
     transport_availability_info_.has_recognized_mac_touch_id_credential =
         platform_authenticator_info->has_recognized_mac_touch_id_credential;
     platform_authenticator_ =

device/fido/fido_request_handler_unittest.cc

@@ -33,14 +33,10 @@ namespace {
 using FakeTaskCallback =
     base::OnceCallback<void(CtapDeviceResponseCode status_code,
                             base::Optional<std::vector<uint8_t>>)>;
-using FakeHandlerCallback =
-    base::OnceCallback<void(FidoReturnCode status_code,
-                            base::Optional<std::vector<uint8_t>> response_data,
-                            FidoTransportProtocol)>;
-using FakeHandlerCallbackReceiver =
-    test::StatusAndValuesCallbackReceiver<FidoReturnCode,
-                                          base::Optional<std::vector<uint8_t>>,
-                                          FidoTransportProtocol>;
+using FakeHandlerCallbackReceiver = test::StatusAndValuesCallbackReceiver<
+    FidoReturnCode,
+    base::Optional<std::vector<uint8_t>>,
+    base::Optional<FidoTransportProtocol>>;
 
 enum class FakeTaskResponse : uint8_t {
   kSuccess = 0x00,
@@ -176,13 +172,13 @@ class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> {
  public:
   FakeFidoRequestHandler(service_manager::Connector* connector,
                          const base::flat_set<FidoTransportProtocol>& protocols,
-                         FakeHandlerCallback callback)
+                         CompletionCallback callback)
       : FidoRequestHandler(connector, protocols, std::move(callback)),
         weak_factory_(this) {
     Start();
   }
   FakeFidoRequestHandler(const base::flat_set<FidoTransportProtocol>& protocols,
-                         FakeHandlerCallback callback)
+                         CompletionCallback callback)
       : FakeFidoRequestHandler(nullptr /* connector */,
                                protocols,
                                std::move(callback)) {}

device/fido/get_assertion_handler_unittest.cc

@@ -43,7 +43,7 @@ constexpr uint8_t kBogusCredentialId[] = {0x01, 0x02, 0x03, 0x04};
 using TestGetAssertionRequestCallback = test::StatusAndValuesCallbackReceiver<
     FidoReturnCode,
     base::Optional<AuthenticatorGetAssertionResponse>,
-    FidoTransportProtocol>;
+    base::Optional<FidoTransportProtocol>>;
 
 }  // namespace
 

device/fido/get_assertion_request_handler.cc

@@ -76,13 +76,14 @@ bool CheckResponseCredentialIdMatchesRequestAllowList(
   }
   // Credential ID may be omitted if allow list has size 1. Otherwise, it needs
   // to match.
-  const auto transport_used = authenticator.AuthenticatorTransport();
+  const auto opt_transport_used = authenticator.AuthenticatorTransport();
   return (allow_list->size() == 1 && !response.credential()) ||
          std::any_of(allow_list->cbegin(), allow_list->cend(),
-                     [&response, transport_used](const auto& credential) {
+                     [&response, opt_transport_used](const auto& credential) {
                        return credential.id() == response.raw_credential_id() &&
-                              base::ContainsKey(credential.transports(),
-                                                transport_used);
+                              (!opt_transport_used ||
+                               base::ContainsKey(credential.transports(),
+                                                 *opt_transport_used));
                      });
 }
 
@@ -158,7 +159,7 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
     service_manager::Connector* connector,
     const base::flat_set<FidoTransportProtocol>& supported_transports,
     CtapGetAssertionRequest request,
-    SignResponseCallback completion_callback)
+    CompletionCallback completion_callback)
     : FidoRequestHandler(
           connector,
           base::STLSetIntersection<base::flat_set<FidoTransportProtocol>>(

device/fido/get_assertion_request_handler.h

@@ -25,11 +25,6 @@ namespace device {
 class FidoAuthenticator;
 class AuthenticatorGetAssertionResponse;
 
-using SignResponseCallback =
-    base::OnceCallback<void(FidoReturnCode,
-                            base::Optional<AuthenticatorGetAssertionResponse>,
-                            FidoTransportProtocol)>;
-
 class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
     : public FidoRequestHandler<AuthenticatorGetAssertionResponse> {
  public:
@@ -37,7 +32,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
       service_manager::Connector* connector,
       const base::flat_set<FidoTransportProtocol>& supported_transports,
       CtapGetAssertionRequest request_parameter,
-      SignResponseCallback completion_callback);
+      CompletionCallback completion_callback);
   ~GetAssertionRequestHandler() override;
 
  private:

device/fido/mac/authenticator.h

@@ -59,7 +59,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
   std::string GetId() const override;
   base::string16 GetDisplayName() const override;
   const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
-  FidoTransportProtocol AuthenticatorTransport() const override;
+  base::Optional<FidoTransportProtocol> AuthenticatorTransport() const override;
   bool IsInPairingMode() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 

device/fido/mac/authenticator.mm

@@ -136,7 +136,8 @@ base::string16 TouchIdAuthenticator::GetDisplayName() const {
   return base::string16();
 }
 
-FidoTransportProtocol TouchIdAuthenticator::AuthenticatorTransport() const {
+base::Optional<FidoTransportProtocol>
+TouchIdAuthenticator::AuthenticatorTransport() const {
   return FidoTransportProtocol::kInternal;
 }
 

device/fido/make_credential_handler_unittest.cc

@@ -39,7 +39,7 @@ namespace {
 using TestMakeCredentialRequestCallback = test::StatusAndValuesCallbackReceiver<
     FidoReturnCode,
     base::Optional<AuthenticatorMakeCredentialResponse>,
-    FidoTransportProtocol>;
+    base::Optional<FidoTransportProtocol>>;
 
 }  // namespace
 

device/fido/make_credential_request_handler.cc

@@ -87,7 +87,7 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
     const base::flat_set<FidoTransportProtocol>& supported_transports,
     CtapMakeCredentialRequest request,
     AuthenticatorSelectionCriteria authenticator_selection_criteria,
-    RegisterResponseCallback completion_callback)
+    CompletionCallback completion_callback)
     : FidoRequestHandler(
           connector,
           base::STLSetIntersection<base::flat_set<FidoTransportProtocol>>(

device/fido/make_credential_request_handler.h

@@ -27,11 +27,6 @@ namespace device {
 class FidoAuthenticator;
 class AuthenticatorMakeCredentialResponse;
 
-using RegisterResponseCallback =
-    base::OnceCallback<void(FidoReturnCode,
-                            base::Optional<AuthenticatorMakeCredentialResponse>,
-                            FidoTransportProtocol)>;
-
 class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
     : public FidoRequestHandler<AuthenticatorMakeCredentialResponse> {
  public:
@@ -40,7 +35,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
       const base::flat_set<FidoTransportProtocol>& supported_transports,
       CtapMakeCredentialRequest request_parameter,
       AuthenticatorSelectionCriteria authenticator_criteria,
-      RegisterResponseCallback completion_callback);
+      CompletionCallback completion_callback);
   ~MakeCredentialRequestHandler() override;
 
   // FidoRequestHandlerBase:

device/fido/win/authenticator.cc

@@ -355,11 +355,11 @@ bool WinNativeCrossPlatformAuthenticator::IsInPairingMode() const {
   return false;
 }
 
-FidoTransportProtocol
+base::Optional<FidoTransportProtocol>
 WinNativeCrossPlatformAuthenticator::AuthenticatorTransport() const {
-  // TODO(martinkr): Note that this may not necessarily be true if not in
-  // U2F-only mode, in which case the authenticator might be NFC, too.
-  return FidoTransportProtocol::kUsbHumanInterfaceDevice;
+  // The Windows API could potentially use any external or
+  // platform authenticator.
+  return base::nullopt;
 }
 
 const base::Optional<AuthenticatorSupportedOptions>&

device/fido/win/authenticator.h

@@ -54,7 +54,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) WinNativeCrossPlatformAuthenticator
   base::string16 GetDisplayName() const override;
   bool IsInPairingMode() const override;
   const base::Optional<AuthenticatorSupportedOptions>& Options() const override;
-  FidoTransportProtocol AuthenticatorTransport() const override;
+  base::Optional<FidoTransportProtocol> AuthenticatorTransport() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 
  private:

device/fido/win/type_conversions.cc

@@ -43,11 +43,7 @@ ToAuthenticatorMakeCredentialResponse(
   }
 
   return AuthenticatorMakeCredentialResponse(
-      // TODO(martinkr): Ultimately, we cannot be sure that the transport is
-      // really USB. We exclude platform by forcing authenticatorAttachment to
-      // "cross-platform"; but that still leaves NFC vs USB. Perhaps we should
-      // wrap this field in a base::Optional?
-      FidoTransportProtocol::kUsbHumanInterfaceDevice,
+      base::nullopt /* transport_used */,
       AttestationObject(
           std::move(*authenticator_data),
           std::make_unique<OpaqueAttestationStatement>(