[Passwords] MultiStoreFormFetcher sorts matches per date_last_used

Change-Id: Ib4b42e493f6799681798c39165424111977fb21a
Bug: 1002000
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800869
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697600}

components/password_manager/core/browser/fake_form_fetcher.cc

@@ -66,9 +66,10 @@ const PasswordForm* FakeFormFetcher::GetPreferredMatch() const {
 void FakeFormFetcher::SetNonFederated(
     const std::vector<const PasswordForm*>& non_federated) {
   non_federated_ = non_federated;
-  password_manager_util::FindBestMatches(non_federated_, scheme_,
-                                         &non_federated_same_scheme_,
-                                         &best_matches_, &preferred_match_);
+  password_manager_util::FindBestMatches(
+      non_federated_, scheme_,
+      /*sort_matches_by_date_last_used=*/false, &non_federated_same_scheme_,
+      &best_matches_, &preferred_match_);
 }
 
 void FakeFormFetcher::SetBlacklisted(

components/password_manager/core/browser/form_fetcher_impl.cc

@@ -219,8 +219,8 @@ std::unique_ptr<FormFetcher> FormFetcherImpl::Clone() {
   result->blacklisted_ = MakeCopies(blacklisted_);
   password_manager_util::FindBestMatches(
       MakeWeakCopies(result->non_federated_), form_digest_.scheme,
-      &result->non_federated_same_scheme_, &result->best_matches_,
-      &result->preferred_match_);
+      sort_matches_by_date_last_used_, &result->non_federated_same_scheme_,
+      &result->best_matches_, &result->preferred_match_);
 
   result->interactions_stats_ = this->interactions_stats_;
   result->state_ = this->state_;
@@ -237,7 +237,8 @@ void FormFetcherImpl::ProcessPasswordStoreResults(
 
   password_manager_util::FindBestMatches(
       MakeWeakCopies(non_federated_), form_digest_.scheme,
-      &non_federated_same_scheme_, &best_matches_, &preferred_match_);
+      sort_matches_by_date_last_used_, &non_federated_same_scheme_,
+      &best_matches_, &preferred_match_);
 
   for (auto* consumer : consumers_)
     consumer->OnFetchCompleted();

components/password_manager/core/browser/form_fetcher_impl.h

@@ -88,6 +88,9 @@ class FormFetcherImpl : public FormFetcher,
   // password store returning results in the meantime.
   bool need_to_refetch_ = false;
 
+  // If false, matches are sorted using the "preferred" field.
+  bool sort_matches_by_date_last_used_ = false;
+
   // Processes password form results and forwards them to the |consumers_|.
   void ProcessPasswordStoreResults(
       std::vector<std::unique_ptr<autofill::PasswordForm>> results);

components/password_manager/core/browser/multi_store_form_fetcher.cc

@@ -20,7 +20,9 @@ MultiStoreFormFetcher::MultiStoreFormFetcher(
     PasswordStore::FormDigest form_digest,
     const PasswordManagerClient* client,
     bool should_migrate_http_passwords)
-    : FormFetcherImpl(form_digest, client, should_migrate_http_passwords) {}
+    : FormFetcherImpl(form_digest, client, should_migrate_http_passwords) {
+  sort_matches_by_date_last_used_ = true;
+}
 
 MultiStoreFormFetcher::~MultiStoreFormFetcher() = default;
 
@@ -83,8 +85,6 @@ void MultiStoreFormFetcher::OnGetPasswordStoreResults(
 
   // TODO(crbug.com/1002000): implement password store migration.
 
-  // TODO(crbug.com/1002000): implement sorting based on "last_time_used"
-
   ProcessPasswordStoreResults(std::move(partial_results_));
 }
 

components/password_manager/core/browser/multi_store_form_fetcher_unittest.cc

@@ -64,26 +64,31 @@ class FakePasswordManagerClient : public StubPasswordManagerClient {
 
 PasswordForm CreateHTMLForm(const std::string& origin_url,
                             const std::string& username_value,
-                            const std::string& password_value) {
+                            const std::string& password_value,
+                            base::Time date_last_used) {
   PasswordForm form;
   form.scheme = PasswordForm::Scheme::kHtml;
   form.origin = GURL(origin_url);
   form.signon_realm = origin_url;
   form.username_value = ASCIIToUTF16(username_value);
   form.password_value = ASCIIToUTF16(password_value);
+  form.date_last_used = date_last_used;
   return form;
 }
 
 // Creates a dummy non-federated form with some basic arbitrary values.
-PasswordForm CreateNonFederated(const std::string& username_value) {
-  PasswordForm form = CreateHTMLForm(kTestHttpsURL, username_value, "password");
+PasswordForm CreateNonFederated(const std::string& username_value,
+                                base::Time date_last_used) {
+  PasswordForm form =
+      CreateHTMLForm(kTestHttpsURL, username_value, "password", date_last_used);
   form.action = GURL(kTestHttpsActionURL);
   return form;
 }
 
 // Creates a dummy federated form with some basic arbitrary values.
-PasswordForm CreateFederated(const std::string& username_value) {
-  PasswordForm form = CreateNonFederated(username_value);
+PasswordForm CreateFederated(const std::string& username_value,
+                             base::Time date_last_used) {
+  PasswordForm form = CreateNonFederated(username_value, date_last_used);
   form.signon_realm = kTestFederatedRealm;
   form.password_value.clear();
   form.federation_origin = url::Origin::Create(GURL(kTestFederationURL));
@@ -91,9 +96,11 @@ PasswordForm CreateFederated(const std::string& username_value) {
 }
 
 // Creates an Android federated credential.
-PasswordForm CreateAndroidFederated(const std::string& username_value) {
+PasswordForm CreateAndroidFederated(const std::string& username_value,
+                                    base::Time date_last_used) {
   PasswordForm form =
-      CreateHTMLForm("android://hash@com.example.android/", username_value, "");
+      CreateHTMLForm("android://hash@com.example.android/", username_value,
+                     /*password_value=*/"", date_last_used);
   form.federation_origin = url::Origin::Create(GURL(kTestFederationURL));
   form.is_affiliation_based_match = true;
   return form;
@@ -101,7 +108,9 @@ PasswordForm CreateAndroidFederated(const std::string& username_value) {
 
 // Creates a dummy blacklisted form.
 PasswordForm CreateBlacklisted() {
-  PasswordForm form = CreateHTMLForm(kTestHttpsURL, "", "");
+  PasswordForm form = CreateHTMLForm(kTestHttpsURL, /*username_value=*/"",
+                                     /*password_value=*/"",
+                                     /*date_last_used=*/base::Time::Now());
   form.blacklisted_by_user = true;
   return form;
 }
@@ -190,13 +199,17 @@ TEST_F(MultiStoreFormFetcherTest, Empty) {
 
 // Check that results from both stores are merged.
 TEST_F(MultiStoreFormFetcherTest, MergeFromBothStores) {
+  const base::Time kLastUsedNow = base::Time::Now();
+  const base::Time kLastUsedYesterday =
+      kLastUsedNow - base::TimeDelta::FromDays(1);
   Fetch();
-  PasswordForm federated1 = CreateFederated("user");
-  PasswordForm federated2 = CreateFederated("user_B");
-  PasswordForm federated3 = CreateAndroidFederated("user_B");
-  PasswordForm non_federated1 = CreateNonFederated("user");
-  PasswordForm non_federated2 = CreateNonFederated("user_C");
-  PasswordForm non_federated3 = CreateNonFederated("user_D");
+  PasswordForm federated1 = CreateFederated("user", kLastUsedNow);
+  PasswordForm federated2 = CreateFederated("user_B", kLastUsedNow);
+  PasswordForm federated3 = CreateAndroidFederated("user_B", kLastUsedNow);
+  PasswordForm non_federated1 = CreateNonFederated("user", kLastUsedYesterday);
+  PasswordForm non_federated2 =
+      CreateNonFederated("user_C", kLastUsedYesterday);
+  PasswordForm non_federated3 = CreateNonFederated("user_D", kLastUsedNow);
   PasswordForm blacklisted = CreateBlacklisted();
 
   form_fetcher_->AddConsumer(&consumer_);
@@ -233,6 +246,7 @@ TEST_F(MultiStoreFormFetcherTest, MergeFromBothStores) {
                                    Pointee(federated3)));
   EXPECT_THAT(form_fetcher_->GetBlacklistedMatches(),
               UnorderedElementsAre(Pointee(blacklisted)));
+  EXPECT_THAT(form_fetcher_->GetPreferredMatch(), Pointee(non_federated3));
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/password_manager_util.cc

@@ -49,6 +49,17 @@ bool IsBetterMatch(const PasswordForm* lhs, const PasswordForm* rhs) {
          std::make_pair(!rhs->is_public_suffix_match, rhs->preferred);
 }
 
+// Return true if 1.|lhs| is non-PSL match, |rhs| is PSL match or 2.|lhs| and
+// |rhs| have the same value of |is_public_suffix_match|, and |lhs| is more
+// recently used than |rhs|.
+// TODO(crbug.com/1002000): Rename to IsBetterMatch when migration to
+// date_last_used is done.
+bool IsBetterMatchUsingLastUsed(const PasswordForm* lhs,
+                                const PasswordForm* rhs) {
+  return std::make_pair(!lhs->is_public_suffix_match, lhs->date_last_used) >
+         std::make_pair(!rhs->is_public_suffix_match, rhs->date_last_used);
+}
+
 }  // namespace
 
 // Update |credential| to reflect usage.
@@ -237,6 +248,7 @@ base::StringPiece GetSignonRealmWithProtocolExcluded(const PasswordForm& form) {
 void FindBestMatches(
     const std::vector<const PasswordForm*>& non_federated_matches,
     PasswordForm::Scheme scheme,
+    bool sort_matches_by_date_last_used,
     std::vector<const autofill::PasswordForm*>* non_federated_same_scheme,
     std::map<base::string16, const PasswordForm*>* best_matches,
     const PasswordForm** preferred_match) {
@@ -259,9 +271,12 @@ void FindBestMatches(
   if (non_federated_same_scheme->empty())
     return;
 
-  // Sort matches using IsBetterMatch predicate.
+  // Sort matches using IsBetterMatchUsingLastUsed or IsBetterMatch predicate.
   std::sort(non_federated_same_scheme->begin(),
-            non_federated_same_scheme->end(), IsBetterMatch);
+            non_federated_same_scheme->end(),
+            sort_matches_by_date_last_used ? IsBetterMatchUsingLastUsed
+                                           : IsBetterMatch);
+
   for (const auto* match : *non_federated_same_scheme) {
     const base::string16& username = match->username_value;
     // The first match for |username| in the sorted array is best match.

components/password_manager/core/browser/password_manager_util.h

@@ -122,6 +122,7 @@ base::StringPiece GetSignonRealmWithProtocolExcluded(
 void FindBestMatches(
     const std::vector<const autofill::PasswordForm*>& non_federated_matches,
     autofill::PasswordForm::Scheme scheme,
+    bool sort_matches_by_date_last_used,
     std::vector<const autofill::PasswordForm*>* non_federated_same_scheme,
     std::map<base::string16, const autofill::PasswordForm*>* best_matches,
     const autofill::PasswordForm** preferred_match);

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -201,8 +201,164 @@ TEST(PasswordManagerUtil, FindBestMatches) {
     const PasswordForm* preferred_match = nullptr;
 
     std::vector<const PasswordForm*> same_scheme_matches;
-    FindBestMatches(matches, PasswordForm::Scheme::kHtml, &same_scheme_matches,
-                    &best_matches, &preferred_match);
+    FindBestMatches(matches, PasswordForm::Scheme::kHtml,
+                    /*sort_matches_by_date_last_used=*/false,
+                    &same_scheme_matches, &best_matches, &preferred_match);
+
+    if (test_case.expected_preferred_match_index == kNotFound) {
+      // Case of empty |matches|.
+      EXPECT_FALSE(preferred_match);
+      EXPECT_TRUE(best_matches.empty());
+    } else {
+      // Check |preferred_match|.
+      EXPECT_EQ(matches[test_case.expected_preferred_match_index],
+                preferred_match);
+      // Check best matches.
+      ASSERT_EQ(test_case.expected_best_matches_indices.size(),
+                best_matches.size());
+
+      for (const auto& username_match : best_matches) {
+        std::string username = base::UTF16ToUTF8(username_match.first);
+        ASSERT_NE(test_case.expected_best_matches_indices.end(),
+                  test_case.expected_best_matches_indices.find(username));
+        size_t expected_index =
+            test_case.expected_best_matches_indices.at(username);
+        size_t actual_index = std::distance(
+            matches.begin(),
+            std::find(matches.begin(), matches.end(), username_match.second));
+        EXPECT_EQ(expected_index, actual_index);
+      }
+    }
+  }
+}
+
+TEST(PasswordManagerUtil, FindBestMatchesByUsageTime) {
+  const base::Time kNow = base::Time::Now();
+  const base::Time kYesterday = kNow - base::TimeDelta::FromDays(1);
+  const base::Time k2DaysAgo = kNow - base::TimeDelta::FromDays(2);
+  const int kNotFound = -1;
+  struct TestMatch {
+    bool is_psl_match;
+    bool preferred;
+    base::Time date_last_used;
+    std::string username;
+  };
+  struct TestCase {
+    const char* description;
+    std::vector<TestMatch> matches;
+    int expected_preferred_match_index;
+    std::map<std::string, size_t> expected_best_matches_indices;
+  } test_cases[] = {
+      {"Empty matches", {}, kNotFound, {}},
+      {"1 preferred non-psl match",
+       {{.is_psl_match = false,
+         .preferred = true,
+         .date_last_used = kNow,
+         .username = "u"}},
+       0,
+       {{"u", 0}}},
+      {"1 non-preferred psl match",
+       {{.is_psl_match = true,
+         .preferred = false,
+         .date_last_used = kNow,
+         .username = "u"}},
+       0,
+       {{"u", 0}}},
+      {"2 matches with the same username",
+       {{.is_psl_match = false,
+         .preferred = false,
+         .date_last_used = kNow,
+         .username = "u"},
+        {.is_psl_match = false,
+         .preferred = true,
+         .date_last_used = kYesterday,
+         .username = "u"}},
+       0,
+       {{"u", 0}}},
+      {"2 matches with different usernames, most recently used taken",
+       {{.is_psl_match = false,
+         .preferred = false,
+         .date_last_used = kNow,
+         .username = "u1"},
+        {.is_psl_match = false,
+         .preferred = true,
+         .date_last_used = kYesterday,
+         .username = "u2"}},
+       0,
+       {{"u1", 0}, {"u2", 1}}},
+      {"2 matches with different usernames, non-psl much taken",
+       {{.is_psl_match = false,
+         .preferred = false,
+         .date_last_used = kYesterday,
+         .username = "u1"},
+        {.is_psl_match = true,
+         .preferred = true,
+         .date_last_used = kNow,
+         .username = "u2"}},
+       0,
+       {{"u1", 0}, {"u2", 1}}},
+      {"8 matches, 3 usernames",
+       {{.is_psl_match = false,
+         .preferred = false,
+         .date_last_used = kYesterday,
+         .username = "u2"},
+        {.is_psl_match = true,
+         .preferred = false,
+         .date_last_used = kYesterday,
+         .username = "u3"},
+        {.is_psl_match = true,
+         .preferred = false,
+         .date_last_used = kYesterday,
+         .username = "u1"},
+        {.is_psl_match = false,
+         .preferred = true,
+         .date_last_used = k2DaysAgo,
+         .username = "u3"},
+        {.is_psl_match = true,
+         .preferred = false,
+         .date_last_used = kNow,
+         .username = "u1"},
+        {.is_psl_match = false,
+         .preferred = false,
+         .date_last_used = kNow,
+         .username = "u2"},
+        {.is_psl_match = true,
+         .preferred = true,
+         .date_last_used = kYesterday,
+         .username = "u3"},
+        {.is_psl_match = false,
+         .preferred = false,
+         .date_last_used = k2DaysAgo,
+         .username = "u1"}},
+       5,
+       {{"u1", 7}, {"u2", 5}, {"u3", 3}}},
+
+  };
+
+  for (const TestCase& test_case : test_cases) {
+    SCOPED_TRACE(testing::Message("Test description: ")
+                 << test_case.description);
+    // Convert TestMatch to PasswordForm.
+    std::vector<PasswordForm> owning_matches;
+    for (const TestMatch& match : test_case.matches) {
+      PasswordForm form;
+      form.is_public_suffix_match = match.is_psl_match;
+      form.preferred = match.preferred;
+      form.date_last_used = match.date_last_used;
+      form.username_value = base::ASCIIToUTF16(match.username);
+      owning_matches.push_back(form);
+    }
+    std::vector<const PasswordForm*> matches;
+    for (const PasswordForm& match : owning_matches)
+      matches.push_back(&match);
+
+    std::map<base::string16, const PasswordForm*> best_matches;
+    const PasswordForm* preferred_match = nullptr;
+
+    std::vector<const PasswordForm*> same_scheme_matches;
+    FindBestMatches(matches, PasswordForm::Scheme::kHtml,
+                    /*sort_matches_by_date_last_used=*/true,
+                    &same_scheme_matches, &best_matches, &preferred_match);
 
     if (test_case.expected_preferred_match_index == kNotFound) {
       // Case of empty |matches|.