[ReEPwdSave] Update the PasswordStore when saving is re-enabled

Tapping on the toggle in the keyboard accessory will now update
the PasswordStore, either by removing the blacklisted entry for
the origin if the user re-enables saving, or by re-adding it if
the user disables it again.

At the moment, the save password infobar is still not shown after
saving is enabled via the toggle, because ChromePasswordManagerClient
queries the old cache (via PasswordFormManager) which doesn't have the
latest store data. This will be changed in the next CL.

Bug: 1044930
Change-Id: I0a5cbcc6085fac5070375093b499a3e2ebc7c4e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135752
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759660}

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -529,6 +529,15 @@ void ChromePasswordManagerClient::UpdateCredentialCache(
 #endif
 }
 
+void ChromePasswordManagerClient::UpdateCacheWithBlacklistedForOrigin(
+    const url::Origin& origin,
+    bool is_blacklisted) {
+#if defined(OS_ANDROID)
+  credential_cache_.UpdateBlacklistedForOrigin(
+      origin, CredentialCache::IsOriginBlacklisted(is_blacklisted));
+#endif
+}
+
 void ChromePasswordManagerClient::PasswordWasAutofilled(
     const std::vector<const PasswordForm*>& best_matches,
     const GURL& origin,

chrome/browser/password_manager/chrome_password_manager_client.h

@@ -38,6 +38,7 @@
 #include "content/public/browser/web_contents_receiver_set.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "ui/gfx/geometry/rect.h"
+#include "url/origin.h"
 
 #if defined(OS_ANDROID)
 #include "components/password_manager/core/browser/credential_cache.h"
@@ -123,6 +124,8 @@ class ChromePasswordManagerClient
       const GURL& origin,
       const std::vector<const autofill::PasswordForm*>& best_matches,
       bool is_blacklisted) override;
+  void UpdateCacheWithBlacklistedForOrigin(const url::Origin& origin,
+                                           bool is_blacklisted) override;
   void PasswordWasAutofilled(
       const std::vector<const autofill::PasswordForm*>& best_matches,
       const GURL& origin,

chrome/browser/password_manager/password_accessory_controller_impl.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/containers/span.h"
 #include "base/feature_list.h"
@@ -16,6 +17,8 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autofill/manual_filling_controller.h"
 #include "chrome/browser/autofill/manual_filling_utils.h"
+#include "chrome/browser/password_manager/chrome_password_manager_client.h"
+#include "chrome/browser/password_manager/password_accessory_controller.h"
 #include "chrome/browser/password_manager/password_accessory_metrics_util.h"
 #include "chrome/browser/password_manager/password_generation_controller.h"
 #include "chrome/browser/password_manager/password_manager_launcher_android.h"
@@ -31,7 +34,9 @@
 #include "components/password_manager/content/browser/content_password_manager_driver.h"
 #include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
 #include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h"
+#include "components/password_manager/core/browser/credential_cache.h"
 #include "components/password_manager/core/browser/origin_credential_store.h"
+#include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
 #include "components/password_manager/core/common/password_manager_features.h"
@@ -45,6 +50,7 @@ using autofill::FooterCommand;
 using autofill::UserInfo;
 using autofill::mojom::FocusedFieldType;
 using password_manager::CredentialCache;
+using password_manager::PasswordStore;
 using password_manager::UiCredential;
 using BlacklistedStatus =
     password_manager::OriginCredentialStore::BlacklistedStatus;
@@ -142,8 +148,10 @@ void PasswordAccessoryControllerImpl::CreateForWebContents(
 
   if (!FromWebContents(web_contents)) {
     web_contents->SetUserData(
-        UserDataKey(), base::WrapUnique(new PasswordAccessoryControllerImpl(
-                           web_contents, credential_cache)));
+        UserDataKey(),
+        base::WrapUnique(new PasswordAccessoryControllerImpl(
+            web_contents, credential_cache, nullptr,
+            ChromePasswordManagerClient::FromWebContents(web_contents))));
   }
 }
 
