fido: remove MakeCredentialRequestHandler dependency on AuthenticatorSelectionCriteria

Enumerate the relevant fields in MakeCredentialRequestHandler::Options
rather than pass in the entire AuthenticatorSelectionCriteria to
simplify things a little. No functional changes intended.

Bug: 1117630
Change-Id: I252d6a091b5b8f68d2558489ce1d528f5e5d5416
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2365134
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801959}

content/browser/webauth/authenticator_common.cc

@@ -613,8 +613,7 @@ void AuthenticatorCommon::StartMakeCredentialRequest(
       discovery_factory(),
       GetAvailableTransports(render_frame_host_, request_delegate_.get(),
                              discovery_factory(), caller_origin_),
-      *ctap_make_credential_request_, *authenticator_selection_criteria_,
-      *make_credential_options_,
+      *ctap_make_credential_request_, *make_credential_options_,
       base::BindOnce(&AuthenticatorCommon::OnRegisterResponse,
                      weak_factory_.GetWeakPtr()));
 
@@ -631,7 +630,7 @@ void AuthenticatorCommon::StartMakeCredentialRequest(
       base::BindRepeating(
           &device::FidoRequestHandlerBase::PowerOnBluetoothAdapter,
           request_->GetWeakPtr()) /* bluetooth_adapter_power_on_callback */);
