Skip to content

GitLab

T
tangled
Commit 33138a00 authored by gcasto's avatar gcasto Committed by Commit bot
Browse files
Options

[Password Manager] Fix password saving on Macys registration page 

This requires a few changes to how we evaluate pages during the saving process.

1) Instead of ignoring forms where there are more than 3 password fields, assume that those fields are not related to actual passwords. Also change the case of 3 passwords to assume that if the first and second password match, that the third is not related. I don't have statistics on this, but I have seen security questions that are entered into password fields much more often than I have seen a new password entered before and old password (which was the previous assumption).

2) Move change password detection to work on the submitted password form instead of the observed form. The submitted form is more likely to be identified correctly because we have additional data after submission. This failed on Macy's because we incorrectly assume that, given the number of password fields, one of them is a current password field during parsing.

3) Don't require HTML field attributes to match for signup forms. This is normally required because otherwise we can't fill the password we have just saved. However in the case of signup forms this doesn't matter because we don't want to fill on the same form again anyway.

BUG=452306, 451631

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

Cr-Commit-Position: refs/heads/master@{#318850}
parent 2de81257
Hide whitespace changes
Inline Side-by-side
Showing with 266 additions and 70 deletions
chrome/browser/password_manager/password_manager_browsertest.cc
View file @ 33138a00
...@@ -1615,3 +1615,48 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, NoPromptOnBack) { ...@@ -1615,3 +1615,48 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, NoPromptOnBack) {
observer.Wait(); observer.Wait();
EXPECT_FALSE(prompt_observer->IsShowingPrompt()); EXPECT_FALSE(prompt_observer->IsShowingPrompt());
} }
// Regression test for http://crbug.com/452306
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
ChangingTextToPasswordFieldOnSignupForm) {
NavigateToFile("/password/signup_form.html");
// In this case, pretend that username_field is actually a password field
// that starts as a text field to simulate placeholder.
NavigationObserver observer(WebContents());
scoped_ptr<PromptObserver> prompt_observer(
PromptObserver::Create(WebContents()));
std::string change_and_submit =
"document.getElementById('other_info').value = 'username';"
"document.getElementById('username_field').type = 'password';"
"document.getElementById('username_field').value = 'mypass';"
"document.getElementById('password_field').value = 'mypass';"
"document.getElementById('testform').submit();";
ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), change_and_submit));
observer.Wait();
EXPECT_TRUE(prompt_observer->IsShowingPrompt());
}
// Regression test for http://crbug.com/451631
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
SavingOnManyPasswordFieldsTest) {
// Simulate Macy's registration page, which contains the normal 2 password
// fields for confirming the new password plus 2 more fields for security
// questions and credit card. Make sure that saving works correctly for such
// sites.
NavigateToFile("/password/many_password_signup_form.html");
NavigationObserver observer(WebContents());
scoped_ptr<PromptObserver> prompt_observer(
PromptObserver::Create(WebContents()));
std::string fill_and_submit =
"document.getElementById('username_field').value = 'username';"
"document.getElementById('password_field').value = 'mypass';"
"document.getElementById('confirm_field').value = 'mypass';"
"document.getElementById('security_answer').value = 'hometown';"
"document.getElementById('SSN').value = '1234';"
"document.getElementById('testform').submit();";
ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
observer.Wait();
EXPECT_TRUE(prompt_observer->IsShowingPrompt());
}
chrome/test/data/password/many_password_signup_form.html 0 → 100644
View file @ 33138a00
<html>
<body>
<form method="POST" action="done.html" onsubmit="return true;" id="testform">
<input type="text" id="username_field" name="username_field">
<input type="password" id="password_field" name="password_field">
<input type="password" id="confirm_field" name="confirm_field">
<input type="password" id="security_answer" name="security_answer">
<input type="text" id="more_info" name="more_info">
<input type="password" id="SSN" name="SSN">
<input type="submit" id="input_submit_button" name="input_submit_button">
</form>
</html>
</body>
components/autofill/content/renderer/password_form_conversion_utils.cc
View file @ 33138a00
...@@ -60,6 +60,9 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords, ...@@ -60,6 +60,9 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
if (!current_password->isNull() || !new_password->isNull()) if (!current_password->isNull() || !new_password->isNull())
return true; return true;
if (passwords.empty())
return false;
switch (passwords.size()) { switch (passwords.size()) {
case 1: case 1:
// Single password, easy. // Single password, easy.
...@@ -77,7 +80,7 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords, ...@@ -77,7 +80,7 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
*new_password = passwords[1]; *new_password = passwords[1];
} }
break; break;
case 3: default:
if (!passwords[0].value().isEmpty() && if (!passwords[0].value().isEmpty() &&
passwords[0].value() == passwords[1].value() && passwords[0].value() == passwords[1].value() &&
passwords[0].value() == passwords[2].value()) { passwords[0].value() == passwords[2].value()) {
...@@ -91,17 +94,15 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords, ...@@ -91,17 +94,15 @@ bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
*new_password = passwords[1]; *new_password = passwords[1];
} else if (passwords[0].value() == passwords[1].value()) { } else if (passwords[0].value() == passwords[1].value()) {
// It is strange that the new password comes first, but trust more which // It is strange that the new password comes first, but trust more which
// fields are duplicated than the ordering of fields. // fields are duplicated than the ordering of fields. Assume that
*current_password = passwords[2]; // any password fields after the new password contain sensitive
// information that isn't actually a password (security hint, SSN, etc.)
*new_password = passwords[0]; *new_password = passwords[0];
} else { } else {
// Three different passwords, or first and last match with middle // Three different passwords, or first and last match with middle
// different. No idea which is which, so no luck. // different. No idea which is which, so no luck.
return false; return false;
} }
break;
default:
return false;
} }
return true; return true;
} }
......
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
View file @ 33138a00
...@@ -342,7 +342,10 @@ TEST_F(PasswordFormConversionUtilsTest, IdentifyingThreePasswordFields) { ...@@ -342,7 +342,10 @@ TEST_F(PasswordFormConversionUtilsTest, IdentifyingThreePasswordFields) {
{{"alpha", "", ""}, "password1", "alpha", "password2", ""}, {{"alpha", "", ""}, "password1", "alpha", "password2", ""},
{{"", "beta", "beta"}, "password1", "", "password2", "beta"}, {{"", "beta", "beta"}, "password1", "", "password2", "beta"},
{{"alpha", "beta", "beta"}, "password1", "alpha", "password2", "beta"}, {{"alpha", "beta", "beta"}, "password1", "alpha", "password2", "beta"},
{{"beta", "beta", "alpha"}, "password3", "alpha", "password1", "beta"}, // If confirmed password comes first, assume that the third password
// field is related to security question, SSN, or credit card and ignore
// it.
{{"beta", "beta", "alpha"}, "", "", "password1", "beta"},
// If the fields are yet empty, we speculate that we will identify them as // If the fields are yet empty, we speculate that we will identify them as
// (current + new + new) once they are filled out, so we should classify // (current + new + new) once they are filled out, so we should classify
// them the same for now to keep our abstract interpretation less flaky. // them the same for now to keep our abstract interpretation less flaky.
......
components/autofill/core/common/save_password_progress_logger.cc
View file @ 33138a00
...@@ -133,6 +133,8 @@ std::string GetStringFromID(SavePasswordProgressLogger::StringID id) { ...@@ -133,6 +133,8 @@ std::string GetStringFromID(SavePasswordProgressLogger::StringID id) {
return "Form manager found, exact match"; return "Form manager found, exact match";
case SavePasswordProgressLogger::STRING_MATCH_WITHOUT_ACTION: case SavePasswordProgressLogger::STRING_MATCH_WITHOUT_ACTION:
return "Form manager found, match except for action"; return "Form manager found, match except for action";
case SavePasswordProgressLogger::STRING_ORIGINS_MATCH:
return "Form manager found, only origins match";
case SavePasswordProgressLogger::STRING_MATCHING_NOT_COMPLETE: case SavePasswordProgressLogger::STRING_MATCHING_NOT_COMPLETE:
return "No form manager has completed matching"; return "No form manager has completed matching";
case SavePasswordProgressLogger::STRING_FORM_BLACKLISTED: case SavePasswordProgressLogger::STRING_FORM_BLACKLISTED:
......
components/autofill/core/common/save_password_progress_logger.h
View file @ 33138a00
...@@ -84,6 +84,7 @@ class SavePasswordProgressLogger { ...@@ -84,6 +84,7 @@ class SavePasswordProgressLogger {
STRING_EMPTY_PASSWORD, STRING_EMPTY_PASSWORD,
STRING_EXACT_MATCH, STRING_EXACT_MATCH,
STRING_MATCH_WITHOUT_ACTION, STRING_MATCH_WITHOUT_ACTION,
STRING_ORIGINS_MATCH,
STRING_MATCHING_NOT_COMPLETE, STRING_MATCHING_NOT_COMPLETE,
STRING_FORM_BLACKLISTED, STRING_FORM_BLACKLISTED,
STRING_INVALID_FORM, STRING_INVALID_FORM,
......
components/password_manager/core/browser/password_form_manager.cc
View file @ 33138a00
...@@ -154,10 +154,14 @@ PasswordFormManager::MatchResultMask PasswordFormManager::DoesManage( ...@@ -154,10 +154,14 @@ PasswordFormManager::MatchResultMask PasswordFormManager::DoesManage(
StartsWithASCII(new_path, old_path, /*case_sensitive=*/true); StartsWithASCII(new_path, old_path, /*case_sensitive=*/true);
} }
if (!origins_match)
return result;
result |= RESULT_ORIGINS_MATCH;
if (form.username_element == observed_form_.username_element && if (form.username_element == observed_form_.username_element &&
form.password_element == observed_form_.password_element && form.password_element == observed_form_.password_element) {
origins_match) { result |= RESULT_HTML_ATTRIBUTES_MATCH;
result |= RESULT_MANDATORY_ATTRIBUTES_MATCH;
} }
// Note: although saved password forms might actually have an empty action // Note: although saved password forms might actually have an empty action
...@@ -405,13 +409,12 @@ bool PasswordFormManager::HasCompletedMatching() const { ...@@ -405,13 +409,12 @@ bool PasswordFormManager::HasCompletedMatching() const {
} }
bool PasswordFormManager::IsIgnorableChangePasswordForm( bool PasswordFormManager::IsIgnorableChangePasswordForm(
const base::string16& typed_username, const PasswordForm& form) const {
const base::string16& typed_password) const { bool is_change_password_form =
bool is_change_password_form = !observed_form_.new_password_element.empty() && !form.new_password_element.empty() && !form.password_element.empty();
!observed_form_.password_element.empty(); return is_change_password_form && !form.username_marked_by_site &&
return is_change_password_form && !observed_form_.username_marked_by_site && !DoesUsenameAndPasswordMatchCredentials(
!DoesUsenameAndPasswordMatchCredentials(typed_username, typed_password, form.username_value, form.password_value, best_matches_);
best_matches_);
} }
void PasswordFormManager::OnRequestDone( void PasswordFormManager::OnRequestDone(
......
components/password_manager/core/browser/password_form_manager.h
View file @ 33138a00
...@@ -42,12 +42,17 @@ class PasswordFormManager : public PasswordStoreConsumer { ...@@ -42,12 +42,17 @@ class PasswordFormManager : public PasswordStoreConsumer {
// DoesMatch. Individual flags are only relevant for HTML forms, but // DoesMatch. Individual flags are only relevant for HTML forms, but
// RESULT_COMPLETE_MATCH will also be returned to indicate non-HTML forms // RESULT_COMPLETE_MATCH will also be returned to indicate non-HTML forms
// completely matching. // completely matching.
// The ordering of these flags is important. Larger matches are more
// preferred than lower matches. That is, since RESULT_HTML_ATTRIBUTES_MATCH
// is greater than RESULT_ACTION_MATCH, a match of only attributes and not
// actions will be preferred to one of actions and not attributes.
enum MatchResultFlags { enum MatchResultFlags {
RESULT_NO_MATCH = 0, RESULT_NO_MATCH = 0,
RESULT_MANDATORY_ATTRIBUTES_MATCH = 1 << 0, // Bare minimum to be a match. RESULT_ACTION_MATCH = 1 << 0,
RESULT_ACTION_MATCH = 1 << 1, // Action URLs match too. RESULT_HTML_ATTRIBUTES_MATCH = 1 << 1,
RESULT_COMPLETE_MATCH = RESULT_ORIGINS_MATCH = 1 << 2,
RESULT_MANDATORY_ATTRIBUTES_MATCH | RESULT_ACTION_MATCH RESULT_COMPLETE_MATCH = RESULT_ACTION_MATCH | RESULT_HTML_ATTRIBUTES_MATCH |
RESULT_ORIGINS_MATCH
}; };
// Use MatchResultMask to contain combinations of MatchResultFlags values. // Use MatchResultMask to contain combinations of MatchResultFlags values.
// It's a signed int rather than unsigned to avoid signed/unsigned mismatch // It's a signed int rather than unsigned to avoid signed/unsigned mismatch
...@@ -84,18 +89,15 @@ class PasswordFormManager : public PasswordStoreConsumer { ...@@ -84,18 +89,15 @@ class PasswordFormManager : public PasswordStoreConsumer {
// the same thread! // the same thread!
bool HasCompletedMatching() const; bool HasCompletedMatching() const;
// Returns true if the observed form has both the current and new password // Returns true if |form| has both the current and new password fields, and
// fields, and the username field was not explicitly marked with // the username field was not explicitly marked with "autocomplete=username"
// "autocomplete=username" and the user-typed username and current password // and |form.username_value| and |form.password_value| fields do not match
// field values do not match the credentials already stored. In these cases it // the credentials already stored. In these cases it is not clear whether
// is not clear whether the username field is the right guess (often such // the username field is the right guess (often such change password forms do
// change password forms do not contain the username at all), and the user // not contain the username at all), and the user should not be bothered with
// should not be bothered with saving a potentially malformed credential. Once // saving a potentially malformed credential. Once we handle change password
// we handle change password forms correctly, this method should be replaced // forms correctly, this method should be replaced accordingly.
// accordingly. bool IsIgnorableChangePasswordForm(const autofill::PasswordForm& form) const;
bool IsIgnorableChangePasswordForm(
const base::string16& typed_username,
const base::string16& typed_password) const;
// Determines if the user opted to 'never remember' passwords for this form. // Determines if the user opted to 'never remember' passwords for this form.
bool IsBlacklisted() const; bool IsBlacklisted() const;
......
components/password_manager/core/browser/password_form_manager_unittest.cc
View file @ 33138a00
...@@ -397,7 +397,6 @@ TEST_F(PasswordFormManagerTest, PSLMatchedCredentialsMetadataUpdated) { ...@@ -397,7 +397,6 @@ TEST_F(PasswordFormManagerTest, PSLMatchedCredentialsMetadataUpdated) {
submitted_form.preferred = true; submitted_form.preferred = true;
submitted_form.username_value = saved_match()->username_value; submitted_form.username_value = saved_match()->username_value;
submitted_form.password_value = saved_match()->password_value; submitted_form.password_value = saved_match()->password_value;
submitted_form.origin = saved_match()->origin;
form_manager.ProvisionallySave( form_manager.ProvisionallySave(
submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES); submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
...@@ -405,6 +404,7 @@ TEST_F(PasswordFormManagerTest, PSLMatchedCredentialsMetadataUpdated) { ...@@ -405,6 +404,7 @@ TEST_F(PasswordFormManagerTest, PSLMatchedCredentialsMetadataUpdated) {
expected_saved_form.times_used = 1; expected_saved_form.times_used = 1;
expected_saved_form.other_possible_usernames.clear(); expected_saved_form.other_possible_usernames.clear();
expected_saved_form.form_data = saved_match()->form_data; expected_saved_form.form_data = saved_match()->form_data;
expected_saved_form.origin = saved_match()->origin;
PasswordForm actual_saved_form; PasswordForm actual_saved_form;
EXPECT_CALL(*(client_with_store.mock_driver()->mock_autofill_manager()), EXPECT_CALL(*(client_with_store.mock_driver()->mock_autofill_manager()),
...@@ -1061,17 +1061,15 @@ TEST_F(PasswordFormManagerTest, NonHTMLFormsDoNotMatchHTMLForms) { ...@@ -1061,17 +1061,15 @@ TEST_F(PasswordFormManagerTest, NonHTMLFormsDoNotMatchHTMLForms) {
ASSERT_EQ(PasswordForm::SCHEME_HTML, observed_form()->scheme); ASSERT_EQ(PasswordForm::SCHEME_HTML, observed_form()->scheme);
PasswordForm non_html_form(*observed_form()); PasswordForm non_html_form(*observed_form());
non_html_form.scheme = PasswordForm::SCHEME_DIGEST; non_html_form.scheme = PasswordForm::SCHEME_DIGEST;
EXPECT_EQ(0, EXPECT_EQ(0, manager.DoesManage(non_html_form) &
manager.DoesManage(non_html_form) & PasswordFormManager::RESULT_HTML_ATTRIBUTES_MATCH);
PasswordFormManager::RESULT_MANDATORY_ATTRIBUTES_MATCH);
// The other way round: observing a non-HTML form, don't match a HTML form. // The other way round: observing a non-HTML form, don't match a HTML form.
PasswordForm html_form(*observed_form()); PasswordForm html_form(*observed_form());
PasswordFormManager non_html_manager(nullptr, client(), kNoDriver, PasswordFormManager non_html_manager(nullptr, client(), kNoDriver,
non_html_form, false); non_html_form, false);
EXPECT_EQ(0, EXPECT_EQ(0, non_html_manager.DoesManage(html_form) &
non_html_manager.DoesManage(html_form) & PasswordFormManager::RESULT_HTML_ATTRIBUTES_MATCH);
PasswordFormManager::RESULT_MANDATORY_ATTRIBUTES_MATCH);
} }
TEST_F(PasswordFormManagerTest, OriginCheck_HostsMatchExactly) { TEST_F(PasswordFormManagerTest, OriginCheck_HostsMatchExactly) {
...@@ -1082,9 +1080,8 @@ TEST_F(PasswordFormManagerTest, OriginCheck_HostsMatchExactly) { ...@@ -1082,9 +1080,8 @@ TEST_F(PasswordFormManagerTest, OriginCheck_HostsMatchExactly) {
PasswordForm form_longer_host(*observed_form()); PasswordForm form_longer_host(*observed_form());
form_longer_host.origin = GURL("http://accounts.google.com.au/a/LoginAuth"); form_longer_host.origin = GURL("http://accounts.google.com.au/a/LoginAuth");
// Check that accounts.google.com does not match accounts.google.com.au. // Check that accounts.google.com does not match accounts.google.com.au.
EXPECT_EQ(0, EXPECT_EQ(0, manager.DoesManage(form_longer_host) &
manager.DoesManage(form_longer_host) & PasswordFormManager::RESULT_HTML_ATTRIBUTES_MATCH);
PasswordFormManager::RESULT_MANDATORY_ATTRIBUTES_MATCH);
} }
TEST_F(PasswordFormManagerTest, OriginCheck_MoreSecureSchemePathsMatchPrefix) { TEST_F(PasswordFormManagerTest, OriginCheck_MoreSecureSchemePathsMatchPrefix) {
...@@ -1095,9 +1092,8 @@ TEST_F(PasswordFormManagerTest, OriginCheck_MoreSecureSchemePathsMatchPrefix) { ...@@ -1095,9 +1092,8 @@ TEST_F(PasswordFormManagerTest, OriginCheck_MoreSecureSchemePathsMatchPrefix) {
PasswordForm form_longer_path(*observed_form()); PasswordForm form_longer_path(*observed_form());
form_longer_path.origin = GURL("https://accounts.google.com/a/LoginAuth/sec"); form_longer_path.origin = GURL("https://accounts.google.com/a/LoginAuth/sec");
EXPECT_NE(0, EXPECT_NE(0, manager.DoesManage(form_longer_path) &
manager.DoesManage(form_longer_path) & PasswordFormManager::RESULT_HTML_ATTRIBUTES_MATCH);
PasswordFormManager::RESULT_MANDATORY_ATTRIBUTES_MATCH);
} }
TEST_F(PasswordFormManagerTest, TEST_F(PasswordFormManagerTest,
...@@ -1110,9 +1106,8 @@ TEST_F(PasswordFormManagerTest, ...@@ -1110,9 +1106,8 @@ TEST_F(PasswordFormManagerTest,
PasswordForm form_longer_path(*observed_form()); PasswordForm form_longer_path(*observed_form());
form_longer_path.origin = GURL("http://accounts.google.com/a/LoginAuth/sec"); form_longer_path.origin = GURL("http://accounts.google.com/a/LoginAuth/sec");
// Check that /a/LoginAuth does not match /a/LoginAuth/more. // Check that /a/LoginAuth does not match /a/LoginAuth/more.
EXPECT_EQ(0, EXPECT_EQ(0, manager.DoesManage(form_longer_path) &
manager.DoesManage(form_longer_path) & PasswordFormManager::RESULT_HTML_ATTRIBUTES_MATCH);
PasswordFormManager::RESULT_MANDATORY_ATTRIBUTES_MATCH);
PasswordForm secure_observed_form(*observed_form()); PasswordForm secure_observed_form(*observed_form());
secure_observed_form.origin = GURL("https://accounts.google.com/a/LoginAuth"); secure_observed_form.origin = GURL("https://accounts.google.com/a/LoginAuth");
...@@ -1120,14 +1115,28 @@ TEST_F(PasswordFormManagerTest, ...@@ -1120,14 +1115,28 @@ TEST_F(PasswordFormManagerTest,
secure_observed_form, true); secure_observed_form, true);
// Also for HTTPS in the observed form, and HTTP in the compared form, an // Also for HTTPS in the observed form, and HTTP in the compared form, an
// exact path match is expected. // exact path match is expected.
EXPECT_EQ(0, EXPECT_EQ(0, secure_manager.DoesManage(form_longer_path) &
secure_manager.DoesManage(form_longer_path) & PasswordFormManager::RESULT_HTML_ATTRIBUTES_MATCH);
PasswordFormManager::RESULT_MANDATORY_ATTRIBUTES_MATCH);
// Not even upgrade to HTTPS in the compared form should help. // Not even upgrade to HTTPS in the compared form should help.
form_longer_path.origin = GURL("https://accounts.google.com/a/LoginAuth/sec"); form_longer_path.origin = GURL("https://accounts.google.com/a/LoginAuth/sec");
EXPECT_EQ(0, EXPECT_EQ(0, secure_manager.DoesManage(form_longer_path) &
secure_manager.DoesManage(form_longer_path) & PasswordFormManager::RESULT_HTML_ATTRIBUTES_MATCH);
PasswordFormManager::RESULT_MANDATORY_ATTRIBUTES_MATCH); }
TEST_F(PasswordFormManagerTest, OriginCheck_OnlyOriginsMatch) {
// Make sure DoesManage() can distinguish when only origins match.
PasswordFormManager manager(NULL, client(), kNoDriver, *observed_form(),
false);
PasswordForm different_html_attributes(*observed_form());
different_html_attributes.password_element = ASCIIToUTF16("random_pass");
different_html_attributes.username_element = ASCIIToUTF16("random_user");
EXPECT_EQ(0, manager.DoesManage(different_html_attributes) &
PasswordFormManager::RESULT_HTML_ATTRIBUTES_MATCH);
EXPECT_EQ(PasswordFormManager::RESULT_ORIGINS_MATCH,
manager.DoesManage(different_html_attributes) &
PasswordFormManager::RESULT_ORIGINS_MATCH);
} }
TEST_F(PasswordFormManagerTest, CorrectlyUpdatePasswordsWithSameUsername) { TEST_F(PasswordFormManagerTest, CorrectlyUpdatePasswordsWithSameUsername) {
...@@ -1397,15 +1406,11 @@ TEST_F(PasswordFormManagerTest, ...@@ -1397,15 +1406,11 @@ TEST_F(PasswordFormManagerTest,
credentials.password_value = saved_match()->password_value; credentials.password_value = saved_match()->password_value;
credentials.new_password_value = ASCIIToUTF16("NewPassword"); credentials.new_password_value = ASCIIToUTF16("NewPassword");
EXPECT_FALSE(manager.IsIgnorableChangePasswordForm( EXPECT_FALSE(manager.IsIgnorableChangePasswordForm(credentials));
credentials.username_value, credentials.password_value));
} }
TEST_F(PasswordFormManagerTest, TEST_F(PasswordFormManagerTest,
IsIngnorableChangePasswordForm_NotMatchingPassword) { IsIngnorableChangePasswordForm_NotMatchingPassword) {
observed_form()->new_password_element =
base::ASCIIToUTF16("new_password_field");
TestPasswordManagerClient client_with_store(mock_store()); TestPasswordManagerClient client_with_store(mock_store());
PasswordFormManager manager(nullptr, &client_with_store, PasswordFormManager manager(nullptr, &client_with_store,
client_with_store.driver(), *observed_form(), client_with_store.driver(), *observed_form(),
...@@ -1417,15 +1422,14 @@ TEST_F(PasswordFormManagerTest, ...@@ -1417,15 +1422,14 @@ TEST_F(PasswordFormManagerTest,
// the username), and the user-typed password do not match anything already // the username), and the user-typed password do not match anything already
// stored. There is not much confidence in the guess being right, so the // stored. There is not much confidence in the guess being right, so the
// password should not be stored. // password should not be stored.
EXPECT_TRUE(manager.IsIgnorableChangePasswordForm( saved_match()->password_value = ASCIIToUTF16("DifferentPassword");
saved_match()->username_value, ASCIIToUTF16("DifferentPassword"))); saved_match()->new_password_element =
base::ASCIIToUTF16("new_password_field");
EXPECT_TRUE(manager.IsIgnorableChangePasswordForm(*saved_match()));
} }
TEST_F(PasswordFormManagerTest, TEST_F(PasswordFormManagerTest,
IsIngnorableChangePasswordForm_NotMatchingUsername) { IsIngnorableChangePasswordForm_NotMatchingUsername) {
observed_form()->new_password_element =
base::ASCIIToUTF16("new_password_field");
TestPasswordManagerClient client_with_store(mock_store()); TestPasswordManagerClient client_with_store(mock_store());
PasswordFormManager manager(nullptr, &client_with_store, PasswordFormManager manager(nullptr, &client_with_store,
client_with_store.driver(), *observed_form(), client_with_store.driver(), *observed_form(),
...@@ -1437,8 +1441,10 @@ TEST_F(PasswordFormManagerTest, ...@@ -1437,8 +1441,10 @@ TEST_F(PasswordFormManagerTest,
// the username), and the user-typed username does not match anything already // the username), and the user-typed username does not match anything already
// stored. There is not much confidence in the guess being right, so the // stored. There is not much confidence in the guess being right, so the
// password should not be stored. // password should not be stored.
EXPECT_TRUE(manager.IsIgnorableChangePasswordForm( saved_match()->username_value = ASCIIToUTF16("DifferentUsername");
ASCIIToUTF16("DifferentUsername"), saved_match()->password_value)); saved_match()->new_password_element =
base::ASCIIToUTF16("new_password_field");
EXPECT_TRUE(manager.IsIgnorableChangePasswordForm(*saved_match()));
} }
} // namespace password_manager } // namespace password_manager
components/password_manager/core/browser/password_manager.cc
View file @ 33138a00
...@@ -107,6 +107,10 @@ void RecordWhetherTargetDomainDiffers(const GURL& src, const GURL& target) { ...@@ -107,6 +107,10 @@ void RecordWhetherTargetDomainDiffers(const GURL& src, const GURL& target) {
target_domain_differs); target_domain_differs);
} }
bool IsSignupForm(const PasswordForm& form) {
return !form.new_password_element.empty() && form.password_element.empty();
}
} // namespace } // namespace
const char PasswordManager::kOtherPossibleUsernamesExperiment[] = const char PasswordManager::kOtherPossibleUsernamesExperiment[] =
...@@ -224,6 +228,8 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) { ...@@ -224,6 +228,8 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
scoped_ptr<PasswordFormManager> manager; scoped_ptr<PasswordFormManager> manager;
ScopedVector<PasswordFormManager>::iterator matched_manager_it = ScopedVector<PasswordFormManager>::iterator matched_manager_it =
pending_login_managers_.end(); pending_login_managers_.end();
PasswordFormManager::MatchResultMask current_match_result =
PasswordFormManager::RESULT_NO_MATCH;
// Below, "matching" is in DoesManage-sense and "not ready" in // Below, "matching" is in DoesManage-sense and "not ready" in
// !HasCompletedMatching sense. We keep track of such PasswordFormManager // !HasCompletedMatching sense. We keep track of such PasswordFormManager
// instances for UMA. // instances for UMA.
...@@ -236,8 +242,7 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) { ...@@ -236,8 +242,7 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
if (result == PasswordFormManager::RESULT_NO_MATCH) if (result == PasswordFormManager::RESULT_NO_MATCH)
continue; continue;
if ((*iter)->IsIgnorableChangePasswordForm(form.username_value, if ((*iter)->IsIgnorableChangePasswordForm(form)) {
form.password_value)) {
if (logger) if (logger)
logger->LogMessage(Logger::STRING_CHANGE_PASSWORD_FORM); logger->LogMessage(Logger::STRING_CHANGE_PASSWORD_FORM);
continue; continue;
...@@ -256,7 +261,8 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) { ...@@ -256,7 +261,8 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
matched_manager_it = iter; matched_manager_it = iter;
break; break;
} else if (result == (PasswordFormManager::RESULT_COMPLETE_MATCH & } else if (result == (PasswordFormManager::RESULT_COMPLETE_MATCH &
~PasswordFormManager::RESULT_ACTION_MATCH)) { ~PasswordFormManager::RESULT_ACTION_MATCH) &&
result > current_match_result) {
// If the current manager matches the submitted form excluding the action // If the current manager matches the submitted form excluding the action
// URL, remember it as a candidate and continue searching for an exact // URL, remember it as a candidate and continue searching for an exact
// match. See http://crbug.com/27246 for an example where actions can // match. See http://crbug.com/27246 for an example where actions can
...@@ -264,6 +270,20 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) { ...@@ -264,6 +270,20 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
if (logger) if (logger)
logger->LogMessage(Logger::STRING_MATCH_WITHOUT_ACTION); logger->LogMessage(Logger::STRING_MATCH_WITHOUT_ACTION);
matched_manager_it = iter; matched_manager_it = iter;
current_match_result = result;
} else if (IsSignupForm(form) && result > current_match_result) {
// Signup forms don't require HTML attributes to match because we don't
// need to fill these saved passwords on the same form in the future.
// Prefer the best possible match (e.g. action and origins match instead
// or just origin matching). Don't break in case there exists a better
// match.
// TODO(gcasto): Matching in this way is very imprecise. Having some
// better way to match the same form when the HTML elements change (e.g.
// text element changed to password element) would be useful.
if (logger)
logger->LogMessage(Logger::STRING_ORIGINS_MATCH);
matched_manager_it = iter;
current_match_result = result;
} }
} }
// If we didn't find a manager, this means a form was submitted without // If we didn't find a manager, this means a form was submitted without
......
components/password_manager/core/browser/password_manager_unittest.cc
View file @ 33138a00
...@@ -937,4 +937,104 @@ TEST_F(PasswordManagerTest, InPageNavigation) { ...@@ -937,4 +937,104 @@ TEST_F(PasswordManagerTest, InPageNavigation) {
manager()->OnInPageNavigation(&driver_, form); manager()->OnInPageNavigation(&driver_, form);
} }
TEST_F(PasswordManagerTest, SavingSignupForms_NoHTMLMatch) {
// Signup forms don't require HTML attributes match in order to save.
// Verify that we prefer a better match (action + origin vs. origin).
std::vector<PasswordForm> observed;
PasswordForm expected_form;
PasswordForm form(MakeSimpleForm());
observed.push_back(form);
form.action = GURL("http://www.google.com/other/action");
observed.push_back(form);
expected_form = form;
// The initial load.
manager()->OnPasswordFormsParsed(&driver_, observed);
// The initial layout.
manager()->OnPasswordFormsRendered(&driver_, observed, true);
// Simulate either form changing or heuristics choosing other fields
// after the user has entered their information.
form.new_password_element = ASCIIToUTF16("new_password");
form.new_password_value = form.password_value;
form.password_element.clear();
form.password_value.clear();
// Saved signup forms don't have password set.
expected_form.password_element.clear();
manager()->ProvisionallySavePassword(form);
scoped_ptr<PasswordFormManager> form_to_save;
EXPECT_CALL(client_,
PromptUserToSavePasswordPtr(
_, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
.WillOnce(WithArg<0>(SaveToScopedPtr(&form_to_save)));
// Now the password manager waits for the navigation to complete.
observed.clear();
manager()->OnPasswordFormsParsed(&driver_,
observed); // The post-navigation load.
manager()->OnPasswordFormsRendered(&driver_, observed,
true); // The post-navigation layout.
ASSERT_TRUE(form_to_save);
EXPECT_CALL(*store_, AddLogin(FormMatches(expected_form)));
// Simulate saving the form, as if the info bar was accepted.
form_to_save->Save();
}
TEST_F(PasswordManagerTest, SavingSignupForms_NoActionMatch) {
// Signup forms don't require HTML attributes match in order to save.
// Verify that we prefer a better match (HTML attributes + origin vs. origin).
std::vector<PasswordForm> observed;
PasswordForm expected_form;
PasswordForm form(MakeSimpleForm());
observed.push_back(form);
// Change the submit element so we can track which of the two forms is
// chosen as a better match.
form.submit_element = ASCIIToUTF16("different_signin");
expected_form = form;
form.new_password_element = ASCIIToUTF16("new_password");
form.new_password_value = form.password_value;
form.password_element.clear();
form.password_value.clear();
observed.push_back(form);
// The initial load.
manager()->OnPasswordFormsParsed(&driver_, observed);
// The initial layout.
manager()->OnPasswordFormsRendered(&driver_, observed, true);
// Simulate form changing it's action. Update expectation as well since
// the action is copied during saving.
form.action = GURL("http://www.google.com/other/action");
expected_form.action = form.action;
// Signup forms clear password element when saving.
expected_form.password_element.clear();
manager()->ProvisionallySavePassword(form);
scoped_ptr<PasswordFormManager> form_to_save;
EXPECT_CALL(client_,
PromptUserToSavePasswordPtr(
_, CredentialSourceType::CREDENTIAL_SOURCE_PASSWORD_MANAGER))
.WillOnce(WithArg<0>(SaveToScopedPtr(&form_to_save)));
// Now the password manager waits for the navigation to complete.
observed.clear();
manager()->OnPasswordFormsParsed(&driver_,
observed); // The post-navigation load.
manager()->OnPasswordFormsRendered(&driver_, observed,
true); // The post-navigation layout.
ASSERT_TRUE(form_to_save);
EXPECT_CALL(*store_, AddLogin(FormMatches(expected_form)));
// Simulate saving the form, as if the info bar was accepted.
form_to_save->Save();
}
} // namespace password_manager } // namespace password_manager
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment