Suppress save and update bubbles when storing a PSL matched credential

chrome/browser/password_manager/credential_manager_browsertest.cc

Index: chrome/browser/password_manager/credential_manager_browsertest.cc
diff --git a/chrome/browser/password_manager/credential_manager_browsertest.cc b/chrome/browser/password_manager/credential_manager_browsertest.cc
index e0cd9cc5a921ab7a26b2fc0f0a164c94edd35513..37523ea03e509e52a65cae92407fe31b4767efbf 100644
--- a/chrome/browser/password_manager/credential_manager_browsertest.cc
+++ b/chrome/browser/password_manager/credential_manager_browsertest.cc
@@ -3,17 +3,23 @@
 // found in the LICENSE file.
 
 #include "base/macros.h"
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/password_manager/password_manager_test_base.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/passwords/passwords_model_delegate.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "components/password_manager/core/browser/password_bubble_experiment.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "components/password_manager/core/browser/test_password_store.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
+#include "net/cert/cert_verify_result.h"
+#include "net/cert/mock_cert_verifier.h"
+#include "net/dns/mock_host_resolver.h"
 
 namespace {
 
@@ -41,7 +47,8 @@ class PasswordStoreResultsObserver
 
 class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
  public:
-  CredentialManagerBrowserTest() = default;
+  CredentialManagerBrowserTest()
+      : mock_cert_verifier_(base::MakeUnique<net::MockCertVerifier>()) {}
 
   bool IsShowingAccountChooser() {
     return PasswordsModelDelegateFromWebContents(WebContents())->
@@ -59,8 +66,33 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
     syncer.Wait();
   }
 
- private:
+  // Similarly to PasswordManagerBrowserTestBase::NavigateToFile this is a
+  // wrapper around ui_test_utils::NavigateURL that waits until DidFinishLoad()
+  // fires. Different to NavigateToFile this method allows passing a test_server
+  // and modifications to the hostname.
+  void NavigateToURL(const net::EmbeddedTestServer& test_server,
+                     const std::string& hostname,
+                     const std::string& relative_url = "/") {

[vabr (Chromium), 2016/12/22 13:08:24]

I don't see where this is only called with 2 arguments. Could we drop the
default value for the third one?

[jdoerrie, 2016/12/22 16:20:00]

Done.

+    NavigationObserver observer(WebContents());
+    GURL url = test_server.GetURL(hostname, relative_url);
+    ui_test_utils::NavigateToURL(browser(), url);
+    observer.Wait();
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    ProfileIOData::SetCertVerifierForTesting(mock_cert_verifier_.get());
+  }
+
+  void TearDownInProcessBrowserTestFixture() override {
+    ProfileIOData::SetCertVerifierForTesting(nullptr);
+  }
 
+  net::MockCertVerifier* mock_cert_verifier() {
+    return mock_cert_verifier_.get();
+  }
+
+ private:
+  std::unique_ptr<net::MockCertVerifier> mock_cert_verifier_;

[vabr (Chromium), 2016/12/22 13:08:24]

This is created in the constructor and never deleted. Could it be just
aggregated without being put into a unique_ptr?

[vasilii, 2016/12/22 13:24:58]

Why not just net::MockCertVerifier> mock_cert_verifier_?

[jdoerrie, 2016/12/22 16:20:01]

Done.

   DISALLOW_COPY_AND_ASSIGN(CredentialManagerBrowserTest);
 };
 
@@ -119,6 +151,83 @@ IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
   EXPECT_FALSE(form.skip_zero_click);
 }
 
+IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
+                       StoreSavesPSLMatchedCredential) {
+  // Setup HTTPS server serving files from standard test directory.
+  const base::FilePath::CharType kDocRoot[] =

[vabr (Chromium), 2016/12/22 13:08:24]

Please make this both static and constexpr, see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/-2PSZ...

[jdoerrie, 2016/12/22 16:20:01]

Done.

+      FILE_PATH_LITERAL("chrome/test/data");
+  net::EmbeddedTestServer https_test_server(
+      net::EmbeddedTestServer::TYPE_HTTPS);
+  https_test_server.ServeFilesFromSourceDirectory(base::FilePath(kDocRoot));
+  ASSERT_TRUE(https_test_server.Start());
+
+  // Setup mock certificate for all origins.
+  auto cert = https_test_server.GetCertificate();
+  net::CertVerifyResult verify_result;
+  verify_result.cert_status = 0;
+  verify_result.is_issued_by_known_root = true;
+  verify_result.verified_cert = cert;
+  mock_cert_verifier()->AddResultForCert(cert.get(), verify_result, net::OK);

[jdoerrie, 2016/12/22 12:33:53]

Even though there is a "--ignore-certificate-errors" flag
(https://codesearch.chromium.org/chromium/src/content/public/common/content_sw...),
we need to setup a mock certificate for the CM API. The reason for this is a
missing check for "--ignore-certificate-errors" in 
|ChromePasswordManagerClient::DidLastPageLoadEncounterSSLErrors|
(https://codesearch.chromium.org/chromium/src/chrome/browser/password_manager/...)
which in turn is called by
|ChromePasswordManagerClient::IsFillingEnabledForCurrentPage| and
|CredentialManagerImpl::Get|.

I am not sure whether this missing check is a bug or a feature, which is why I
would like to hear your opinion here.

[vabr (Chromium), 2016/12/22 13:08:24]

It depends on the purpose of the --ignore-certificate-errors. If it is only
meant for testing, then it might be fine to stay as it is. Whoever introduced
this flag might have an answer to that.

In general, I prefer what you did here anyway -- setting the certificates
properly ensures that the codepaths seen in this test are more similar to what
the users generate than if it uses shortcuts through internal flags.

[vasilii, 2016/12/22 13:24:58]

Leave it as it is.

I wonder how  NoPromptForLoginFailedAndServerPushSeperateLoginForm_HttpsToHttp
passes. Seems like the test is just broken.

[jdoerrie, 2016/12/22 16:20:00]

Acknowledged.

+
+  // Redirect all requests to localhost.
+  host_resolver()->AddRule("*", "127.0.0.1");
+
+  scoped_refptr<password_manager::TestPasswordStore> password_store =
+      static_cast<password_manager::TestPasswordStore*>(
+          PasswordStoreFactory::GetForProfile(
+              browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS)
+              .get());
+
+  // The call to |GetURL| is needed to get the correct port.
+  GURL psl_url = https_test_server.GetURL("psl.example.com", "/");
+
+  autofill::PasswordForm signin_form;
+  signin_form.signon_realm = psl_url.spec();
+  signin_form.password_value = base::ASCIIToUTF16("password");
+  signin_form.username_value = base::ASCIIToUTF16("user");
+  signin_form.origin = psl_url;
+  password_store->AddLogin(signin_form);
+
+  NavigateToURL(https_test_server, "www.example.com",
+                "/password/password_form.html");
+
+  // Call the API to trigger |get| and |store| and redirect.
+  ASSERT_TRUE(
+      content::ExecuteScript(RenderViewHost(),
+                             "navigator.credentials.get({password: true})"
+                             ".then(cred => "
+                             "navigator.credentials.store(cred)"
+                             ".then(cred => "
+                             "window.location = '/password/done.html'))"));
+
+  WaitForPasswordStore();
+  ASSERT_EQ(password_manager::ui::CREDENTIAL_REQUEST_STATE,
+            PasswordsModelDelegateFromWebContents(WebContents())->GetState());
+  PasswordsModelDelegateFromWebContents(WebContents())
+      ->ChooseCredential(
+          signin_form,
+          password_manager::CredentialType::CREDENTIAL_TYPE_PASSWORD);
+
+  NavigationObserver observer(WebContents());
+  observer.SetPathToWaitFor("/password/done.html");
+  observer.Wait();
+
+  // Wait for the password store before checking the prompt because it pops up
+  // after the store replies.
+  WaitForPasswordStore();
+  std::unique_ptr<BubbleObserver> prompt_observer(

[vabr (Chromium), 2016/12/22 13:08:24]

auto prompt_observer = base::MakeUnique<BubbleObserver>(WebContents());
avoids duplicating the type name.

[jdoerrie, 2016/12/22 16:20:01]

Acknowledged. As discussed offline I prefer MakeUnique as well, but in this case
I will use the raw new for consistency with other tests in this file. I will
follow up with a cleanup CL soon.

+      new BubbleObserver(WebContents()));
+  EXPECT_FALSE(prompt_observer->IsShowingSavePrompt());
+  EXPECT_FALSE(prompt_observer->IsShowingUpdatePrompt());
+
+  // There should be an entry for both psl.example.com and www.example.com.
+  password_manager::TestPasswordStore::PasswordMap passwords =
+      password_store->stored_passwords();
+  GURL www_url = https_test_server.GetURL("www.example.com", "/");
+  EXPECT_EQ(2U, passwords.size());
+  EXPECT_TRUE(base::ContainsKey(passwords, psl_url.spec()));
+  EXPECT_TRUE(base::ContainsKey(passwords, www_url.spec()));
+}
 
 IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
                        AutoSigninOldCredentialAndNavigation) {