[AF] Stricter Validity Logic

Change the address validation logic according to this doc:
(Solution 6)
go/autofill-validation-dilemma

Bug: 899251
Change-Id: I93435025754106d5c61889097690288d812f5f0c
Reviewed-on: https://chromium-review.googlesource.com/c/1301797
Commit-Queue: Parastoo Geranmayeh <parastoog@google.com>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603512}

components/autofill/core/browser/autofill_profile_validation_util.cc

@@ -4,6 +4,7 @@
 
 #include "components/autofill/core/browser/autofill_profile_validation_util.h"
 
+#include <string>
 #include <utility>
 
 #include "base/i18n/case_conversion.h"
@@ -125,6 +126,41 @@ void SetEmptyValidityIfEmpty(AutofillProfile* profile) {
                               AutofillProfile::CLIENT);
 }
 
+void SetInvalidIfUnvalidated(AutofillProfile* profile) {
+  if (profile->GetValidityState(ADDRESS_HOME_COUNTRY,
+                                AutofillProfile::CLIENT) ==
+      AutofillProfile::UNVALIDATED) {
+    profile->SetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::INVALID,
+                              AutofillProfile::CLIENT);
+  }
+
+  if (profile->GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT) ==
+      AutofillProfile::UNVALIDATED) {
+    profile->SetValidityState(ADDRESS_HOME_STATE, AutofillProfile::INVALID,
+                              AutofillProfile::CLIENT);
+  }
+
+  if (profile->GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT) ==
+      AutofillProfile::UNVALIDATED) {
+    profile->SetValidityState(ADDRESS_HOME_CITY, AutofillProfile::INVALID,
+                              AutofillProfile::CLIENT);
+  }
+
+  if (profile->GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
+                                AutofillProfile::CLIENT) ==
+      AutofillProfile::UNVALIDATED) {
+    profile->SetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
+                              AutofillProfile::INVALID,
+                              AutofillProfile::CLIENT);
+  }
+
+  if (profile->GetValidityState(ADDRESS_HOME_ZIP, AutofillProfile::CLIENT) ==
+      AutofillProfile::UNVALIDATED) {
+    profile->SetValidityState(ADDRESS_HOME_ZIP, AutofillProfile::INVALID,
+                              AutofillProfile::CLIENT);
+  }
+}
+
 void MaybeApplyValidToFields(AutofillProfile* profile) {
   // The metadata works from top to bottom. Therefore, a so far UNVALIDATED
   // subregion can only be validated if its super-region is VALID. In  this
@@ -162,6 +198,29 @@ void MaybeApplyValidToFields(AutofillProfile* profile) {
   }
 }
 
