Add a first integration test involving both Sync and PasswordManager

The test sets up Sync-the-transport, has the user opt-in to the account
password storage, and makes sure that saves go to the correct store.

This requires a few changes to password_manager_test_base.h/cc:
- Most notably: The password manager tests used to set up a
  MockPasswordFeatureManager (in order to make IsGenerationEnabled()
  return true), but the new combined tests require a real
  PasswordFeatureManager. So this CL removes the mock (which also lets
  us remove CustomPasswordManagerClient), and instead sets up a
  TestSyncService which claims "everything is active" and so also makes
  IsGenerationEnabled() true.
- Some simple refactorings to avoid duplicating test setup code:
  - Split the "GetNewTab" part out of SetUpOnMainThreadAndGetNewTab(),
    into a new static helper.
  - Exposed PasswordStoreResultsObserver (it used to be fully defined
    in the .cc).
  - Exposed a version of CheckThatCredentialsStored() that takes a
    PasswordStore parameter.

Bug: 1058339
Change-Id: If9b040cbd56ee10ba6b6badd1b4b4d772603f13a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089825
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749220}

chrome/browser/password_manager/password_manager_test_base.cc

@@ -19,16 +19,17 @@
 #include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/autofill/chrome_autofill_client.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/autofill/core/browser/autofill_test_utils.h"
-#include "components/password_manager/core/browser/mock_password_feature_manager.h"
-#include "components/password_manager/core/browser/password_feature_manager.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
 #include "components/password_manager/core/browser/test_password_store.h"
+#include "components/sync/driver/test_sync_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_handle.h"
@@ -45,56 +46,10 @@
 
 namespace {
 
-// A helper class that synchronously waits until the password store handles a
-// GetLogins() request.
-class PasswordStoreResultsObserver
-    : public password_manager::PasswordStoreConsumer {
- public:
-  PasswordStoreResultsObserver() = default;
-
-  void OnGetPasswordStoreResults(
-      std::vector<std::unique_ptr<autofill::PasswordForm>> results) override {
-    run_loop_.Quit();
-  }
-
-  void Wait() { run_loop_.Run(); }
-
- private:
-  base::RunLoop run_loop_;
-
-  DISALLOW_COPY_AND_ASSIGN(PasswordStoreResultsObserver);
-};
-
-// Custom class is required to enable password generation.
-class CustomPasswordManagerClient : public ChromePasswordManagerClient {
- public:
-  using ChromePasswordManagerClient::ChromePasswordManagerClient;
-  CustomPasswordManagerClient(content::WebContents* contents,
-                              autofill::AutofillClient* autofill_client)
-      : ChromePasswordManagerClient(contents, autofill_client) {
-    ON_CALL(password_feature_manager_, IsGenerationEnabled())
-        .WillByDefault(testing::Return(true));
-  }
-
-  static void CreateForWebContentsWithAutofillClient(
-      content::WebContents* contents,
-      autofill::AutofillClient* autofill_client) {
-    ASSERT_FALSE(FromWebContents(contents));
-    contents->SetUserData(UserDataKey(),
-                          std::make_unique<CustomPasswordManagerClient>(
-                              contents, autofill_client));
-  }
-
-  // PasswordManagerClient:
-  password_manager::PasswordFeatureManager* GetPasswordFeatureManager()
-      override {
-    return &password_feature_manager_;
-  }
-
- private:
-  testing::NiceMock<password_manager::MockPasswordFeatureManager>
-      password_feature_manager_;
-};
+std::unique_ptr<KeyedService> BuildTestSyncService(
+    content::BrowserContext* context) {
+  return std::make_unique<syncer::TestSyncService>();
+}
 
 // ManagePasswordsUIController subclass to capture the UI events.
 class CustomManagePasswordsUIController : public ManagePasswordsUIController {
@@ -435,6 +390,21 @@ bool BubbleObserver::WaitForFallbackForSaving(
   return controller->WaitForFallbackForSaving(timeout);
 }
 
+PasswordStoreResultsObserver::PasswordStoreResultsObserver() = default;
+PasswordStoreResultsObserver::~PasswordStoreResultsObserver() = default;
+
+void PasswordStoreResultsObserver::OnGetPasswordStoreResults(
+    std::vector<std::unique_ptr<autofill::PasswordForm>> results) {
+  results_ = std::move(results);
+  run_loop_.Quit();
+}
+
+std::vector<std::unique_ptr<autofill::PasswordForm>>
+PasswordStoreResultsObserver::WaitForResults() {
+  run_loop_.Run();
+  return std::move(results_);
+}
+
 PasswordManagerBrowserTestBase::PasswordManagerBrowserTestBase()
     : https_test_server_(net::EmbeddedTestServer::TYPE_HTTPS),
       web_contents_(nullptr) {}
@@ -468,6 +438,7 @@ void PasswordManagerBrowserTestBase::TearDownOnMainThread() {
   ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete());
 }
 
+// static
 void PasswordManagerBrowserTestBase::SetUpOnMainThreadAndGetNewTab(
     Browser* browser,
     content::WebContents** web_contents) {
@@ -482,19 +453,26 @@ void PasswordManagerBrowserTestBase::SetUpOnMainThreadAndGetNewTab(
           &password_manager::BuildPasswordStore<
               content::BrowserContext, password_manager::TestPasswordStore>));
 
+  GetNewTab(browser, web_contents);
+}
+
+// static
+void PasswordManagerBrowserTestBase::GetNewTab(
+    Browser* browser,
+    content::WebContents** web_contents) {
   // Add a tab with a customized ManagePasswordsUIController. Thus, we can
   // intercept useful UI events.
-  content::WebContents* tab =
+  content::WebContents* preexisting_tab =
       browser->tab_strip_model()->GetActiveWebContents();
   std::unique_ptr<content::WebContents> owned_web_contents =
       content::WebContents::Create(
-          content::WebContents::CreateParams(tab->GetBrowserContext()));
+          content::WebContents::CreateParams(browser->profile()));
   *web_contents = owned_web_contents.get();
   ASSERT_TRUE(*web_contents);
 
   // ManagePasswordsUIController needs ChromePasswordManagerClient for logging.
   autofill::ChromeAutofillClient::CreateForWebContents(*web_contents);
