[Passwords] Obtain List of Passwords in SavedPasswordsPresenter

This change implements obtaining the list of saved passwords in
SavedPasswordsPresenter and makes it listen to appropriate events and
notifying its observers.

Bug: 1047726
Change-Id: Ic859e4f1aca19be36d6df8ef525564dc023191ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2063129Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742673}

components/password_manager/core/browser/BUILD.gn

@@ -577,6 +577,7 @@ source_set("unit_tests") {
     "sync_username_test_base.h",
     "ui/bulk_leak_check_service_adapter_unittest.cc",
     "ui/compromised_credentials_provider_unittest.cc",
+    "ui/saved_passwords_presenter_unittest.cc",
     "vote_uploads_test_matchers.h",
     "votes_uploader_unittest.cc",
   ]

components/password_manager/core/browser/ui/bulk_leak_check_service_adapter_unittest.cc

@@ -9,8 +9,10 @@
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/bulk_leak_check_service.h"
 #include "components/password_manager/core/browser/leak_detection/mock_leak_detection_check_factory.h"
+#include "components/password_manager/core/browser/test_password_store.h"
 #include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
+#include "components/sync/model/syncable_service.h"
 #include "services/network/test/test_shared_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -18,25 +20,18 @@
 namespace password_manager {
 namespace {
 
-struct MockSavedPasswordsPresenter : SavedPasswordsPresenter {
-  MOCK_METHOD(void,
-              EditPassword,
-              (const autofill::PasswordForm&, base::StringPiece16),
-              (override));
-
-  MOCK_METHOD(std::vector<autofill::PasswordForm>,
-              GetSavedPasswords,
-              (),
-              (override));
-};
-
 class BulkLeakCheckServiceAdapterTest : public ::testing::Test {
  public:
-  BulkLeakCheckServiceAdapterTest()
-      : service_(identity_test_env_.identity_manager(),
-                 base::MakeRefCounted<network::TestSharedURLLoaderFactory>()) {
+  BulkLeakCheckServiceAdapterTest() {
     service_.set_leak_factory(
         std::make_unique<MockLeakDetectionCheckFactory>());
+
+    store_->Init(syncer::SyncableService::StartSyncFlare(), /*prefs=*/nullptr);
+  }
+
+  ~BulkLeakCheckServiceAdapterTest() override {
+    store_->ShutdownOnUIThread();
+    task_env_.RunUntilIdle();
   }
 
   BulkLeakCheckServiceAdapter& adapter() { return adapter_; }
@@ -44,8 +39,12 @@ class BulkLeakCheckServiceAdapterTest : public ::testing::Test {
  private:
   base::test::TaskEnvironment task_env_;
   signin::IdentityTestEnvironment identity_test_env_;
-  ::testing::StrictMock<MockSavedPasswordsPresenter> presenter_;
-  BulkLeakCheckService service_;
+  scoped_refptr<TestPasswordStore> store_ =
+      base::MakeRefCounted<TestPasswordStore>();
+  SavedPasswordsPresenter presenter_{store_};
+  BulkLeakCheckService service_{
+      identity_test_env_.identity_manager(),
+      base::MakeRefCounted<network::TestSharedURLLoaderFactory>()};
   BulkLeakCheckServiceAdapter adapter_{&presenter_, &service_};
 };
 

components/password_manager/core/browser/ui/saved_passwords_presenter.cc

@@ -3,13 +3,48 @@
 // found in the LICENSE file.
 
 #include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
+
+#include <algorithm>
+#include <utility>
+
+#include "base/logging.h"
+#include "base/strings/string16.h"
 #include "components/autofill/core/common/password_form.h"
 
 namespace password_manager {
 
-SavedPasswordsPresenter::SavedPasswordsPresenter() = default;
+SavedPasswordsPresenter::SavedPasswordsPresenter(
+    scoped_refptr<PasswordStore> store)
+    : store_(std::move(store)) {
+  DCHECK(store_);
+  store_->AddObserver(this);
+}
+
+SavedPasswordsPresenter::~SavedPasswordsPresenter() {
+  store_->RemoveObserver(this);
+}
+
+void SavedPasswordsPresenter::Init() {
+  store_->GetAllLoginsWithAffiliationAndBrandingInformation(this);
+}
 
-SavedPasswordsPresenter::~SavedPasswordsPresenter() = default;
+bool SavedPasswordsPresenter::EditPassword(
+    const autofill::PasswordForm& password,
+    base::string16 new_password) {
+  auto found = std::find(passwords_.begin(), passwords_.end(), password);
+  if (found == passwords_.end())
+    return false;
+
+  found->password_value = std::move(new_password);
+  store_->UpdateLogin(*found);
+  NotifyEdited(*found);
+  return true;
+}
+
+SavedPasswordsPresenter::SavedPasswordsView
+SavedPasswordsPresenter::GetSavedPasswords() const {
+  return passwords_;
+}
 
 void SavedPasswordsPresenter::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
@@ -27,7 +62,22 @@ void SavedPasswordsPresenter::NotifyEdited(
 
 void SavedPasswordsPresenter::NotifySavedPasswordsChanged() {
   for (auto& observer : observers_)
-    observer.OnSavedPasswordsChanged();
+    observer.OnSavedPasswordsChanged(passwords_);
+}
+
+void SavedPasswordsPresenter::OnLoginsChanged(
+    const PasswordStoreChangeList& changes) {
+  // Cancel ongoing requests to the password store and issue a new request.
+  cancelable_task_tracker()->TryCancelAll();
+  store_->GetAllLoginsWithAffiliationAndBrandingInformation(this);
+}
+
+void SavedPasswordsPresenter::OnGetPasswordStoreResults(
+    std::vector<std::unique_ptr<autofill::PasswordForm>> results) {
+  passwords_.resize(results.size());
+  std::transform(results.begin(), results.end(), passwords_.begin(),
+                 [](auto& result) { return std::move(*result); });
+  NotifySavedPasswordsChanged();
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/ui/saved_passwords_presenter.h

@@ -7,8 +7,12 @@
 
 #include <vector>
 
+#include "base/containers/span.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/observer_list.h"
 #include "base/strings/string_piece_forward.h"
+#include "components/password_manager/core/browser/password_store.h"
+#include "components/password_manager/core/browser/password_store_consumer.h"
 
 namespace autofill {
 struct PasswordForm;
@@ -27,8 +31,11 @@ namespace password_manager {
 // in the new password to be checked, whereas other password edit operations
 // (such as visiting a change password form and then updating the password in
 // Chrome) should not trigger a check.
-class SavedPasswordsPresenter {
+class SavedPasswordsPresenter : public PasswordStore::Observer,
+                                public PasswordStoreConsumer {
  public:
+  using SavedPasswordsView = base::span<const autofill::PasswordForm>;
+
   // Observer interface. Clients can implement this to get notified about
   // changes to the list of saved passwords or if a given password was edited
   // Clients can register and de-register themselves, and are expected to do so
@@ -38,42 +45,57 @@ class SavedPasswordsPresenter {
     // Notifies the observer when a password is edited or the list of saved
     // passwords changed.
     //
-    // OnEdited() will be invoked asynchronously if EditPassword() is invoked
-    // with a password that was present in the store. |password.password_value|
-    // will be equal to |new_password| in this case.
+    // OnEdited() will be invoked synchronously if EditPassword() is invoked
+    // with a password that was present in |passwords_|.
+    // |password.password_value| will be equal to |new_password| in this case.
     virtual void OnEdited(const autofill::PasswordForm& password) {}
     // OnSavedPasswordsChanged() gets invoked asynchronously after a change to
     // the underlying password store happens. This might be due to a call to
     // EditPassword(), but can also happen if passwords are added or removed due
-    // to other reasons. Clients are then expected to call GetSavedPassword() in
-    // order to obtain the new list of credentials.
-    virtual void OnSavedPasswordsChanged() {}
+    // to other reasons.
+    virtual void OnSavedPasswordsChanged(SavedPasswordsView passwords) {}
   };
 
-  SavedPasswordsPresenter();
-  virtual ~SavedPasswordsPresenter();
+  explicit SavedPasswordsPresenter(scoped_refptr<PasswordStore> store);
+  ~SavedPasswordsPresenter() override;
+
+  // Initializes the presenter and makes it issue the first request for all
+  // saved passwords.
+  void Init();
 
-  // Edits |password|. This will ask the password store to change the underlying
-  // password_value to |new_password|. This will also notify clients that an
-  // edit event happened in case |password| was present in the store.
-  virtual void EditPassword(const autofill::PasswordForm& password,
-                            base::StringPiece16 new_password) = 0;
+  // Tries to edit |password|. After checking whether |password| is present in
+  // |passwords_|, this will ask the password store to change the underlying
+  // password_value to |new_password| in case it was found. This will also
+  // notify clients that an edit event happened in case |password| was present
+  // in |passwords_|.
+  bool EditPassword(const autofill::PasswordForm& password,
+                    base::string16 new_password);
 
-  // Returns a list of the currently saved credentials. Note that this is not a
-  // read-only view, as it combines the result of multiple password stores that
-  // change independently. This list is created on demand and callers should be
-  // mindful to not create unnecessary copies.
-  virtual std::vector<autofill::PasswordForm> GetSavedPasswords() = 0;
+  // Returns a list of the currently saved credentials.
+  SavedPasswordsView GetSavedPasswords() const;
 
   // Allows clients and register and de-register themselves.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
  private:
+  // PasswordStore::Observer
+  void OnLoginsChanged(const PasswordStoreChangeList& changes) override;
+
+  // PasswordStoreConsumer:
+  void OnGetPasswordStoreResults(
+      std::vector<std::unique_ptr<autofill::PasswordForm>> results) override;
+
   // Notify observers about changes in the compromised credentials.
   void NotifyEdited(const autofill::PasswordForm& password);
   void NotifySavedPasswordsChanged();
 
+  // The password store containing the saved passwords.
+  scoped_refptr<PasswordStore> store_;
+
+  // Cache of the most recently obtained saved passwords.
+  std::vector<autofill::PasswordForm> passwords_;
+
   base::ObserverList<Observer, /*check_empty=*/true> observers_;
 };
 

components/password_manager/core/browser/ui/saved_passwords_presenter_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
+
+#include "base/memory/scoped_refptr.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/task_environment.h"
+#include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/test_password_store.h"
+#include "components/sync/model/syncable_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace password_manager {
+
+namespace {
+
+using autofill::PasswordForm;
+using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
+using ::testing::IsEmpty;
+using ::testing::Pair;
+
+struct MockSavedPasswordsPresenterObserver : SavedPasswordsPresenter::Observer {
+  MOCK_METHOD(void, OnEdited, (const autofill::PasswordForm&), (override));
+  MOCK_METHOD(void,
+              OnSavedPasswordsChanged,
+              (SavedPasswordsPresenter::SavedPasswordsView),
+              (override));
+};
+
+using StrictMockSavedPasswordsPresenterObserver =
+    ::testing::StrictMock<MockSavedPasswordsPresenterObserver>;
+
+class SavedPasswordsPresenterTest : public ::testing::Test {
+ protected:
+  SavedPasswordsPresenterTest() {
+    store_->Init(syncer::SyncableService::StartSyncFlare(), /*prefs=*/nullptr);
+  }
+
+  ~SavedPasswordsPresenterTest() override {
+    store_->ShutdownOnUIThread();
+    task_env_.RunUntilIdle();
+  }
+
+  TestPasswordStore& store() { return *store_; }
+  SavedPasswordsPresenter& presenter() { return presenter_; }
+
+  void RunUntilIdle() { task_env_.RunUntilIdle(); }
+
+ private:
+  base::test::SingleThreadTaskEnvironment task_env_;
+  scoped_refptr<TestPasswordStore> store_ =
+      base::MakeRefCounted<TestPasswordStore>();
+  SavedPasswordsPresenter presenter_{store_};
+};
+
+}  // namespace
+
+// Tests whether adding and removing an observer works as expected.
+TEST_F(SavedPasswordsPresenterTest, NotifyObservers) {
+  PasswordForm form;
+
+  StrictMockSavedPasswordsPresenterObserver observer;
+  presenter().AddObserver(&observer);
+
+  // Adding a credential should notify observers. Furthermore, the credential
+  // should be present of the list that is passed along.
+  EXPECT_CALL(observer, OnSavedPasswordsChanged(ElementsAre(form)));
+  store().AddLogin(form);
+  RunUntilIdle();
+  EXPECT_FALSE(store().IsEmpty());
+
+  // Remove should notify, and observers should be passed an empty list.
+  EXPECT_CALL(observer, OnSavedPasswordsChanged(IsEmpty()));
+  store().RemoveLogin(form);
+  RunUntilIdle();
+  EXPECT_TRUE(store().IsEmpty());
+
+  // After an observer is removed it should no longer receive notifications.
+  presenter().RemoveObserver(&observer);
+  EXPECT_CALL(observer, OnSavedPasswordsChanged).Times(0);
+  store().AddLogin(form);
+  RunUntilIdle();
+  EXPECT_FALSE(store().IsEmpty());
+}
+
+// Tests whether editing a password works and results in the right
+// notifications.
+TEST_F(SavedPasswordsPresenterTest, EditPassword) {
+  PasswordForm form;
+
+  StrictMockSavedPasswordsPresenterObserver observer;
+  presenter().AddObserver(&observer);
+
+  EXPECT_CALL(observer, OnSavedPasswordsChanged);
+  store().AddLogin(form);
+  RunUntilIdle();
+  EXPECT_FALSE(store().IsEmpty());
+
+  const base::string16 new_password = base::ASCIIToUTF16("new_password");
+  PasswordForm updated = form;
+  updated.password_value = new_password;
+
+  // Verify that editing a password triggers the right notifications.
+  EXPECT_CALL(observer, OnEdited(updated));
+  EXPECT_CALL(observer, OnSavedPasswordsChanged(ElementsAre(updated)));
+  EXPECT_TRUE(presenter().EditPassword(form, new_password));
+  RunUntilIdle();
+  EXPECT_THAT(store().stored_passwords(),
+              ElementsAre(Pair(updated.signon_realm, ElementsAre(updated))));
+
+  // Verify that editing a password that does not exist does not triggers
+  // notifications.
+  EXPECT_CALL(observer, OnEdited).Times(0);
+  EXPECT_CALL(observer, OnSavedPasswordsChanged).Times(0);
+  EXPECT_FALSE(presenter().EditPassword(form, new_password));
+  RunUntilIdle();
+
+  presenter().RemoveObserver(&observer);
+}
+
+}  // namespace password_manager