[PasswordManager] RemoveByKey() shouldn't decrypt the password value

LoginDatabase::RemoveLoginByPrimaryKey() returns a change list that
contains the forms that have been deleted. Therefore, it needs to first
load the forms from the database.

Before this CL:
LoginDatabase::RemoveLoginByPrimaryKey() would actually decrypt and
fill in the password value. Therefore, it fails in case of a decryption
failure.

After this CL:
Since it doesn't make sense to block the removal of a form in case of
decryption failure, LoginDatabase::RemoveLoginByPrimaryKey() won't
actually fill in the password value. Hence, the password in the change
list will be missing a password value. This change list is used to
communicate the change to other observers of the password store.
This should be fine because none of the password store observers is
interested in the password value in case of deletion.

Change-Id: I697e507584d77406280604a1481e437b80184890
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1503322
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637664}

components/password_manager/core/browser/login_database.cc

@@ -1073,19 +1073,20 @@ bool LoginDatabase::RemoveLogin(const PasswordForm& form,
 
 bool LoginDatabase::RemoveLoginByPrimaryKey(int primary_key,
                                             PasswordStoreChangeList* changes) {
-  PrimaryKeyToFormMap key_to_form_map;
+  PasswordForm form;
   if (changes) {
     changes->clear();
     sql::Statement s1(db_.GetCachedStatement(
         SQL_FROM_HERE, "SELECT * FROM logins WHERE id = ?"));
     s1.BindInt(0, primary_key);
-    // TODO(mamir): StatementToForms() fails if the password field couldn't be
-    // decrypted. However, this is not relevant for RemoveLoginByPrimaryKey(),
-    // and hence it shouldn't be used here.
-    if (StatementToForms(&s1, nullptr, &key_to_form_map) !=
-        FormRetrievalResult::kSuccess) {
+    if (!s1.Step()) {
       return false;
     }
+    int db_primary_key = -1;
+    EncryptionResult result = InitPasswordFormFromStatement(
+        s1, /*decrypt_and_fill_password_value=*/false, &db_primary_key, &form);
+    DCHECK_EQ(result, ENCRYPTION_RESULT_SUCCESS);
+    DCHECK_EQ(db_primary_key, primary_key);
   }
 
 #if defined(OS_IOS)
@@ -1099,8 +1100,8 @@ bool LoginDatabase::RemoveLoginByPrimaryKey(int primary_key,
     return false;
   }
   if (changes) {
-    changes->emplace_back(PasswordStoreChange::REMOVE,
-                          *key_to_form_map[primary_key], primary_key);
+    changes->emplace_back(PasswordStoreChange::REMOVE, std::move(form),
+                          primary_key);
   }
   return true;
 }
@@ -1211,11 +1212,13 @@ bool LoginDatabase::DisableAutoSignInForOrigin(const GURL& origin) {
 
 LoginDatabase::EncryptionResult LoginDatabase::InitPasswordFormFromStatement(
     const sql::Statement& s,
+    bool decrypt_and_fill_password_value,
     int* primary_key,
     PasswordForm* form) const {
   std::string encrypted_password;
   s.ColumnBlobAsString(COLUMN_PASSWORD_VALUE, &encrypted_password);
   base::string16 decrypted_password;
+  if (decrypt_and_fill_password_value) {
     EncryptionResult encryption_result =
         DecryptedString(encrypted_password, &decrypted_password);
     if (encryption_result != ENCRYPTION_RESULT_SUCCESS) {
@@ -1223,6 +1226,7 @@ LoginDatabase::EncryptionResult LoginDatabase::InitPasswordFormFromStatement(
               << encryption_result;
       return encryption_result;
     }
+  }
 
   *primary_key = s.ColumnInt(COLUMN_ID);
   std::string tmp = s.ColumnString(COLUMN_ORIGIN_URL);
@@ -1744,8 +1748,9 @@ FormRetrievalResult LoginDatabase::StatementToForms(
   while (statement->Step()) {
     auto new_form = std::make_unique<PasswordForm>();
     int primary_key = -1;
-    EncryptionResult result =
-        InitPasswordFormFromStatement(*statement, &primary_key, new_form.get());
+    EncryptionResult result = InitPasswordFormFromStatement(
+        *statement, /*decrypt_and_fill_password_value=*/true, &primary_key,
+        new_form.get());
     if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
       return FormRetrievalResult::kEncrytionServiceFailure;
     if (result == ENCRYPTION_RESULT_ITEM_FAILURE) {

components/password_manager/core/browser/login_database.h

@@ -256,11 +256,15 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
 
   // Fills |form| from the values in the given statement (which is assumed to be
   // of the form used by the Get*Logins methods). Fills the corresponding DB
-  // primary key in |primary_key|. Returns the EncryptionResult from decrypting
-  // the password in |s|; if not ENCRYPTION_RESULT_SUCCESS, |form| is not
-  // filled.
+  // primary key in |primary_key|. If |decrypt_and_fill_password_value| is set
+  // to true, it tries to decrypt the stored password and returns the
+  // EncryptionResult from decrypting the password in |s|; if not
+  // ENCRYPTION_RESULT_SUCCESS, |form| is not filled. If
+  // |decrypt_and_fill_password_value| is set to false, it always returns
+  // ENCRYPTION_RESULT_SUCCESS.
   EncryptionResult InitPasswordFormFromStatement(
       const sql::Statement& s,
+      bool decrypt_and_fill_password_value,
       int* primary_key,
       autofill::PasswordForm* form) const WARN_UNUSED_RESULT;
 

components/password_manager/core/browser/login_database_unittest.cc

@@ -427,6 +427,9 @@ TEST_F(LoginDatabaseTest, RemoveLoginsByPrimaryKey) {
   EXPECT_EQ(form, *result[0]);
   result.clear();
 
+  // RemoveLoginByPrimaryKey() doesn't decrypt or fill the password value.
+  form.password_value = ASCIIToUTF16("");
+
   EXPECT_TRUE(db().RemoveLoginByPrimaryKey(primary_key, &change_list));
   EXPECT_EQ(RemoveChangeForForm(form), change_list);
   EXPECT_TRUE(db().GetAutofillableLogins(&result));