[AF] Skip fields with invalid data
When filling/previewing the form, leave the fields with invalid data
empty when the relevant feature is enabled.
Bug: 899256
Change-Id: I3bb5ba264395ddccbc8d916c4f8fd14aefa8b0fb
Reviewed-on: https://chromium-review.googlesource.com/c/1450533
Commit-Queue: Parastoo Geranmayeh <parastoog@google.com>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629211}diff --git a/components/autofill/core/browser/field_filler.cc b/components/autofill/core/browser/field_filler.cc
index e1e56eb0..8e4270d 100644
--- a/components/autofill/core/browser/field_filler.cc
+++ b/components/autofill/core/browser/field_filler.cc
@@ -624,6 +624,7 @@
FormFieldData* field_data,
const base::string16& cvc) {
const AutofillType type = field.Type();
+
// Don't fill if autocomplete=off is set on |field| on desktop for non credit
// card related fields.
if (!base::FeatureList::IsEnabled(features::kAutofillAlwaysFillAddresses) &&
@@ -632,6 +633,25 @@
return false;
}
+ // Check for the validity of the data. Leave the field empty if the data is
+ // invalid and the relevant feature is enabled.
+ bool use_server_validation = base::FeatureList::IsEnabled(
+ autofill::features::kAutofillProfileServerValidation);
+ bool use_client_validation = base::FeatureList::IsEnabled(
+ autofill::features::kAutofillProfileClientValidation);
+ ServerFieldType server_field_type = type.GetStorableType();
+
+ if ((use_client_validation &&
+ data_model.GetValidityState(server_field_type,
+ AutofillProfile::CLIENT) ==
+ AutofillProfile::INVALID) ||
+ (use_server_validation &&
+ data_model.GetValidityState(server_field_type,
+ AutofillProfile::SERVER) ==
+ AutofillProfile::INVALID)) {
+ return false;
+ }
+
base::string16 value = data_model.GetInfo(type, app_locale_);
if (type.GetStorableType() == CREDIT_CARD_VERIFICATION_CODE)
value = cvc;
diff --git a/components/autofill/core/browser/field_filler_unittest.cc b/components/autofill/core/browser/field_filler_unittest.cc
index eb2f6986..3bed68d2 100644
--- a/components/autofill/core/browser/field_filler_unittest.cc
+++ b/components/autofill/core/browser/field_filler_unittest.cc
@@ -110,6 +110,29 @@
EXPECT_EQ(ASCIIToUTF16("Nov"), field.option_contents[content_index]);
}
+void TestFillingInvalidFields(const base::string16& state,
+ const base::string16& city) {
+ AutofillProfile profile = test::GetFullProfile();
+ profile.SetValidityState(ADDRESS_HOME_STATE, AutofillProfile::INVALID,
+ AutofillProfile::SERVER);
+ profile.SetValidityState(ADDRESS_HOME_CITY, AutofillProfile::INVALID,
+ AutofillProfile::CLIENT);
+
+ AutofillField field_state;
+ field_state.set_heuristic_type(ADDRESS_HOME_STATE);
+ AutofillField field_city;
+ field_city.set_heuristic_type(ADDRESS_HOME_CITY);
+
+ FieldFiller filler(/*app_locale=*/"en-US", /*address_normalizer=*/nullptr);
+ filler.FillFormField(field_state, profile, &field_state,
+ /*cvc=*/base::string16());
+ EXPECT_EQ(state, field_state.value);
+
+ filler.FillFormField(field_city, profile, &field_city,
+ /*cvc=*/base::string16());
+ EXPECT_EQ(city, field_city.value);
+}
+
struct CreditCardTestCase {
std::string card_number_;
size_t total_splits_;
@@ -388,6 +411,50 @@
EXPECT_EQ(ASCIIToUTF16("4111111111111111"), field.value);
}
+// Verify that when the relevant feature is enabled, the invalid fields don't
+// get filled.
+TEST_F(AutofillFieldFillerTest, FillFormField_Validity_ServerClient) {
+ base::test::ScopedFeatureList scoped_features;
+ scoped_features.InitWithFeatures(
+ /*enabled_features=*/{features::kAutofillProfileServerValidation,
+ features::kAutofillProfileClientValidation},
+ /*disabled_features=*/{});
+ // State's validity is set by server and city's validity by client.
+ TestFillingInvalidFields(/*state=*/base::string16(),
+ /*city=*/base::string16());
+}
+
+TEST_F(AutofillFieldFillerTest, FillFormField_Validity_OnlyServer) {
+ base::test::ScopedFeatureList scoped_features;
+ scoped_features.InitWithFeatures(
+ /*enabled_features=*/{features::kAutofillProfileServerValidation},
+ /*disabled_features=*/{features::kAutofillProfileClientValidation});
+ // State's validity is set by server and city's validity by client.
+ TestFillingInvalidFields(/*state=*/base::string16(),
+ /*city=*/ASCIIToUTF16("Elysium"));
+}
+
+TEST_F(AutofillFieldFillerTest, FillFormField_Validity_OnlyClient) {
+ base::test::ScopedFeatureList scoped_features;
+ scoped_features.InitWithFeatures(
+ /*enabled_features=*/{features::kAutofillProfileClientValidation},
+ /*disabled_features=*/{features::kAutofillProfileServerValidation});
+ // State's validity is set by server and city's validity by client.
+ TestFillingInvalidFields(/*state=*/ASCIIToUTF16("CA"),
+ /*city=*/base::string16());
+}
+
+TEST_F(AutofillFieldFillerTest, FillFormField_NoValidity) {
+ base::test::ScopedFeatureList scoped_features;
+ scoped_features.InitWithFeatures(
+ /*enabled_features=*/{},
+ /*disabled_features=*/{features::kAutofillProfileServerValidation,
+ features::kAutofillProfileClientValidation});
+ // State's validity is set by server and city's validity by client.
+ TestFillingInvalidFields(/*state=*/ASCIIToUTF16("CA"),
+ /*city=*/ASCIIToUTF16("Elysium"));
+}
+
struct AutofillFieldFillerTestCase {
HtmlFieldType field_type;
size_t field_max_length;
diff --git a/components/autofill/core/browser/personal_data_manager_unittest.cc b/components/autofill/core/browser/personal_data_manager_unittest.cc
index de9efa4d..e9f57feb 100644
--- a/components/autofill/core/browser/personal_data_manager_unittest.cc
+++ b/components/autofill/core/browser/personal_data_manager_unittest.cc
@@ -2303,18 +2303,27 @@
/*enabled_features=*/{features::kAutofillProfileServerValidation,
features::kAutofillProfileClientValidation},
/*disabled_features=*/{features::kAutofillSuppressDisusedAddresses});
- std::vector<Suggestion> suggestions = personal_data_->GetProfileSuggestions(
- AutofillType(EMAIL_ADDRESS), base::string16(), false,
- std::vector<ServerFieldType>());
+ std::vector<Suggestion> email_suggestions =
+ personal_data_->GetProfileSuggestions(AutofillType(EMAIL_ADDRESS),
+ base::string16(), false,
+ std::vector<ServerFieldType>());
for (auto* profile : profiles) {
ASSERT_EQ(profile->guid() == valid_profile.guid(),
profile->IsValidByClient());
ASSERT_TRUE(profile->IsValidByServer());
}
- ASSERT_EQ(2U, suggestions.size());
- EXPECT_EQ(base::ASCIIToUTF16("alice@wonderland.ca"), suggestions[0].value);
- EXPECT_EQ(base::ASCIIToUTF16("invalid email"), suggestions[1].value);
+ ASSERT_EQ(1U, email_suggestions.size());
+ EXPECT_EQ(base::ASCIIToUTF16("alice@wonderland.ca"),
+ email_suggestions[0].value);
+
+ std::vector<Suggestion> name_suggestions =
+ personal_data_->GetProfileSuggestions(AutofillType(NAME_FIRST),
+ base::string16(), false,
+ std::vector<ServerFieldType>());
+ ASSERT_EQ(2U, name_suggestions.size());
+ EXPECT_EQ(base::ASCIIToUTF16("Alice"), name_suggestions[0].value);
+ EXPECT_EQ(base::ASCIIToUTF16("Marion1"), name_suggestions[1].value);
}
// Set the validity state of ADDRESS_HOME_STATE to INVALID on the prefs.
@@ -2324,8 +2333,9 @@
std::string autofill_profile_validity;
personal_data_->pref_service_->SetString(prefs::kAutofillProfileValidity,
autofill_profile_validity);
- (*profile_validity_map.mutable_field_validity_states())[static_cast<int>(
- ADDRESS_HOME_STATE)] = static_cast<int>(AutofillDataModel::INVALID);
+ (*profile_validity_map
+ .mutable_field_validity_states())[static_cast<int>(EMAIL_ADDRESS)] =
+ static_cast<int>(AutofillProfile::INVALID);
(*user_profile_validity_map
.mutable_profile_validity())[invalid_profile.guid()] =
profile_validity_map;
@@ -2343,9 +2353,10 @@
features::kAutofillProfileServerValidation},
/*disabled_features=*/{features::kAutofillSuppressDisusedAddresses});
- std::vector<Suggestion> suggestions = personal_data_->GetProfileSuggestions(
- AutofillType(EMAIL_ADDRESS), base::string16(), false,
- std::vector<ServerFieldType>());
+ std::vector<Suggestion> email_suggestions =
+ personal_data_->GetProfileSuggestions(AutofillType(EMAIL_ADDRESS),
+ base::string16(), false,
+ std::vector<ServerFieldType>());
for (auto* profile : profiles) {
ASSERT_EQ(profile->guid() == valid_profile.guid(),
@@ -2353,9 +2364,17 @@
ASSERT_EQ(profile->guid() == valid_profile.guid(),
profile->IsValidByServer());
}
- ASSERT_EQ(2U, suggestions.size());
- EXPECT_EQ(base::ASCIIToUTF16("alice@wonderland.ca"), suggestions[0].value);
- EXPECT_EQ(base::ASCIIToUTF16("invalid email"), suggestions[1].value);
+ ASSERT_EQ(1U, email_suggestions.size());
+ EXPECT_EQ(base::ASCIIToUTF16("alice@wonderland.ca"),
+ email_suggestions[0].value);
+
+ std::vector<Suggestion> name_suggestions =
+ personal_data_->GetProfileSuggestions(AutofillType(NAME_FIRST),
+ base::string16(), false,
+ std::vector<ServerFieldType>());
+ ASSERT_EQ(2U, name_suggestions.size());
+ EXPECT_EQ(base::ASCIIToUTF16("Alice"), name_suggestions[0].value);
+ EXPECT_EQ(base::ASCIIToUTF16("Marion1"), name_suggestions[1].value);
}
// Invalid based on client, and server. Relying only on the client source.
{
@@ -2364,9 +2383,10 @@
/*enabled_features=*/{features::kAutofillProfileClientValidation},
/*disabled_features=*/{features::kAutofillSuppressDisusedAddresses,
features::kAutofillProfileServerValidation});
- std::vector<Suggestion> suggestions = personal_data_->GetProfileSuggestions(
- AutofillType(EMAIL_ADDRESS), base::string16(), false,
- std::vector<ServerFieldType>());
+ std::vector<Suggestion> email_suggestions =
+ personal_data_->GetProfileSuggestions(AutofillType(EMAIL_ADDRESS),
+ base::string16(), false,
+ std::vector<ServerFieldType>());
for (auto* profile : profiles) {
ASSERT_EQ(profile->guid() == valid_profile.guid(),
@@ -2374,9 +2394,17 @@
ASSERT_EQ(profile->guid() == valid_profile.guid(),
profile->IsValidByServer());
}
- ASSERT_EQ(2U, suggestions.size());
- EXPECT_EQ(base::ASCIIToUTF16("alice@wonderland.ca"), suggestions[0].value);
- EXPECT_EQ(base::ASCIIToUTF16("invalid email"), suggestions[1].value);
+ ASSERT_EQ(1U, email_suggestions.size());
+ EXPECT_EQ(base::ASCIIToUTF16("alice@wonderland.ca"),
+ email_suggestions[0].value);
+
+ std::vector<Suggestion> name_suggestions =
+ personal_data_->GetProfileSuggestions(AutofillType(NAME_FIRST),
+ base::string16(), false,
+ std::vector<ServerFieldType>());
+ ASSERT_EQ(2U, name_suggestions.size());
+ EXPECT_EQ(base::ASCIIToUTF16("Alice"), name_suggestions[0].value);
+ EXPECT_EQ(base::ASCIIToUTF16("Marion1"), name_suggestions[1].value);
}
// Invalid based on client, and server. Relying on server as a validity
// source.
@@ -2386,10 +2414,11 @@
/*enabled_features=*/{features::kAutofillProfileServerValidation},
/*disabled_features=*/{features::kAutofillProfileClientValidation,
features::kAutofillSuppressDisusedAddresses});
-
- std::vector<Suggestion> suggestions = personal_data_->GetProfileSuggestions(
- AutofillType(EMAIL_ADDRESS), base::string16(), false,
- std::vector<ServerFieldType>());
+ LOG(ERROR) << __FUNCTION__;
+ std::vector<Suggestion> email_suggestions =
+ personal_data_->GetProfileSuggestions(AutofillType(EMAIL_ADDRESS),
+ base::string16(), false,
+ std::vector<ServerFieldType>());
for (auto* profile : profiles) {
ASSERT_EQ(profile->guid() == valid_profile.guid(),
@@ -2397,9 +2426,17 @@
ASSERT_EQ(profile->guid() == valid_profile.guid(),
profile->IsValidByServer());
}
- ASSERT_EQ(2U, suggestions.size());
- EXPECT_EQ(base::ASCIIToUTF16("alice@wonderland.ca"), suggestions[0].value);
- EXPECT_EQ(base::ASCIIToUTF16("invalid email"), suggestions[1].value);
+ ASSERT_EQ(1U, email_suggestions.size());
+ EXPECT_EQ(base::ASCIIToUTF16("alice@wonderland.ca"),
+ email_suggestions[0].value);
+
+ std::vector<Suggestion> name_suggestions =
+ personal_data_->GetProfileSuggestions(AutofillType(NAME_FIRST),
+ base::string16(), false,
+ std::vector<ServerFieldType>());
+ ASSERT_EQ(2U, name_suggestions.size());
+ EXPECT_EQ(base::ASCIIToUTF16("Alice"), name_suggestions[0].value);
+ EXPECT_EQ(base::ASCIIToUTF16("Marion1"), name_suggestions[1].value);
}
}
diff --git a/components/autofill/core/browser/suggestion_selection.cc b/components/autofill/core/browser/suggestion_selection.cc
index 354c174..c4f1237 100644
--- a/components/autofill/core/browser/suggestion_selection.cc
+++ b/components/autofill/core/browser/suggestion_selection.cc
@@ -69,6 +69,23 @@
matched_profiles->size() < kMaxSuggestedProfilesCount;
i++) {
AutofillProfile* profile = profiles[i];
+ bool use_server_validation = base::FeatureList::IsEnabled(
+ autofill::features::kAutofillProfileServerValidation);
+ bool use_client_validation = base::FeatureList::IsEnabled(
+ autofill::features::kAutofillProfileClientValidation);
+
+ ServerFieldType server_field_type = type.GetStorableType();
+ if ((use_client_validation &&
+ profile->GetValidityState(server_field_type,
+ AutofillProfile::CLIENT) ==
+ AutofillProfile::INVALID) ||
+ (use_server_validation &&
+ profile->GetValidityState(server_field_type,
+ AutofillProfile::SERVER) ==
+ AutofillProfile::INVALID)) {
+ continue;
+ }
+
base::string16 value =
GetInfoInOneLine(profile, type, comparator.app_locale());
if (value.empty())
