[PasswordManager] StatementToForms() should return fine-grained result

Before this CL:
StatmentToForms() used to return false in case of DB error or Decryption
failure. This doesn't allow upper layers from acting differently in each
case.

After this CL:
StatmentToForms() return one of three outcomes:
SUCCESS, DB_ERROR, ENCRYPTION_SERVICE_FAILURE
This allow later patches to use of the outcome and do cleanup of
passwords don't are not decryptable.

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

chrome/browser/password_manager/password_store_x.cc

@@ -555,7 +555,7 @@ void PasswordStoreX::ShutdownOnUIThread() {
   PasswordStoreDefault::ShutdownOnUIThread();
 }
 
-bool PasswordStoreX::ReadAllLogins(
+password_manager::FormRetrievalResult PasswordStoreX::ReadAllLogins(
     password_manager::PrimaryKeyToFormMap* key_to_form_map) {
   // This method is called from the PasswordSyncBridge which supports only
   // PasswordStoreDefault. Therefore, on Linux, it should be called only if the

chrome/browser/password_manager/password_store_x.h

@@ -139,7 +139,7 @@ class PasswordStoreX : public password_manager::PasswordStoreDefault {
 
  protected:
   // Implements PasswordStoreSync interface.
-  bool ReadAllLogins(
+  password_manager::FormRetrievalResult ReadAllLogins(
       password_manager::PrimaryKeyToFormMap* key_to_form_map) override;
   password_manager::PasswordStoreChangeList RemoveLoginByPrimaryKeySync(
       int primary_key) override;

components/password_manager/core/browser/login_database.cc

@@ -1079,7 +1079,11 @@ bool LoginDatabase::RemoveLoginByPrimaryKey(int primary_key,
     sql::Statement s1(db_.GetCachedStatement(
         SQL_FROM_HERE, "SELECT * FROM logins WHERE id = ?"));
     s1.BindInt(0, primary_key);
-    if (!StatementToForms(&s1, nullptr, &key_to_form_map)) {
+    // 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) {
       return false;
     }
   }
@@ -1189,11 +1193,11 @@ bool LoginDatabase::GetAutoSignInLogins(
   sql::Statement s(
       db_.GetCachedStatement(SQL_FROM_HERE, autosignin_statement_.c_str()));
   PrimaryKeyToFormMap key_to_form_map;
-  bool result = StatementToForms(&s, nullptr, &key_to_form_map);
+  FormRetrievalResult result = StatementToForms(&s, nullptr, &key_to_form_map);
   for (auto& pair : key_to_form_map) {
     forms->push_back(std::move(pair.second));
   }
-  return result;
+  return result == FormRetrievalResult::kSuccess;
 }
 
 bool LoginDatabase::DisableAutoSignInForOrigin(const GURL& origin) {
@@ -1354,10 +1358,10 @@ bool LoginDatabase::GetLogins(
                               PSL_DOMAIN_MATCH_COUNT);
   }
   PrimaryKeyToFormMap key_to_form_map;
-  bool success = StatementToForms(
+  FormRetrievalResult result = StatementToForms(
       &s, should_PSL_matching_apply || should_federated_apply ? &form : nullptr,
       &key_to_form_map);
-  if (!success) {
+  if (result != FormRetrievalResult::kSuccess) {
     return false;
   }
   for (auto& pair : key_to_form_map) {
@@ -1394,7 +1398,7 @@ bool LoginDatabase::GetLoginsForSameOrganizationName(
   s.BindString(0, signon_realms_with_same_organization_name_regexp);
 
   PrimaryKeyToFormMap key_to_form_map;
-  bool success = StatementToForms(&s, nullptr, &key_to_form_map);
+  FormRetrievalResult result = StatementToForms(&s, nullptr, &key_to_form_map);
   for (auto& pair : key_to_form_map) {
     forms->push_back(std::move(pair.second));
   }
@@ -1409,7 +1413,7 @@ bool LoginDatabase::GetLoginsForSameOrganizationName(
     return candidate_form_organization_name != organization_name;
   });
 
-  return success;
+  return result == FormRetrievalResult::kSuccess;
 }
 
 bool LoginDatabase::GetLoginsCreatedBetween(
@@ -1424,7 +1428,8 @@ bool LoginDatabase::GetLoginsCreatedBetween(
   s.BindInt64(1, end.is_null() ? std::numeric_limits<int64_t>::max()
                                : end.ToInternalValue());
 
-  return StatementToForms(&s, nullptr, key_to_form_map);
+  return StatementToForms(&s, nullptr, key_to_form_map) ==
+         FormRetrievalResult::kSuccess;
 }
 
 bool LoginDatabase::GetLoginsSyncedBetween(
@@ -1440,10 +1445,12 @@ bool LoginDatabase::GetLoginsSyncedBetween(
               end.is_null() ? base::Time::Max().ToInternalValue()
                             : end.ToInternalValue());
 
-  return StatementToForms(&s, nullptr, key_to_form_map);
+  return StatementToForms(&s, nullptr, key_to_form_map) ==
+         FormRetrievalResult::kSuccess;
 }
 
-bool LoginDatabase::GetAllLogins(PrimaryKeyToFormMap* key_to_form_map) {
+FormRetrievalResult LoginDatabase::GetAllLogins(
+    PrimaryKeyToFormMap* key_to_form_map) {
   DCHECK(key_to_form_map);
   key_to_form_map->clear();
 
@@ -1476,7 +1483,8 @@ bool LoginDatabase::GetAllLoginsWithBlacklistSetting(
 
   PrimaryKeyToFormMap key_to_form_map;
 
-  if (!StatementToForms(&s, nullptr, &key_to_form_map)) {
+  if (StatementToForms(&s, nullptr, &key_to_form_map) !=
+      FormRetrievalResult::kSuccess) {
     return false;
   }
 
@@ -1724,7 +1732,7 @@ std::unique_ptr<sync_pb::ModelTypeState> LoginDatabase::GetModelTypeState() {
   return nullptr;
 }
 
-bool LoginDatabase::StatementToForms(
+FormRetrievalResult LoginDatabase::StatementToForms(
     sql::Statement* statement,
     const PasswordStore::FormDigest* matched_form,
     PrimaryKeyToFormMap* key_to_form_map) {
@@ -1739,7 +1747,7 @@ bool LoginDatabase::StatementToForms(
     EncryptionResult result =
         InitPasswordFormFromStatement(*statement, &primary_key, new_form.get());
     if (result == ENCRYPTION_RESULT_SERVICE_FAILURE)
-      return false;
+      return FormRetrievalResult::kEncrytionServiceFailure;
     if (result == ENCRYPTION_RESULT_ITEM_FAILURE) {
       if (IsUsingCleanupMechanism())
         forms_to_be_deleted.push_back(GetFormForRemoval(*statement));
@@ -1800,8 +1808,8 @@ bool LoginDatabase::StatementToForms(
 #endif
 
   if (!statement->Succeeded())
-    return false;
-  return true;
+    return FormRetrievalResult::kDbError;
+  return FormRetrievalResult::kSuccess;
 }
 
 void LoginDatabase::InitializeStatementStrings(const SQLTableBuilder& builder) {

components/password_manager/core/browser/login_database.h

@@ -145,7 +145,8 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
       WARN_UNUSED_RESULT;
 
   // Gets the complete list of all credentials.
-  bool GetAllLogins(PrimaryKeyToFormMap* key_to_form_map) WARN_UNUSED_RESULT;
+  FormRetrievalResult GetAllLogins(PrimaryKeyToFormMap* key_to_form_map)
+      WARN_UNUSED_RESULT;
 
   // Gets the complete list of not blacklisted credentials.
   bool GetAutofillableLogins(
@@ -290,17 +291,18 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
   // Reads the stored ModelTypeState. Returns nullptr in case of failure.
   std::unique_ptr<sync_pb::ModelTypeState> GetModelTypeState();
 
-  // Overwrites |forms| with credentials retrieved from |statement|. If
-  // |matched_form| is not null, filters out all results but those PSL-matching
+  // Overwrites |key_to_form_map| with credentials retrieved from |statement|.
+  // If |matched_form| is not null, filters out all results but those
+  // PSL-matching
   // |*matched_form| or federated credentials for it. If feature for recovering
   // passwords is enabled, it removes all passwords that couldn't be decrypted
   // when encryption was available from the database. On success returns true.
   // |key_to_form_map| must not be null and will be used to return the results.
   // The key of the map is the DB primary key.
-  bool StatementToForms(sql::Statement* statement,
-                        const PasswordStore::FormDigest* matched_form,
-                        PrimaryKeyToFormMap* key_to_form_map)
-      WARN_UNUSED_RESULT;
+  FormRetrievalResult StatementToForms(
+      sql::Statement* statement,
+      const PasswordStore::FormDigest* matched_form,
+      PrimaryKeyToFormMap* key_to_form_map) WARN_UNUSED_RESULT;
 
   // Initializes all the *_statement_ data members with appropriate SQL
   // fragments based on |builder|.

components/password_manager/core/browser/login_database_unittest.cc

@@ -277,7 +277,7 @@ TEST_F(LoginDatabaseTest, Logins) {
   EXPECT_TRUE(db().GetAutofillableLogins(&result));
   EXPECT_EQ(0U, result.size());
 
-  EXPECT_TRUE(db().GetAllLogins(&key_to_form_map));
+  EXPECT_EQ(db().GetAllLogins(&key_to_form_map), FormRetrievalResult::kSuccess);
   EXPECT_EQ(0U, key_to_form_map.size());
 
   // Example password form.
@@ -294,7 +294,7 @@ TEST_F(LoginDatabaseTest, Logins) {
   EXPECT_EQ(form, *result[0]);
   result.clear();
 
-  EXPECT_TRUE(db().GetAllLogins(&key_to_form_map));
+  EXPECT_EQ(db().GetAllLogins(&key_to_form_map), FormRetrievalResult::kSuccess);
   EXPECT_EQ(1U, key_to_form_map.size());
   EXPECT_EQ(form, *key_to_form_map[1]);
   key_to_form_map.clear();

components/password_manager/core/browser/mock_password_store.h

@@ -90,7 +90,7 @@ class MockPasswordStore : public PasswordStore {
 #endif
   MOCK_METHOD0(BeginTransaction, bool());
   MOCK_METHOD0(CommitTransaction, bool());
-  MOCK_METHOD1(ReadAllLogins, bool(PrimaryKeyToFormMap*));
+  MOCK_METHOD1(ReadAllLogins, FormRetrievalResult(PrimaryKeyToFormMap*));
   MOCK_METHOD1(RemoveLoginByPrimaryKeySync, PasswordStoreChangeList(int));
   MOCK_METHOD0(GetMetadataStore, PasswordStoreSync::MetadataStore*());
 

components/password_manager/core/browser/password_store_default.cc

@@ -239,9 +239,12 @@ bool PasswordStoreDefault::CommitTransaction() {
   return false;
 }
 
-bool PasswordStoreDefault::ReadAllLogins(PrimaryKeyToFormMap* key_to_form_map) {
+FormRetrievalResult PasswordStoreDefault::ReadAllLogins(
+    PrimaryKeyToFormMap* key_to_form_map) {
   DCHECK(background_task_runner()->RunsTasksInCurrentSequence());
-  return login_db_ && login_db_->GetAllLogins(key_to_form_map);
+  if (!login_db_)
+    return FormRetrievalResult::kDbError;
+  return login_db_->GetAllLogins(key_to_form_map);
 }
 
 PasswordStoreChangeList PasswordStoreDefault::RemoveLoginByPrimaryKeySync(

components/password_manager/core/browser/password_store_default.h

@@ -87,7 +87,8 @@ class PasswordStoreDefault : public PasswordStore {
   // Implements PasswordStoreSync interface.
   bool BeginTransaction() override;
   bool CommitTransaction() override;
-  bool ReadAllLogins(PrimaryKeyToFormMap* key_to_form_map) override;
+  FormRetrievalResult ReadAllLogins(
+      PrimaryKeyToFormMap* key_to_form_map) override;
   PasswordStoreChangeList RemoveLoginByPrimaryKeySync(int primary_key) override;
   PasswordStoreSync::MetadataStore* GetMetadataStore() override;
 

components/password_manager/core/browser/password_store_sync.h

@@ -36,6 +36,17 @@ enum class DatabaseCleanupResult {
   kEncryptionUnavailable,
 };
 
+// Result values for retrieving form from the store.
+enum class FormRetrievalResult {
+  // Success.
+  kSuccess,
+  // Database error.
+  kDbError,
+  // A service-level failure (e.g., on a platform using a keyring, the keyring
+  // is temporarily unavailable).
+  kEncrytionServiceFailure,
+};
+
 // PasswordStore interface for PasswordSyncableService. It provides access to
 // synchronous methods of PasswordStore which shouldn't be accessible to other
 // classes. These methods are to be called on the PasswordStore background
@@ -67,8 +78,8 @@ class PasswordStoreSync {
 
   // Overwrites |key_to_form_map| with a map from the DB primary key to the
   // corresponding form for all stored credentials. Returns true on success.
-  virtual bool ReadAllLogins(PrimaryKeyToFormMap* key_to_form_map)
-      WARN_UNUSED_RESULT = 0;
+  virtual FormRetrievalResult ReadAllLogins(
+      PrimaryKeyToFormMap* key_to_form_map) WARN_UNUSED_RESULT = 0;
 
   // Deletes logins that cannot be decrypted.
   virtual DatabaseCleanupResult DeleteUndecryptableLogins() = 0;

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

@@ -249,7 +249,8 @@ base::Optional<syncer::ModelError> PasswordSyncBridge::MergeSyncData(
                                            true);
   // Read all local passwords.
   PrimaryKeyToFormMap key_to_local_form_map;
-  if (!password_store_sync_->ReadAllLogins(&key_to_local_form_map)) {
+  if (password_store_sync_->ReadAllLogins(&key_to_local_form_map) !=
+      FormRetrievalResult::kSuccess) {
     return syncer::ModelError(FROM_HERE,
                               "Failed to load entries from password store.");
   }
@@ -488,7 +489,8 @@ void PasswordSyncBridge::GetData(StorageKeyList storage_keys,
   // There are more efficient implementations, but since this method is rarely
   // called, simplicity is preferred over efficiency.
   PrimaryKeyToFormMap key_to_form_map;
-  if (!password_store_sync_->ReadAllLogins(&key_to_form_map)) {
+  if (password_store_sync_->ReadAllLogins(&key_to_form_map) !=
+      FormRetrievalResult::kSuccess) {
     change_processor()->ReportError(
         {FROM_HERE, "Failed to load entries from the password store."});
     return;
@@ -508,7 +510,8 @@ void PasswordSyncBridge::GetAllDataForDebugging(DataCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   PrimaryKeyToFormMap key_to_form_map;
-  if (!password_store_sync_->ReadAllLogins(&key_to_form_map)) {
+  if (password_store_sync_->ReadAllLogins(&key_to_form_map) !=
+      FormRetrievalResult::kSuccess) {
     change_processor()->ReportError(
         {FROM_HERE, "Failed to load entries from the password store."});
     return;

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

@@ -115,13 +115,13 @@ class FakeDatabase {
   FakeDatabase() = default;
   ~FakeDatabase() = default;
 
-  bool ReadAllLogins(PrimaryKeyToFormMap* map) {
+  FormRetrievalResult ReadAllLogins(PrimaryKeyToFormMap* map) {
     map->clear();
     for (const auto& pair : data_) {
       map->emplace(pair.first,
                    std::make_unique<autofill::PasswordForm>(*pair.second));
     }
-    return true;
+    return FormRetrievalResult::kSuccess;
   }
 
   PasswordStoreChangeList AddLogin(const autofill::PasswordForm& form) {
@@ -193,7 +193,7 @@ class MockPasswordStoreSync : public PasswordStoreSync {
                bool(std::vector<std::unique_ptr<autofill::PasswordForm>>*));
   MOCK_METHOD1(FillBlacklistLogins,
                bool(std::vector<std::unique_ptr<autofill::PasswordForm>>*));
-  MOCK_METHOD1(ReadAllLogins, bool(PrimaryKeyToFormMap*));
+  MOCK_METHOD1(ReadAllLogins, FormRetrievalResult(PrimaryKeyToFormMap*));
   MOCK_METHOD1(RemoveLoginByPrimaryKeySync, PasswordStoreChangeList(int));
   MOCK_METHOD0(DeleteUndecryptableLogins, DatabaseCleanupResult());
   MOCK_METHOD1(AddLoginSync,
@@ -627,9 +627,9 @@ TEST_F(PasswordSyncBridgeTest,
 // ShouldMergeSync() would return an error without crashing.
 TEST_F(PasswordSyncBridgeTest,
        ShouldMergeSyncRemoteAndLocalPasswordsWithErrorWhenStoreReadFails) {
-  // Simulate a failed ReadAllLogins() by returning false.
+  // Simulate a failed ReadAllLogins() by returning a kDbError.
   ON_CALL(*mock_password_store_sync(), ReadAllLogins(_))
-      .WillByDefault(testing::Return(false));
+      .WillByDefault(testing::Return(FormRetrievalResult::kDbError));
   base::Optional<syncer::ModelError> error =
       bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(), {});
   EXPECT_TRUE(error);

components/password_manager/core/browser/test_password_store.cc

@@ -220,9 +220,10 @@ bool TestPasswordStore::CommitTransaction() {
   return true;
 }
 
-bool TestPasswordStore::ReadAllLogins(PrimaryKeyToFormMap* key_to_form_map) {
+FormRetrievalResult TestPasswordStore::ReadAllLogins(
+    PrimaryKeyToFormMap* key_to_form_map) {
   NOTIMPLEMENTED();
-  return true;
+  return FormRetrievalResult::kSuccess;
 }
 
 PasswordStoreChangeList TestPasswordStore::RemoveLoginByPrimaryKeySync(

components/password_manager/core/browser/test_password_store.h

@@ -90,7 +90,8 @@ class TestPasswordStore : public PasswordStore {
   // PasswordStoreSync interface.
   bool BeginTransaction() override;
   bool CommitTransaction() override;
-  bool ReadAllLogins(PrimaryKeyToFormMap* key_to_form_map) override;
+  FormRetrievalResult ReadAllLogins(
+      PrimaryKeyToFormMap* key_to_form_map) override;
   PasswordStoreChangeList RemoveLoginByPrimaryKeySync(int primary_key) override;
   PasswordStoreSync::MetadataStore* GetMetadataStore() override;