@@ -151,15 +159,17 @@ void PasswordAccessoryControllerImpl::CreateForWebContents(
 void PasswordAccessoryControllerImpl::CreateForWebContentsForTesting(
     content::WebContents* web_contents,
     password_manager::CredentialCache* credential_cache,
-    base::WeakPtr<ManualFillingController> mf_controller) {
+    base::WeakPtr<ManualFillingController> mf_controller,
+    password_manager::PasswordManagerClient* password_client) {
   DCHECK(web_contents) << "Need valid WebContents to attach controller to!";
   DCHECK(!FromWebContents(web_contents)) << "Controller already attached!";
   DCHECK(mf_controller);
+  DCHECK(password_client);
 
   web_contents->SetUserData(
-      UserDataKey(),
-      base::WrapUnique(new PasswordAccessoryControllerImpl(
-          web_contents, credential_cache, std::move(mf_controller))));
+      UserDataKey(), base::WrapUnique(new PasswordAccessoryControllerImpl(
+                         web_contents, credential_cache,
+                         std::move(mf_controller), password_client)));
 }
 
 // static
@@ -214,8 +224,7 @@ void PasswordAccessoryControllerImpl::OnToggleChanged(
     autofill::AccessoryAction toggled_action,
     bool enabled) {
   if (toggled_action == autofill::AccessoryAction::TOGGLE_SAVE_PASSWORDS) {
-    // TODO(crbug.com/1044930): Update the cache and the password store
-    // according to the toggle value.
+    ChangeCurrentOriginSavePasswordsStatus(enabled);
     return;
   }
   NOTREACHED() << "Unhandled selected action: "
@@ -300,19 +309,41 @@ void PasswordAccessoryControllerImpl::OnGenerationRequested(
   pwd_generation_controller->OnGenerationRequested(type);
 }
 
-PasswordAccessoryControllerImpl::PasswordAccessoryControllerImpl(
-    content::WebContents* web_contents,
-    password_manager::CredentialCache* credential_cache)
-    : web_contents_(web_contents), credential_cache_(credential_cache) {}
-
-// Additional creation functions in unit tests only:
 PasswordAccessoryControllerImpl::PasswordAccessoryControllerImpl(
     content::WebContents* web_contents,
     password_manager::CredentialCache* credential_cache,
-    base::WeakPtr<ManualFillingController> mf_controller)
+    base::WeakPtr<ManualFillingController> mf_controller,
+    password_manager::PasswordManagerClient* password_client)
     : web_contents_(web_contents),
       credential_cache_(credential_cache),
-      mf_controller_(std::move(mf_controller)) {}
+      mf_controller_(std::move(mf_controller)),
+      password_client_(password_client) {}
+
+void PasswordAccessoryControllerImpl::ChangeCurrentOriginSavePasswordsStatus(
+    bool saving_enabled) {
+  const url::Origin origin = GetFocusedFrameOrigin();
+  if (origin.opaque())
+    return;
+
+  const GURL origin_as_gurl = origin.GetURL();
+  password_manager::PasswordStore::FormDigest form_digest(
+      autofill::PasswordForm::Scheme::kHtml,
+      password_manager::GetSignonRealm(origin_as_gurl), origin_as_gurl);
+  password_manager::PasswordStore* store =
+      password_client_->GetProfilePasswordStore();
+  password_client_->UpdateCacheWithBlacklistedForOrigin(origin,
+                                                        !saving_enabled);
+  if (saving_enabled) {
+    store->Unblacklist(form_digest, base::NullCallback());
+    return;
+  }
+
+  autofill::PasswordForm form =
+      password_manager_util::MakeNormalizedBlacklistedForm(
+          std::move(form_digest));
+  form.date_created = base::Time::Now();
+  store->AddLogin(form);
+}
 
 bool PasswordAccessoryControllerImpl::AppearsInSuggestions(
     const base::string16& suggestion,

chrome/browser/password_manager/password_accessory_controller_impl.h

@@ -19,6 +19,7 @@
 #include "components/autofill/core/common/mojom/autofill_types.mojom-forward.h"
 #include "components/autofill/core/common/password_generation_util.h"
 #include "components/password_manager/core/browser/credential_cache.h"
+#include "components/password_manager/core/browser/password_manager_client.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "url/gurl.h"
 
@@ -60,11 +61,12 @@ class PasswordAccessoryControllerImpl
 
   // Like |CreateForWebContents|, it creates the controller and attaches it to
   // the given |web_contents|. Additionally, it allows inject a manual filling
-  // controller.
+  // controller and a |PasswordManagerClient|.
   static void CreateForWebContentsForTesting(
       content::WebContents* web_contents,
       password_manager::CredentialCache* credential_cache,
-      base::WeakPtr<ManualFillingController> mf_controller);
+      base::WeakPtr<ManualFillingController> mf_controller,
+      password_manager::PasswordManagerClient* password_client);
 
   // True if the focus event was sent for the current focused frame or if it is
   // a blur event and no frame is focused. This check avoids reacting to