+void ApplyValidOnlyIfAllChildrenNotInvalid(AutofillProfile* profile) {
+  if (profile->GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT) ==
+          AutofillProfile::INVALID &&
+      profile->GetValidityState(ADDRESS_HOME_ZIP, AutofillProfile::CLIENT) ==
+          AutofillProfile::INVALID) {
+    profile->SetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::INVALID,
+                              AutofillProfile::CLIENT);
+  }
+
+  if (profile->GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT) ==
+      AutofillProfile::INVALID) {
+    profile->SetValidityState(ADDRESS_HOME_STATE, AutofillProfile::INVALID,
+                              AutofillProfile::CLIENT);
+  }
+
+  if (profile->GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
+                                AutofillProfile::CLIENT) ==
+      AutofillProfile::INVALID) {
+    profile->SetValidityState(ADDRESS_HOME_CITY, AutofillProfile::INVALID,
+                              AutofillProfile::CLIENT);
+  }
+}
+
 }  // namespace
 
 namespace profile_validation_util {
@@ -170,13 +229,13 @@ void ValidateProfile(AutofillProfile* profile,
                      AddressValidator* address_validator) {
   DCHECK(address_validator);
   DCHECK(profile);
-  ValidateAddress(profile, address_validator);
+  ValidateAddressStrictly(profile, address_validator);
   ValidatePhoneNumber(profile);
   ValidateEmailAddress(profile);
 }
 
-void ValidateAddress(AutofillProfile* profile,
-                     AddressValidator* address_validator) {
+AddressValidator::Status ValidateAddress(AutofillProfile* profile,
+                                         AddressValidator* address_validator) {
   DCHECK(address_validator);
   DCHECK(profile);
 
@@ -190,7 +249,7 @@ void ValidateAddress(AutofillProfile* profile,
     // unclear which, if any, rule should apply.
     SetValidityStateForAddressField(profile, COUNTRY, AutofillProfile::INVALID);
     SetEmptyValidityIfEmpty(profile);
-    return;
+    return AddressValidator::SUCCESS;
   }
 
   // The COUNTRY was already listed in the CountryDataMap, therefore it's valid.
@@ -212,6 +271,31 @@ void ValidateAddress(AutofillProfile* profile,
   // Fields (except COUNTRY) could be VALID, only if the rules were available.
   if (status == AddressValidator::SUCCESS)
     MaybeApplyValidToFields(profile);
+
+  return status;
+}
+
+void ValidateAddressStrictly(AutofillProfile* profile,
+                             AddressValidator* address_validator) {
+  DCHECK(address_validator);
+  DCHECK(profile);
+
+  // If the rules were loaded successfully, add a second layer of validation:
+  // 1. For a field to stay valid after the first run, all the fields that
+  // depend on that field for validation need to not be invalid on the first
+  // run, otherwise there is a chance that the data on that field was also
+  // invalid (incorrect.)
+  // Example: 1225 Notre-Dame Ouest, Montreal, Quebec, H3C 2A3, United States.
+  // A human validator can see that the country is most probably the invalid
+  // field. The first step helps us validate the rules interdependently.
+  // 2. All the address fields that could not be validated (UNVALIDATED),
+  // should be considered as invalid.
+
+  if (ValidateAddress(profile, address_validator) ==
+      AddressValidator::SUCCESS) {
+    ApplyValidOnlyIfAllChildrenNotInvalid(profile);
+    SetInvalidIfUnvalidated(profile);
+  }
 }
 
 void ValidateEmailAddress(AutofillProfile* profile) {

components/autofill/core/browser/autofill_profile_validation_util.h

@@ -16,8 +16,14 @@ void ValidateProfile(AutofillProfile* profile,
                      AddressValidator* address_validator);
 
 // Sets the validity state of the address fields of the |profile|.
-void ValidateAddress(AutofillProfile* profile,
-                     AddressValidator* address_validator);
+AddressValidator::Status ValidateAddress(AutofillProfile* profile,
+                                         AddressValidator* address_validator);
+
+// Sets the validity state of the address fields of the |profile| in two passes.
+// First runs the ValidateAddress, then adds a second layer of validation based
+// on the results.
+void ValidateAddressStrictly(AutofillProfile* profile,
+                             AddressValidator* address_validator);
 
 // Sets the validity state of the phone number field of the |profile|.
 void ValidatePhoneNumber(AutofillProfile* profile);

components/autofill/core/browser/autofill_profile_validation_util_unittest.cc

@@ -54,7 +54,7 @@ class AutofillProfileValidationUtilTest : public testing::Test,
   }
 
   void ValidateAddressTest(AutofillProfile* profile) {
-    profile_validation_util::ValidateAddress(profile, validator_.get());
+    profile_validation_util::ValidateAddressStrictly(profile, validator_.get());
   }
 
   void ValidatePhoneTest(AutofillProfile* profile) {
@@ -115,17 +115,19 @@ TEST_F(AutofillProfileValidationUtilTest,
   EXPECT_EQ(
       AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::CLIENT));
+  // The zip, the state and the city can't be validated, because we don't know
+  // the country, in the strict validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::EMPTY,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
                                      AutofillProfile::CLIENT));
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_ZIP, AutofillProfile::CLIENT));
 }
 
@@ -139,17 +141,19 @@ TEST_F(AutofillProfileValidationUtilTest,
   EXPECT_EQ(
       AutofillProfile::EMPTY,
       profile.GetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::CLIENT));
+  // The zip, the state and the city can't be validated, because we don't know
+  // the country, in the strict validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::EMPTY,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
                                      AutofillProfile::CLIENT));
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_ZIP, AutofillProfile::CLIENT));
 }
 
