[PwdEditAndroid] Add validation for username and password

Errors should be shown when: - The password field is left empty. - Attempting to modify a username to an already existing saved username for the same site. The "Done" button should be disabled if there is an error. Bug: 1175785 Change-Id: Idcc6125a40dc0f329cdd76651f09edbdd5a53bdf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2720225 Commit-Queue: Ioana Pandele <ioanap@chromium.org> Reviewed-by: Friedrich [CET] <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#857887}
chromium · Feb 25, 2021 · 413718a · 413718a
1 parent 710689e
commit 413718a
Show file tree

Hide file tree

Showing 14 changed files with 145 additions and 10 deletions.
diff --git a/chrome/browser/password_entry_edit/android/credential_edit_bridge.cc b/chrome/browser/password_entry_edit/android/credential_edit_bridge.cc
@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/android/jni_android.h"
+#include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/memory/ptr_util.h"
@@ -21,6 +22,7 @@
 
 std::unique_ptr<CredentialEditBridge> CredentialEditBridge::MaybeCreate(
     const password_manager::PasswordForm* credential,
+    std::vector<base::string16> existing_usernames,
     password_manager::SavedPasswordsPresenter* saved_passwords_presenter,
     base::OnceClosure dismissal_callback,
     const base::android::JavaRef<jobject>& context,
@@ -32,18 +34,21 @@ std::unique_ptr<CredentialEditBridge> CredentialEditBridge::MaybeCreate(
     return nullptr;
   }
   return base::WrapUnique(new CredentialEditBridge(
-      credential, saved_passwords_presenter, std::move(dismissal_callback),
-      context, settings_launcher, std::move(java_bridge)));
+      credential, std::move(existing_usernames), saved_passwords_presenter,
+      std::move(dismissal_callback), context, settings_launcher,
+      std::move(java_bridge)));
 }
 
 CredentialEditBridge::CredentialEditBridge(
     const password_manager::PasswordForm* credential,
+    std::vector<base::string16> existing_usernames,
     password_manager::SavedPasswordsPresenter* saved_passwords_presenter,
     base::OnceClosure dismissal_callback,
     const base::android::JavaRef<jobject>& context,
     const base::android::JavaRef<jobject>& settings_launcher,
     base::android::ScopedJavaGlobalRef<jobject> java_bridge)
     : credential_(credential),
+      existing_usernames_(std::move(existing_usernames)),
       saved_passwords_presenter_(saved_passwords_presenter),
       dismissal_callback_(std::move(dismissal_callback)),
       java_bridge_(java_bridge) {
@@ -67,6 +72,12 @@ void CredentialEditBridge::GetCredential(JNIEnv* env) {
                                               GetDisplayFederationOrigin()));
 }
 
