Show PSL-matched credentials in the account chooser.

components/password_manager/content/browser/credential_manager_impl_unittest.cc

 // Copyright 2014 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/content/browser/credential_manager_impl.h"
 
 #include <stdint.h>
 
 #include <string>
 #include <tuple>
@@ skipping 209 matching lines @@
         .WillByDefault(testing::Return(true));
     ON_CALL(*client_, IsOffTheRecord()).WillByDefault(testing::Return(false));
 
     NavigateAndCommit(GURL("https://example.com/test.html"));
 
     form_.username_value = base::ASCIIToUTF16("Username");
     form_.display_name = base::ASCIIToUTF16("Display Name");
     form_.icon_url = GURL("https://example.com/icon.png");
     form_.password_value = base::ASCIIToUTF16("Password");
     form_.origin = web_contents()->GetLastCommittedURL().GetOrigin();
-    form_.signon_realm = form_.origin.spec();
+    form_.signon_realm = form_.origin.GetOrigin().spec();
     form_.scheme = autofill::PasswordForm::SCHEME_HTML;
     form_.skip_zero_click = false;
 
     affiliated_form1_.username_value = base::ASCIIToUTF16("Affiliated 1");
     affiliated_form1_.display_name = base::ASCIIToUTF16("Display Name");
     affiliated_form1_.password_value = base::ASCIIToUTF16("Password");
     affiliated_form1_.origin = GURL();
     affiliated_form1_.signon_realm = kTestAndroidRealm1;
     affiliated_form1_.scheme = autofill::PasswordForm::SCHEME_HTML;
     affiliated_form1_.skip_zero_click = false;
 
     affiliated_form2_.username_value = base::ASCIIToUTF16("Affiliated 2");
     affiliated_form2_.display_name = base::ASCIIToUTF16("Display Name");
     affiliated_form2_.password_value = base::ASCIIToUTF16("Password");
     affiliated_form2_.origin = GURL();
     affiliated_form2_.signon_realm = kTestAndroidRealm2;
     affiliated_form2_.scheme = autofill::PasswordForm::SCHEME_HTML;
     affiliated_form2_.skip_zero_click = false;
 
     origin_path_form_.username_value = base::ASCIIToUTF16("Username 2");
     origin_path_form_.display_name = base::ASCIIToUTF16("Display Name 2");
     origin_path_form_.password_value = base::ASCIIToUTF16("Password 2");
     origin_path_form_.origin = GURL("https://example.com/path");
-    origin_path_form_.signon_realm = origin_path_form_.origin.spec();
+    origin_path_form_.signon_realm =
+        origin_path_form_.origin.GetOrigin().spec();
     origin_path_form_.scheme = autofill::PasswordForm::SCHEME_HTML;
     origin_path_form_.skip_zero_click = false;
 
     subdomain_form_.username_value = base::ASCIIToUTF16("Username 2");
     subdomain_form_.display_name = base::ASCIIToUTF16("Display Name 2");
     subdomain_form_.password_value = base::ASCIIToUTF16("Password 2");
     subdomain_form_.origin = GURL("https://subdomain.example.com/path");
-    subdomain_form_.signon_realm = subdomain_form_.origin.spec();
+    subdomain_form_.signon_realm = subdomain_form_.origin.GetOrigin().spec();
     subdomain_form_.scheme = autofill::PasswordForm::SCHEME_HTML;
     subdomain_form_.skip_zero_click = false;
 
     cross_origin_form_.username_value = base::ASCIIToUTF16("Username");
     cross_origin_form_.display_name = base::ASCIIToUTF16("Display Name");
     cross_origin_form_.password_value = base::ASCIIToUTF16("Password");
     cross_origin_form_.origin = GURL("https://example.net/");
