[Passwords] Respect default password when saving credentials.

Bug: 1012203
Change-Id: I777d82887b16ebde005e4ff4d7638e5930cb359d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1943143
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720699}

components/password_manager/core/browser/multi_store_password_save_manager.cc

@@ -7,6 +7,7 @@
 #include "components/password_manager/core/browser/form_fetcher.h"
 #include "components/password_manager/core/browser/form_saver.h"
 #include "components/password_manager/core/browser/form_saver_impl.h"
+#include "components/password_manager/core/browser/password_feature_manager_impl.h"
 #include "components/password_manager/core/browser/password_form_metrics_recorder.h"
 #include "components/password_manager/core/browser/password_generation_manager.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
@@ -30,35 +31,44 @@ FormSaver* MultiStorePasswordSaveManager::GetFormSaverForGeneration() {
 }
 
 void MultiStorePasswordSaveManager::SaveInternal(
-    const PasswordForm& pending,
     const std::vector<const PasswordForm*>& matches,
     const base::string16& old_password) {
-  switch (pending.in_store) {
+  // For New Credentials, we should respect the default password store selected
+  // by user. In other cases such PSL matching, we respect the store in the
+  // retrieved credentials.
+  if (pending_credentials_state_ == PendingCredentialsState::NEW_LOGIN) {
+    pending_credentials_.in_store =
+        client_->GetPasswordFeatureManager()->GetDefaultPasswordStore();
+  }
+
+  switch (pending_credentials_.in_store) {
     case PasswordForm::Store::kAccountStore:
       if (account_store_form_saver_ && IsAccountStoreActive())
-        account_store_form_saver_->Save(pending, matches, old_password);
+        account_store_form_saver_->Save(pending_credentials_, matches,
+                                        old_password);
       break;
     case PasswordForm::Store::kProfileStore:
-      form_saver_->Save(pending, matches, old_password);
+      form_saver_->Save(pending_credentials_, matches, old_password);
       break;
     case PasswordForm::Store::kNotSet:
       if (account_store_form_saver_ && IsAccountStoreActive())
-        account_store_form_saver_->Save(pending, matches, old_password);
+        account_store_form_saver_->Save(pending_credentials_, matches,
+                                        old_password);
       else
-        form_saver_->Save(pending, matches, old_password);
+        form_saver_->Save(pending_credentials_, matches, old_password);
       break;
   }
 }
 
 void MultiStorePasswordSaveManager::UpdateInternal(
-    const PasswordForm& pending,
     const std::vector<const PasswordForm*>& matches,
     const base::string16& old_password) {
   // Try to update both stores anyway because if credentials don't exist, the
   // update operation is no-op.
-  form_saver_->Update(pending, matches, old_password);
+  form_saver_->Update(pending_credentials_, matches, old_password);
   if (account_store_form_saver_ && IsAccountStoreActive()) {
-    account_store_form_saver_->Update(pending, matches, old_password);
+    account_store_form_saver_->Update(pending_credentials_, matches,
+                                      old_password);
   }
 }
 

components/password_manager/core/browser/multi_store_password_save_manager.h

@@ -31,12 +31,10 @@ class MultiStorePasswordSaveManager : public PasswordSaveManagerImpl {
                                 std::unique_ptr<FormSaver> account_form_saver);
   ~MultiStorePasswordSaveManager() override;
 
-  void SaveInternal(const autofill::PasswordForm& pending,
-                    const std::vector<const autofill::PasswordForm*>& matches,
+  void SaveInternal(const std::vector<const autofill::PasswordForm*>& matches,
                     const base::string16& old_password) override;
 
-  void UpdateInternal(const autofill::PasswordForm& pending,
-                      const std::vector<const autofill::PasswordForm*>& matches,
+  void UpdateInternal(const std::vector<const autofill::PasswordForm*>& matches,
                       const base::string16& old_password) override;
 
   void PermanentlyBlacklist(

components/password_manager/core/browser/multi_store_password_save_manager_unittest.cc

@@ -183,6 +183,12 @@ class MultiStorePasswordSaveManagerTest : public testing::Test {
         .WillByDefault(Return(password_manager::SYNCING_NORMAL_ENCRYPTION));
   }
 
+  void SetDefaultPasswordStore(const autofill::PasswordForm::Store& store) {
+    ON_CALL(*client()->GetMockPasswordFeatureManager(),
+            GetDefaultPasswordStore())
+        .WillByDefault(Return(store));
+  }
+
   MockPasswordManagerClient* client() { return &client_; }
   MockFormSaver* mock_account_form_saver() { return mock_account_form_saver_; }
   MockFormSaver* mock_profile_form_saver() { return mock_profile_form_saver_; }
@@ -220,9 +226,7 @@ TEST_F(MultiStorePasswordSaveManagerTest,
   fetcher()->NotifyFetchCompleted();
 
   PasswordForm parsed_submitted_form(parsed_submitted_form_);
-  // TODO(crbug.com/1012203): change to use an API in the PasswordSaveManager to
-  // set the store once the API is introduced.
-  parsed_submitted_form.in_store = PasswordForm::Store::kAccountStore;
+  SetDefaultPasswordStore(PasswordForm::Store::kAccountStore);
 
   password_save_manager()->CreatePendingCredentials(
       parsed_submitted_form, observed_form_, submitted_form_,
@@ -244,7 +248,7 @@ TEST_F(MultiStorePasswordSaveManagerTest,
   fetcher()->NotifyFetchCompleted();
 
   PasswordForm parsed_submitted_form(parsed_submitted_form_);
-  parsed_submitted_form.in_store = PasswordForm::Store::kAccountStore;
+  SetDefaultPasswordStore(PasswordForm::Store::kAccountStore);
 
   password_save_manager()->CreatePendingCredentials(
       parsed_submitted_form, observed_form_, submitted_form_,
@@ -265,7 +269,7 @@ TEST_F(MultiStorePasswordSaveManagerTest, SaveInProfileStore) {
   fetcher()->NotifyFetchCompleted();
 
   PasswordForm parsed_submitted_form(parsed_submitted_form_);
-  parsed_submitted_form.in_store = PasswordForm::Store::kProfileStore;
+  SetDefaultPasswordStore(PasswordForm::Store::kProfileStore);
 
   password_save_manager()->CreatePendingCredentials(
       parsed_submitted_form, observed_form_, submitted_form_,
@@ -287,7 +291,6 @@ TEST_F(MultiStorePasswordSaveManagerTest,
   fetcher()->NotifyFetchCompleted();
 
   PasswordForm parsed_submitted_form(parsed_submitted_form_);
-  parsed_submitted_form.in_store = PasswordForm::Store::kNotSet;
 
   password_save_manager()->CreatePendingCredentials(
       parsed_submitted_form, observed_form_, submitted_form_,
@@ -309,7 +312,6 @@ TEST_F(MultiStorePasswordSaveManagerTest,
   fetcher()->NotifyFetchCompleted();
 
   PasswordForm parsed_submitted_form(parsed_submitted_form_);
-  parsed_submitted_form.in_store = PasswordForm::Store::kNotSet;
 
   password_save_manager()->CreatePendingCredentials(
       parsed_submitted_form, observed_form_, submitted_form_,

components/password_manager/core/browser/password_save_manager_impl.cc

@@ -378,11 +378,9 @@ void PasswordSaveManagerImpl::SavePendingToStore(
         pending_credentials_, form_fetcher_->GetAllRelevantMatches(),
         old_password, GetFormSaverForGeneration());
   } else if (update) {
-    UpdateInternal(pending_credentials_, form_fetcher_->GetAllRelevantMatches(),
-                   old_password);
+    UpdateInternal(form_fetcher_->GetAllRelevantMatches(), old_password);
   } else {
-    SaveInternal(pending_credentials_, form_fetcher_->GetAllRelevantMatches(),
-                 old_password);
+    SaveInternal(form_fetcher_->GetAllRelevantMatches(), old_password);
   }
 }
 
@@ -430,17 +428,15 @@ FormSaver* PasswordSaveManagerImpl::GetFormSaverForGeneration() {
 }
 
 void PasswordSaveManagerImpl::SaveInternal(
-    const PasswordForm& pending,
     const std::vector<const PasswordForm*>& matches,
     const base::string16& old_password) {
-  form_saver_->Save(pending, matches, old_password);
+  form_saver_->Save(pending_credentials_, matches, old_password);
 }
 
 void PasswordSaveManagerImpl::UpdateInternal(
-    const PasswordForm& pending,
     const std::vector<const PasswordForm*>& matches,
     const base::string16& old_password) {
-  form_saver_->Update(pending, matches, old_password);
+  form_saver_->Update(pending_credentials_, matches, old_password);
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/password_save_manager_impl.h

@@ -73,12 +73,10 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
   virtual FormSaver* GetFormSaverForGeneration();
 
   virtual void SaveInternal(
-      const autofill::PasswordForm& pending,
       const std::vector<const autofill::PasswordForm*>& matches,
       const base::string16& old_password);
 
   virtual void UpdateInternal(
-      const autofill::PasswordForm& pending,
       const std::vector<const autofill::PasswordForm*>& matches,
       const base::string16& old_password);
 
@@ -89,6 +87,15 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
   // The client which implements embedder-specific PasswordManager operations.
   PasswordManagerClient* client_;
 
+  // Stores updated credentials when the form was submitted but success is still
+  // unknown. This variable contains credentials that are ready to be written
+  // (saved or updated) to a password store. It is calculated based on
+  // |submitted_form_| and |best_matches_|.
+  autofill::PasswordForm pending_credentials_;
+
+  PendingCredentialsState pending_credentials_state_ =
+      PendingCredentialsState::NONE;
+
  private:
   // Create pending credentials from provisionally saved form when this form
   // represents credentials that were not previously saved.
@@ -115,15 +122,6 @@ class PasswordSaveManagerImpl : public PasswordSaveManager {
   // FormFetcher instance which owns the login data from PasswordStore.
   const FormFetcher* form_fetcher_;
 
-  // Stores updated credentials when the form was submitted but success is still
-  // unknown. This variable contains credentials that are ready to be written
-  // (saved or updated) to a password store. It is calculated based on
-  // |submitted_form_| and |best_matches_|.
-  autofill::PasswordForm pending_credentials_;
-
-  PendingCredentialsState pending_credentials_state_ =
-      PendingCredentialsState::NONE;
-
   // Takes care of recording metrics and events for |*this|.
   scoped_refptr<PasswordFormMetricsRecorder> metrics_recorder_;