-  CustomPasswordManagerClient::CreateForWebContentsWithAutofillClient(
+  ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
       *web_contents,
       autofill::ChromeAutofillClient::FromWebContents(*web_contents));
   ASSERT_TRUE(ChromePasswordManagerClient::FromWebContents(*web_contents));
@@ -502,20 +480,24 @@ void PasswordManagerBrowserTestBase::SetUpOnMainThreadAndGetNewTab(
       new CustomManagePasswordsUIController(*web_contents);
   browser->tab_strip_model()->AppendWebContents(std::move(owned_web_contents),
                                                 true);
-  browser->tab_strip_model()->CloseWebContentsAt(0, TabStripModel::CLOSE_NONE);
+  if (preexisting_tab) {
+    browser->tab_strip_model()->CloseWebContentsAt(0,
+                                                   TabStripModel::CLOSE_NONE);
+  }
   ASSERT_EQ(controller,
             ManagePasswordsUIController::FromWebContents(*web_contents));
   ASSERT_EQ(*web_contents, browser->tab_strip_model()->GetActiveWebContents());
   ASSERT_FALSE((*web_contents)->IsLoading());
 }
 
+// static
 void PasswordManagerBrowserTestBase::WaitForPasswordStore(Browser* browser) {
   scoped_refptr<password_manager::PasswordStore> password_store =
       PasswordStoreFactory::GetForProfile(browser->profile(),
                                           ServiceAccessType::IMPLICIT_ACCESS);
   PasswordStoreResultsObserver syncer;
   password_store->GetAllLoginsWithAffiliationAndBrandingInformation(&syncer);
-  syncer.Wait();
+  syncer.WaitForResults();
 }
 
 content::WebContents* PasswordManagerBrowserTestBase::WebContents() const {
@@ -681,6 +663,23 @@ void PasswordManagerBrowserTestBase::CheckElementValue(
   EXPECT_EQ(expected_value, return_value) << "element_id = " << element_id;
 }
 
+void PasswordManagerBrowserTestBase::SetUpInProcessBrowserTestFixture() {
+  will_create_browser_context_services_subscription_ =
+      BrowserContextDependencyManager::GetInstance()
+          ->RegisterWillCreateBrowserContextServicesCallbackForTesting(
+              base::BindRepeating(&PasswordManagerBrowserTestBase::
+                                      OnWillCreateBrowserContextServices));
+}
+
+// static
+void PasswordManagerBrowserTestBase::OnWillCreateBrowserContextServices(
+    content::BrowserContext* context) {
+  // Set up a TestSyncService which will happily return "everything is active"
+  // so that password generation is considered enabled.
+  ProfileSyncServiceFactory::GetInstance()->SetTestingFactory(
+      context, base::BindRepeating(&BuildTestSyncService));
+}
+
 void PasswordManagerBrowserTestBase::AddHSTSHost(const std::string& host) {
   network::mojom::NetworkContext* network_context =
       content::BrowserContext::GetDefaultStoragePartition(browser()->profile())

chrome/browser/password_manager/password_manager_test_base.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/callback_list.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "chrome/browser/ssl/cert_verifier_browser_test.h"
@@ -134,12 +135,34 @@ class BubbleObserver {
   DISALLOW_COPY_AND_ASSIGN(BubbleObserver);
 };
 
+// A helper class that synchronously waits until the password store handles a
+// GetLogins() request.
+class PasswordStoreResultsObserver
+    : public password_manager::PasswordStoreConsumer {
+ public:
+  PasswordStoreResultsObserver();
+  ~PasswordStoreResultsObserver() override;
+
+  // Waits for OnGetPasswordStoreResults() and returns the result.
+  std::vector<std::unique_ptr<autofill::PasswordForm>> WaitForResults();
+
+ private:
+  void OnGetPasswordStoreResults(
+      std::vector<std::unique_ptr<autofill::PasswordForm>> results) override;
+
+  base::RunLoop run_loop_;
+  std::vector<std::unique_ptr<autofill::PasswordForm>> results_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreResultsObserver);
+};
+
 class PasswordManagerBrowserTestBase : public CertVerifierBrowserTest {
  public:
   PasswordManagerBrowserTestBase();
   ~PasswordManagerBrowserTestBase() override;
 
   // InProcessBrowserTest:
+  void SetUpInProcessBrowserTestFixture() override;
   void SetUpOnMainThread() override;
   void TearDownOnMainThread() override;
 
@@ -151,6 +174,11 @@ class PasswordManagerBrowserTestBase : public CertVerifierBrowserTest {
   static void SetUpOnMainThreadAndGetNewTab(
       Browser* browser,
       content::WebContents** web_contents);
+
+  // Creates a new tab with all the password manager test hooks and returns it
+  // in |web_contents|.
+  static void GetNewTab(Browser* browser, content::WebContents** web_contents);
+
   // Make sure that the password store associated with the given browser
   // processed all the previous calls, calls executed on another thread.
   static void WaitForPasswordStore(Browser* browser);
@@ -217,9 +245,17 @@ class PasswordManagerBrowserTestBase : public CertVerifierBrowserTest {
   net::EmbeddedTestServer& https_test_server() { return https_test_server_; }
 
  private:
+  static void OnWillCreateBrowserContextServices(
+      content::BrowserContext* context);
+
   net::EmbeddedTestServer https_test_server_;
   // A tab with some hooks injected.
   content::WebContents* web_contents_;
+
+  std::unique_ptr<
+      base::CallbackList<void(content::BrowserContext*)>::Subscription>
+      will_create_browser_context_services_subscription_;
+
   DISALLOW_COPY_AND_ASSIGN(PasswordManagerBrowserTestBase);
 };
 

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

@@ -282,7 +282,8 @@ std::unique_ptr<views::Combobox> CreateDestinationCombobox(
   else
     combobox->SetSelectedRow(1);
 
-  // TODO(crbug.com/1044038): SetAccessibleName of the combobox.
+  // TODO(crbug.com/1044038): Use an internationalized string instead.
+  combobox->SetAccessibleName(base::ASCIIToUTF16("Destination"));
   return combobox;
 }
 

chrome/test/BUILD.gn

@@ -6282,6 +6282,7 @@ if (!is_fuchsia && !is_android) {
       "../browser/sync/test/integration/enable_disable_test.cc",
       "../browser/sync/test/integration/local_sync_test.cc",
       "../browser/sync/test/integration/migration_test.cc",
+      "../browser/sync/test/integration/password_manager_sync_test.cc",
       "../browser/sync/test/integration/single_client_app_settings_sync_test.cc",
       "../browser/sync/test/integration/single_client_apps_sync_test.cc",
       "../browser/sync/test/integration/single_client_autofill_profile_sync_test.cc",
@@ -6333,6 +6334,7 @@ if (!is_fuchsia && !is_android) {
     ]
 
     data = [
+      "//chrome/test/data/password/",
       "//chrome/test/data/sync/",
       "//net/tools/testserver/",
       "//third_party/pywebsocket3/src/mod_pywebsocket/",