device/fido: appidExclude moot if nothing excluded.

If there are no excluded credentials then appidExclude shouldn't matter.
Previously we hit an assertion failure and crashed in this case. (See
bug.) This was introduced with M80.

BUG=1054499

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

content/browser/webauth/authenticator_impl_unittest.cc

@@ -1259,6 +1259,46 @@ TEST_F(AuthenticatorImplTest, AppIdExcludeExtension) {
       }
     }
   }
+
+  {
+    // Using appidExclude with an empty exclude list previously caused a crash.
+    // See https://bugs.chromium.org/p/chromium/issues/detail?id=1054499.
+    virtual_device_factory_->SetSupportedProtocol(
+        device::ProtocolVersion::kCtap2);
+    PublicKeyCredentialCreationOptionsPtr options =
+        GetTestPublicKeyCredentialCreationOptions();
+    options->appid_exclude = kTestOrigin1;
+    options->exclude_credentials.clear();
+    TestMakeCredentialCallback callback_receiver;
+    authenticator->MakeCredential(std::move(options),
+                                  callback_receiver.callback());
+    callback_receiver.WaitForCallback();
+    ASSERT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+  }
+
+  {
+    // Also test the case where all credential IDs are eliminated because of
+    // their size.
+    device::VirtualCtap2Device::Config config;
+    config.max_credential_count_in_list = 1;
+    config.max_credential_id_length = 1;
+    virtual_device_factory_->SetCtap2Config(config);
+
+    PublicKeyCredentialCreationOptionsPtr options =
+        GetTestPublicKeyCredentialCreationOptions();
+    options->appid_exclude = kTestOrigin1;
+    options->exclude_credentials = GetTestCredentials();
+
+    for (const auto& cred : options->exclude_credentials) {
+      ASSERT_GT(cred.id().size(), config.max_credential_id_length);
+    }
+
+    TestMakeCredentialCallback callback_receiver;
+    authenticator->MakeCredential(std::move(options),
+                                  callback_receiver.callback());
+    callback_receiver.WaitForCallback();
+    ASSERT_EQ(AuthenticatorStatus::SUCCESS, callback_receiver.status());
+  }
 }
 
 TEST_F(AuthenticatorImplTest, TestGetAssertionTimeout) {

device/fido/make_credential_task.cc

@@ -142,31 +142,25 @@ CtapGetAssertionRequest MakeCredentialTask::NextSilentRequest() {
 void MakeCredentialTask::MakeCredential() {
   DCHECK_EQ(device()->supported_protocol(), ProtocolVersion::kCtap2);
 
-  if (!request_.app_id && request_.exclude_list.empty()) {
-    register_operation_ = std::make_unique<Ctap2DeviceOperation<
-        CtapMakeCredentialRequest, AuthenticatorMakeCredentialResponse>>(
-        device(), request_, std::move(callback_),
-        base::BindOnce(&ReadCTAPMakeCredentialResponse,
-                       device()->DeviceTransport()),
-        /*string_fixup_predicate=*/nullptr);
-    register_operation_->Start();
-    return;
-  }
-
   // Most authenticators can only process excludeList parameters up to a certain
   // size. Batch the list into chunks according to what the device can handle
   // and filter out IDs that are too large to originate from this device.
   exclude_list_batches_ =
       FilterAndBatchCredentialDescriptors(request_.exclude_list, *device());
+  DCHECK(!exclude_list_batches_.empty());
 
   // If the filtered excludeList is small enough to be sent in a single request,
-  // do so. (Note that the list may be empty now, even if it wasn't previously,
-  // due to filtering.)
-  if (!request_.app_id && exclude_list_batches_.size() <= 1) {
+  // do so. (Note that the exclude list may be empty now, even if it wasn't
+  // previously, due to filtering.)
+  //
+  // Handling appidExclude requires that the |HandleResponseToSilentSignRequest|
+  // path be used below, so this is only valid if either there's no
+  // appidExclude, or the single batch is empty and thus there are no excluded
+  // credentials.
+  if (exclude_list_batches_.size() == 1 &&
+      (!request_.app_id || exclude_list_batches_.front().empty())) {
     auto request = request_;
-    request.exclude_list = exclude_list_batches_.empty()
-                               ? std::vector<PublicKeyCredentialDescriptor>{}
-                               : exclude_list_batches_.front();
+    request.exclude_list = exclude_list_batches_.front();
     register_operation_ = std::make_unique<Ctap2DeviceOperation<
         CtapMakeCredentialRequest, AuthenticatorMakeCredentialResponse>>(
         device(), std::move(request), std::move(callback_),
@@ -194,8 +188,6 @@ void MakeCredentialTask::MakeCredential() {
 void MakeCredentialTask::HandleResponseToSilentSignRequest(
     CtapDeviceResponseCode response_code,
     base::Optional<AuthenticatorGetAssertionResponse> response_data) {
-  DCHECK(!request_.exclude_list.empty());
-
   if (canceled_) {
     return;
   }
@@ -316,7 +308,6 @@ std::vector<std::vector<PublicKeyCredentialDescriptor>>
 FilterAndBatchCredentialDescriptors(
     const std::vector<PublicKeyCredentialDescriptor>& in,
     const FidoDevice& device) {
-  DCHECK(!in.empty());
   DCHECK_EQ(device.supported_protocol(), ProtocolVersion::kCtap2);
   DCHECK(device.device_info().has_value());
 
@@ -342,13 +333,14 @@ FilterAndBatchCredentialDescriptors(
           : 1;
 
   std::vector<std::vector<PublicKeyCredentialDescriptor>> result;
+  result.emplace_back();
+
   for (const PublicKeyCredentialDescriptor& credential : in) {
     if (0 < max_credential_id_length &&
         max_credential_id_length < credential.id().size()) {
       continue;
     }
-    if (result.empty() ||
-        result.back().size() == max_credential_count_in_list) {
+    if (result.back().size() == max_credential_count_in_list) {
       result.emplace_back();
     }
     result.back().push_back(credential);

device/fido/make_credential_task.h

@@ -94,9 +94,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialTask : public FidoTask {
 // |device| must be a fully initialized CTAP2 device, i.e. its device_info()
 // method must return an AuthenticatorGetInfoResponse.
 //
-// If |in| contains only credential descriptors with IDs longer than the
-// device's |max_credential_id_length|, the result will be empty (rather than
-// containing a single empty vector).
+// The result will never be empty. It will, at least, contain a single empty
+// vector.
 std::vector<std::vector<PublicKeyCredentialDescriptor>>
 FilterAndBatchCredentialDescriptors(
     const std::vector<PublicKeyCredentialDescriptor>& in,