[DeviceSync v2] Handle empty encrypted metadata

After http://cl/299870071, it is possible for CryptAuth to send device
metadata packets with empty |encrypted_metadata|. This can happen if a
device has not uploaded its metadata encrypted with the correct group
public key. Note: We still always expect nontrivial encrypted metadata
for the local device.

Bug: 951969
Change-Id: If18ddf4bc20559be47a7bf9382ab4fb8fcdd6d16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2097240
Auto-Submit: Josh Nohle <nohle@chromium.org>
Commit-Queue: James Vecore <vecore@google.com>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#749340}

chromeos/services/device_sync/cryptauth_device_syncer_impl.cc

@@ -522,11 +522,26 @@ void CryptAuthDeviceSyncerImpl::ProcessEncryptedDeviceMetadata() {
     const auto it =
         id_to_device_metadata_packet_map_.find(id_device_pair.first);
     DCHECK(it != id_to_device_metadata_packet_map_.end());
+
+    // Do not try to decrypt metadata that is not sent. This can happen if a
+    // device has not uploaded metadata encrypted with the correct group public
+    // key.
+    if (it->second.encrypted_metadata().empty())
+      continue;
+
     id_to_encrypted_metadata_map[id_device_pair.first] =
         CryptAuthEciesEncryptor::PayloadAndKey(it->second.encrypted_metadata(),
                                                group_key->private_key());
   }
 
