MultiStorePasswordSaveManager: Track action for each store

This CL adds profile_store_action_ and account_store_action_ to
MultiStorePasswordSaveManager (in addition to pending_credentials_state_
from the base class). Explicitly tracking the two states, instead of
just a single "canonical" one, allows us to be more explicit in how
conflicts are resolved, and in particular it allows us to handle some
edge cases that we couldn't handle correctly before.
It also adds an integration test for one such edge case.

Bug: 1012203, 1060524
Change-Id: I9602c3641da761a76b2dc9a1f62b630ef2fbcb2e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2139654
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Auto-Submit: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758777}

chrome/browser/sync/test/integration/password_manager_sync_test.cc

@@ -541,6 +541,36 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerSyncTest,
   ASSERT_THAT(GetAllLoginsFromAccountPasswordStore(),
               ElementsAre(MatchesLogin("user", "pass")));
 }
+
+IN_PROC_BROWSER_TEST_F(PasswordManagerSyncTest,
+                       AutoUpdatePSLMatchInBothStoresOnSuccessfulUse) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  // Add the same PSL-matched credential to both stores (i.e. it's stored for
+  // psl.example.com instead of www.example.com).
+  AddCredentialToFakeServer(CreateTestPSLPasswordForm("user", "pass"));
+  AddLocalCredential(CreateTestPSLPasswordForm("user", "pass"));
+
+  SetupSyncTransportWithPasswordAccountStorage();
+
+  content::WebContents* web_contents = nullptr;
+  GetNewTab(GetBrowser(0), &web_contents);
+
+  // Go to a form (on www.) and submit it with the saved credentials.
+  NavigateToFile(web_contents, "/password/simple_password.html");
+  FillAndSubmitPasswordForm(web_contents, "user", "pass");
+
+  // Now the PSL-matched credential should have been automatically saved for
+  // www. as well, in both stores.
+  EXPECT_THAT(GetAllLoginsFromAccountPasswordStore(),
+              UnorderedElementsAre(
+                  MatchesLoginAndRealm("user", "pass", GetWWWOrigin()),
+                  MatchesLoginAndRealm("user", "pass", GetPSLOrigin())));
+  EXPECT_THAT(GetAllLoginsFromProfilePasswordStore(),
+              UnorderedElementsAre(
+                  MatchesLoginAndRealm("user", "pass", GetWWWOrigin()),
+                  MatchesLoginAndRealm("user", "pass", GetPSLOrigin())));
+}
 #endif  // !defined(OS_CHROMEOS)
 
 }  // namespace

components/password_manager/core/browser/multi_store_password_save_manager.h

@@ -27,12 +27,6 @@ class MultiStorePasswordSaveManager : public PasswordSaveManagerImpl {
                                 std::unique_ptr<FormSaver> account_form_saver);
   ~MultiStorePasswordSaveManager() override;
 
-  void SaveInternal(const std::vector<const autofill::PasswordForm*>& matches,
-                    const base::string16& old_password) override;
-
-  void UpdateInternal(const std::vector<const autofill::PasswordForm*>& matches,
-                      const base::string16& old_password) override;
-
   void PermanentlyBlacklist(
       const PasswordStore::FormDigest& form_digest) override;
   void Unblacklist(const PasswordStore::FormDigest& form_digest) override;
