device/fido: move UV mapping into request handlers.

This change moves the mapping of Webauthn's ternary UV preference to
CTAP2's binary value into the request handlers. This puts it next to the
logic that deals with PIN support.

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

content/browser/webauth/authenticator_impl_unittest.cc

@@ -2819,16 +2819,10 @@ TEST_F(InternalUVAuthenticatorImplTest, GetAssertion) {
                   callback_receiver.status());
       } else {
         EXPECT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
-
-        if (false) {
-          // TODO: make this true as I believe it's the correct behaviour.
-          EXPECT_EQ(
-              fingerprints_enrolled &&
-                  uv != blink::mojom::UserVerificationRequirement::DISCOURAGED,
-              HasUV(callback_receiver));
-        } else {
-          EXPECT_EQ(fingerprints_enrolled, HasUV(callback_receiver));
-        }
+        EXPECT_EQ(
+            fingerprints_enrolled &&
+                uv != blink::mojom::UserVerificationRequirement::DISCOURAGED,
+            HasUV(callback_receiver));
       }
     }
   }

device/fido/ctap_get_assertion_request.h

@@ -98,7 +98,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapGetAssertionRequest {
   std::string client_data_json_;
   std::array<uint8_t, kClientDataHashLength> client_data_hash_;
   UserVerificationRequirement user_verification_ =
-      UserVerificationRequirement::kPreferred;
+      UserVerificationRequirement::kDiscouraged;
   bool user_presence_required_ = true;
 
   base::Optional<std::vector<PublicKeyCredentialDescriptor>> allow_list_;

device/fido/ctap_make_credential_request.h

@@ -106,7 +106,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CtapMakeCredentialRequest {
   PublicKeyCredentialUserEntity user_;
   PublicKeyCredentialParams public_key_credential_params_;
   UserVerificationRequirement user_verification_ =
-      UserVerificationRequirement::kPreferred;
+      UserVerificationRequirement::kDiscouraged;
   AuthenticatorAttachment authenticator_attachment_ =
       AuthenticatorAttachment::kAny;
   bool resident_key_required_ = false;

device/fido/fido_authenticator.cc

@@ -45,6 +45,17 @@ void FidoAuthenticator::ChangePIN(const std::string& old_pin,
   NOTREACHED();
 }
 
+AuthenticatorSupportedOptions::ClientPinAvailability
+FidoAuthenticator::WillNeedPINToMakeCredential(const CtapMakeCredentialRequest&
+    request) {
+  return AuthenticatorSupportedOptions::ClientPinAvailability::kNotSupported;
+}
+
+bool FidoAuthenticator::WillNeedPINToGetAssertion(const
+    CtapGetAssertionRequest& request) {
+  return false;
+}
+
 void FidoAuthenticator::Reset(ResetCallback callback) {
   std::move(callback).Run(CtapDeviceResponseCode::kCtap1ErrInvalidCommand,
                           base::nullopt);

device/fido/fido_authenticator.h

@@ -100,6 +100,18 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
                          const std::string& new_pin,
                          pin::KeyAgreementResponse& peer_key,
                          SetPINCallback callback);
+  // WillNeedPINToMakeCredential returns what type of PIN intervention will be
+  // needed to serve
+  // the given request on this authenticator.
+  //   |kNotSupported|: no PIN involved.
+  //   |kSupportedButPinNotSet|: will need to set a new PIN.
+  //   |kSupportedAndPinSet|: will need to prompt for an existing PIN.
+  virtual AuthenticatorSupportedOptions::ClientPinAvailability
+    WillNeedPINToMakeCredential( const CtapMakeCredentialRequest& request);
+  // WillNeedPINToGetAssertion returns whether a PIN prompt will be needed to
+  // serve the given request on this authenticator.
+  virtual bool WillNeedPINToGetAssertion(const CtapGetAssertionRequest&
+      request);
   // Reset triggers a reset operation on the authenticator. This erases all
   // stored resident keys and any configured PIN.
   virtual void Reset(ResetCallback callback);

device/fido/fido_device_authenticator.cc

@@ -59,24 +59,6 @@ void FidoDeviceAuthenticator::MakeCredential(CtapMakeCredentialRequest request,
                                              MakeCredentialCallback callback) {
   DCHECK(device_->SupportedProtocolIsInitialized())
       << "InitializeAuthenticator() must be called first.";
-  DCHECK(Options());
-
-  // When PIN support is enabled, the mapping from Webauthn's ternary user-
-  // verification preference to CTAP2's binary option is done inside the request
-  // handler instead.
-  if (!base::FeatureList::IsEnabled(device::kWebAuthPINSupport) ||
-      request.user_verification() == UserVerificationRequirement::kPreferred) {
-    if (Options()->user_verification_availability ==
-        AuthenticatorSupportedOptions::UserVerificationAvailability::
-            kSupportedAndConfigured) {
-      request.SetUserVerification(UserVerificationRequirement::kRequired);
-    } else {
-      request.SetUserVerification(UserVerificationRequirement::kDiscouraged);
-    }
-  }
-
-  // TODO(martinkr): Change FidoTasks to take all request parameters by const
-  // reference, so we can avoid copying these from the RequestHandler.
   task_ = std::make_unique<MakeCredentialTask>(
       device_.get(), std::move(request), std::move(callback));
 }
@@ -85,26 +67,6 @@ void FidoDeviceAuthenticator::GetAssertion(CtapGetAssertionRequest request,
                                            GetAssertionCallback callback) {
   DCHECK(device_->SupportedProtocolIsInitialized())
       << "InitializeAuthenticator() must be called first.";
-  DCHECK(Options());
-  const bool pin_support =
-      base::FeatureList::IsEnabled(device::kWebAuthPINSupport);
-
-  // Update the request to the "effective" user verification requirement.
-  // https://w3c.github.io/webauthn/#effective-user-verification-requirement-for-assertion
-  if (!pin_support ||
-      request.user_verification() == UserVerificationRequirement::kPreferred) {
-    if (Options()->user_verification_availability ==
-            AuthenticatorSupportedOptions::UserVerificationAvailability::
-                kSupportedAndConfigured ||
-        (pin_support && Options()->client_pin_availability ==
-                            AuthenticatorSupportedOptions::
-                                ClientPinAvailability::kSupportedAndPinSet)) {
-      request.SetUserVerification(UserVerificationRequirement::kRequired);
-    } else {
-      request.SetUserVerification(UserVerificationRequirement::kDiscouraged);
-    }
-  }
-
   task_ = std::make_unique<GetAssertionTask>(device_.get(), std::move(request),
                                              std::move(callback));
 }
@@ -247,6 +209,34 @@ void FidoDeviceAuthenticator::ChangePIN(const std::string& old_pin,
   operation_->Start();
 }
 
+AuthenticatorSupportedOptions::ClientPinAvailability
+FidoDeviceAuthenticator::WillNeedPINToMakeCredential(const
+    CtapMakeCredentialRequest& request) {
+  if (request.user_verification() != UserVerificationRequirement::kRequired ||
+      Options()->user_verification_availability ==
+          AuthenticatorSupportedOptions::UserVerificationAvailability::
+              kSupportedAndConfigured) {
+    return AuthenticatorSupportedOptions::ClientPinAvailability::kNotSupported;
+  }
+
+  return Options()->client_pin_availability;
+}
+
+bool FidoDeviceAuthenticator::WillNeedPINToGetAssertion(
+    const CtapGetAssertionRequest& request) {
+  if (request.user_verification() ==
+          UserVerificationRequirement::kDiscouraged ||
+      Options()->user_verification_availability ==
+          AuthenticatorSupportedOptions::UserVerificationAvailability::
+              kSupportedAndConfigured) {
+    return false;
+  }
+
+  return Options()->client_pin_availability ==
+         AuthenticatorSupportedOptions::ClientPinAvailability::
+             kSupportedAndPinSet;
+}
+
 void FidoDeviceAuthenticator::Reset(ResetCallback callback) {
   DCHECK(device_->SupportedProtocolIsInitialized())
       << "InitializeAuthenticator() must be called first.";

device/fido/fido_device_authenticator.h

@@ -58,6 +58,13 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
                  const std::string& new_pin,
                  pin::KeyAgreementResponse& peer_key,
                  SetPINCallback callback) override;
+  AuthenticatorSupportedOptions::ClientPinAvailability
+    WillNeedPINToMakeCredential(
+      const CtapMakeCredentialRequest& request) override;
+  // WillNeedPINToGetAssertion returns whether a PIN prompt will be needed to
+  // serve the given request on this authenticator.
+  bool WillNeedPINToGetAssertion(const CtapGetAssertionRequest& request)
+    override;
   void Reset(ResetCallback callback) override;
   void Cancel() override;
   std::string GetId() const override;

device/fido/get_assertion_request_handler.cc

@@ -198,23 +198,6 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
 
 GetAssertionRequestHandler::~GetAssertionRequestHandler() = default;
 
-static bool WillNeedPIN(const CtapGetAssertionRequest& request,
-                        const FidoAuthenticator* authenticator) {
-  const auto& opt_options = authenticator->Options();
-  if (request.user_verification() ==
-          UserVerificationRequirement::kDiscouraged ||
-      !opt_options ||
-      opt_options->user_verification_availability ==
-          AuthenticatorSupportedOptions::UserVerificationAvailability::
-              kSupportedAndConfigured) {
-    return false;
-  }
-
-  return opt_options->client_pin_availability ==
-         AuthenticatorSupportedOptions::ClientPinAvailability::
-             kSupportedAndPinSet;
-}
-
 void GetAssertionRequestHandler::DispatchRequest(
     FidoAuthenticator* authenticator) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(my_sequence_checker_);
@@ -224,18 +207,33 @@ void GetAssertionRequestHandler::DispatchRequest(
     return;
   }
 
+  CtapGetAssertionRequest request(request_);
+
   if (base::FeatureList::IsEnabled(device::kWebAuthPINSupport) &&
-      WillNeedPIN(request_, authenticator)) {
-    CtapGetAssertionRequest request(request_);
+      authenticator->WillNeedPINToGetAssertion(request_)) {
+    request.SetUserVerification(UserVerificationRequirement::kRequired);
     // Set empty pinAuth to trigger just a touch.
     request.SetPinAuth({});
     authenticator->GetAssertion(
-        request, base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
-                                weak_factory_.GetWeakPtr(), authenticator));
+        std::move(request),
+        base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
+                       weak_factory_.GetWeakPtr(), authenticator));
   } else {
+    if (authenticator->Options()) {
+      if (authenticator->Options()->user_verification_availability ==
+              AuthenticatorSupportedOptions::UserVerificationAvailability::
+                  kSupportedAndConfigured &&
+          request_.user_verification() !=
+              UserVerificationRequirement::kDiscouraged) {
+        request.SetUserVerification(UserVerificationRequirement::kRequired);
+      } else {
+        request.SetUserVerification(UserVerificationRequirement::kDiscouraged);
+      }
+    }
     authenticator->GetAssertion(
-        request_, base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
-                                 weak_factory_.GetWeakPtr(), authenticator));
+        std::move(request),
+        base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
+                       weak_factory_.GetWeakPtr(), authenticator));
   }
 }
 