+  if (id_to_encrypted_metadata_map.empty()) {
+    PA_LOG(ERROR) << "No encrypted metadata sent by CryptAuth. We expect the "
+                  << "local device's encrypted metadata, at a minimum.";
+    did_non_fatal_error_occur_ = true;
+    AttemptNextStep();
+    return;
+  }
+
   encryptor_ = CryptAuthEciesEncryptorImpl::Factory::Create();
   encryptor_->BatchDecrypt(
       id_to_encrypted_metadata_map,
@@ -556,41 +571,42 @@ void CryptAuthDeviceSyncerImpl::AddDecryptedMetadataToNewDeviceRegistry(
   DCHECK(new_device_registry_map_);
 
   // Update the new device registry with BetterTogether device metadata.
-  for (auto& id_device_pair : *new_device_registry_map_) {
-    cryptauthv2::BetterTogetherDeviceMetadata decrypted_metadata;
-
-    const auto it = id_to_decrypted_metadata_map.find(id_device_pair.first);
-    DCHECK(it != id_to_decrypted_metadata_map.end());
-
-    bool was_metadata_decrypted = it->second.has_value();
+  for (const auto& id_metadata_pair : id_to_decrypted_metadata_map) {
+    bool was_metadata_decrypted = id_metadata_pair.second.has_value();
     base::UmaHistogramBoolean(
         "CryptAuth.DeviceSyncV2.DeviceSyncer.MetadataDecryptionSuccess",
         was_metadata_decrypted);
     if (!was_metadata_decrypted) {
-      PA_LOG(ERROR) << "Metadata for device with Instance ID " << it->first
+      PA_LOG(ERROR) << "Metadata for device with Instance ID "
+                    << id_metadata_pair.first
                     << " was not able to be decrypted.";
       did_non_fatal_error_occur_ = true;
       continue;
     }
 
-    bool was_metadata_parsed = decrypted_metadata.ParseFromString(*it->second);
+    cryptauthv2::BetterTogetherDeviceMetadata decrypted_metadata;
+    bool was_metadata_parsed =
+        decrypted_metadata.ParseFromString(*id_metadata_pair.second);
     base::UmaHistogramBoolean(
         "CryptAuth.DeviceSyncV2.DeviceSyncer.MetadataParsingSuccess",
         was_metadata_parsed);
     if (!was_metadata_parsed) {
-      PA_LOG(ERROR) << "Metadata for device with Instance ID " << it->first
-                    << " was not able to be parsed.";
+      PA_LOG(ERROR) << "Metadata for device with Instance ID "
+                    << id_metadata_pair.first << " was not able to be parsed.";
       did_non_fatal_error_occur_ = true;
       continue;
     }
 
+    auto it = new_device_registry_map_->find(id_metadata_pair.first);
+    DCHECK(it != new_device_registry_map_->end());
+
     // The local device should already have its metadata set. Verify consistency
     // with data from CryptAuth.
-    if (id_device_pair.first == request_context_.device_id()) {
-      DCHECK(id_device_pair.second.better_together_device_metadata);
+    if (id_metadata_pair.first == request_context_.device_id()) {
+      DCHECK(it->second.better_together_device_metadata);
       bool is_local_device_metadata_consistent =
-          *it->second == id_device_pair.second.better_together_device_metadata
-                             ->SerializeAsString();
+          id_metadata_pair.second ==
+          it->second.better_together_device_metadata->SerializeAsString();
       base::UmaHistogramBoolean(
           "CryptAuth.DeviceSyncV2.DeviceSyncer.IsLocalDeviceMetadataConsistent",
           is_local_device_metadata_consistent);
@@ -601,11 +617,10 @@ void CryptAuthDeviceSyncerImpl::AddDecryptedMetadataToNewDeviceRegistry(
                       << "response.";
         did_non_fatal_error_occur_ = true;
       }
-
       continue;
     }
 
-    id_device_pair.second.better_together_device_metadata = decrypted_metadata;
+    it->second.better_together_device_metadata = decrypted_metadata;
   }
 }
 

chromeos/services/device_sync/cryptauth_device_syncer_impl_unittest.cc

@@ -508,6 +508,65 @@ TEST_F(DeviceSyncCryptAuthDeviceSyncerImplTest, Success_InitialGroupKeyValid) {
       GetAllTestDevices());
 }
 
+TEST_F(DeviceSyncCryptAuthDeviceSyncerImplTest,
+       Success_InitialGroupKeyValid_SomeDevicesHaveNoEncryptedMetadata) {
+  // Add the correct group key to the registry.
+  AddInitialGroupKeyToRegistry(GetGroupKey());
+
+  CallSync();
+
+  std::string encrypted_group_private_key = MakeFakeEncryptedString(
+      GetGroupKey().private_key(),
+      GetLocalDeviceForTest().device_better_together_public_key);
+
+  // In addition to the local device, remote devices are returned that do not
+  // have any encrypted metadata. This can happen if the remote device has not
+  // uploaded metadata encrypted with the correct group public key.
+  std::vector<cryptauthv2::DeviceMetadataPacket> device_metadata_packets =
+      GetAllTestDeviceMetadataPackets();
+  device_metadata_packets[1].clear_encrypted_metadata();
+  device_metadata_packets[2].clear_encrypted_metadata();
+
+  // The initial group key is valid, so a new group key was not created.
+  VerifyMetadataSyncerInput(&GetGroupKey());
+  FinishMetadataSyncerAttempt(
+      device_metadata_packets, base::nullopt /* new_group_key */,
+      encrypted_group_private_key, cryptauthv2::GetClientDirectiveForTest(),
+      CryptAuthDeviceSyncResult::ResultCode::kSuccess);
+
+  VerifyGroupKeyInRegistry(GetGroupKey());
+
+  VerifyFeatureStatusGetterInput(GetAllTestDeviceIds());
+  FinishFeatureStatusGetterAttempt(
+      GetAllTestDeviceIds(), CryptAuthDeviceSyncResult::ResultCode::kSuccess);
+
+  // Even though we have the unencrypted group private key in the key registry,
+  // we decrypt the group private key from CryptAuth and check consistency.
+  RunGroupPrivateKeyDecryptor(encrypted_group_private_key, true /* succeed */);
+
+  // We should only attempt to decrypt the local device metadata because the
+  // remote devices did not return any encrypted metadata.
+  RunDeviceMetadataDecryptor({GetLocalDeviceMetadataPacketForTest()},
+                             GetGroupKey().private_key(),
+                             {} /* device_ids_to_fail */);
+
+  VerifyGroupPrivateKeySharerInput(
+      GetGroupKey(), GetAllTestDeviceIdsThatNeedGroupPrivateKey());
+  FinishShareGroupPrivateKeyAttempt(
+      CryptAuthDeviceSyncResult::ResultCode::kSuccess);
+
+  // The remote devices did not send encrypted metadata.
+  std::vector<CryptAuthDevice> devices = GetAllTestDevices();
+  devices[1].better_together_device_metadata.reset();
+  devices[2].better_together_device_metadata.reset();
+
+  VerifyDeviceSyncResult(
+      CryptAuthDeviceSyncResult(CryptAuthDeviceSyncResult::ResultCode::kSuccess,
+                                true /* device_registry_changed */,
+                                cryptauthv2::GetClientDirectiveForTest()),
+      devices);
+}
+
 TEST_F(DeviceSyncCryptAuthDeviceSyncerImplTest,
        Success_InitialGroupKeyValid_NoDevicesNeedGroupPrivateKey) {
   // Add the correct group key to the registry.
@@ -923,6 +982,62 @@ TEST_F(DeviceSyncCryptAuthDeviceSyncerImplTest,
       GetAllTestDevices());
 }
 
+TEST_F(DeviceSyncCryptAuthDeviceSyncerImplTest,
+       NonFatalError_NoDevicesHaveEncryptedMetadata) {
+  // Add the correct group key to the registry.
+  AddInitialGroupKeyToRegistry(GetGroupKey());
+
+  CallSync();
+
+  std::string encrypted_group_private_key = MakeFakeEncryptedString(
+      GetGroupKey().private_key(),
+      GetLocalDeviceForTest().device_better_together_public_key);
+
+  // All metadata packets are missing encrypted metadata, though we always
+  // expect the local device to have encrypted metadata, at a minimum.
+  std::vector<cryptauthv2::DeviceMetadataPacket> device_metadata_packets =
+      GetAllTestDeviceMetadataPackets();
+  for (auto& packet : device_metadata_packets)
+    packet.clear_encrypted_metadata();
+
+  // The initial group key is valid, so a new group key was not created.
+  VerifyMetadataSyncerInput(&GetGroupKey());
+  FinishMetadataSyncerAttempt(
+      device_metadata_packets, base::nullopt /* new_group_key */,
+      encrypted_group_private_key, cryptauthv2::GetClientDirectiveForTest(),
+      CryptAuthDeviceSyncResult::ResultCode::kSuccess);
+
+  VerifyGroupKeyInRegistry(GetGroupKey());
+
+  VerifyFeatureStatusGetterInput(GetAllTestDeviceIds());
+  FinishFeatureStatusGetterAttempt(
+      GetAllTestDeviceIds(), CryptAuthDeviceSyncResult::ResultCode::kSuccess);
+
+  // Even though we have the unencrypted group private key in the key registry,
+  // we decrypt the group private key from CryptAuth and check consistency.
+  RunGroupPrivateKeyDecryptor(encrypted_group_private_key, true /* succeed */);
+
+  // The metadata decryptor will not be run because there is no metadata to
+  // decrypt.
+
+  VerifyGroupPrivateKeySharerInput(
+      GetGroupKey(), GetAllTestDeviceIdsThatNeedGroupPrivateKey());
+  FinishShareGroupPrivateKeyAttempt(
+      CryptAuthDeviceSyncResult::ResultCode::kSuccess);
+
+  // The local device is still able to populate its own metadata.
+  std::vector<CryptAuthDevice> devices = GetAllTestDevices();
+  devices[1].better_together_device_metadata.reset();
+  devices[2].better_together_device_metadata.reset();
+
+  VerifyDeviceSyncResult(
+      CryptAuthDeviceSyncResult(
+          CryptAuthDeviceSyncResult::ResultCode::kFinishedWithNonFatalErrors,
+          true /* device_registry_changed */,
+          cryptauthv2::GetClientDirectiveForTest()),
+      devices);
+}
+
 TEST_F(DeviceSyncCryptAuthDeviceSyncerImplTest,
        NonFatalError_MetadataDecryptionFailed) {
   AddInitialGroupKeyToRegistry(GetGroupKey());