Do not prompt and/or create copies when Android logins are filled int…

…o websites.

Presently, when logins from Android apps are filled into the corresponding websites and are used for a successful login, the user is prompted to save a new credential, and ultimately a copy of the credential, keyed by the web site, gets saved to the password store.

This is a very confusing UX, and also results in duplicate logins being created.  Later, these can lead to an out-of-date password stored for the Android app if the Web password is ever updated in the future.

Instead, in this scenario, do not prompt at all, do not save any new credentials, but simply update the usage count of the existing Android credential in-place while leaving the other meta-attributes untouched.

BUG=540609

Review URL: https://codereview.chromium.org/1571813002

Cr-Commit-Position: refs/heads/master@{#368421}

components/password_manager/core/browser/password_form_manager.cc

@@ -735,6 +735,7 @@ void PasswordFormManager::UpdateLogin() {
     password_store->UpdateLoginWithPrimaryKey(pending_credentials_,
                                               old_primary_key);
   } else if (observed_form_.new_password_element.empty() &&
+             !IsValidAndroidFacetURI(pending_credentials_.signon_realm) &&
              (pending_credentials_.password_element.empty() ||
               pending_credentials_.username_element.empty() ||
               pending_credentials_.submit_element.empty())) {
@@ -991,8 +992,7 @@ void PasswordFormManager::CreatePendingCredentials() {
     pending_credentials_ = *saved_form;
     password_overridden_ =
         pending_credentials_.password_value != password_to_save;
-    if (IsPendingCredentialsPublicSuffixMatch() ||
-        IsValidAndroidFacetURI(preferred_match_->signon_realm)) {
+    if (IsPendingCredentialsPublicSuffixMatch()) {
       // If the autofilled credentials were a PSL match or credentials stored
       // from Android apps store a copy with the current origin and signon
       // realm. This ensures that on the next visit, a precise match is found.
@@ -1089,13 +1089,14 @@ void PasswordFormManager::CreatePendingCredentials() {
     CreatePendingCredentialsForNewCredentials();
   }
 
-  pending_credentials_.action = provisionally_saved_form_->action;
-
-  // If the user selected credentials we autofilled from a PasswordForm
-  // that contained no action URL (IE6/7 imported passwords, for example),
-  // bless it with the action URL from the observed form. See bug 1107719.
-  if (pending_credentials_.action.is_empty())
-    pending_credentials_.action = observed_form_.action;
+  if (!IsValidAndroidFacetURI(pending_credentials_.signon_realm)) {
+    pending_credentials_.action = provisionally_saved_form_->action;
+    // If the user selected credentials we autofilled from a PasswordForm
+    // that contained no action URL (IE6/7 imported passwords, for example),
+    // bless it with the action URL from the observed form. See bug 1107719.
+    if (pending_credentials_.action.is_empty())
+      pending_credentials_.action = observed_form_.action;
+  }
 
   pending_credentials_.password_value = password_to_save;
   pending_credentials_.preferred = provisionally_saved_form_->preferred;

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -1321,25 +1321,56 @@ TEST_F(PasswordFormManagerTest, AndroidCredentialsAreAutofilled) {
   EXPECT_CALL(*(client()->mock_driver()), AllowPasswordGenerationForForm(_));
 
   // Although Android-based credentials are treated similarly to PSL-matched
-  // credentials in most respects, they should be autofilled as opposed to be
+  // credentials in some respects, they should be autofilled as opposed to be
   // filled on username-select.
-  ScopedVector<PasswordForm> simulated_results;
-  simulated_results.push_back(new PasswordForm());
-  simulated_results[0]->signon_realm = "android://[email protected]";
-  simulated_results[0]->origin = observed_form()->origin;
-  simulated_results[0]->username_value = saved_match()->username_value;
-  simulated_results[0]->password_value = saved_match()->password_value;
-
-  form_manager()->SimulateFetchMatchingLoginsFromPasswordStore();
+  PasswordForm android_login;
+  android_login.signon_realm = "android://[email protected]";
+  android_login.origin = GURL("android://[email protected]/");
+  android_login.is_affiliation_based_match = true;
+  android_login.username_value = saved_match()->username_value;
+  android_login.password_value = saved_match()->password_value;
+  android_login.preferred = false;
+  android_login.times_used = 42;
 
   autofill::PasswordFormFillData fill_data;
   EXPECT_CALL(*client()->mock_driver(), FillPasswordForm(_))
       .WillOnce(SaveArg<0>(&fill_data));
 
+  ScopedVector<PasswordForm> simulated_results;
+  simulated_results.push_back(new PasswordForm(android_login));
+  form_manager()->SimulateFetchMatchingLoginsFromPasswordStore();
   form_manager()->OnGetPasswordStoreResults(std::move(simulated_results));
   EXPECT_TRUE(fill_data.additional_logins.empty());
   EXPECT_FALSE(fill_data.wait_for_username);
   EXPECT_EQ(1u, form_manager()->best_matches().size());
+
+  // When the user submits the filled form, no copy of the credential should be
+  // created, instead the usage counter of the original credential should be
+  // incremented in-place, as if it were a regular credential for that website.
+  PasswordForm credential(*observed_form());
+  credential.username_value = android_login.username_value;
+  credential.password_value = android_login.password_value;
+  credential.preferred = true;
+  form_manager()->ProvisionallySave(
+      credential, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  EXPECT_FALSE(form_manager()->IsNewLogin());
+
+  PasswordForm updated_credential;
+  EXPECT_CALL(*mock_store(), UpdateLogin(_))
+      .WillOnce(testing::SaveArg<0>(&updated_credential));
+  EXPECT_CALL(*mock_store(), UpdateLoginWithPrimaryKey(_, _)).Times(0);
+  EXPECT_CALL(*mock_store(), AddLogin(_)).Times(0);
+  form_manager()->Save();
+  Mock::VerifyAndClearExpectations(mock_store());
+
+  EXPECT_EQ(android_login.username_value, updated_credential.username_value);
+  EXPECT_EQ(android_login.password_value, updated_credential.password_value);
+  EXPECT_EQ(android_login.times_used + 1, updated_credential.times_used);
+  EXPECT_TRUE(updated_credential.preferred);
+  EXPECT_EQ(GURL(), updated_credential.action);
+  EXPECT_EQ(base::string16(), updated_credential.username_element);
+  EXPECT_EQ(base::string16(), updated_credential.password_element);
+  EXPECT_EQ(base::string16(), updated_credential.submit_element);
 }
 
 // Credentials saved through Android apps should always be shown in the drop-

components/password_manager/core/browser/password_manager_unittest.cc

@@ -1313,6 +1313,22 @@ TEST_F(PasswordManagerTest, AutofillingOfAffiliatedCredentials) {
   EXPECT_EQ(android_form.password_value, form_data.password_field.value);
   EXPECT_FALSE(form_data.wait_for_username);
   EXPECT_EQ(android_form.signon_realm, form_data.preferred_realm);
+
+  EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
+      .WillRepeatedly(Return(true));
+
+  PasswordForm filled_form(form);
+  filled_form.username_value = android_form.username_value;
+  filled_form.password_value = android_form.password_value;
+  OnPasswordFormSubmitted(filled_form);
+
+  observed.clear();
+  EXPECT_CALL(*store_, UpdateLogin(_));
+  EXPECT_CALL(client_, PromptUserToSaveOrUpdatePasswordPtr(_, _)).Times(0);
+  EXPECT_CALL(*store_, AddLogin(_)).Times(0);
+  EXPECT_CALL(*store_, UpdateLoginWithPrimaryKey(_, _)).Times(0);
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
 }
 
 }  // namespace password_manager