[Sync::USS] Issue password db clean up in case of encryption failure.

The password manager stores the password in an encrypted form.
The decryption key is stored in platform-specific storage, but in
case that decryption key is lost (e.g. migrating the user directory
from a machine to another), the passwords in the stores will always
be undecryptable.

For syncing user, during initial merge, we try to clean up the password
store by deleting all undecryptable passwords. At merge time, the store
is empty and hence will be filled with data from the sync server.

This functionality has been implemented in the PasswordSyncableService
in the directory implementation. This CL implements the same
functionality in the PasswordSyncBridge the USS counter part.

It is worth noting though that the merge logic is run on every start
in the directory, while in USS, it's invoked only during initial sync.

Bug: 935996,902349
Change-Id: I755f887a3849176cb3b9a7e05f3d4c333f8f623c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1503315
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637673}

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

@@ -11,7 +11,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/common/password_form.h"
-#include "components/password_manager/core/browser/password_store_sync.h"
+#include "components/password_manager/core/common/password_manager_features.h"
 #include "components/sync/model/metadata_batch.h"
 #include "components/sync/model/metadata_change_list.h"
 #include "components/sync/model/model_type_change_processor.h"
@@ -145,6 +145,12 @@ bool AreLocalAndRemotePasswordsEqual(
                   .Serialize() == password_form.federation_origin.Serialize());
 }
 
+bool ShouldRecoverPasswordsDuringMerge() {
+  return base::FeatureList::IsEnabled(
+             features::kRecoverPasswordsForSyncUsers) &&
+         !base::FeatureList::IsEnabled(features::kDeleteCorruptedPasswords);
+}
+
 // A simple class for scoping a password store sync transaction. This does not
 // support rollback since the password store sync doesn't either.
 class ScopedStoreTransaction {
@@ -247,13 +253,36 @@ base::Optional<syncer::ModelError> PasswordSyncBridge::MergeSyncData(
 
   base::AutoReset<bool> processing_changes(&is_processing_remote_sync_changes_,
                                            true);
+
   // Read all local passwords.
   PrimaryKeyToFormMap key_to_local_form_map;
-  if (password_store_sync_->ReadAllLogins(&key_to_local_form_map) !=
-      FormRetrievalResult::kSuccess) {
+  FormRetrievalResult read_result =
+      password_store_sync_->ReadAllLogins(&key_to_local_form_map);
+
+  if (read_result == FormRetrievalResult::kDbError) {
     return syncer::ModelError(FROM_HERE,
                               "Failed to load entries from password store.");
   }
+  if (read_result == FormRetrievalResult::kEncrytionServiceFailure) {
+    if (!ShouldRecoverPasswordsDuringMerge()) {
+      return syncer::ModelError(FROM_HERE,
+                                "Failed to load entries from password store. "
+                                "Encryption service failure.");
+    }
+    base::Optional<syncer::ModelError> cleanup_result_error =
+        CleanupPasswordStore();
+    if (cleanup_result_error) {
+      return cleanup_result_error;
+    }
+    // Clean up done successfully, try to read again.
+    read_result = password_store_sync_->ReadAllLogins(&key_to_local_form_map);
+    if (read_result != FormRetrievalResult::kSuccess) {
+      return syncer::ModelError(
+          FROM_HERE,
+          "Failed to load entries from password store after cleanup.");
+    }
+  }
+  DCHECK_EQ(read_result, FormRetrievalResult::kSuccess);
 
   // Collect the client tags of remote passwords and the corresponding
   // EntityChange. Note that |entity_data| only contains client tag *hashes*.
@@ -559,4 +588,20 @@ void PasswordSyncBridge::ApplyStopSyncChanges(
       std::move(delete_metadata_change_list));
 }
 
+base::Optional<syncer::ModelError> PasswordSyncBridge::CleanupPasswordStore() {
+  DatabaseCleanupResult cleanup_result =
+      password_store_sync_->DeleteUndecryptableLogins();
+  switch (cleanup_result) {
+    case DatabaseCleanupResult::kSuccess:
+      break;
+    case DatabaseCleanupResult::kEncryptionUnavailable:
+      return syncer::ModelError(
+          FROM_HERE, "Failed to get encryption key during database cleanup.");
+    case DatabaseCleanupResult::kItemFailure:
+    case DatabaseCleanupResult::kDatabaseUnavailable:
+      return syncer::ModelError(FROM_HERE, "Failed to cleanup database.");
+  }
+  return base::nullopt;
+}
+
 }  // namespace password_manager

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

@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "base/sequence_checker.h"
 #include "components/password_manager/core/browser/password_store_change.h"
+#include "components/password_manager/core/browser/password_store_sync.h"
 #include "components/sync/model/model_type_sync_bridge.h"
 
 namespace syncer {
@@ -58,6 +59,13 @@ class PasswordSyncBridge : public syncer::ModelTypeSyncBridge {
                                 delete_metadata_change_list) override;
 
  private:
+  // On MacOS it may happen that some passwords cannot be decrypted due to
+  // modification of encryption key in Keychain (https://crbug.com/730625). This
+  // method deletes those logins from the store. So during merge, the data in
+  // sync will be added to the password store. This should be called during
+  // MergeSyncData().
+  base::Optional<syncer::ModelError> CleanupPasswordStore();
+
   // Password store responsible for persistence.
   PasswordStoreSync* const password_store_sync_;
 

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

@@ -697,4 +697,22 @@ TEST_F(PasswordSyncBridgeTest,
                             mock_password_store_sync());
 }
 
+// Tests that in case ReadAllLogins() during initial merge returns encryption
+// service failure, the bridge would try to do a DB clean up.
+TEST_F(PasswordSyncBridgeTest, ShouldDeleteUndecryptableLoginsDuringMerge) {
+  ON_CALL(*mock_password_store_sync(), DeleteUndecryptableLogins())
+      .WillByDefault(Return(DatabaseCleanupResult::kSuccess));
+
+  // We should try to read first, and simulate an encryption failure. Then,
+  // cleanup the database and try to read again which should be successful now.
+  EXPECT_CALL(*mock_password_store_sync(), ReadAllLogins(_))
+      .WillOnce(Return(FormRetrievalResult::kEncrytionServiceFailure))
+      .WillOnce(Return(FormRetrievalResult::kSuccess));
+  EXPECT_CALL(*mock_password_store_sync(), DeleteUndecryptableLogins());
+
+  base::Optional<syncer::ModelError> error =
+      bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(), {});
+  EXPECT_FALSE(error);
+}
+
 }  // namespace password_manager