[PwdCheckAndroid] Add credentials sorting in java

When a credential from the list changes (is added in a new check or
removed) the entire list has to change. However, in order to keep the UI
stable, the list should maintain its order, which has to be as follows:

1. Phished credentials appear before leaked credentials.
2. Compromised credentials found in previous checks should appear
in descending order of creation time (most recent first).
3. Compromised credentials found in the current check (and all checks
ran while the list UI is open) have to be appended to the list.
This means that they need to be ordered in ascending order of creation
time.

Bug: 1102025, 1092444
Change-Id: I9e262791227454065fd556c0974b12b43fc9843a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362609
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Friedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800156}

chrome/browser/password_check/android/internal/java/src/org/chromium/chrome/browser/password_check/PasswordCheckBridge.java

@@ -57,14 +57,14 @@ class PasswordCheckBridge {
     // TODO(crbug.com/1102025): Add call from native.
     void onCompromisedCredentialFound(String signonRealm, GURL origin, String username,
             String displayOrigin, String displayUsername, String password, String passwordChangeUrl,
-            String associatedApp, boolean hasScript) {
+            String associatedApp, long creationTime, boolean hasScript) {
         assert signonRealm != null;
         assert displayOrigin != null;
         assert username != null;
         assert password != null;
         mPasswordCheckObserver.onCompromisedCredentialFound(new CompromisedCredential(signonRealm,
                 origin, username, displayOrigin, displayUsername, password, passwordChangeUrl,
-                associatedApp, true, false, hasScript));
+                associatedApp, creationTime, true, false, hasScript));
     }
 
     @CalledByNative