+void CredentialEditBridge::GetExistingUsernames(JNIEnv* env) {
+  Java_CredentialEditBridge_setExistingUsernames(
+      env, java_bridge_,
+      base::android::ToJavaArrayOfStrings(env, existing_usernames_));
+}
+
 void CredentialEditBridge::SaveChanges(
     JNIEnv* env,
     const base::android::JavaParamRef<jstring>& username,

diff --git a/chrome/browser/password_entry_edit/android/credential_edit_bridge.h b/chrome/browser/password_entry_edit/android/credential_edit_bridge.h
@@ -22,6 +22,7 @@ class CredentialEditBridge {
   // shared.
   static std::unique_ptr<CredentialEditBridge> MaybeCreate(
       const password_manager::PasswordForm* credential,
+      std::vector<base::string16> existing_usernames,
       password_manager::SavedPasswordsPresenter* saved_passwords_presenter,
       base::OnceClosure dismissal_callback,
       const base::android::JavaRef<jobject>& context,
@@ -34,6 +35,9 @@ class CredentialEditBridge {
   // Called by Java to get the credential to be edited.
   void GetCredential(JNIEnv* env);
 
+  // Called by Java to get the existing usernames.
+  void GetExistingUsernames(JNIEnv* env);
+
   // Called by Java to save the changes to the edited credential.
   void SaveChanges(JNIEnv* env,
                    const base::android::JavaParamRef<jstring>& username,
@@ -45,6 +49,7 @@ class CredentialEditBridge {
  private:
   CredentialEditBridge(
       const password_manager::PasswordForm* credential,
+      std::vector<base::string16> existing_usernames,
       password_manager::SavedPasswordsPresenter* saved_passwords_presenter,
       base::OnceClosure dismissal_callback,
       const base::android::JavaRef<jobject>& context,
@@ -63,6 +68,9 @@ class CredentialEditBridge {
   // The credential to be edited.
   const password_manager::PasswordForm* credential_ = nullptr;
 
+  // All the usernames saved for the current site/app.
+  std::vector<base::string16> existing_usernames_;
+
   // The backend to route the edit event to. Should be owned by the the owner of
   // the bridge.
   password_manager::SavedPasswordsPresenter* saved_passwords_presenter_ =

diff --git a/...ternal/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditBridge.java b/...ternal/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditBridge.java
@@ -52,6 +52,9 @@ public void initialize(CredentialEditCoordinator coordinator) {
         mCoordinator = coordinator;
         // This will result in setCredential being called from native with the required data.
         CredentialEditBridgeJni.get().getCredential(mNativeCredentialEditBridge);
+
+        // This will result in setExistingUsernames being called from native with the required data.
+        CredentialEditBridgeJni.get().getExistingUsernames(mNativeCredentialEditBridge);
     }
 
     @CalledByNative
@@ -61,6 +64,11 @@ void setCredential(String displayUrlOrAppName, String username, String password,
                 displayUrlOrAppName, username, password, displayFederationOrigin);
     }
 
+    @CalledByNative
+    void setExistingUsernames(String[] existingUsernames) {
+        mCoordinator.setExistingUsernames(existingUsernames);
+    }
+
     // This can be called either before or after the native counterpart has gone away, depending
     // on where the edit component is being destroyed from.
     @Override
@@ -88,6 +96,7 @@ void destroy() {
     @NativeMethods
     interface Natives {
         void getCredential(long nativeCredentialEditBridge);
+        void getExistingUsernames(long nativeCredentialEditBridge);
         void saveChanges(long nativeCredentialEditBridge, String username, String password);
         void onUIDismissed(long nativeCredentialEditBridge);
     }

diff --git a/...ava/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditControllerTest.java b/...ava/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditControllerTest.java
@@ -17,6 +17,8 @@
 import static org.mockito.Mockito.when;
 
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.ALL_KEYS;
+import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.DUPLICATE_USERNAME_ERROR;
+import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.EMPTY_PASSWORD_ERROR;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.FEDERATION_ORIGIN;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.PASSWORD;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.PASSWORD_VISIBLE;
@@ -53,7 +55,9 @@
 public class CredentialEditControllerTest {
     private static final String TEST_URL = "https://m.a.xyz/signin";
     private static final String TEST_USERNAME = "TestUsername";
+    private static final String NEW_TEST_USERNAME = "TestNewUsername";
     private static final String TEST_PASSWORD = "TestPassword";
+    private static final String NEW_TEST_PASSWORD = "TestNewPassword";
 
     @Mock
     private PasswordAccessReauthenticationHelper mReauthenticationHelper;
@@ -190,4 +194,41 @@ public void callsTheDelegateWithCorrectDataWhenSaving() {
         mMediator.onSave();
         verify(mCredentialActionDelegate).saveChanges(TEST_USERNAME, TEST_PASSWORD);
     }
+
+    @Test
+    public void testUsernameTextChangedUpdatesModel() {
+        mMediator.setCredential(TEST_USERNAME, TEST_PASSWORD);
+        mMediator.setExistingUsernames(new String[] {TEST_USERNAME});
+        mMediator.onUsernameTextChanged(NEW_TEST_USERNAME);
+        assertEquals(NEW_TEST_USERNAME, mModel.get(USERNAME));
+    }
+
+    @Test
+    public void testPasswordTextChangedUpdatesModel() {
+        mMediator.setCredential(TEST_USERNAME, TEST_PASSWORD);
+        mMediator.onPasswordTextChanged(NEW_TEST_PASSWORD);
+        assertEquals(NEW_TEST_PASSWORD, mModel.get(PASSWORD));
+    }
+
+    @Test
+    public void testEmptyPasswordTriggersError() {
+        mMediator.setCredential(TEST_USERNAME, TEST_PASSWORD);
+        mMediator.onPasswordTextChanged("");
+        assertTrue(mModel.get(EMPTY_PASSWORD_ERROR));
+
+        mMediator.onPasswordTextChanged(TEST_PASSWORD);
+        assertFalse(mModel.get(EMPTY_PASSWORD_ERROR));
+    }
+
+    @Test
+    public void testDuplicateUsernameTriggersError() {
+        mMediator.setCredential(TEST_USERNAME, TEST_PASSWORD);
+        mMediator.setExistingUsernames(new String[] {TEST_USERNAME, NEW_TEST_USERNAME});
+
+        mMediator.onUsernameTextChanged(NEW_TEST_USERNAME);
+        assertTrue(mModel.get(DUPLICATE_USERNAME_ERROR));
+
+        mMediator.onUsernameTextChanged(TEST_USERNAME);
+        assertFalse(mModel.get(DUPLICATE_USERNAME_ERROR));
+    }
 }
diff --git a/...l/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditCoordinator.java b/...l/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditCoordinator.java
@@ -59,6 +59,10 @@ void setCredential(String displayUrlOrAppName, String username, String password,
         mMediator.setCredential(username, password);
     }
 
+    void setExistingUsernames(String[] existingUsernames) {
+        mMediator.setExistingUsernames(existingUsernames);
+    }
+
     void dismiss() {
         mMediator.dismiss();
     }

diff --git a/...rnal/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditMediator.java b/...rnal/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditMediator.java
@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.password_entry_edit;
 
+import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.DUPLICATE_USERNAME_ERROR;
+import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.EMPTY_PASSWORD_ERROR;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.FEDERATION_ORIGIN;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.PASSWORD;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.PASSWORD_VISIBLE;
@@ -23,6 +25,10 @@
 import org.chromium.ui.modelutil.PropertyModel.ReadableObjectPropertyKey;
 import org.chromium.ui.widget.Toast;
 
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
 /**
  * Contains the logic for the edit component. It  updates the model when needed and reacts to UI
  * events (e.g. button clicks).
@@ -31,6 +37,8 @@ public class CredentialEditMediator implements UiActionHandler {
     private final PasswordAccessReauthenticationHelper mReauthenticationHelper;
     private final CredentialActionDelegate mCredentialActionDelegate;
     private PropertyModel mModel;
+    private String mOriginalUsername;
+    private Set<String> mExistingUsernames;
 
     CredentialEditMediator(PasswordAccessReauthenticationHelper reauthenticationHelper,
             CredentialActionDelegate credentialActionDelegate) {
@@ -43,13 +51,18 @@ void initialize(PropertyModel model) {
     }
 
     void setCredential(String username, String password) {
+        mOriginalUsername = username;
         mModel.set(USERNAME, username);
         mModel.set(PASSWORD, password);
         // TODO(crbug.com/1175785): Replace the password with the identity provider in the case
         // of federated credentials.
         mModel.set(PASSWORD_VISIBLE, !mModel.get(FEDERATION_ORIGIN).isEmpty());
     }
 
+    void setExistingUsernames(String[] existingUsernames) {
+        mExistingUsernames = new HashSet<>(Arrays.asList(existingUsernames));
+    }
+
     void dismiss() {
         mModel.set(UI_DISMISSED_BY_NATIVE, true);
     }
@@ -75,11 +88,14 @@ public void onSave() {
     @Override
     public void onUsernameTextChanged(String username) {
         mModel.set(USERNAME, username);
+        mModel.set(DUPLICATE_USERNAME_ERROR,
+                !mOriginalUsername.equals(username) && mExistingUsernames.contains(username));
     }
 
     @Override
     public void onPasswordTextChanged(String password) {
         mModel.set(PASSWORD, password);
+        mModel.set(EMPTY_PASSWORD_ERROR, password.isEmpty());
     }
 
     @Override

diff --git a/...al/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditProperties.java b/...al/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditProperties.java
@@ -18,18 +18,23 @@ class CredentialEditProperties {
             new PropertyModel.ReadableObjectPropertyKey<>("url or app");
     static final PropertyModel.WritableObjectPropertyKey<String> USERNAME =
             new PropertyModel.WritableObjectPropertyKey<>("username");
+    static final PropertyModel.WritableBooleanPropertyKey DUPLICATE_USERNAME_ERROR =
+            new PropertyModel.WritableBooleanPropertyKey("duplicate username error");
     static final PropertyModel.WritableBooleanPropertyKey PASSWORD_VISIBLE =
             new PropertyModel.WritableBooleanPropertyKey("password visible");
     static final PropertyModel.WritableObjectPropertyKey<String> PASSWORD =
             new PropertyModel.WritableObjectPropertyKey<>("password");
+    static final PropertyModel.WritableBooleanPropertyKey EMPTY_PASSWORD_ERROR =
+            new PropertyModel.WritableBooleanPropertyKey("empty password error");
     static final PropertyModel.ReadableObjectPropertyKey<String> FEDERATION_ORIGIN =
             new PropertyModel.ReadableObjectPropertyKey<>("federation origin");
 
     static final PropertyModel.WritableBooleanPropertyKey UI_DISMISSED_BY_NATIVE =
             new PropertyModel.WritableBooleanPropertyKey("ui dismissed by native");
 
     static final PropertyKey[] ALL_KEYS = {UI_ACTION_HANDLER, URL_OR_APP, USERNAME,
-            PASSWORD_VISIBLE, PASSWORD, FEDERATION_ORIGIN, UI_DISMISSED_BY_NATIVE};
+            DUPLICATE_USERNAME_ERROR, PASSWORD_VISIBLE, PASSWORD, EMPTY_PASSWORD_ERROR,
+            FEDERATION_ORIGIN, UI_DISMISSED_BY_NATIVE};
 
     private CredentialEditProperties() {}
 }
diff --git a/...al/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditViewBinder.java b/...al/java/src/org/chromium/chrome/browser/password_entry_edit/CredentialEditViewBinder.java
@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.password_entry_edit;
 
+import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.DUPLICATE_USERNAME_ERROR;
+import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.EMPTY_PASSWORD_ERROR;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.FEDERATION_ORIGIN;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.PASSWORD;
 import static org.chromium.chrome.browser.password_entry_edit.CredentialEditProperties.PASSWORD_VISIBLE;
@@ -31,12 +33,16 @@ static void bindCredentialEditView(
             // layout is in place.
         } else if (propertyKey == USERNAME) {
             fragmentView.setUsername(model.get(USERNAME));
+        } else if (propertyKey == DUPLICATE_USERNAME_ERROR) {
+            fragmentView.changeUsernameError(model.get(DUPLICATE_USERNAME_ERROR));
         } else if (propertyKey == PASSWORD_VISIBLE) {
             fragmentView.changePasswordVisibility(model.get(PASSWORD_VISIBLE));
         } else if (propertyKey == PASSWORD) {
             if (model.get(FEDERATION_ORIGIN).isEmpty()) {
                 fragmentView.setPassword(model.get(PASSWORD));
             }
+        } else if (propertyKey == EMPTY_PASSWORD_ERROR) {
+            fragmentView.changePasswordError(model.get(EMPTY_PASSWORD_ERROR));
         } else if (propertyKey == UI_DISMISSED_BY_NATIVE) {
             fragmentView.dismiss();
         } else {

diff --git a/chrome/browser/password_entry_edit/android/java/res/layout/credential_edit_view.xml b/chrome/browser/password_entry_edit/android/java/res/layout/credential_edit_view.xml
@@ -44,14 +44,15 @@
             android:layout_height="wrap_content">
 
             <com.google.android.material.textfield.TextInputLayout
-                android:id="@+id/username_label"
+                android:id="@+id/username_text_input_layout"
                 android:labelFor="@+id/username"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
                 android:layout_marginTop="@dimen/password_entry_editor_field_top_margin"
                 android:layout_marginBottom="@dimen/password_entry_editor_field_bottom_margin"
                 android:hint="@string/password_entry_viewer_username_title"
-                app:hintTextAppearance="@style/TextAppearance.TextSmall.Secondary">
+                app:hintTextAppearance="@style/TextAppearance.TextSmall.Secondary"
+                app:errorTextAppearance="@style/TextAppearance.ErrorCaption">
 
                 <com.google.android.material.textfield.TextInputEditText
                     tools:ignore="LabelFor"
@@ -82,14 +83,15 @@
             android:layout_height="wrap_content">
 
             <com.google.android.material.textfield.TextInputLayout
-                android:id="@+id/password_label"
+                android:id="@+id/password_text_input_layout"
                 android:labelFor="@+id/password"
                 android:layout_height="wrap_content"
                 android:layout_width="match_parent"
                 android:layout_marginTop="@dimen/password_entry_editor_field_top_margin"
                 android:layout_marginBottom="@dimen/password_entry_editor_field_bottom_margin"
                 android:hint="@string/password_entry_viewer_password"
-                app:hintTextAppearance="@style/TextAppearance.TextSmall.Secondary">
+                app:hintTextAppearance="@style/TextAppearance.TextSmall.Secondary"
+                app:errorTextAppearance="@style/TextAppearance.ErrorCaption">
 
                 <com.google.android.material.textfield.TextInputEditText
                     tools:ignore="LabelFor"