@@ -42,6 +36,8 @@ class MultiStorePasswordSaveManager : public PasswordSaveManagerImpl {
   void MoveCredentialsToAccountStore() override;
 
  protected:
+  void SavePendingToStoreImpl(
+      const autofill::PasswordForm& parsed_submitted_form) override;
   std::pair<const autofill::PasswordForm*, PendingCredentialsState>
   FindSimilarSavedFormAndComputeState(
       const autofill::PasswordForm& parsed_submitted_form) const override;
@@ -50,9 +46,24 @@ class MultiStorePasswordSaveManager : public PasswordSaveManagerImpl {
       const std::vector<const autofill::PasswordForm*>& matches) override;
 
  private:
-  bool IsOptedInForAccountStorage();
+  struct PendingCredentialsStates {
+    PendingCredentialsState profile_store_state = PendingCredentialsState::NONE;
+    PendingCredentialsState account_store_state = PendingCredentialsState::NONE;
+
+    const autofill::PasswordForm* similar_saved_form_from_profile_store =
+        nullptr;
+    const autofill::PasswordForm* similar_saved_form_from_account_store =
+        nullptr;
+  };
+  static PendingCredentialsStates ComputePendingCredentialsStates(
+      const autofill::PasswordForm& parsed_submitted_form,
+      const std::vector<const autofill::PasswordForm*>& matches);
+
+  bool IsOptedInForAccountStorage() const;
+  bool AccountStoreIsDefault() const;
 
   const std::unique_ptr<FormSaver> account_store_form_saver_;
+
   DISALLOW_COPY_AND_ASSIGN(MultiStorePasswordSaveManager);
 };
 

components/password_manager/core/browser/multi_store_password_save_manager_unittest.cc

@@ -298,8 +298,7 @@ TEST_F(MultiStorePasswordSaveManagerTest, SaveInProfileStore) {
   password_save_manager()->Save(observed_form_, parsed_submitted_form);
 }
 