@@ -86,10 +86,10 @@ class PasswordCheckBridge {
     private static void insertCredential(CompromisedCredential[] credentials, int index,
             String signonRealm, GURL origin, String username, String displayOrigin,
             String displayUsername, String password, String passwordChangeUrl, String associatedApp,
-            boolean leaked, boolean phished, boolean hasScript) {
+            long creationTime, boolean leaked, boolean phished, boolean hasScript) {
         credentials[index] = new CompromisedCredential(signonRealm, origin, username, displayOrigin,
-                displayUsername, password, passwordChangeUrl, associatedApp, leaked, phished,
-                hasScript);
+                displayUsername, password, passwordChangeUrl, associatedApp, creationTime, leaked,
+                phished, hasScript);
     }
 
     /**

chrome/browser/password_check/android/internal/java/src/org/chromium/chrome/browser/password_check/PasswordCheckMediator.java

@@ -32,6 +32,11 @@ import org.chromium.ui.modelutil.ListModel;
 import org.chromium.ui.modelutil.MVCListAdapter.ListItem;
 import org.chromium.ui.modelutil.PropertyModel;
 
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+
 /**
  * Contains the logic for the PasswordCheck component. It sets the state of the model and reacts to
  * events like clicks.
@@ -43,6 +48,7 @@ class PasswordCheckMediator
     private PropertyModel mModel;
     private PasswordCheckComponentUi.Delegate mDelegate;
     private Runnable mLaunchCheckupInAccount;
+    private HashSet<CompromisedCredential> mPreCheckSet;
 
     PasswordCheckMediator(PasswordCheckChangePasswordHelper changePasswordDelegate,
             PasswordCheckReauthenticationHelper reauthenticationHelper) {
@@ -76,6 +82,10 @@ class PasswordCheckMediator
         }
         CompromisedCredential[] credentials = getPasswordCheck().getCompromisedCredentials();
         assert credentials != null;
+
+        List<CompromisedCredential> credentialsList = Arrays.asList(credentials);
+        sortCredentials(credentialsList);
+
         ListModel<ListItem> items = mModel.get(ITEMS);
         if (items.size() == 0) {
             items.add(new ListItem(PasswordCheckProperties.ItemType.HEADER,
@@ -87,7 +97,7 @@ class PasswordCheckMediator
         }
         if (items.size() > 1) items.removeRange(1, items.size() - 1);
 
-        for (CompromisedCredential credential : credentials) {
+        for (CompromisedCredential credential : credentialsList) {
             items.add(createEntryForCredential(credential));
         }
     }
@@ -240,4 +250,43 @@ class PasswordCheckMediator
                         .with(CREDENTIAL_HANDLER, this)
                         .build());
     }
+
+    private void sortCredentials(List<CompromisedCredential> credentials) {
+        if (mPreCheckSet == null) {
+            mPreCheckSet = new HashSet<>(credentials);
+        }
+
+        Collections.sort(credentials, (CompromisedCredential lhs, CompromisedCredential rhs) -> {
+            // Phished credentials should always appear first.
+            if (lhs.isPhished() != rhs.isPhished()) {
+                return lhs.isPhished() ? -1 : 1;
+            }
+
+            boolean lhsInitial = mPreCheckSet.contains(lhs);
+            boolean rhsInitial = mPreCheckSet.contains(rhs);
+            // If one is the in initial set and the other one isn't, then the credential in
+            // the initial set goes first.
+            if (lhsInitial != rhsInitial) {
+                return lhsInitial ? -1 : 1;
+            }
+
+            // If they are both in the initial set, the most recent credential should appear first.
+            if (lhsInitial && rhsInitial && lhs.getCreationTime() != rhs.getCreationTime()) {
+                return -Long.compare(lhs.getCreationTime(), rhs.getCreationTime());
+            }
+
+            // If they both are not in the initial set, the older credential should appear
+            // first.
+            if (!lhsInitial && !rhsInitial && lhs.getCreationTime() != rhs.getCreationTime()) {
+                return Long.compare(lhs.getCreationTime(), rhs.getCreationTime());
+            }
+
+            // In case of creation time equality, order alphabetically (first by origin,
+            // then by username), so that the list remains stable.
+            int originComparisonResult = lhs.getDisplayOrigin().compareTo(rhs.getDisplayOrigin());
+            int usernameComparisonResult =
+                    lhs.getDisplayUsername().compareTo(rhs.getDisplayUsername());
+            return originComparisonResult == 0 ? usernameComparisonResult : originComparisonResult;
+        });
+    }
 }

chrome/browser/password_check/android/java/src/org/chromium/chrome/browser/password_check/CompromisedCredential.java

@@ -30,6 +30,7 @@ public class CompromisedCredential implements Parcelable {
                     final String password = in.readString();
                     final String passwordChangeUrl = in.readString();
                     final String associatedApp = in.readString();
+                    final long creationTime = in.readLong();
                     boolean[] boolArguments = new boolean[3];
                     in.readBooleanArray(boolArguments);
                     final boolean leaked = boolArguments[0];
@@ -37,8 +38,8 @@ public class CompromisedCredential implements Parcelable {
                     final boolean hasScript = boolArguments[2];
 
                     return new CompromisedCredential(signonRealm, origin, username, displayOrigin,
-                            displayUsername, password, passwordChangeUrl, associatedApp, leaked,
-                            phished, hasScript);
+                            displayUsername, password, passwordChangeUrl, associatedApp,
+                            creationTime, leaked, phished, hasScript);
                 }
 
                 @Override
@@ -55,6 +56,7 @@ public class CompromisedCredential implements Parcelable {
     private final String mPassword;
     private final String mPasswordChangeUrl;
     private final String mAssociatedApp;
+    private final long mCreationTime;
     private final boolean mLeaked;
     private final boolean mPhished;
     private final boolean mHasScript;
@@ -69,12 +71,16 @@ public class CompromisedCredential implements Parcelable {
      * @param password The compromised password.
      * @param passwordChangeUrl A URL that links to the password change form of the affected site.
      * @param associatedApp The associated app if the password originates from it.
+     * @param creationTime The time at which the compromised credential was created, which is the
+     *        time at which the compromised credential was first found to be compromised during
+     *        a check.
      * @param phished True iff the credential was entered on an unsafe site.
      * @param hasScript True iff the credential can be automatically fixed.
      */
     public CompromisedCredential(String signonRealm, GURL origin, String username,
             String displayOrigin, String displayUsername, String password, String passwordChangeUrl,
-            String associatedApp, boolean leaked, boolean phished, boolean hasScript) {
+            String associatedApp, long creationTime, boolean leaked, boolean phished,
+            boolean hasScript) {
         assert origin != null : "Credential origin is null! Pass an empty one instead.";
         assert signonRealm != null;
         assert passwordChangeUrl != null : "Change URL may be empty but not null!";
@@ -91,6 +97,7 @@ public class CompromisedCredential implements Parcelable {
         mPassword = password;
         mPasswordChangeUrl = passwordChangeUrl;
         mAssociatedApp = associatedApp;
+        mCreationTime = creationTime;
         mLeaked = leaked;
         mPhished = phished;
         mHasScript = hasScript;
@@ -121,6 +128,9 @@ public class CompromisedCredential implements Parcelable {
     public String getAssociatedApp() {
         return mAssociatedApp;
     }
+    public long getCreationTime() {
+        return mCreationTime;
+    }
     public String getPasswordChangeUrl() {
         return mPasswordChangeUrl;
     }
@@ -144,8 +154,9 @@ public class CompromisedCredential implements Parcelable {
                 && mDisplayUsername.equals(that.mDisplayUsername)
                 && mPassword.equals(that.mPassword)
                 && mPasswordChangeUrl.equals(that.mPasswordChangeUrl)
-                && mAssociatedApp.equals(that.mAssociatedApp) && mLeaked == that.mLeaked
-                && mPhished == that.mPhished && mHasScript == that.mHasScript;
+                && mAssociatedApp.equals(that.mAssociatedApp) && mCreationTime == that.mCreationTime
+                && mLeaked == that.mLeaked && mPhished == that.mPhished
+                && mHasScript == that.mHasScript;
     }
 
     @Override
@@ -155,15 +166,15 @@ public class CompromisedCredential implements Parcelable {
                 + ", username='" + mUsername + '\'' + ", displayOrigin='" + mDisplayOrigin + '\''
                 + ", displayUsername='" + mDisplayUsername + '\'' + ", password='" + mPassword
                 + '\'' + ", passwordChangeUrl='" + mPasswordChangeUrl + '\'' + ", associatedApp='"
-                + mAssociatedApp + '\'' + ", leaked=" + mLeaked + ", phished=" + mPhished
-                + ", hasScript=" + mHasScript + '}';
+                + mAssociatedApp + '\'' + ", creationTime=" + mCreationTime + ", leaked=" + mLeaked
+                + ", phished=" + mPhished + ", hasScript=" + mHasScript + '}';
     }
 
     @Override
     public int hashCode() {
         return Objects.hash(mSignonRealm, mOrigin.getPossiblyInvalidSpec(), mUsername,
                 mDisplayOrigin, mDisplayUsername, mPassword, mPasswordChangeUrl, mAssociatedApp,
-                mLeaked, mPhished, mHasScript);
+                mCreationTime, mLeaked, mPhished, mHasScript);
     }
 
     @Override
@@ -176,6 +187,7 @@ public class CompromisedCredential implements Parcelable {
         parcel.writeString(mPassword);
         parcel.writeString(mPasswordChangeUrl);
         parcel.writeString(mAssociatedApp);
+        parcel.writeLong(mCreationTime);
         parcel.writeBooleanArray(new boolean[] {mLeaked, mPhished, mHasScript});
     }
 

chrome/browser/password_check/android/javatests/src/org/chromium/chrome/browser/password_check/PasswordCheckEditViewTest.java

@@ -68,7 +68,7 @@ public class PasswordCheckEditViewTest {
     private static final CompromisedCredential ANA =
             new CompromisedCredential("https://some-url.com/signin",
                     new GURL("https://some-url.com/"), "Ana", "some-url.com", "Ana", "password",
-                    "https://some-url.com/.well-known/change-password", "", true, false, false);
+                    "https://some-url.com/.well-known/change-password", "", 1, true, false, false);
 
     private PasswordCheckEditFragmentView mPasswordCheckEditView;
 

chrome/browser/password_check/android/javatests/src/org/chromium/chrome/browser/password_check/PasswordCheckViewTest.java

@@ -98,23 +98,23 @@ public class PasswordCheckViewTest {
     private static final CompromisedCredential ANA =
             new CompromisedCredential("https://some-url.com/signin",
                     new GURL("https://some-url.com/"), "Ana", "some-url.com", "Ana", "password",
-                    "https://some-url.com/.well-known/change-password", "", true, false, false);
+                    "https://some-url.com/.well-known/change-password", "", 1, true, false, false);
     private static final CompromisedCredential PHISHED =
             new CompromisedCredential("http://example.com/signin", new GURL("http://example.com/"),
                     "", "http://example.com", "(No username)", "DoSomething",
-                    "http://example.com/.well-known/change-password", "", false, true, false);
+                    "http://example.com/.well-known/change-password", "", 1, false, true, false);
     private static final CompromisedCredential LEAKED =
             new CompromisedCredential("https://some-other-url.com/signin",
                     new GURL("https://some-other-url.com/"), "AZiegler", "some-other-url.com",
-                    "AZiegler", "N0M3rcy", "", "com.other.package", true, false, false);
+                    "AZiegler", "N0M3rcy", "", "com.other.package", 1, true, false, false);
     private static final CompromisedCredential LEAKED_AND_PHISHED =
             new CompromisedCredential("https://super-important.com/signin",
                     new GURL("https://super-important.com/"), "HSong", "super-important.com",
-                    "HSong", "N3rfTh1s", "", "com.important.super", true, true, false);
+                    "HSong", "N3rfTh1s", "", "com.important.super", 1, true, true, false);
     private static final CompromisedCredential SCRIPTED =
             new CompromisedCredential("https://script.com/signin", new GURL("https://script.com/"),
                     "Charlie", "script.com", "Charlie", "secret",
-                    "https://script.com/.well-known/change-password", "", true, false, true);
+                    "https://script.com/.well-known/change-password", "", 1, true, false, true);
 
     private static final int LEAKS_COUNT = 2;
 