@@ -271,7 +269,7 @@ void GetAssertionRequestHandler::HandleResponse(
 
   if (state_ == State::kWaitingForTouch &&
       base::FeatureList::IsEnabled(device::kWebAuthPINSupport) &&
-      WillNeedPIN(request_, authenticator)) {
+      authenticator->WillNeedPINToGetAssertion(request_)) {
     if (response_code != CtapDeviceResponseCode::kCtap2ErrPinAuthInvalid &&
         response_code != CtapDeviceResponseCode::kCtap2ErrPinInvalid) {
       VLOG(1) << "Expected invalid pinAuth error but got "
@@ -417,12 +415,15 @@ void GetAssertionRequestHandler::OnHavePINToken(
 
   observer()->FinishCollectPIN();
   state_ = State::kWaitingForSecondTouch;
-  request_.SetPinAuth(response->PinAuth(request_.client_data_hash()));
-  request_.SetPinProtocol(pin::kProtocolVersion);
+  CtapGetAssertionRequest request(request_);
+  request.SetPinAuth(response->PinAuth(request.client_data_hash()));
+  request.SetPinProtocol(pin::kProtocolVersion);
+  request.SetUserVerification(UserVerificationRequirement::kRequired);
 
   authenticator_->GetAssertion(
-      request_, base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
-                               weak_factory_.GetWeakPtr(), authenticator_));
+      std::move(request),
+      base::BindOnce(&GetAssertionRequestHandler::HandleResponse,
+                     weak_factory_.GetWeakPtr(), authenticator_));
 }
 
 }  // namespace device

device/fido/get_assertion_task.cc

@@ -35,7 +35,12 @@ GetAssertionTask::GetAssertionTask(FidoDevice* device,
     : FidoTask(device),
       request_(std::move(request)),
       callback_(std::move(callback)),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  // The UV parameter should have been made binary by this point because CTAP2
+  // only takes a binary value.
+  DCHECK_NE(request_.user_verification(),
+            UserVerificationRequirement::kPreferred);
+}
 
 GetAssertionTask::~GetAssertionTask() = default;