-TEST_F(MultiStorePasswordSaveManagerTest,
-       UpdateBothStoresIfCredentialsExistInAccountStoreOnly) {
+TEST_F(MultiStorePasswordSaveManagerTest, UpdateInAccountStoreOnly) {
   SetAccountStoreEnabled(/*is_enabled=*/true);
 
   PasswordForm saved_match_in_account_store(saved_match_);
@@ -317,14 +316,13 @@ TEST_F(MultiStorePasswordSaveManagerTest,
   // An update prompt should be shown.
   EXPECT_TRUE(password_save_manager()->IsPasswordUpdate());
 
-  EXPECT_CALL(*mock_profile_form_saver(), Update(_, _, _));
+  EXPECT_CALL(*mock_profile_form_saver(), Update(_, _, _)).Times(0);
   EXPECT_CALL(*mock_account_form_saver(), Update(_, _, _));
 
   password_save_manager()->Save(observed_form_, parsed_submitted_form_);
 }
 
-TEST_F(MultiStorePasswordSaveManagerTest,
-       UpdateBothStoresIfCredentialsExistInProfileStoreOnly) {
+TEST_F(MultiStorePasswordSaveManagerTest, UpdateInProfileStoreOnly) {
   SetAccountStoreEnabled(/*is_enabled=*/true);
 
   PasswordForm saved_match_in_profile_store(saved_match_);
@@ -343,13 +341,12 @@ TEST_F(MultiStorePasswordSaveManagerTest,
   EXPECT_TRUE(password_save_manager()->IsPasswordUpdate());
 
   EXPECT_CALL(*mock_profile_form_saver(), Update(_, _, _));
-  EXPECT_CALL(*mock_account_form_saver(), Update(_, _, _));
+  EXPECT_CALL(*mock_account_form_saver(), Update(_, _, _)).Times(0);
 
   password_save_manager()->Save(observed_form_, parsed_submitted_form_);
 }
 
-TEST_F(MultiStorePasswordSaveManagerTest,
-       UpdateBothStoresIfCredentialsExistInBothStores) {
+TEST_F(MultiStorePasswordSaveManagerTest, UpdateInBothStores) {
   SetAccountStoreEnabled(/*is_enabled=*/true);
 
   PasswordForm saved_match_in_profile_store(saved_match_);

components/password_manager/core/browser/password_save_manager_impl.cc

@@ -157,7 +157,7 @@ void PasswordSaveManagerImpl::CreatePendingCredentials(
     const FormData& submitted_form,
     bool is_http_auth,
     bool is_credential_api_save) {
-  const PasswordForm* similar_saved_form;
+  const PasswordForm* similar_saved_form = nullptr;
   std::tie(similar_saved_form, pending_credentials_state_) =
       FindSimilarSavedFormAndComputeState(parsed_submitted_form);
 
@@ -451,29 +451,37 @@ void PasswordSaveManagerImpl::SavePendingToStore(
     const PasswordForm& parsed_submitted_form) {
   UploadVotesAndMetrics(observed_form, parsed_submitted_form);
 
-  bool update = !IsNewLogin();
-  const PasswordForm* similar_saved_form =
-      FindSimilarSavedFormAndComputeState(parsed_submitted_form).first;
-  if (update && !pending_credentials_.IsFederatedCredential())
-    DCHECK(similar_saved_form);
-
-  base::string16 old_password = similar_saved_form
-                                    ? similar_saved_form->password_value
-                                    : base::string16();
   if (HasGeneratedPassword()) {
     generation_manager_->CommitGeneratedPassword(
         pending_credentials_, form_fetcher_->GetAllRelevantMatches(),
-        old_password, GetFormSaverForGeneration());
-  } else if (update) {
+        GetOldPassword(parsed_submitted_form), GetFormSaverForGeneration());
+  } else {
+    SavePendingToStoreImpl(parsed_submitted_form);
+  }
+}
+
+void PasswordSaveManagerImpl::SavePendingToStoreImpl(
+    const PasswordForm& parsed_submitted_form) {
+  auto matches = form_fetcher_->GetAllRelevantMatches();
+  base::string16 old_password = GetOldPassword(parsed_submitted_form);
+  if (IsNewLogin()) {
+    form_saver_->Save(pending_credentials_, matches, old_password);
+  } else {
     // It sounds wrong that we still update even if the state is NONE. We
     // should double check if this actually necessary. Currently some tests
     // depend on this behavior.
-    UpdateInternal(form_fetcher_->GetAllRelevantMatches(), old_password);
-  } else {
-    SaveInternal(form_fetcher_->GetAllRelevantMatches(), old_password);
+    form_saver_->Update(pending_credentials_, matches, old_password);
   }
 }
 
+base::string16 PasswordSaveManagerImpl::GetOldPassword(
+    const PasswordForm& parsed_submitted_form) const {
+  const PasswordForm* similar_saved_form =
+      FindSimilarSavedFormAndComputeState(parsed_submitted_form).first;
+  return similar_saved_form ? similar_saved_form->password_value
+                            : base::string16();
+}
+
 void PasswordSaveManagerImpl::UploadVotesAndMetrics(
     const FormData& observed_form,
     const PasswordForm& parsed_submitted_form) {
@@ -530,18 +538,6 @@ PasswordSaveManagerImpl::GetRelevantMatchesForGeneration(
   return matches;
 }
 
-void PasswordSaveManagerImpl::SaveInternal(
-    const std::vector<const PasswordForm*>& matches,
-    const base::string16& old_password) {
-  form_saver_->Save(pending_credentials_, matches, old_password);
-}
-
-void PasswordSaveManagerImpl::UpdateInternal(
-    const std::vector<const PasswordForm*>& matches,
-    const base::string16& old_password) {
-  form_saver_->Update(pending_credentials_, matches, old_password);
-}
-
 void PasswordSaveManagerImpl::CloneInto(PasswordSaveManagerImpl* clone) {
   DCHECK(clone);
   if (generation_manager_)

components/password_manager/core/browser/password_save_manager_impl.h

@@ -111,13 +111,8 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
   GetRelevantMatchesForGeneration(
       const std::vector<const autofill::PasswordForm*>& matches);
 
-  virtual void SaveInternal(
-      const std::vector<const autofill::PasswordForm*>& matches,
-      const base::string16& old_password);
-
-  virtual void UpdateInternal(
-      const std::vector<const autofill::PasswordForm*>& matches,
-      const base::string16& old_password);
+  virtual void SavePendingToStoreImpl(
+      const autofill::PasswordForm& parsed_submitted_form);
 
   // Clones the current object into |clone|. |clone| must not be null.
   void CloneInto(PasswordSaveManagerImpl* clone);
@@ -142,6 +137,9 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
   const FormFetcher* form_fetcher_;
 
  private:
+  base::string16 GetOldPassword(
+      const autofill::PasswordForm& parsed_submitted_form) const;
+
   void SetVotesAndRecordMetricsForPendingCredentials(
       const autofill::PasswordForm& parsed_submitted_form);