@@ -79,16 +81,18 @@ class PasswordAccessoryControllerImpl
  private:
   friend class content::WebContentsUserData<PasswordAccessoryControllerImpl>;
 
-  // Required for construction via |CreateForWebContents|:
-  PasswordAccessoryControllerImpl(
-      content::WebContents* contents,
-      password_manager::CredentialCache* credential_cache);
-
-  // Constructor that allows to inject a mock or fake view.
+  // This constructor can also be used by |CreateForWebContentsForTesting|
+  // to inject a fake |ManualFillingController| and a fake
+  // |PasswordManagerClient|.
   PasswordAccessoryControllerImpl(
       content::WebContents* web_contents,
       password_manager::CredentialCache* credential_cache,
-      base::WeakPtr<ManualFillingController> mf_controller);
+      base::WeakPtr<ManualFillingController> mf_controller,
+      password_manager::PasswordManagerClient* password_client);
+
+  // Enables or disables saving for the focused origin. This involves removing
+  // or adding blacklisted entry in the |PasswordStore|.
+  void ChangeCurrentOriginSavePasswordsStatus(bool enabled);
 
   // Returns true if |suggestion| matches a credential for |origin|.
   bool AppearsInSuggestions(const base::string16& suggestion,
@@ -106,14 +110,18 @@ class PasswordAccessoryControllerImpl
   // ------------------------------------------------------------------------
 
   // The tab for which this class is scoped.
-  content::WebContents* web_contents_;
+  content::WebContents* web_contents_ = nullptr;
 
   // Keeps track of credentials which are stored for all origins in this tab.
-  password_manager::CredentialCache* credential_cache_;
+  password_manager::CredentialCache* credential_cache_ = nullptr;
 
   // The password accessory controller object to forward client requests to.
   base::WeakPtr<ManualFillingController> mf_controller_;
 
+  // The password manager client is used to update the save passwords status
+  // for the currently focused origin.
+  password_manager::PasswordManagerClient* password_client_ = nullptr;
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 
   DISALLOW_COPY_AND_ASSIGN(PasswordAccessoryControllerImpl);

chrome/browser/password_manager/password_accessory_controller_impl_unittest.cc

@@ -22,21 +22,25 @@
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/autofill/core/browser/ui/accessory_sheet_enums.h"
 #include "components/autofill/core/common/autofill_features.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/autofill/core/common/password_generation_util.h"
 #include "components/autofill/core/common/signatures_util.h"
 #include "components/password_manager/core/browser/credential_cache.h"
+#include "components/password_manager/core/browser/mock_password_store.h"
+#include "components/password_manager/core/browser/origin_credential_store.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "components/password_manager/core/browser/stub_password_manager_client.h"
 #include "components/password_manager/core/browser/stub_password_manager_driver.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/strings/grit/components_strings.h"
 #include "content/public/test/web_contents_tester.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/codec/png_codec.h"
+#include "url/origin.h"
 
 namespace {
 using autofill::AccessoryAction;
@@ -49,8 +53,11 @@ using autofill::mojom::FocusedFieldType;
 using base::ASCIIToUTF16;
 using password_manager::CreateEntry;
 using password_manager::CredentialCache;
+using password_manager::MockPasswordStore;
+using password_manager::OriginCredentialStore;
 using testing::_;
 using testing::ByMove;
+using testing::Eq;
 using testing::Mock;
 using testing::NiceMock;
 using testing::Return;
@@ -61,6 +68,7 @@ using IsPslMatch = autofill::UserInfo::IsPslMatch;
 
 constexpr char kExampleSite[] = "https://example.com";
 constexpr char kExampleSiteMobile[] = "https://m.example.com";
+constexpr char kExampleSignonRealm[] = "https://example.com/";
 constexpr char kExampleDomain[] = "example.com";
 
 class MockPasswordGenerationController
@@ -70,8 +78,9 @@ class MockPasswordGenerationController
 
   explicit MockPasswordGenerationController(content::WebContents* web_contents);
 
-  MOCK_METHOD1(OnGenerationRequested,
-               void(autofill::password_generation::PasswordGenerationType));
+  MOCK_METHOD(void,
+              OnGenerationRequested,
+              (autofill::password_generation::PasswordGenerationType));
 };
 
 // static
@@ -87,6 +96,24 @@ MockPasswordGenerationController::MockPasswordGenerationController(
     content::WebContents* web_contents)
     : PasswordGenerationControllerImpl(web_contents) {}
 
+class MockPasswordManagerClient
+    : public password_manager::StubPasswordManagerClient {
+ public:
+  explicit MockPasswordManagerClient(MockPasswordStore* mock_password_store)
+      : mock_password_store_(mock_password_store) {}
+
+  MOCK_METHOD(void,
+              UpdateCacheWithBlacklistedForOrigin,
+              (const url::Origin&, bool));
+
+  password_manager::PasswordStore* GetProfilePasswordStore() const override {
+    return mock_password_store_;
+  }
+
+ private:
+  MockPasswordStore* mock_password_store_;
+};
+
 base::string16 password_for_str(const base::string16& user) {
   return l10n_util::GetStringFUTF16(
       IDS_PASSWORD_MANAGER_ACCESSORY_PASSWORD_DESCRIPTION, user);
@@ -134,7 +161,9 @@ AccessorySheetData::Builder PasswordAccessorySheetDataBuilder(
 
 class PasswordAccessoryControllerTest : public ChromeRenderViewHostTestHarness {
  public:
-  PasswordAccessoryControllerTest() = default;
+  PasswordAccessoryControllerTest()
+      : ChromeRenderViewHostTestHarness(
+            base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
@@ -146,22 +175,36 @@ class PasswordAccessoryControllerTest : public ChromeRenderViewHostTestHarness {
               web_contents()->GetFocusedFrame()->GetLastCommittedOrigin());
 
     MockPasswordGenerationController::CreateForWebContents(web_contents());
+    mock_password_store_ = base::MakeRefCounted<MockPasswordStore>();
+    mock_password_store_->Init(nullptr);
+    mock_pwd_manager_client_ =
+        std::make_unique<MockPasswordManagerClient>(mock_password_store_.get());
+
     PasswordAccessoryControllerImpl::CreateForWebContentsForTesting(
-        web_contents(), cache(), mock_manual_filling_controller_.AsWeakPtr());
+        web_contents(), cache(), mock_manual_filling_controller_.AsWeakPtr(),
+        mock_pwd_manager_client_.get());
     NavigateAndCommit(GURL(kExampleSite));
   }
 
+  void TearDown() override { mock_password_store_->ShutdownOnUIThread(); }
+
   PasswordAccessoryController* controller() {
     return PasswordAccessoryControllerImpl::FromWebContents(web_contents());
   }
 
   password_manager::CredentialCache* cache() { return &credential_cache_; }
 
+  MockPasswordManagerClient* password_client() {
+    return mock_pwd_manager_client_.get();
+  }
+
  protected:
   StrictMock<MockManualFillingController> mock_manual_filling_controller_;
+  scoped_refptr<MockPasswordStore> mock_password_store_;
 
  private:
   password_manager::CredentialCache credential_cache_;
+  std::unique_ptr<MockPasswordManagerClient> mock_pwd_manager_client_;
 };
 
 TEST_F(PasswordAccessoryControllerTest, IsNotRecreatedForSameWebContents) {
@@ -593,7 +636,8 @@ TEST_F(PasswordAccessoryControllerTest,
   FocusWebContentsOnMainFrame();
 
   PasswordAccessoryControllerImpl::CreateForWebContentsForTesting(
-      raw_web_contents, cache(), mock_manual_filling_controller_.AsWeakPtr());
+      raw_web_contents, cache(), mock_manual_filling_controller_.AsWeakPtr(),
+      password_client());
   PasswordAccessoryController* incognito_accessory =
       PasswordAccessoryControllerImpl::FromWebContents(raw_web_contents);
   cache()->SaveCredentialsAndBlacklistedForOrigin(
@@ -638,3 +682,32 @@ TEST_F(PasswordAccessoryControllerTest, AddsSaveToggleIfWasBlacklisted) {
       FocusedFieldType::kFillablePasswordField,
       /*is_manual_generation_available=*/false);
 }
+
+TEST_F(PasswordAccessoryControllerTest, SavePasswordsToggledUpdatesCache) {
+  url::Origin example_origin = url::Origin::Create(GURL(kExampleSite));
+  EXPECT_CALL(*password_client(),
+              UpdateCacheWithBlacklistedForOrigin(example_origin, false));
+  controller()->OnToggleChanged(
+      autofill::AccessoryAction::TOGGLE_SAVE_PASSWORDS, true);
+}
+
+TEST_F(PasswordAccessoryControllerTest, SavePasswordsEnabledUpdatesStore) {
+  password_manager::PasswordStore::FormDigest form_digest(
+      autofill::PasswordForm::Scheme::kHtml, kExampleSignonRealm,
+      GURL(kExampleSite));
+  EXPECT_CALL(*mock_password_store_, Unblacklist(form_digest, _));
+  controller()->OnToggleChanged(
+      autofill::AccessoryAction::TOGGLE_SAVE_PASSWORDS, true);
+}
+
+TEST_F(PasswordAccessoryControllerTest, SavePasswordsDisabledUpdatesStore) {
+  autofill::PasswordForm expected_form;
+  expected_form.blacklisted_by_user = true;
+  expected_form.scheme = autofill::PasswordForm::Scheme::kHtml;
+  expected_form.signon_realm = kExampleSignonRealm;
+  expected_form.origin = GURL(kExampleSite);
+  expected_form.date_created = base::Time::Now();
+  EXPECT_CALL(*mock_password_store_, AddLogin(Eq(expected_form)));
+  controller()->OnToggleChanged(
+      autofill::AccessoryAction::TOGGLE_SAVE_PASSWORDS, false);
+}

components/password_manager/core/browser/mock_password_store.h

@@ -23,6 +23,8 @@ class MockPasswordStore : public PasswordStore {
   MockPasswordStore();
 
   MOCK_METHOD1(RemoveLogin, void(const autofill::PasswordForm&));
+  MOCK_METHOD2(Unblacklist,
+               void(const PasswordStore::FormDigest&, base::OnceClosure));
   MOCK_METHOD2(GetLogins,
                void(const PasswordStore::FormDigest&, PasswordStoreConsumer*));
   MOCK_METHOD2(GetLoginsByPassword,

components/password_manager/core/browser/password_manager_client.cc

@@ -9,6 +9,7 @@
 #include "components/password_manager/core/browser/password_form_manager_for_ui.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/signin/public/base/signin_metrics.h"
+#include "url/origin.h"
 
 namespace password_manager {
 
@@ -46,6 +47,10 @@ void PasswordManagerClient::UpdateCredentialCache(
     const std::vector<const autofill::PasswordForm*>& best_matches,
     bool is_blacklisted) {}
 
+void PasswordManagerClient::UpdateCacheWithBlacklistedForOrigin(
+    const url::Origin& origin,
+    bool is_blacklisted) {}
+
 void PasswordManagerClient::PasswordWasAutofilled(
     const std::vector<const autofill::PasswordForm*>& best_matches,
     const GURL& origin,

components/password_manager/core/browser/password_manager_client.h

@@ -214,6 +214,12 @@ class PasswordManagerClient {
       const std::vector<const autofill::PasswordForm*>& best_matches,
       bool is_blacklisted);
 
+  // Update the CredentialCache with information on whether the origin is
+  // blacklisted for passwords saving. This change is user-iniated from UI
+  // provided for this purpose. Currently only implemented on Android.
+  virtual void UpdateCacheWithBlacklistedForOrigin(const url::Origin& origin,
+                                                   bool is_blacklisted);
+
   // Called when a password is saved in an automated fashion. Embedder may
   // inform the user that this save has occurred.
   virtual void AutomaticPasswordSave(

components/password_manager/core/browser/password_store.h

@@ -213,8 +213,8 @@ class PasswordStore : protected PasswordStoreSync,
   // blacklisted entries. If |completion| is not null, it will be posted to the
   // |main_task_runner_| after deletions have been completed. Should be called
   // on the UI thread.
-  void Unblacklist(const PasswordStore::FormDigest& form_digest,
-                   base::OnceClosure completion);
+  virtual void Unblacklist(const PasswordStore::FormDigest& form_digest,
+                           base::OnceClosure completion);
 
   // Searches for a matching PasswordForm, and notifies |consumer| on
   // completion. The request will be cancelled if the consumer is destroyed.