-    cross_origin_form_.signon_realm = cross_origin_form_.origin.spec();
+    cross_origin_form_.signon_realm =
+        cross_origin_form_.origin.GetOrigin().spec();
     cross_origin_form_.scheme = autofill::PasswordForm::SCHEME_HTML;
     cross_origin_form_.skip_zero_click = false;
 
     store_->Clear();
     EXPECT_TRUE(store_->IsEmpty());
   }
 
   void TearDown() override {
     cm_service_impl_.reset();
 
@@ skipping 447 matching lines @@
   EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _))
       .Times(testing::Exactly(0));
   EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0));
 
   std::vector<GURL> federations;
   ExpectCredentialType(false, true, federations,
                        CredentialType::CREDENTIAL_TYPE_EMPTY);
 }
 
 TEST_F(CredentialManagerImplTest,
+       CredentialManagerOnRequestCredentialWithPSLCredential) {
+  subdomain_form_.skip_zero_click = false;

[vabr (Chromium), 2016/11/23 18:49:11]

Is the skip_zero_click bit being false a part of the contract? Or is the get()
call expected to ignore that value and pretend it is false?

In the first case, we should maybe point the contract out in a comment.

In the second case, we may need to do two tests, for both values of the flag, to
verify that the get() call handles them correctly.

[vasilii, 2016/11/24 12:44:10]

It's the second. The test should pass with any value of the flag.
skip_zero_click == false makes it more likely to fail. I can't think of an
implementation where the test passes with skip_zero_click == false and fails
with skip_zero_click == true.
I added a test to check that PSL credentials are not returned for zero-click
only get().

[vabr (Chromium), 2016/11/24 13:58:14]

Acknowledged.

+  store_->AddLogin(subdomain_form_);
+  EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _));
+  EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(0);
+
+  ExpectCredentialType(false, true, std::vector<GURL>(),
+                       CredentialType::CREDENTIAL_TYPE_PASSWORD);
+}
+
+TEST_F(CredentialManagerImplTest,
        CredentialManagerOnRequestCredentialWithEmptyAndNonemptyUsernames) {
   store_->AddLogin(form_);
   autofill::PasswordForm empty = form_;
   empty.username_value.clear();
   store_->AddLogin(empty);
   autofill::PasswordForm duplicate = form_;
   duplicate.username_element = base::ASCIIToUTF16("different_username_element");
   store_->AddLogin(duplicate);
+  store_->AddLogin(subdomain_form_);
 
   std::vector<GURL> federations;
   ExpectZeroClickSignInSuccess(false, true, federations,
                                CredentialType::CREDENTIAL_TYPE_PASSWORD);
 }
 
 TEST_F(CredentialManagerImplTest,
        CredentialManagerOnRequestCredentialWithDuplicates) {
   // Add 8 credentials. Two buckets of duplicates, one empty username and one

[vabr (Chromium), 2016/11/23 18:49:11]

I'm wondering if this particular test is trying to test too much at once. It has
used PSL matches, but did not even mention them in the name or the comment here.

Would it be clearer to have a separate test that would verify that in a mix of
PSL and non-PSL matches only the latter is shown, without also testing
duplicates etc.?

[vasilii, 2016/11/24 12:44:10]

Done. See CredentialManagerOnRequestCredentialWithPSLAndNormalCredentials

   // federated one. There should be just 3 in the account chooser.
   form_.preferred = true;
   form_.username_element = base::ASCIIToUTF16("username_element");
   store_->AddLogin(form_);
   autofill::PasswordForm empty = form_;
   empty.username_value.clear();
   store_->AddLogin(empty);
-  autofill::PasswordForm duplicate = form_;
-  duplicate.username_element = base::ASCIIToUTF16("username_element1");
-  duplicate.is_public_suffix_match = true;
+  autofill::PasswordForm duplicate = subdomain_form_;
+  duplicate.username_value = form_.username_value;
   store_->AddLogin(duplicate);
   duplicate = form_;
   duplicate.username_element = base::ASCIIToUTF16("username_element2");
   duplicate.preferred = false;
   store_->AddLogin(duplicate);
 
   origin_path_form_.preferred = true;
   store_->AddLogin(origin_path_form_);
-  duplicate = origin_path_form_;
+  duplicate = subdomain_form_;
   duplicate.username_element = base::ASCIIToUTF16("username_element3");
-  duplicate.is_public_suffix_match = true;
+  duplicate.username_value = subdomain_form_.username_value;
   store_->AddLogin(duplicate);
   duplicate = origin_path_form_;
   duplicate.username_element = base::ASCIIToUTF16("username_element4");
   duplicate.preferred = false;
   store_->AddLogin(duplicate);
   autofill::PasswordForm federated = origin_path_form_;
   federated.password_value.clear();
   federated.federation_origin = url::Origin(GURL("https://google.com/"));
   federated.signon_realm =
       "federation://" + federated.origin.host() + "/google.com";
@@ skipping 55 matching lines @@
 }
 
 TEST_F(
     CredentialManagerImplTest,
     CredentialManagerOnRequestCredentialWithZeroClickOnlyEmptyPasswordStore) {
   std::vector<GURL> federations;
   EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _))
       .Times(testing::Exactly(0));
   EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0));
 
-  bool called = false;
-  mojom::CredentialManagerError error;
-  base::Optional<CredentialInfo> credential;
-  CallGet(true, true, federations,
-          base::Bind(&GetCredentialCallback, &called, &error, &credential));
-
-  RunAllPendingTasks();
-
-  EXPECT_TRUE(called);
-  EXPECT_EQ(mojom::CredentialManagerError::SUCCESS, error);
+  ExpectZeroClickSignInFailure(true, true, federations);
 }
 
 TEST_F(CredentialManagerImplTest,
        CredentialManagerOnRequestCredentialWithZeroClickOnlyFullPasswordStore) {
   store_->AddLogin(form_);
   client_->set_first_run_seen(true);
 
   std::vector<GURL> federations;
 
   EXPECT_CALL(*client_, NotifyUserCouldBeAutoSignedInPtr(_)).Times(0);
@@ skipping 552 matching lines @@
                             _, CredentialSourceType::CREDENTIAL_SOURCE_API));
   CallStore(info, base::Bind(&RespondCallback, &called));
   // Allow the PasswordFormManager to talk to the password store
   RunAllPendingTasks();
 
   ASSERT_TRUE(client_->pending_manager());
   EXPECT_TRUE(client_->pending_manager()->IsBlacklisted());
 }
 
 }  // namespace password_manager