Save in profile store when the unsynced credentials UI button is clicked

This CL introduces the functionality for the save button in the new
PasswordSaveUnsyncedCredentialsLocallyView. The saving logic lives in
ManagePasswordsUIController, as is done for the save bubble. The bubble
controller now also retrieves the credentials from the UI controller
when needed, instead of copying them. In this way, the credentials live
in a single place (ManagePasswordsState) and can be easily cleared.

Bug: 1060132
Change-Id: Id59f4fd3a4d642aade389d207e3290b049c8d867
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2139653
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771817}

chrome/browser/ui/passwords/bubble_controllers/save_unsynced_credentials_locally_bubble_controller.cc

@@ -19,9 +19,7 @@ SaveUnsyncedCredentialsLocallyBubbleController::
           std::move(delegate),
           /*display_disposition=*/metrics_util::
               AUTOMATIC_SAVE_UNSYNCED_CREDENTIALS_LOCALLY),
-      unsynced_credentials_(delegate_->GetUnsyncedCredentials()),
       dismissal_reason_(metrics_util::NO_DIRECT_INTERACTION) {
-  DCHECK(!unsynced_credentials_.empty());
 }
 
 SaveUnsyncedCredentialsLocallyBubbleController::
@@ -31,7 +29,7 @@ SaveUnsyncedCredentialsLocallyBubbleController::
 }
 
 void SaveUnsyncedCredentialsLocallyBubbleController::OnSaveClicked() {
-  NOTIMPLEMENTED();
+  delegate_->SaveUnsyncedCredentialsInProfileStore();
 }
 
 void SaveUnsyncedCredentialsLocallyBubbleController::OnCancelClicked() {
@@ -40,6 +38,11 @@ void SaveUnsyncedCredentialsLocallyBubbleController::OnCancelClicked() {
   NOTIMPLEMENTED();
 }
 
+const std::vector<autofill::PasswordForm>&
+SaveUnsyncedCredentialsLocallyBubbleController::GetUnsyncedCredentials() const {
+  return delegate_->GetUnsyncedCredentials();
+}
+
 void SaveUnsyncedCredentialsLocallyBubbleController::ReportInteractions() {
   metrics_util::LogGeneralUIDismissalReason(dismissal_reason_);
   // Record UKM statistics on dismissal reason.

chrome/browser/ui/passwords/bubble_controllers/save_unsynced_credentials_locally_bubble_controller.h

@@ -29,16 +29,13 @@ class SaveUnsyncedCredentialsLocallyBubbleController
   // Drops the unsynced credentials.
   void OnCancelClicked();
 
-  const std::vector<autofill::PasswordForm>& unsynced_credentials() const {
-    return unsynced_credentials_;
-  }
+  const std::vector<autofill::PasswordForm>& GetUnsyncedCredentials() const;
 
  private:
   // PasswordBubbleControllerBase methods:
   base::string16 GetTitle() const override;
   void ReportInteractions() override;
 
-  std::vector<autofill::PasswordForm> unsynced_credentials_;
   password_manager::metrics_util::UIDismissalReason dismissal_reason_;
 };
 

chrome/browser/ui/passwords/bubble_controllers/save_unsynced_credentials_locally_bubble_controller_unittest.cc

@@ -20,9 +20,11 @@ class SaveUnsyncedCredentialsLocallyBubbleControllerTest
     : public ::testing::Test {
  public:
   SaveUnsyncedCredentialsLocallyBubbleControllerTest() {
-    unsynced_credentials_.resize(1);
-    unsynced_credentials_[0].username_value = ASCIIToUTF16("user");
-    unsynced_credentials_[0].password_value = ASCIIToUTF16("password");
+    unsynced_credentials_.resize(2);
+    unsynced_credentials_[0].username_value = ASCIIToUTF16("user1");
+    unsynced_credentials_[0].password_value = ASCIIToUTF16("password1");
+    unsynced_credentials_[1].username_value = ASCIIToUTF16("user2");
+    unsynced_credentials_[1].password_value = ASCIIToUTF16("password2");
   }
   ~SaveUnsyncedCredentialsLocallyBubbleControllerTest() override = default;
 
@@ -37,5 +39,13 @@ TEST_F(SaveUnsyncedCredentialsLocallyBubbleControllerTest,
       .WillOnce(ReturnRef(unsynced_credentials_));
   SaveUnsyncedCredentialsLocallyBubbleController controller(
       model_delegate_mock_.AsWeakPtr());
-  EXPECT_EQ(controller.unsynced_credentials(), unsynced_credentials_);
+  EXPECT_EQ(controller.GetUnsyncedCredentials(), unsynced_credentials_);
+}
+
+TEST_F(SaveUnsyncedCredentialsLocallyBubbleControllerTest,
+       ShouldSaveCredentialsInProfileStoreOnSaveButtonClicked) {
+  SaveUnsyncedCredentialsLocallyBubbleController controller(
+      model_delegate_mock_.AsWeakPtr());
+  EXPECT_CALL(model_delegate_mock_, SaveUnsyncedCredentialsInProfileStore);
+  controller.OnSaveClicked();
 }

chrome/browser/ui/passwords/manage_passwords_state.cc

@@ -239,6 +239,7 @@ void ManagePasswordsState::ClearData() {
   form_manager_.reset();
   local_credentials_forms_.clear();
   credentials_callback_.Reset();
+  unsynced_credentials_.clear();
 }
 
 bool ManagePasswordsState::AddForm(const PasswordForm& form) {

chrome/browser/ui/passwords/manage_passwords_ui_controller.cc

@@ -36,6 +36,7 @@
 #include "chrome/grit/generated_resources.h"
 #include "components/browsing_data/content/browsing_data_helper.h"
 #include "components/password_manager/core/browser/browser_save_password_progress_logger.h"
+#include "components/password_manager/core/browser/form_saver_impl.h"
 #include "components/password_manager/core/browser/password_bubble_experiment.h"
 #include "components/password_manager/core/browser/password_form_manager_for_ui.h"
 #include "components/password_manager/core/browser/password_manager_constants.h"
@@ -485,6 +486,24 @@ void ManagePasswordsUIController::SavePassword(const base::string16& username,
     browser->window()->GetAutofillBubbleHandler()->OnPasswordSaved();
 }
 
+void ManagePasswordsUIController::SaveUnsyncedCredentialsInProfileStore() {
+  auto profile_store_form_saver =
+      std::make_unique<password_manager::FormSaverImpl>(
+          passwords_data_.client()->GetProfilePasswordStore());
+  for (const autofill::PasswordForm& form :
+       passwords_data_.unsynced_credentials()) {
+    // Only newly-saved or newly-updated credentials can be unsynced. Since
+    // conflicts are solved in that process, any entry in the profile store
+    // similar to |form| actually contains the same essential information. This
+    // means Save() can be safely called here, no password loss happens.
+    profile_store_form_saver->Save(form, /*matches=*/{},
+                                   /*old_password=*/base::string16());
+  }
+  ClearPopUpFlagForBubble();
+  passwords_data_.OnInactive();
+  UpdateBubbleAndIconVisibility();
+}
+
 void ManagePasswordsUIController::MovePasswordToAccountStore() {
   DCHECK_EQ(GetState(),
             password_manager::ui::CAN_MOVE_PASSWORD_TO_ACCOUNT_STATE)

chrome/browser/ui/passwords/manage_passwords_ui_controller.h

@@ -140,6 +140,7 @@ class ManagePasswordsUIController
   void OnPasswordsRevealed() override;
   void SavePassword(const base::string16& username,
                     const base::string16& password) override;
+  void SaveUnsyncedCredentialsInProfileStore() override;
   void MovePasswordToAccountStore() override;
   void ChooseCredential(
       const autofill::PasswordForm& form,

chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc

@@ -25,6 +25,7 @@
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/mock_password_form_manager_for_ui.h"
+#include "components/password_manager/core/browser/mock_password_store.h"
 #include "components/password_manager/core/browser/password_bubble_experiment.h"
 #include "components/password_manager/core/browser/password_form_metrics_recorder.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
@@ -60,6 +61,15 @@ using ::testing::SaveArg;
 
 namespace {
 
+MATCHER_P3(MatchesLoginAndOrigin,
+           username,
+           password,
+           origin,
+           "matches username, password and origin") {
+  return arg.username_value == username && arg.password_value == password &&
+         arg.origin == origin;
+}
+
 // A random URL.
 constexpr char kExampleUrl[] = "http://example.com";
 
@@ -89,13 +99,26 @@ class TestManagePasswordsIconView : public ManagePasswordsIconView {
   password_manager::ui::State state_;
 };
 
-class MockPasswordManagerClient
+class TestPasswordManagerClient
     : public password_manager::StubPasswordManagerClient {
  public:
+  TestPasswordManagerClient()
+      : mock_profile_store_(new password_manager::MockPasswordStore()) {}
+  ~TestPasswordManagerClient() override {
+    mock_profile_store_->ShutdownOnUIThread();
+  }
+
   MOCK_METHOD(void,
               TriggerReauthForPrimaryAccount,
               (base::OnceCallback<void(ReauthSucceeded)>),
               (override));
+
+  password_manager::PasswordStore* GetProfilePasswordStore() const override {
+    return mock_profile_store_.get();
+  }
+
+ private:
+  scoped_refptr<password_manager::MockPasswordStore> mock_profile_store_;
 };
 
 // This subclass is used to disable some code paths which are not essential for
@@ -166,13 +189,24 @@ void TestManagePasswordsUIController::HidePasswordBubble() {
   }
 }
 
+autofill::PasswordForm BuildFormFromLoginAndOrigin(const std::string& username,
+                                                   const std::string& password,
+                                                   const std::string& origin) {
+  autofill::PasswordForm form;
+  form.username_value = base::ASCIIToUTF16(username);
+  form.password_value = base::ASCIIToUTF16(password);
+  form.origin = GURL(origin);
+  form.signon_realm = form.origin.GetOrigin().spec();
+  return form;
+}
+
 }  // namespace
 
 class ManagePasswordsUIControllerTest : public ChromeRenderViewHostTestHarness {
  public:
   void SetUp() override;
 
-  MockPasswordManagerClient& client() { return client_; }
+  TestPasswordManagerClient& client() { return client_; }
   PasswordForm& test_local_form() { return test_local_form_; }
   PasswordForm& test_federated_form() { return test_federated_form_; }
   PasswordForm& submitted_form() { return submitted_form_; }
@@ -197,7 +231,7 @@ class ManagePasswordsUIControllerTest : public ChromeRenderViewHostTestHarness {
   void TestNotChangingStateOnAutofill(password_manager::ui::State state);
 
  private:
-  MockPasswordManagerClient client_;
+  TestPasswordManagerClient client_;
 
   PasswordForm test_local_form_;
   PasswordForm test_federated_form_;
@@ -1334,16 +1368,52 @@ TEST_F(ManagePasswordsUIControllerTest, UpdateBubbleAfterLeakCheck) {
 TEST_F(ManagePasswordsUIControllerTest,
        NotifyUnsyncedCredentialsWillBeDeleted) {
   EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
-  std::vector<autofill::PasswordForm> credentials(1);
-  credentials[0].username_value = base::ASCIIToUTF16("unsynced_login");
-  credentials[0].password_value = base::ASCIIToUTF16("unsynced_password");
+  std::vector<autofill::PasswordForm> credentials(2);
+  credentials[0] =
+      BuildFormFromLoginAndOrigin("user1", "password1", "http://a.com");
+  credentials[1] =
+      BuildFormFromLoginAndOrigin("user2", "password2", "http://b.com");
+
   controller()->NotifyUnsyncedCredentialsWillBeDeleted(credentials);
+
   EXPECT_EQ(controller()->GetUnsyncedCredentials(), credentials);
   EXPECT_TRUE(controller()->opened_bubble());
   ExpectIconAndControllerStateIs(
       password_manager::ui::WILL_DELETE_UNSYNCED_ACCOUNT_PASSWORDS_STATE);
 }
 
+TEST_F(ManagePasswordsUIControllerTest, SaveUnsyncedCredentialsInProfileStore) {
+  // Setup state with unsynced credentials.
+  EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
+  std::vector<autofill::PasswordForm> credentials(2);
+  credentials[0] =
+      BuildFormFromLoginAndOrigin("user1", "password1", "http://a.com");
+  credentials[1] =
+      BuildFormFromLoginAndOrigin("user2", "password2", "http://b.com");
+  controller()->NotifyUnsyncedCredentialsWillBeDeleted(credentials);
+
+  // Set expectations on the store.
+  auto* profile_store = static_cast<password_manager::MockPasswordStore*>(
+      client().GetProfilePasswordStore());
+  EXPECT_CALL(*profile_store,
+              AddLogin(MatchesLoginAndOrigin(credentials[0].username_value,
+                                             credentials[0].password_value,
+                                             credentials[0].origin)));
+  EXPECT_CALL(*profile_store,
+              AddLogin(MatchesLoginAndOrigin(credentials[1].username_value,
+                                             credentials[1].password_value,
+                                             credentials[1].origin)));
+
+  // Save.
+  EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
+  controller()->SaveUnsyncedCredentialsInProfileStore();
+
+  // Check the credentials are gone and the bubble is closed.
+  EXPECT_TRUE(controller()->GetUnsyncedCredentials().empty());
+  EXPECT_FALSE(controller()->opened_bubble());
+  ExpectIconAndControllerStateIs(password_manager::ui::INACTIVE_STATE);
+}
+
 TEST_F(ManagePasswordsUIControllerTest, OpenBubbleForMovableForm) {
   std::vector<const PasswordForm*> matches = {&test_local_form()};
   auto test_form_manager = CreateFormManagerWithBestMatches(&matches);

chrome/browser/ui/passwords/passwords_model_delegate.h

@@ -102,6 +102,10 @@ class PasswordsModelDelegate {
   virtual void SavePassword(const base::string16& username,
                             const base::string16& password) = 0;
 
+  // Called when the user chooses to save the unsynced credentials deleted on
+  // signout (the ones returned by GetUnsyncedCredentials()).
+  virtual void SaveUnsyncedCredentialsInProfileStore() = 0;
+
   // Called from the dialog controller when a user confirms moving the recently
   // used credential to their account store.
   virtual void MovePasswordToAccountStore() = 0;

chrome/browser/ui/passwords/passwords_model_delegate_mock.h

@@ -47,6 +47,7 @@ class PasswordsModelDelegateMock
   MOCK_METHOD0(OnPasswordsRevealed, void());
   MOCK_METHOD2(SavePassword,
                void(const base::string16&, const base::string16&));
+  MOCK_METHOD0(SaveUnsyncedCredentialsInProfileStore, void());
   MOCK_METHOD0(MovePasswordToAccountStore, void());
   MOCK_METHOD2(ChooseCredential,
                void(const autofill::PasswordForm&,

chrome/browser/ui/views/passwords/password_save_unsynced_credentials_locally_view.cc

@@ -55,7 +55,9 @@ PasswordSaveUnsyncedCredentialsLocallyView::GetController() const {
 void PasswordSaveUnsyncedCredentialsLocallyView::CreateLayout() {
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::Orientation::kVertical));
-  for (const autofill::PasswordForm& row : controller_.unsynced_credentials()) {
+  DCHECK(!controller_.GetUnsyncedCredentials().empty());
+  for (const autofill::PasswordForm& row :
+       controller_.GetUnsyncedCredentials()) {
     auto* row_view = AddChildView(std::make_unique<views::View>());
     auto* username_label = row_view->AddChildView(CreateUsernameLabel(row));
     auto* password_label = row_view->AddChildView(