Updating list of compromised credentials only if required

This change checks whether remote and local compromised credentials are
equal before updating local storage.

Bug: 1167109
Change-Id: I61737e97f76dfc5f3c6522a302fce6daaf634f52
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2637693
Commit-Queue: Viktor Semeniuk <vsemeniuk@google.com>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845180}

components/password_manager/core/browser/sync/password_sync_bridge.cc

@@ -536,9 +536,17 @@ base::Optional<syncer::ModelError> PasswordSyncBridge::MergeSyncData(
                                      /*sync_time=*/time_now);
         PasswordStoreChangeList changes =
             password_store_sync_->UpdateLoginSync(form, &update_login_error);
-        // TODO(1167109): Check if update is required.
-        password_store_sync_->UpdateCompromisedCredentialsSync(
-            form, CompromisedCredentialsFromEntityChange(remote_entity_change));
+        // Check if compromised credentials changed before updating.
+        std::vector<CompromisedCredentials> remote_compromised_credentials =
+            CompromisedCredentialsFromEntityChange(remote_entity_change);
+        std::vector<CompromisedCredentials> local_compromised_credentials =
+            password_store_sync_->ReadSecurityIssues(
+                FormPrimaryKey(primary_key));
+        if (!base::ranges::is_permutation(remote_compromised_credentials,
+                                          local_compromised_credentials)) {
+          password_store_sync_->UpdateCompromisedCredentialsSync(
+              form, remote_compromised_credentials);
+        }
         DCHECK_LE(changes.size(), 1U);
         base::UmaHistogramEnumeration(
             "PasswordManager.MergeSyncData.UpdateLoginSyncError",
@@ -752,9 +760,19 @@ base::Optional<syncer::ModelError> PasswordSyncBridge::ApplySyncChanges(
               PasswordFromEntityChange(*entity_change, /*sync_time=*/time_now);
           changes =
               password_store_sync_->UpdateLoginSync(form, &update_login_error);
-          // TODO(vsemeniuk): Check if update is required.
-          password_store_sync_->UpdateCompromisedCredentialsSync(
-              form, CompromisedCredentialsFromEntityChange(*entity_change));
+          FormPrimaryKey primary_key =
+              FormPrimaryKey(ParsePrimaryKey(entity_change->storage_key()));
+          // Check if compromised credentials changed before updating.
+          std::vector<CompromisedCredentials> remote_compromised_credentials =
+              CompromisedCredentialsFromEntityChange(*entity_change);
+          std::vector<CompromisedCredentials> local_compromised_credentials =
+              password_store_sync_->ReadSecurityIssues(
+                  FormPrimaryKey(primary_key));
+          if (!base::ranges::is_permutation(remote_compromised_credentials,
+                                            local_compromised_credentials)) {
+            password_store_sync_->UpdateCompromisedCredentialsSync(
+                form, remote_compromised_credentials);
+          }
           base::UmaHistogramEnumeration(
               "PasswordManager.ApplySyncChanges.UpdateLoginSyncError",
               update_login_error);
@@ -772,8 +790,7 @@ base::Optional<syncer::ModelError> PasswordSyncBridge::ApplySyncChanges(
                 FROM_HERE, "Failed to update an entry in the password store.");
           }
           DCHECK_EQ(1U, changes.size());
-          DCHECK(changes[0].primary_key() ==
-                 ParsePrimaryKey(entity_change->storage_key()));
+          DCHECK(changes[0].primary_key() == *primary_key);
           break;
         }
         case syncer::EntityChange::ACTION_DELETE: {

components/password_manager/core/browser/sync/password_sync_bridge_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/password_manager/core/browser/sync/password_sync_bridge.h"
 
 #include <memory>
+#include <random>
 #include <string>
 #include <utility>
 
@@ -1370,4 +1371,45 @@ TEST_F(PasswordSyncBridgeTest,
   EXPECT_FALSE(error);
 }
 
+TEST_F(PasswordSyncBridgeTest,
+       EqualCompromisedCredentialsRequireNoUpdateDuringRemoteUpdate) {
+  const int kPrimaryKey = 1000;
+  // Add a password form with a corresponding list of compromised credentials of
+  // types Leaked and Reused.
+  const std::string kStorageKey = "1000";
+  const PasswordForm kForm = MakePasswordForm(kSignonRealm1);
+  std::vector<CompromiseType> kIssuesTypes = {
+      CompromiseType::kLeaked, CompromiseType::kReused, CompromiseType::kWeak};
+  const std::vector<CompromisedCredentials> kIssues =
+      MakeCompromisedCredentials(kForm, kIssuesTypes);
+
+  fake_db()->AddLoginForPrimaryKey(kPrimaryKey, kForm);
+  fake_db()->AddCompromisedCredentials(kIssues);
+
+  // Simulate a remote update to the password that contains the same set of
+  // issues.
+  std::shuffle(kIssuesTypes.begin(), kIssuesTypes.end(),
+               std::default_random_engine{});
+  sync_pb::PasswordSpecifics specifics =
+      CreateSpecificsWithSignonRealmAndIssues(kSignonRealm1, kIssuesTypes);
+  sync_pb::PasswordSpecificsData* password_data =
+      specifics.mutable_client_only_encrypted_data();
+  password_data->set_times_used(1);
+
+  // Test that only UpdateLoginSync() is invoked,
+  // UpdateCompromisedCredentialsSync() isn't invoked because there are no
+  // change in the compromised credentials information.
+  EXPECT_CALL(*mock_password_store_sync(),
+              UpdateLoginSync(FormHasSignonRealm(kSignonRealm1), _));
+  EXPECT_CALL(*mock_password_store_sync(), UpdateCompromisedCredentialsSync)
+      .Times(0);
+
+  syncer::EntityChangeList entity_change_list;
+  entity_change_list.push_back(syncer::EntityChange::CreateUpdate(
+      kStorageKey, SpecificsToEntity(specifics)));
+  base::Optional<syncer::ModelError> error = bridge()->ApplySyncChanges(
+      bridge()->CreateMetadataChangeList(), std::move(entity_change_list));
+  EXPECT_FALSE(error);
+}
+
 }  // namespace password_manager