@@ -190,8 +194,10 @@ TEST_F(AutofillProfileValidationUtilTest, ValidateAddress_AdminAreaNotExists) {
   EXPECT_EQ(
       AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
+  // The city can't be validated, because we don't know the state, in the strict
+  // validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::EMPTY,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
@@ -212,8 +218,10 @@ TEST_F(AutofillProfileValidationUtilTest, ValidateAddress_EmptyAdminArea) {
   EXPECT_EQ(
       AutofillProfile::EMPTY,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
+  // The city can't be validated, because we don't know the state, in the strict
+  // validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::EMPTY,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
@@ -499,13 +507,17 @@ TEST_F(AutofillProfileValidationUtilTest,
   EXPECT_EQ(
       AutofillProfile::VALID,
       profile.GetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::CLIENT));
+  // The city which is the only dependent field on state is invalid, in the
+  // strict validation the state would also be considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::VALID,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
   EXPECT_EQ(
       AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
-  EXPECT_EQ(AutofillProfile::UNVALIDATED,
+  // The dependent locality can't be validated, because we don't know the city,
+  // in the strict validation this is considered as invalid.
+  EXPECT_EQ(AutofillProfile::INVALID,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
                                      AutofillProfile::CLIENT));
   EXPECT_EQ(
@@ -526,13 +538,17 @@ TEST_F(AutofillProfileValidationUtilTest,
   EXPECT_EQ(
       AutofillProfile::VALID,
       profile.GetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::CLIENT));
+  // The city which is the only dependent field on state is invalid, in the
+  // strict validation the state would also be considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::VALID,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
   EXPECT_EQ(
       AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
-  EXPECT_EQ(AutofillProfile::UNVALIDATED,
+  // The dependent locality can't be validated, because we don't know the city,
+  // in the strict validation this is considered as invalid.
+  EXPECT_EQ(AutofillProfile::INVALID,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
                                      AutofillProfile::CLIENT));
   EXPECT_EQ(
@@ -609,8 +625,10 @@ TEST_F(AutofillProfileValidationUtilTest,
   EXPECT_EQ(
       AutofillProfile::VALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
+  // The dependent locality which is the only dependent field on city is
+  // invalid, in the strict validation the city would also be invalid.
   EXPECT_EQ(
-      AutofillProfile::VALID,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::INVALID,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
@@ -636,8 +654,10 @@ TEST_F(AutofillProfileValidationUtilTest,
   EXPECT_EQ(
       AutofillProfile::VALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
+  // The only that depend on city (dependent locality) is invalid,
+  //  in the strict validation city would also be considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::VALID,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::INVALID,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
@@ -946,16 +966,19 @@ TEST_F(AutofillProfileValidationUtilTest,
   profile.SetRawInfo(ADDRESS_HOME_COUNTRY, base::ASCIIToUTF16("CN"));
 
   ValidateProfileTest(&profile);
-  // The country is validated independently, so it's considered as valid.
+  // The fields that depend on country (state and zip) are both invalid,
+  // therefore in the strict validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::VALID,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::CLIENT));
   // The state is not a Chinese state, so it's considered as invalid.
   EXPECT_EQ(
       AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
+  // The city can't be validated, because the state value is not
+  // valid, in the strict validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
 
   EXPECT_EQ(AutofillProfile::EMPTY,
@@ -985,17 +1008,19 @@ TEST_F(AutofillProfileValidationUtilTest,
 
   ValidateProfileTest(&profile);
 
-  // The country is validated independently, so it's considered as valid.
+  // The fields that depend on Country (state and zip) are both invalid,
+  // therefore in the strict validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::VALID,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::CLIENT));
   // The state is not a Canadian state, so it's considered as invalid.
   EXPECT_EQ(
       AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
-  // We can't validate city, because state is not valid.
+  // We can't validate city, because state is not valid, in the strict
+  // validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
   // The dependent locality is not a Canadian field, so it's considered as
   // invalid.
@@ -1030,17 +1055,18 @@ TEST_F(AutofillProfileValidationUtilTest,
   EXPECT_EQ(
       AutofillProfile::VALID,
       profile.GetValidityState(ADDRESS_HOME_COUNTRY, AutofillProfile::CLIENT));
-  // Considered as valid because of the top to bottom approach.
+  // The only field that depends on state (city) is invalid, in the strict
+  // validation this makes state also invalid.
   EXPECT_EQ(
-      AutofillProfile::VALID,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
   // The city is in another province.
   EXPECT_EQ(
       AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
   // The dependent locality can't be validated, because the city value is not
-  // valid.
-  EXPECT_EQ(AutofillProfile::UNVALIDATED,
+  // valid, in the strict validation this is considered as invalid.
+  EXPECT_EQ(AutofillProfile::INVALID,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
                                      AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::UNSUPPORTED,
@@ -1058,7 +1084,7 @@ TEST_F(AutofillProfileValidationUtilTest,
 
 TEST_F(AutofillProfileValidationUtilTest,
        ValidateProfile_TopToBottomValidationChina_StateMissing) {
-  // This is a full valid profile, with the wrong province:
+  // This is a full valid profile, with the empty province:
   // Address Address: "100 Century Avenue",
   // District: "赫章县", City: "毕节地区", Province: "",
   // Postal Code: "200120", Country Code: "CN",
@@ -1073,12 +1099,14 @@ TEST_F(AutofillProfileValidationUtilTest,
   EXPECT_EQ(
       AutofillProfile::EMPTY,
       profile.GetValidityState(ADDRESS_HOME_STATE, AutofillProfile::CLIENT));
-  // City can't be validated, because the state is missing.
+  // City can't be validated, because the state is missing, in the strict
+  // validation this is considered as invalid.
   EXPECT_EQ(
-      AutofillProfile::UNVALIDATED,
+      AutofillProfile::INVALID,
       profile.GetValidityState(ADDRESS_HOME_CITY, AutofillProfile::CLIENT));
-  // The dependent locality can't be validated, because we don't know the city.
-  EXPECT_EQ(AutofillProfile::UNVALIDATED,
+  // The dependent locality can't be validated, because we don't know the city,
+  // in the strict validation this is considered as invalid.
+  EXPECT_EQ(AutofillProfile::INVALID,
             profile.GetValidityState(ADDRESS_HOME_DEPENDENT_LOCALITY,
                                      AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::UNSUPPORTED,

components/autofill/core/browser/autofill_profile_validator_unittest.cc

@@ -165,8 +165,8 @@ TEST_F(AutofillProfileValidatorTest,
 
   // Set up the test expectations.
   expected_validity_ = {{ADDRESS_HOME_COUNTRY, AutofillProfile::INVALID},
-                        {ADDRESS_HOME_STATE, AutofillProfile::UNVALIDATED},
-                        {ADDRESS_HOME_ZIP, AutofillProfile::UNVALIDATED},
+                        {ADDRESS_HOME_STATE, AutofillProfile::INVALID},
+                        {ADDRESS_HOME_ZIP, AutofillProfile::INVALID},
                         {PHONE_HOME_WHOLE_NUMBER, AutofillProfile::UNVALIDATED},
                         {EMAIL_ADDRESS, AutofillProfile::VALID}};
 
@@ -205,8 +205,8 @@ TEST_F(AutofillProfileValidatorTest, ValidateAddress_EmptyCountryCode) {
   // Set up the test expectations.
   // The phone is validated for the US.
   expected_validity_ = {{ADDRESS_HOME_COUNTRY, AutofillProfile::EMPTY},
-                        {ADDRESS_HOME_STATE, AutofillProfile::UNVALIDATED},
-                        {ADDRESS_HOME_ZIP, AutofillProfile::UNVALIDATED},
+                        {ADDRESS_HOME_STATE, AutofillProfile::INVALID},
+                        {ADDRESS_HOME_ZIP, AutofillProfile::INVALID},
                         {PHONE_HOME_WHOLE_NUMBER, AutofillProfile::UNVALIDATED},
                         {EMAIL_ADDRESS, AutofillProfile::VALID}};
 

components/autofill/core/browser/personal_data_manager_unittest.cc

@@ -6752,7 +6752,7 @@ TEST_F(PersonalDataManagerTest, UpdateClientValidityStates) {
   EXPECT_EQ(
       AutofillProfile::VALID,
       profiles[2]->GetValidityState(ADDRESS_HOME_ZIP, AutofillProfile::CLIENT));
-  EXPECT_EQ(AutofillProfile::UNVALIDATED,
+  EXPECT_EQ(AutofillProfile::INVALID,
             profiles[2]->GetValidityState(ADDRESS_HOME_CITY,
                                           AutofillProfile::CLIENT));
   EXPECT_EQ(AutofillProfile::EMPTY,