chrome/browser/password_check/android/junit/src/org/chromium/chrome/browser/password_check/PasswordCheckControllerTest.java

@@ -72,10 +72,10 @@ import org.chromium.url.GURL;
 public class PasswordCheckControllerTest {
     private static final CompromisedCredential ANA =
             new CompromisedCredential("https://m.a.xyz/signin", mock(GURL.class), "Ana", "m.a.xyz",
-                    "Ana", "password", "", "xyz.a.some.package", true, false, false);
+                    "Ana", "password", "", "xyz.a.some.package", 2, true, false, false);
     private static final CompromisedCredential BOB = new CompromisedCredential(
             "http://www.b.ch/signin", mock(GURL.class), "", "http://www.b.ch", "(No username)",
-            "DoneSth", "http://www.b.ch/.well-known/change-password", "", true, false, true);
+            "DoneSth", "http://www.b.ch/.well-known/change-password", "", 1, true, false, true);
 
     @Rule
     public TestRule mFeaturesProcessorRule = new Features.JUnitProcessor();
@@ -261,6 +261,70 @@ public class PasswordCheckControllerTest {
         assertThat(mModel.get(ITEMS).get(1).model.get(CREDENTIAL_HANDLER), is(mMediator));
     }
 
+    @Test
+    public void testSortsInitialSetOfCredentals() {
+        mMediator.onPasswordCheckStatusChanged(IDLE);
+
+        CompromisedCredential phishedEarly = makeCredential("example.com", "alice", 1, false, true);
+        CompromisedCredential phishedLeakedLate = makeCredential("test.com", "bob", 3, true, true);
+        CompromisedCredential leakedEarly = makeCredential("example.org", "alice", 2, true, false);
+        CompromisedCredential leakedLate = makeCredential("site.com", "john", 4, true, false);
+
+        when(mPasswordCheck.areScriptsRefreshed()).thenReturn(true);
+        when(mPasswordCheck.getCompromisedCredentials())
+                .thenReturn(new CompromisedCredential[] {
+                        phishedEarly, leakedEarly, leakedLate, phishedLeakedLate});
+        mMediator.onCompromisedCredentialsFetchCompleted();
+
+        assertThat(mModel.get(ITEMS).size(), is(5));
+        assertThat(
+                mModel.get(ITEMS).get(1).model.get(COMPROMISED_CREDENTIAL), is(phishedLeakedLate));
+        assertThat(mModel.get(ITEMS).get(2).model.get(COMPROMISED_CREDENTIAL), is(phishedEarly));
+        assertThat(mModel.get(ITEMS).get(3).model.get(COMPROMISED_CREDENTIAL), is(leakedLate));
+        assertThat(mModel.get(ITEMS).get(4).model.get(COMPROMISED_CREDENTIAL), is(leakedEarly));
+    }
+
+    @Test
+    public void testSortsAppendedCredentials() {
+        mMediator.onPasswordCheckStatusChanged(IDLE);
+
+        CompromisedCredential phishedEarly = makeCredential("example.com", "alice", 1, false, true);
+        CompromisedCredential phishedLeakedLate = makeCredential("test.com", "bob", 3, true, true);
+        CompromisedCredential leakedEarly = makeCredential("example.org", "alice", 2, true, false);
+        CompromisedCredential leakedLate = makeCredential("site.com", "john", 4, true, false);
+
+        when(mPasswordCheck.areScriptsRefreshed()).thenReturn(true);
+
+        // Send the initial set of credentials (to simulate loading them from disk).
+        when(mPasswordCheck.getCompromisedCredentials())
+                .thenReturn(new CompromisedCredential[] {
+                        phishedEarly, leakedEarly, leakedLate, phishedLeakedLate});
+        mMediator.onCompromisedCredentialsFetchCompleted();
+
+        // Send an updated list simulating credentials found in the current check.
+        CompromisedCredential leakedNewEarly1 =
+                makeCredential("example.com", "john", 5, true, false);
+        CompromisedCredential leakedNewEarly2 = makeCredential("test.com", "john", 5, true, false);
+        CompromisedCredential leakedNewLate = makeCredential("site.org", "alice", 6, true, false);
+        when(mPasswordCheck.getCompromisedCredentials())
+                .thenReturn(new CompromisedCredential[] {phishedEarly, leakedEarly, leakedLate,
+                        leakedNewEarly2, leakedNewLate, leakedNewEarly1, phishedLeakedLate});
+        mMediator.onCompromisedCredentialsFetchCompleted();
+
+        // Expect that the order of the original set has been maintained and that the newly found
+        // leaked credentials appear at the end in ascending order of creation time (or in ascending
+        // alphabetical order for equal times).
+        assertThat(mModel.get(ITEMS).size(), is(8));
+        assertThat(
+                mModel.get(ITEMS).get(1).model.get(COMPROMISED_CREDENTIAL), is(phishedLeakedLate));
+        assertThat(mModel.get(ITEMS).get(2).model.get(COMPROMISED_CREDENTIAL), is(phishedEarly));
+        assertThat(mModel.get(ITEMS).get(3).model.get(COMPROMISED_CREDENTIAL), is(leakedLate));
+        assertThat(mModel.get(ITEMS).get(4).model.get(COMPROMISED_CREDENTIAL), is(leakedEarly));
+        assertThat(mModel.get(ITEMS).get(5).model.get(COMPROMISED_CREDENTIAL), is(leakedNewEarly1));
+        assertThat(mModel.get(ITEMS).get(6).model.get(COMPROMISED_CREDENTIAL), is(leakedNewEarly2));
+        assertThat(mModel.get(ITEMS).get(7).model.get(COMPROMISED_CREDENTIAL), is(leakedNewLate));
+    }
+
     @Test
     public void testRemovingElementTriggersDelegate() {
         // Removing sets a valid handler:
@@ -322,4 +386,10 @@ public class PasswordCheckControllerTest {
         assertNotNull(header.model.get(RESTART_BUTTON_ACTION));
         assertNotNull(header.model.get(LAUNCH_ACCOUNT_CHECKUP_ACTION));
     }
+
+    private CompromisedCredential makeCredential(
+            String origin, String username, long creationTime, boolean leaked, boolean phished) {
+        return new CompromisedCredential(origin, mock(GURL.class), username, origin, username,
+                "password", origin, new String(), creationTime, leaked, phished, false);
+    }
 }

chrome/browser/password_check/android/password_check_bridge.cc

@@ -85,6 +85,7 @@ void PasswordCheckBridge::GetCompromisedCredentials(
         base::android::ConvertUTF8ToJavaString(env,
                                                credential.change_password_url),
         base::android::ConvertUTF8ToJavaString(env, credential.package_name),
+        credential.create_time.ToJavaTime(),
         (credential.compromise_type ==
          password_manager::CompromiseTypeFlags::kCredentialLeaked),
         (credential.compromise_type ==