[PwdCheckAndroid] Add sorting for compromised credentials

Puts phished credentials before leaked ones and orders them by recency
(from most recent to least recent) within each category.

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

chrome/browser/password_check/android/password_check_manager.cc

@@ -41,6 +41,29 @@ std::string CreateChangeUrl(const GURL& url) {
   return url.GetOrigin().spec();
 }
 
+// Orders |compromised_credentials| in such a way that phished credentials
+// precede leaked credentials, and that credentials of the same compromise type
+// are ordered by recency.
+// TODO(crbug.com/1117579): Share the common logic with desktop.
+void OrderCompromisedCredentials(
+    std::vector<password_manager::CredentialWithPassword>& credentials) {
+  // Reordering phished credential to the beginning.
+  auto last_phished = std::partition(
+      credentials.begin(), credentials.end(), [](const auto& credential) {
+        return credential.compromise_type !=
+               password_manager::CompromiseTypeFlags::kCredentialLeaked;
+      });
+
+  // By construction the phished credentials precede the leaked credentials in
+  // |credentials|. Now sort both groups by their creation date so that most recent
+  // compromises appear first in both lists.
+  auto create_time_cmp = [](const auto& lhs, const auto& rhs) {
+    return lhs.create_time > rhs.create_time;
+  };
+  std::sort(credentials.begin(), last_phished, create_time_cmp);
+  std::sort(last_phished, credentials.end(), create_time_cmp);
+}
+
 }  // namespace
 
 using autofill::PasswordForm;