-  if (authenticator_selection_criteria_->require_resident_key()) {
+  if (make_credential_options_->require_resident_key) {
     request_delegate_->SetMightCreateResidentCredential(true);
   }
   request_->set_observer(request_delegate_.get());
@@ -788,9 +787,9 @@ void AuthenticatorCommon::MakeCredential(
     return;
   }
 
-  authenticator_selection_criteria_ =
+  device::AuthenticatorSelectionCriteria authenticator_selection_criteria =
       options->authenticator_selection
-          ? options->authenticator_selection
+          ? *options->authenticator_selection
           : device::AuthenticatorSelectionCriteria();
 
   // Reject any non-sensical credProtect extension values.
@@ -808,7 +807,7 @@ void AuthenticatorCommon::MakeCredential(
       // UV_REQUIRED only makes sense if UV is required overall.
       (options->protection_policy ==
            blink::mojom::ProtectionPolicy::UV_REQUIRED &&
-       authenticator_selection_criteria_->user_verification_requirement() !=
+       authenticator_selection_criteria.user_verification_requirement() !=
            device::UserVerificationRequirement::kRequired)) {
     InvokeCallbackAndCleanup(
         std::move(callback),
@@ -837,11 +836,13 @@ void AuthenticatorCommon::MakeCredential(
       break;
   }
 
-  make_credential_options_.emplace();
-  if (cred_protect_request) {
-    make_credential_options_->cred_protect_request.emplace(
-        *cred_protect_request, options->enforce_protection_policy);
-  }
+  make_credential_options_ =
+      cred_protect_request
+          ? device::MakeCredentialRequestHandler::Options(
+                authenticator_selection_criteria, *cred_protect_request,
+                options->enforce_protection_policy)
+          : device::MakeCredentialRequestHandler::Options(
+                authenticator_selection_criteria);
 
   DCHECK(make_credential_response_callback_.is_null());
   make_credential_response_callback_ = std::move(callback);

content/browser/webauth/authenticator_common.h

@@ -193,8 +193,6 @@ class CONTENT_EXPORT AuthenticatorCommon {
   scoped_refptr<WebAuthRequestSecurityChecker> security_checker_;
   std::unique_ptr<base::OneShotTimer> timer_ =
       std::make_unique<base::OneShotTimer>();
-  base::Optional<device::AuthenticatorSelectionCriteria>
-      authenticator_selection_criteria_;
   base::Optional<std::string> app_id_;
   base::Optional<device::CtapMakeCredentialRequest>
       ctap_make_credential_request_;

device/fido/make_credential_request_handler.cc

@@ -79,7 +79,7 @@ base::Optional<MakeCredentialStatus> ConvertDeviceResponseCode(
 // should even blink for a request.
 bool IsCandidateAuthenticatorPreTouch(
     FidoAuthenticator* authenticator,
-    const AuthenticatorSelectionCriteria& authenticator_selection_criteria) {
+    AuthenticatorAttachment requested_attachment) {
   const auto& opt_options = authenticator->Options();
   if (!opt_options) {
     // This authenticator doesn't know its capabilities yet, so we need
@@ -88,11 +88,9 @@ bool IsCandidateAuthenticatorPreTouch(
     return true;
   }
 
-  if ((authenticator_selection_criteria.authenticator_attachment() ==
-           AuthenticatorAttachment::kPlatform &&
+  if ((requested_attachment == AuthenticatorAttachment::kPlatform &&
        !opt_options->is_platform_device) ||
-      (authenticator_selection_criteria.authenticator_attachment() ==
-           AuthenticatorAttachment::kCrossPlatform &&
+      (requested_attachment == AuthenticatorAttachment::kCrossPlatform &&
        opt_options->is_platform_device)) {
     return false;
   }
@@ -106,7 +104,6 @@ MakeCredentialStatus IsCandidateAuthenticatorPostTouch(
     const CtapMakeCredentialRequest& request,
     FidoAuthenticator* authenticator,
     const MakeCredentialRequestHandler::Options& options,
-    const AuthenticatorSelectionCriteria& authenticator_selection_criteria,
     const FidoRequestHandlerBase::Observer* observer) {
   if (options.cred_protect_request && options.cred_protect_request->second &&
       !authenticator->SupportsCredProtectExtension()) {
@@ -121,8 +118,7 @@ MakeCredentialStatus IsCandidateAuthenticatorPostTouch(
     return MakeCredentialStatus::kSuccess;
   }
 
-  if (authenticator_selection_criteria.require_resident_key() &&
-      !auth_options->supports_resident_key) {
+  if (options.require_resident_key && !auth_options->supports_resident_key) {
     return MakeCredentialStatus::kAuthenticatorMissingResidentKeys;
   }
 
@@ -160,10 +156,8 @@ MakeCredentialStatus IsCandidateAuthenticatorPostTouch(
 }
 
 base::flat_set<FidoTransportProtocol> GetTransportsAllowedByRP(
-    const AuthenticatorSelectionCriteria& authenticator_selection_criteria) {
-  const auto attachment_type =
-      authenticator_selection_criteria.authenticator_attachment();
-  switch (attachment_type) {
+    AuthenticatorAttachment authenticator_attachment) {
+  switch (authenticator_attachment) {
     case AuthenticatorAttachment::kPlatform:
       return {FidoTransportProtocol::kInternal};
     case AuthenticatorAttachment::kCrossPlatform:
@@ -329,23 +323,41 @@ bool ResponseValid(const FidoAuthenticator& authenticator,
 MakeCredentialRequestHandler::Options::Options() = default;
 MakeCredentialRequestHandler::Options::~Options() = default;
 MakeCredentialRequestHandler::Options::Options(const Options&) = default;
+MakeCredentialRequestHandler::Options::Options(
+    const AuthenticatorSelectionCriteria& authenticator_selection_criteria)
+    : authenticator_attachment(
+          authenticator_selection_criteria.authenticator_attachment()),
+      require_resident_key(
+          authenticator_selection_criteria.require_resident_key()),
+      user_verification(
+          authenticator_selection_criteria.user_verification_requirement()) {}
+MakeCredentialRequestHandler::Options::Options(
+    const AuthenticatorSelectionCriteria& authenticator_selection_criteria,
+    CredProtectRequest cred_protect_request_,
+    bool enforce_cred_protect_policy)
+    : Options(authenticator_selection_criteria) {
+  cred_protect_request.emplace(std::move(cred_protect_request_),
+                               enforce_cred_protect_policy);
+}
+MakeCredentialRequestHandler::Options::Options(Options&&) = default;
+MakeCredentialRequestHandler::Options&
+MakeCredentialRequestHandler::Options::operator=(const Options&) = default;
+MakeCredentialRequestHandler::Options&
+MakeCredentialRequestHandler::Options::operator=(Options&&) = default;
 
 MakeCredentialRequestHandler::MakeCredentialRequestHandler(
     FidoDiscoveryFactory* fido_discovery_factory,
     const base::flat_set<FidoTransportProtocol>& supported_transports,
     CtapMakeCredentialRequest request,
-    AuthenticatorSelectionCriteria authenticator_selection_criteria,
     const Options& options,
     CompletionCallback completion_callback)
     : FidoRequestHandlerBase(
           fido_discovery_factory,
           base::STLSetIntersection<base::flat_set<FidoTransportProtocol>>(
               supported_transports,
-              GetTransportsAllowedByRP(authenticator_selection_criteria))),
+              GetTransportsAllowedByRP(options.authenticator_attachment))),
       completion_callback_(std::move(completion_callback)),
       request_(std::move(request)),
-      authenticator_selection_criteria_(
-          std::move(authenticator_selection_criteria)),
       options_(options) {
   // These parts of the request should be filled in by
   // |SpecializeRequestForAuthenticator|.
@@ -357,19 +369,15 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
       FidoRequestHandlerBase::RequestType::kMakeCredential;
 
   // Set the rk, uv and attachment fields, which were only initialized to
-  // default values up to here.  TODO(martinkr): Initialize these fields earlier
-  // (in AuthenticatorImpl) and get rid of the separate
-  // AuthenticatorSelectionCriteriaParameter.
-  if (authenticator_selection_criteria_.require_resident_key()) {
+  // default values up to here.
+  if (options_.require_resident_key) {
     request_.resident_key_required = true;
     request_.user_verification = UserVerificationRequirement::kRequired;
   } else {
     request_.resident_key_required = false;
-    request_.user_verification =
-        authenticator_selection_criteria_.user_verification_requirement();
+    request_.user_verification = options_.user_verification;
   }
-  request_.authenticator_attachment =
-      authenticator_selection_criteria_.authenticator_attachment();
+  request_.authenticator_attachment = options_.authenticator_attachment;
 
   Start();
 }
@@ -382,7 +390,7 @@ void MakeCredentialRequestHandler::DispatchRequest(
 
   if (state_ != State::kWaitingForTouch ||
       !IsCandidateAuthenticatorPreTouch(authenticator,
-                                        authenticator_selection_criteria_)) {
+                                        options_.authenticator_attachment)) {
     return;
   }
 
@@ -391,7 +399,6 @@ void MakeCredentialRequestHandler::DispatchRequest(
   SpecializeRequestForAuthenticator(request.get(), authenticator);
 
   if (IsCandidateAuthenticatorPostTouch(*request.get(), authenticator, options_,
-                                        authenticator_selection_criteria_,
                                         observer()) !=
       MakeCredentialStatus::kSuccess) {
 #if defined(OS_WIN)
@@ -671,7 +678,6 @@ void MakeCredentialRequestHandler::HandleInapplicableAuthenticator(
   CancelActiveAuthenticators(authenticator->GetId());
   const MakeCredentialStatus capability_error =
       IsCandidateAuthenticatorPostTouch(*request.get(), authenticator, options_,
-                                        authenticator_selection_criteria_,
                                         observer());
   DCHECK_NE(capability_error, MakeCredentialStatus::kSuccess);
   std::move(completion_callback_).Run(capability_error, base::nullopt, nullptr);

device/fido/make_credential_request_handler.h

@@ -76,24 +76,52 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
   // |CtapMakeCredentialRequest|.
   struct COMPONENT_EXPORT(DEVICE_FIDO) Options {
     Options();
+    Options(
+        const AuthenticatorSelectionCriteria& authenticator_selection_criteria);
+    Options(
+        const AuthenticatorSelectionCriteria& authenticator_selection_criteria,
+        CredProtectRequest cred_protect_request,
+        bool enforce_cred_protect_policy);
     ~Options();
     Options(const Options&);
+    Options(Options&&);
+    Options& operator=(const Options&);
+    Options& operator=(Options&&);
 
-    bool allow_skipping_pin_touch = false;
-    base::Optional<AndroidClientDataExtensionInput> android_client_data_ext;
+    // authenticator_attachment is a constraint on the type of authenticator
+    // that a credential should be created on.
+    AuthenticatorAttachment authenticator_attachment =
+        AuthenticatorAttachment::kAny;
+
+    // require_resident_key indicates whether the request must result in the
+    // creation of a client-side discoverable credential (aka resident key).
+    bool require_resident_key = false;
+
+    // user_verification indicates whether the authenticator should (or must)
+    // perform user verficiation before creating the credential.
+    UserVerificationRequirement user_verification =
+        UserVerificationRequirement::kPreferred;
 
     // cred_protect_request extends |CredProtect| to include information that
     // applies at request-routing time. The second element is true if the
     // indicated protection level must be provided by the target authenticator
     // for the MakeCredential request to be sent.
     base::Optional<std::pair<CredProtectRequest, bool>> cred_protect_request;
+
+    // allow_skipping_pin_touch causes the handler to forego the first
+    // "touch-only" step to collect a PIN if exactly one authenticator is
+    // discovered.
+    bool allow_skipping_pin_touch = false;
+
+    // android_client_data_ext is a compatibility hack to support the Clank
+    // caBLEv2 authenticator.
+    base::Optional<AndroidClientDataExtensionInput> android_client_data_ext;
   };
 
   MakeCredentialRequestHandler(
       FidoDiscoveryFactory* fido_discovery_factory,
       const base::flat_set<FidoTransportProtocol>& supported_transports,
       CtapMakeCredentialRequest request_parameter,
-      AuthenticatorSelectionCriteria authenticator_criteria,
       const Options& options,
       CompletionCallback completion_callback);
   ~MakeCredentialRequestHandler() override;
@@ -176,7 +204,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
   State state_ = State::kWaitingForTouch;
   CtapMakeCredentialRequest request_;
   base::Optional<base::RepeatingClosure> bio_enrollment_complete_barrier_;
-  AuthenticatorSelectionCriteria authenticator_selection_criteria_;
   const Options options_;
 
   // authenticator_ points to the authenticator that will be used for this