From 90098fdb2f7fe52185ec1bce37252e89942712ad Mon Sep 17 00:00:00 2001 From: engedy Date: Fri, 8 Jan 2016 12:12:42 -0800 Subject: [PATCH] Do not prompt and/or create copies when Android logins are filled into 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} --- .../core/browser/password_form_manager.cc | 19 +++---- .../browser/password_form_manager_unittest.cc | 49 +++++++++++++++---- .../core/browser/password_manager_unittest.cc | 16 ++++++ 3 files changed, 66 insertions(+), 18 deletions(-) diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc index 9ff8533e788f5..e1fee32d2f502 100644 --- a/components/password_manager/core/browser/password_form_manager.cc +++ b/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; diff --git a/components/password_manager/core/browser/password_form_manager_unittest.cc b/components/password_manager/core/browser/password_form_manager_unittest.cc index e9e1571c00028..56af53edb53cf 100644 --- a/components/password_manager/core/browser/password_form_manager_unittest.cc +++ b/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 simulated_results; - simulated_results.push_back(new PasswordForm()); - simulated_results[0]->signon_realm = "android://hash@com.google.android"; - 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://hash@com.google.android"; + android_login.origin = GURL("android://hash@com.google.android/"); + 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 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- diff --git a/components/password_manager/core/browser/password_manager_unittest.cc b/components/password_manager/core/browser/password_manager_unittest.cc index 734b15a06249e..31b3f6c6cda87 100644 --- a/components/password_manager/core/browser/password_manager_unittest.cc +++ b/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