@@ -119,6 +142,7 @@ std::vector<CompromisedCredentialForUI>
 PasswordCheckManager::GetCompromisedCredentials() const {
   std::vector<CredentialWithPassword> credentials =
       compromised_credentials_manager_.GetCompromisedCredentials();
+  OrderCompromisedCredentials(credentials);
   std::vector<CompromisedCredentialForUI> ui_credentials;
   ui_credentials.reserve(credentials.size());
   for (const auto& credential : credentials) {

chrome/browser/password_check/android/password_check_manager_unittest.cc

@@ -46,11 +46,13 @@ using testing::NiceMock;
 
 using CompromisedCredentialForUI =
     PasswordCheckManager::CompromisedCredentialForUI;
+using CompromiseTypeFlags = password_manager::CompromiseTypeFlags;
 using State = password_manager::BulkLeakCheckService::State;
 
 namespace {
 
 constexpr char kExampleCom[] = "https://example.com";
+constexpr char kExampleOrg[] = "http://www.example.org";
 constexpr char kExampleApp[] = "com.example.app";
 
 constexpr char kUsername1[] = "alice";
@@ -143,6 +145,7 @@ auto ExpectCompromisedCredentialForUI(
     const base::string16& display_origin,
     const base::Optional<std::string>& package_name,
     const base::Optional<std::string>& change_password_url,
+    CompromiseTypeFlags compromise_type,
     bool has_script) {
   auto package_name_field_matcher =
       package_name.has_value()
@@ -158,6 +161,7 @@ auto ExpectCompromisedCredentialForUI(
       Field(&CompromisedCredentialForUI::display_username, display_username),
       Field(&CompromisedCredentialForUI::display_origin, display_origin),
       package_name_field_matcher, change_password_url_field_matcher,
+      Field(&CompromisedCredentialForUI::compromise_type, compromise_type),
       Field(&CompromisedCredentialForUI::has_script, has_script));
 }
 
@@ -221,9 +225,7 @@ TEST_F(PasswordCheckManagerTest, OnCompromisedCredentialsChanged) {
   RunUntilIdle();
 
   EXPECT_CALL(mock_observer_, OnCompromisedCredentialsChanged(1));
-  store().AddCompromisedCredentials(
-      MakeCompromised(kExampleCom, kUsername1, base::TimeDelta::FromMinutes(1),
-                      CompromiseType::kLeaked));
+  store().AddCompromisedCredentials(MakeCompromised(kExampleCom, kUsername1));
   RunUntilIdle();
 }
 
@@ -236,7 +238,9 @@ TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForSiteCredential) {
       manager_->GetCompromisedCredentials(),
       ElementsAre(ExpectCompromisedCredentialForUI(
           base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
-          base::nullopt, "https://example.com/", /*has_script=*/false)));
+          base::nullopt, "https://example.com/",
+          CompromiseTypeFlags::kCredentialLeaked,
+          /*has_script=*/false)));
 }
 
 TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForAppCredentials) {
@@ -253,16 +257,18 @@ TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForAppCredentials) {
 
   RunUntilIdle();
 
-  EXPECT_THAT(manager_->GetCompromisedCredentials(),
-              ElementsAre(ExpectCompromisedCredentialForUI(
+  EXPECT_THAT(
+      manager_->GetCompromisedCredentials(),
+      UnorderedElementsAre(
+          ExpectCompromisedCredentialForUI(
               base::ASCIIToUTF16(kUsername1),
-                              base::ASCIIToUTF16("App (com.example.app)"),
-                              "com.example.app", base::nullopt,
+              base::ASCIIToUTF16("App (com.example.app)"), "com.example.app",
+              base::nullopt, CompromiseTypeFlags::kCredentialLeaked,
               /*has_script=*/false),
           ExpectCompromisedCredentialForUI(
-                              base::ASCIIToUTF16(kUsername2),
-                              base::ASCIIToUTF16("Example App"),
+              base::ASCIIToUTF16(kUsername2), base::ASCIIToUTF16("Example App"),
               "com.example.app", base::nullopt,
+              CompromiseTypeFlags::kCredentialLeaked,
               /*has_script=*/false)));
 }
 
@@ -291,3 +297,48 @@ TEST_F(PasswordCheckManagerTest, DoesntRecordTimestampOfUnsuccessfulCheck) {
   service()->set_state_and_notify(State::kSignedOut);
   EXPECT_EQ(0.0, manager_->GetLastCheckTimestamp().ToDoubleT());
 }
+
+TEST_F(PasswordCheckManagerTest, GetCompromisedCredentialsOrder) {
+  InitializeManager();
+  store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1));
+  store().AddLogin(MakeSavedPassword(kExampleCom, kUsername2));
+  store().AddLogin(MakeSavedPassword(kExampleOrg, kUsername1));
+  store().AddLogin(MakeSavedPassword(kExampleOrg, kUsername2));
+  store().AddCompromisedCredentials(
+      MakeCompromised(kExampleCom, kUsername1, base::TimeDelta::FromMinutes(1),
+                      CompromiseType::kLeaked));
+  store().AddCompromisedCredentials(
+      MakeCompromised(kExampleCom, kUsername2, base::TimeDelta::FromMinutes(5),
+                      CompromiseType::kLeaked));
+  store().AddCompromisedCredentials(
+      MakeCompromised(kExampleCom, kUsername2, base::TimeDelta::FromMinutes(3),
+                      CompromiseType::kPhished));
+  store().AddCompromisedCredentials(
+      MakeCompromised(kExampleOrg, kUsername2, base::TimeDelta::FromMinutes(4),
+                      CompromiseType::kLeaked));
+  store().AddCompromisedCredentials(
+      MakeCompromised(kExampleOrg, kUsername1, base::TimeDelta::FromMinutes(2),
+                      CompromiseType::kPhished));
+  RunUntilIdle();
+  EXPECT_THAT(
+      manager_->GetCompromisedCredentials(),
+      ElementsAre(
+          ExpectCompromisedCredentialForUI(
+              base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.org"),
+              base::nullopt, "http://www.example.org/",
+              CompromiseTypeFlags::kCredentialPhished, /*has_script_=*/false),
+          ExpectCompromisedCredentialForUI(
+              base::ASCIIToUTF16(kUsername2), base::ASCIIToUTF16("example.com"),
+              base::nullopt, "https://example.com/",
+              password_manager::CompromiseTypeFlags::kCredentialLeaked |
+                  password_manager::CompromiseTypeFlags::kCredentialPhished,
+              /*has_script_=*/false),
+          ExpectCompromisedCredentialForUI(
+              base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
+              base::nullopt, "https://example.com/",
+              CompromiseTypeFlags::kCredentialLeaked, /*has_script_=*/false),
+          ExpectCompromisedCredentialForUI(
+              base::ASCIIToUTF16(kUsername2), base::ASCIIToUTF16("example.org"),
+              base::nullopt, "http://www.example.org/",
+              CompromiseTypeFlags::kCredentialLeaked, /*has_script_=*/false)));
+}