Skip to content

GitLab

T
tangled
Commit 9c4e31f4 authored by Vaclav Brozek's avatar Vaclav Brozek Committed by Commit Bot
Browse files
Options

Teach form_parser.cc other_possible_usernames 

When Chrome tries to figure out which text field of a password form
is the username, it picks one and stores the rest in
PasswordForm::other_possible_usernames.

This field has so far only been populated by the old form parser
(password_form_conversion_utils.cc). This CL also teaches the new
parser to populate that field.

However, there is a slight change: the new parser will include also
the username which Chrome picked, for simplicity of the processing of
the information later. The change in meaning is not breaking, because
the output of the old and the new parser is consumed by different
versions of PasswordFormManager. https://crbug.com/881346 tracks
renaming of the other_possible_usernames field once there is only one
meaning to it.

Bug: 880721
Change-Id: I114472dc8d1b37a8f77bdb79689008e7795d81a4
Reviewed-on: https://chromium-review.googlesource.com/1210503
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: default avatarVadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589301}
parent 04da4d99
Show whitespace changes
Inline Side-by-side
Showing with 67 additions and 25 deletions
components/password_manager/core/browser/form_parsing/form_parser.cc
View file @ 9c4e31f4
......@@ -546,10 +546,12 @@ void SetFields(const SignificantFields& significant_fields,
// parsing and wraps that in a ProcessedField. Returns the vector of all those
// ProcessedField instances, or an empty vector if there was not a single
// password field. Also, computes the vector of all password values and
// associated element names in |all_possible_passwords|.
// associated element names in |all_possible_passwords|, and similarly for
// usernames and |all_possible_usernames|.
std::vector<ProcessedField> ProcessFields(
const std::vector<FormFieldData>& fields,
autofill::ValueElementVector* all_possible_passwords) {
autofill::ValueElementVector* all_possible_passwords,
autofill::ValueElementVector* all_possible_usernames) {
DCHECK(all_possible_passwords);
DCHECK(all_possible_passwords->empty());
......@@ -558,24 +560,28 @@ std::vector<ProcessedField> ProcessFields(
result.reserve(fields.size());
// |all_possible_passwords| should only contain each value once. |seen_values|
// ensures that duplicates are ignored.
std::set<base::StringPiece16> seen_values;
// Pretend that an empty value has been already seen, so that empty-valued
// password elements won't get added to |all_possible_passwords|.
seen_values.insert(base::StringPiece16());
// |all_possible_passwords| should only contain each value once.
// |seen_password_values| ensures that duplicates are ignored.
std::set<base::StringPiece16> seen_password_values;
// Similarly for usernames.
std::set<base::StringPiece16> seen_username_values;
for (const FormFieldData& field : fields) {
if (!field.IsTextInputElement())
continue;
const bool is_password = field.form_control_type == "password";
if (is_password) {
// Only the field name of the first occurrence is added to
// |all_possible_passwords|.
if (!field.value.empty()) {
std::set<base::StringPiece16>& seen_values =
is_password ? seen_password_values : seen_username_values;
autofill::ValueElementVector* all_possible_fields =
is_password ? all_possible_passwords : all_possible_usernames;
// Only the field name of the first occurrence is added.
auto insertion = seen_values.insert(base::StringPiece16(field.value));
if (insertion.second) // There was no such element in |seen_values|.
all_possible_passwords->push_back({field.value, field.name});
if (insertion.second) {
// There was no such element in |seen_values|.
all_possible_fields->push_back({field.value, field.name});
}
}
const AutocompleteFlag flag =
......@@ -641,14 +647,17 @@ bool GetMayUsePrefilledPlaceholder(
// Puts together a PasswordForm, the result of the parsing, based on the
// |form_data| description of the form metadata (e.g., action), the already
// parsed information about what are the |significant_fields|, and the list
// |all_possible_passwords| of all non-empty password values and associated
// element names which occurred in the form. |form_predictions| is used to find
// fields that may have preffilled placeholders.
// parsed information about what are the |significant_fields|, the list
// |all_possible_passwords| of all non-empty password values which occurred in
// the form and their associated element names, and the list
// |all_possible_usernames| of all non-empty username values which
// occurred in the form and their associated elements. |form_predictions| is
// used to find fields that may have preffilled placeholders.
std::unique_ptr<PasswordForm> AssemblePasswordForm(
const autofill::FormData& form_data,
const SignificantFields* significant_fields,
autofill::ValueElementVector all_possible_passwords,
autofill::ValueElementVector all_possible_usernames,
const FormPredictions* form_predictions) {
if (!significant_fields || !significant_fields->HasPasswords())
return nullptr;
......@@ -660,6 +669,9 @@ std::unique_ptr<PasswordForm> AssemblePasswordForm(
result->action = form_data.action;
result->form_data = form_data;
result->all_possible_passwords = std::move(all_possible_passwords);
// TODO(crbug.com/881346) Rename PasswordForm::other_possible_usernames to
// all_possible_usernames once the old parser is gone.
result->other_possible_usernames = std::move(all_possible_usernames);
result->scheme = PasswordForm::SCHEME_HTML;
result->preferred = false;
result->blacklisted_by_user = false;
......@@ -679,8 +691,9 @@ std::unique_ptr<PasswordForm> ParseFormData(
const FormPredictions* form_predictions,
FormParsingMode mode) {
autofill::ValueElementVector all_possible_passwords;
std::vector<ProcessedField> processed_fields =
ProcessFields(form_data.fields, &all_possible_passwords);
autofill::ValueElementVector all_possible_usernames;
std::vector<ProcessedField> processed_fields = ProcessFields(
form_data.fields, &all_possible_passwords, &all_possible_usernames);
if (processed_fields.empty())
return nullptr;
......@@ -751,9 +764,9 @@ std::unique_ptr<PasswordForm> ParseFormData(
username_detection_method,
UsernameDetectionMethod::kCount);
return AssemblePasswordForm(form_data, significant_fields.get(),
std::move(all_possible_passwords),
form_predictions);
return AssemblePasswordForm(
form_data, significant_fields.get(), std::move(all_possible_passwords),
std::move(all_possible_usernames), form_predictions);
}
} // namespace password_manager
components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
View file @ 9c4e31f4
......@@ -79,8 +79,10 @@ struct FormParsingTestCase {
std::vector<FieldDataDescription> fields;
// -1 just mean no checking.
int number_of_all_possible_passwords = -1;
int number_of_all_possible_usernames = -1;
// null means no checking
const autofill::ValueElementVector* all_possible_passwords = nullptr;
const autofill::ValueElementVector* all_possible_usernames = nullptr;
bool username_may_use_prefilled_placeholder = false;
};
......@@ -326,6 +328,7 @@ void CheckTestData(const std::vector<FormParsingTestCase>& test_cases) {
parsed_form->username_may_use_prefilled_placeholder);
CheckPasswordFormFields(*parsed_form, form_data, expected_ids);
CheckAllValuesUnique(parsed_form->all_possible_passwords);
CheckAllValuesUnique(parsed_form->other_possible_usernames);
if (test_case.number_of_all_possible_passwords >= 0) {
EXPECT_EQ(
static_cast<size_t>(test_case.number_of_all_possible_passwords),
......@@ -335,6 +338,15 @@ void CheckTestData(const std::vector<FormParsingTestCase>& test_cases) {
EXPECT_EQ(*test_case.all_possible_passwords,
parsed_form->all_possible_passwords);
}
if (test_case.number_of_all_possible_usernames >= 0) {
EXPECT_EQ(
static_cast<size_t>(test_case.number_of_all_possible_usernames),
parsed_form->other_possible_usernames.size());
}
if (test_case.all_possible_usernames) {
EXPECT_EQ(*test_case.all_possible_usernames,
parsed_form->other_possible_usernames);
}
}
}
}
......@@ -352,6 +364,7 @@ TEST(FormParserTest, NotPasswordForm) {
{.form_control_type = "text"}, {.form_control_type = "text"},
},
.number_of_all_possible_passwords = 0,
.number_of_all_possible_usernames = 0,
},
});
}
......@@ -359,13 +372,15 @@ TEST(FormParserTest, NotPasswordForm) {
TEST(FormParserTest, SkipNotTextFields) {
CheckTestData({
{
"Select between username and password fields",
"A 'select' between username and password fields",
{
{.role = ElementRole::USERNAME},
{.form_control_type = "select"},
{.role = ElementRole::CURRENT_PASSWORD,
.form_control_type = "password"},
},
.number_of_all_possible_passwords = 1,
.number_of_all_possible_usernames = 1,
},
});
}
......@@ -380,6 +395,7 @@ TEST(FormParserTest, OnlyPasswordFields) {
.form_control_type = "password"},
},
.number_of_all_possible_passwords = 1,
.number_of_all_possible_usernames = 0,
},
{
"2 password fields, new and confirmation password",
......@@ -508,6 +524,7 @@ TEST(FormParserTest, TestFocusability) {
.form_control_type = "password",
.is_focusable = true},
},
.number_of_all_possible_usernames = 2,
},
{
"focusable and non-focusable text fields before password",
......@@ -564,6 +581,9 @@ TEST(FormParserTest, TextAndPasswordFields) {
.form_control_type = "password",
.value = ""},
},
// all_possible_* only count fields with non-empty values.
.number_of_all_possible_passwords = 0,
.number_of_all_possible_usernames = 0,
},
{
.description_for_logging = "Simple sign-in form with filled data",
......@@ -696,6 +716,7 @@ TEST(FormParserTest, TestAutocomplete) {
.autocomplete_attribute = "password"},
},
.number_of_all_possible_passwords = 3,
.number_of_all_possible_usernames = 2,
},
{
"Basic heuristics kick in if autocomplete analysis fails",
......@@ -1095,6 +1116,10 @@ TEST(FormParserTest, AllPossiblePasswords) {
{ASCIIToUTF16("a"), ASCIIToUTF16("p1")},
{ASCIIToUTF16("b"), ASCIIToUTF16("p3")},
};
const autofill::ValueElementVector kUsernames = {
{ASCIIToUTF16("b"), ASCIIToUTF16("chosen")},
{ASCIIToUTF16("a"), ASCIIToUTF16("first")},
};
CheckTestData({
{
.description_for_logging = "It is always the first field name which "
......@@ -1105,18 +1130,22 @@ TEST(FormParserTest, AllPossiblePasswords) {
{.form_control_type = "password", .name = "p1", .value = "a"},
{.role = ElementRole::USERNAME,
.form_control_type = "text",
.name = "chosen",
.value = "b",
.autocomplete_attribute = "username"},
{.role = ElementRole::CURRENT_PASSWORD,
.form_control_type = "password",
.autocomplete_attribute = "current-password",
.value = "a"},
{.form_control_type = "text"},
{.form_control_type = "text"},
{.form_control_type = "text", .name = "first", .value = "a"},
{.form_control_type = "text", .value = "a"},
{.form_control_type = "password", .name = "p3", .value = "b"},
{.form_control_type = "password", .value = "b"},
},
.number_of_all_possible_passwords = 2,
.all_possible_passwords = &kPasswords,
.number_of_all_possible_usernames = 2,
.all_possible_usernames = &kUsernames,
},
{
.description_for_logging =
......
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