Bug 1839134 - Do not capture address fields whose value are invalid r=mtigley,credential-management-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D181359

browser/extensions/formautofill/test/browser/address/browser.ini

@@ -10,4 +10,5 @@ support-files =
   head_address.js
 
 [browser_address_doorhanger_display.js]
+[browser_address_doorhanger_tel.js]
 [browser_address_telemetry.js]

browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_display.js

@@ -55,7 +55,7 @@ add_task(async function test_save_doorhanger_shown_different_address() {
       await focusUpdateSubmitForm(browser, {
         focusSelector: "#given-name",
         newValues: {
-          "#given-name": TEST_ADDRESS_2["give-name"],
+          "#given-name": TEST_ADDRESS_2["given-name"],
           "#street-address": TEST_ADDRESS_2["street-address"],
           "#country": TEST_ADDRESS_2.country,
         },

browser/extensions/formautofill/test/browser/address/browser_address_doorhanger_tel.js

@@ -0,0 +1,67 @@
+"use strict";
+
+async function expectSavedAddresses(expectedAddresses) {
+  const addresses = await getAddresses();
+  is(
+    addresses.length,
+    expectedAddresses.length,
+    `${addresses.length} address in the storage`
+  );
+
+  for (let i = 0; i < expectedAddresses.length; i++) {
+    for (const [key, value] of Object.entries(expectedAddresses[i])) {
+      is(addresses[i][key] ?? "", value, `field ${key} should be equal`);
+    }
+  }
+  return addresses;
+}
+
+add_setup(async function () {
+  await SpecialPowers.pushPrefEnv({
+    set: [["extensions.formautofill.addresses.capture.v2.enabled", true]],
+  });
+});
+
+add_task(async function test_save_doorhanger_tel_invalid() {
+  const EXPECTED = [
+    {
+      "given-name": "Test User",
+      organization: "Mozilla",
+      "street-address": "123 Sesame Street",
+      tel: "",
+    },
+  ];
+
+  const TEST_CASES = [
+    "1234", // length is too short
+    "1234567890123456", // length is too long
+    "12345###!!", // contains invalid characters
+  ];
+
+  for (const TEST of TEST_CASES) {
+    await expectSavedAddresses([]);
+
+    await BrowserTestUtils.withNewTab(
+      { gBrowser, url: ADDRESS_FORM_URL },
+      async function (browser) {
+        let onPopupShown = waitForPopupShown();
+
+        await focusUpdateSubmitForm(browser, {
+          focusSelector: "#given-name",
+          newValues: {
+            "#given-name": "Test User",
+            "#organization": "Mozilla",
+            "#street-address": "123 Sesame Street",
+            "#tel": TEST,
+          },
+        });
+
+        await onPopupShown;
+        await clickDoorhangerButton(MAIN_BUTTON, 0);
+      }
+    );
+
+    await expectSavedAddresses(EXPECTED);
+    await removeAllRecords();
+  }
+});

browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html

@@ -20,7 +20,6 @@ Form autofill test: Test autofill submission for a country without address-level
 const TEST_ADDRESSES = [{
   organization: "Mozilla",
   "street-address": "123 Sesame Street",
-  "address-level1": "AL",
   country: "DE",
   timesUsed: 1,
 }];

browser/extensions/formautofill/test/unit/head_addressComponent.js

@@ -20,10 +20,9 @@ const SAME = AddressComparison.SAME;
 const DIFFERENT = AddressComparison.DIFFERENT;
 
 function runIsValidTest(tests, fieldName, funcSetupRecord) {
-  let region = FormAutofill.DEFAULT_REGION;
   for (const test of tests) {
     if (!Array.isArray(test)) {
-      region = test.region;
+      FormAutofill.DEFAULT_REGION = test.region;
       info(`Change region to ${JSON.stringify(test.region)}`);
       continue;
     }
@@ -31,7 +30,9 @@ function runIsValidTest(tests, fieldName, funcSetupRecord) {
     const [testValue, expected] = test;
     const record = funcSetupRecord(testValue);
 
-    const field = new AddressComponent(record, region).getField(fieldName);
+    const field = new AddressComponent(record, {
+      ignoreInvalid: false,
+    }).getField(fieldName);
     const result = field.isValid();
     Assert.equal(
       result,
@@ -39,23 +40,27 @@ function runIsValidTest(tests, fieldName, funcSetupRecord) {
       `Expect isValid returns ${expected} for ${testValue}`
     );
   }
+  FormAutofill.DEFAULT_REGION = null;
 }
 
 function runCompareTest(tests, fieldName, funcSetupRecord) {
-  let region = FormAutofill.DEFAULT_REGION;
   for (const test of tests) {
     if (!Array.isArray(test)) {
       info(`change region to ${JSON.stringify(test.region)}`);
-      region = test.region;
+      FormAutofill.DEFAULT_REGION = test.region;
       continue;
     }
 
     const [v1, v2, expected] = test;
     const r1 = funcSetupRecord(v1);
-    const f1 = new AddressComponent(r1, region).getField(fieldName);
+    const f1 = new AddressComponent(r1, { ignoreInvalid: false }).getField(
+      fieldName
+    );
 
     const r2 = funcSetupRecord(v2);
-    const f2 = new AddressComponent(r2, region).getField(fieldName);
+    const f2 = new AddressComponent(r2, { ignoreInvalid: false }).getField(
+      fieldName
+    );
 
     const result = AddressComparison.compare(f1, f2);
     const resultString = AddressComparison.resultToString(result);
@@ -66,4 +71,5 @@ function runCompareTest(tests, fieldName, funcSetupRecord) {
       `Expect ${expectedString} when comparing "${v1}" & "${v2}", got ${resultString}`
     );
   }
+  FormAutofill.DEFAULT_REGION = null;
 }

browser/extensions/formautofill/test/unit/test_addressComponent_city.js

@@ -12,7 +12,7 @@ const COMPARE_TESTS = [
   ["New York City", "City New York", DIFFERENT],
 ];
 
-const TEST_FIELD_NAME = "City";
+const TEST_FIELD_NAME = "address-level2";
 
 add_task(async function test_isValid() {
   runIsValidTest(VALID_TESTS, TEST_FIELD_NAME, value => {

browser/extensions/formautofill/test/unit/test_addressComponent_country.js

@@ -32,7 +32,7 @@ const COMPARE_TESTS = [
   ["AAA", "BBB", SAME],
 ];
 
-const TEST_FIELD_NAME = "Country";
+const TEST_FIELD_NAME = "country";
 
 add_task(async function test_isValid() {
   runIsValidTest(VALID_TESTS, TEST_FIELD_NAME, value => {

browser/extensions/formautofill/test/unit/test_addressComponent_email.js

@@ -7,7 +7,6 @@ const VALID_TESTS = [
   // [email1, expected]
   ["john.doe@mozilla.org", true],
 
-  ["", false], // empty
   ["@mozilla.org", false], // without username
   ["john.doe@", false], // without domain
   ["john.doe@-mozilla.org", false], // domain starts with '-'
@@ -57,7 +56,7 @@ const COMPARE_TESTS = [
   ["john#smith@gmail.com", "johnsmith@gmail.com", DIFFERENT],
 ];
 
-const TEST_FIELD_NAME = "Email";
+const TEST_FIELD_NAME = "email";
 
 add_setup(async () => {});
 

browser/extensions/formautofill/test/unit/test_addressComponent_name.js

@@ -84,7 +84,7 @@ const COMPARE_TESTS = [
   ["John Middle Doe", "J. M. D.", DIFFERENT],
 ];
 
-const TEST_FIELD_NAME = "Name";
+const TEST_FIELD_NAME = "name";
 
 add_setup(async () => {});
 

browser/extensions/formautofill/test/unit/test_addressComponent_organization.js

@@ -5,7 +5,6 @@ const VALID_TESTS = [
   ["Mozilla", true],
   ["mozilla", true],
   ["@Mozilla", true],
-  ["  ", true], // A string only contains whitespace is treated as empty, which is considered as valid
   ["-!@#%&*_(){}[:;\"',.?]", false], // Not valid when the organization name only contains punctuations
 ];
 
@@ -38,7 +37,7 @@ const COMPARE_TESTS = [
   ["Linens 'n Things", "Linens'n Things", SIMILAR], // Punctuation is replaced with whitespace, so both strings become "Linesns n Things"
 ];
 
-const TEST_FIELD_NAME = "Organization";
+const TEST_FIELD_NAME = "organization";
 
 add_setup(async () => {});
 

browser/extensions/formautofill/test/unit/test_addressComponent_postal_code.js

@@ -40,7 +40,7 @@ const COMPARE_TESTS = [
   ["12345-1234", "1234", A_CONTAINS_B], // B is invalid
 ];
 
-const TEST_FIELD_NAME = "PostalCode";
+const TEST_FIELD_NAME = "postal-code";
 
 add_setup(async () => {});
 

browser/extensions/formautofill/test/unit/test_addressComponent_state.js

@@ -17,7 +17,7 @@ const COMPARE_TESTS = [
   ["New York", "New Jersey", DIFFERENT],
 ];
 
-const TEST_FIELD_NAME = "State";
+const TEST_FIELD_NAME = "address-level1";
 
 add_task(async function test_isValid() {
   runIsValidTest(VALID_TESTS, TEST_FIELD_NAME, value => {

browser/extensions/formautofill/test/unit/test_addressComponent_street_address.js

@@ -39,7 +39,7 @@ const COMPARE_TESTS = [
   ["123 Main St. Apt 4, Floor 2", "123 Main St. Floor 2, Apt 4", DIFFERENT], //
 ];
 
-const TEST_FIELD_NAME = "StreetAddress";
+const TEST_FIELD_NAME = "street-address";
 
 add_setup(async () => {});
 

browser/extensions/formautofill/test/unit/test_addressComponent_tel.js

@@ -53,7 +53,7 @@ const COMPARE_TESTS = [
   //["+64 3 331-6005", "3 331 6005#1234", A_CONTAINS_B],
 ];
 
-const TEST_FIELD_NAME = "Tel";
+const TEST_FIELD_NAME = "tel";
 
 add_setup(async () => {
   Services.prefs.setBoolPref("browser.search.region", "US");

browser/extensions/formautofill/test/unit/test_onFormSubmitted.js

@@ -552,99 +552,6 @@ const TESTCASES = [
       },
     },
   },
-  {
-    description: "Shouldn't save tel whose length is too short",
-    document: DEFAULT_TEST_DOC,
-    targetElementId: TARGET_ELEMENT_ID,
-    formValue: {
-      "street-addr": "331 E. Evelyn Avenue",
-      "address-level1": "CA",
-      country: "US",
-      tel: "1234",
-    },
-    expectedResult: {
-      formSubmission: true,
-      records: {
-        address: [
-          {
-            guid: null,
-            record: {
-              "street-address": "331 E. Evelyn Avenue",
-              "address-level1": "CA",
-              "address-level2": "",
-              country: "US",
-              tel: "",
-              email: "",
-            },
-            untouchedFields: [],
-          },
-        ],
-        creditCard: [],
-      },
-    },
-  },
-  {
-    description: "Shouldn't save tel whose length is too long",
-    document: DEFAULT_TEST_DOC,
-    targetElementId: TARGET_ELEMENT_ID,
-    formValue: {
-      "street-addr": "331 E. Evelyn Avenue",
-      "address-level1": "CA",
-      country: "US",
-      tel: "1234567890123456",
-    },
-    expectedResult: {
-      formSubmission: true,
-      records: {
-        address: [
-          {
-            guid: null,
-            record: {
-              "street-address": "331 E. Evelyn Avenue",
-              "address-level1": "CA",
-              "address-level2": "",
-              country: "US",
-              tel: "",
-              email: "",
-            },
-            untouchedFields: [],
-          },
-        ],
-        creditCard: [],
-      },
-    },
-  },
-  {
-    description: "Shouldn't save tel which contains invalid characters",
-    document: DEFAULT_TEST_DOC,
-    targetElementId: TARGET_ELEMENT_ID,
-    formValue: {
-      "street-addr": "331 E. Evelyn Avenue",
-      "address-level1": "CA",
-      country: "US",
-      tel: "12345###!!",
-    },
-    expectedResult: {
-      formSubmission: true,
-      records: {
-        address: [
-          {
-            guid: null,
-            record: {
-              "street-address": "331 E. Evelyn Avenue",
-              "address-level1": "CA",
-              "address-level2": "",
-              country: "US",
-              tel: "",
-              email: "",
-            },
-            untouchedFields: [],
-          },
-        ],
-        creditCard: [],
-      },
-    },
-  },
 ];
 
 add_task(async function handle_invalid_form() {

toolkit/components/formautofill/FormAutofill.sys.mjs

@@ -44,9 +44,16 @@ export const FormAutofill = {
   AUTOFILL_CREDITCARDS_AUTOCOMPLETE_OFF_PREF,
   AUTOFILL_ADDRESSES_AUTOCOMPLETE_OFF_PREF,
 
+  _region: null,
+
   get DEFAULT_REGION() {
-    return Region.home || "US";
+    return this._region || Region.home || "US";
   },
+
+  set DEFAULT_REGION(region) {
+    this._region = region;
+  },
+
   /**
    * Determines if an autofill feature should be enabled based on the "available"
    * and "supportedCountries" parameters.

toolkit/components/formautofill/FormAutofillParent.sys.mjs

@@ -482,16 +482,16 @@ export class FormAutofillParent extends JSWindowActorParent {
       // filter invalid field
       const result = newAddress.compare(savedAddress);
 
-      // If any of the fields in the new address are different from the corresponding fields
-      // in the saved address, the two addresses are considered different. For example, if
-      // the name, email, country are the same but the street address is different, the two
-      // addresses are not considered the same.
       if (Object.values(result).includes("different")) {
+        // If any of the fields in the new address are different from the corresponding fields
+        // in the saved address, the two addresses are considered different. For example, if
+        // the name, email, country are the same but the street address is different, the two
+        // addresses are not considered the same.
         continue;
+      } else if (
         // If every field of the new address is either the same or is subset of the corresponding
         // field in the saved address, the new address is duplicated. We don't need capture
         // the new address.
-      } else if (
         Object.values(result).every(r => ["same", "subset"].includes(r))
       ) {
         lazy.log.debug(
@@ -499,9 +499,9 @@ export class FormAutofillParent extends JSWindowActorParent {
         );
         storage.notifyUsed(record.guid);
         return false;
+      } else {
         // If the new address is neither a duplicate of the saved address nor a different address.
         // There must be at least one field we can merge, show the update doorhanger
-      } else {
         lazy.log.debug(
           "A mergeable address record is found, show the update prompt"
         );
@@ -524,11 +524,12 @@ export class FormAutofillParent extends JSWindowActorParent {
       return false;
     }
 
+    dump(`[Dimi]Save: ${JSON.stringify(newAddress.record)}\n`);
     return async () => {
       await lazy.FormAutofillPrompter.promptToSaveAddress(
         browser,
         storage,
-        address.record,
+        newAddress.record,
         address.flowId,
         { mergeableRecord, mergeableFields }
       );

toolkit/components/formautofill/shared/AddressComponent.sys.mjs

@@ -193,6 +193,8 @@ class AddressField {
  * See autocomplete="street-address".
  */
 class StreetAddress extends AddressField {
+  static ac = "street-address";
+
   #structuredStreetAddress = null;
 
   constructor(value, region) {
@@ -272,6 +274,10 @@ class StreetAddress extends AddressField {
       this.localeCompare(a, b)
     );
   }
+
+  static fromRecord(record, region) {
+    return new StreetAddress(record[StreetAddress.ac], region);
+  }
 }
 
 /**
@@ -279,6 +285,8 @@ class StreetAddress extends AddressField {
  * See autocomplete="postal-code"
  */
 class PostalCode extends AddressField {
+  static ac = "postal-code";
+
   constructor(value, region) {
     super(value, region);
   }
@@ -318,13 +326,19 @@ class PostalCode extends AddressField {
       self_normalized_value.startsWith(other_normalized_value)
     );
   }
+
+  static fromRecord(record, region) {
+    return new PostalCode(record[PostalCode.ac], region);
+  }
 }
 
 /**
  * City name.
- * See autocomplete="address-level1"
+ * See autocomplete="address-level2"
  */
 class City extends AddressField {
+  static ac = "address-level2";
+
   #city = null;
 
   constructor(value, region) {
@@ -362,13 +376,19 @@ class City extends AddressField {
       this.localeCompare(a, b)
     );
   }
+
+  static fromRecord(record, region) {
+    return new City(record[City.ac], region);
+  }
 }
 
 /**
  * State.
- * See autocomplete="address-level2"
+ * See autocomplete="address-level1"
  */
 class State extends AddressField {
+  static ac = "address-level1";
+
   // The abbreviated region name. For example, California is abbreviated as CA
   #state = null;
 
@@ -411,6 +431,10 @@ class State extends AddressField {
   contains(other) {
     return this.equals(other);
   }
+
+  static fromRecord(record, region) {
+    return new State(record[State.ac], region);
+  }
 }
 
 /**
@@ -418,6 +442,8 @@ class State extends AddressField {
  * See autocomplete="country"
  */
 class Country extends AddressField {
+  static ac = "country";
+
   // iso 3166 2-alpha code
   #country_code = null;
 
@@ -457,6 +483,10 @@ class Country extends AddressField {
   contains(other) {
     return false;
   }
+
+  static fromRecord(record, region) {
+    return new Country(record[Country.ac], region);
+  }
 }
 
 /**
@@ -464,6 +494,8 @@ class Country extends AddressField {
  * See autocomplete="name"
  */
 class Name extends AddressField {
+  static ac = "name";
+
   constructor(value, region) {
     super(value, region);
   }
@@ -600,6 +632,20 @@ class Name extends AddressField {
 
     return false;
   }
+
+  static fromRecord(record, region) {
+    let name;
+    if (record.name) {
+      name = record.name;
+    } else {
+      name = lazy.FormAutofillNameUtils.joinNameParts({
+        given: record["given-name"],
+        middle: record["additional-name"],
+        family: record["family-name"],
+      });
+    }
+    return new Name(name, region);
+  }
 }
 
 /**
@@ -607,6 +653,8 @@ class Name extends AddressField {
  * See autocomplete="tel"
  */
 class Tel extends AddressField {
+  static ac = "tel";
+
   #valid = false;
 
   // The country code part of a telphone number, such as "1" for the United States
@@ -674,6 +722,10 @@ class Tel extends AddressField {
   toString() {
     return `${this.constructor.name}: ${this.country_code} ${this.national_number}\n`;
   }
+
+  static fromRecord(record, region) {
+    return new Tel(record[Tel.ac], region);
+  }
 }
 
 /**
@@ -681,6 +733,8 @@ class Tel extends AddressField {
  * See autocomplete="organization".
  */
 class Organization extends AddressField {
+  static ac = "organization";
+
   constructor(value, region) {
     super(value, region);
   }
@@ -713,6 +767,10 @@ class Organization extends AddressField {
 
     return otherTokens.isSubset(selfTokens, (a, b) => this.localeCompare(a, b));
   }
+
+  static fromRecord(record, region) {
+    return new Organization(record[Organization.ac], region);
+  }
 }
 
 /**
@@ -720,6 +778,8 @@ class Organization extends AddressField {
  * See autocomplete="email".
  */
 class Email extends AddressField {
+  static ac = "email";
+
   constructor(value, region) {
     super(value, region);
   }
@@ -783,6 +843,10 @@ class Email extends AddressField {
   contains(other) {
     return false;
   }
+
+  static fromRecord(record, region) {
+    return new Email(record[Email.ac], region);
+  }
 }
 
 /**
@@ -816,7 +880,7 @@ export class AddressComparison {
    */
   constructor(addressA, addressB) {
     for (const fieldA of addressA.getAllFields()) {
-      const fieldName = fieldA.constructor.name;
+      const fieldName = fieldA.constructor.ac;
       const fieldB = addressB.getField(fieldName);
       if (fieldB) {
         this.#result[fieldName] = AddressComparison.compare(fieldA, fieldB);
@@ -826,7 +890,7 @@ export class AddressComparison {
     }
 
     for (const fieldB of addressB.getAllFields()) {
-      const fieldName = fieldB.constructor.name;
+      const fieldName = fieldB.constructor.ac;
       if (!addressB.getField(fieldName)) {
         this.#result[fieldName] = AddressComparison.A_IS_EMPTY;
       }
@@ -941,81 +1005,48 @@ export class AddressComponent {
    *
    * @class
    * @param {object}  record         The address record object containing address data.
-   * @param {string}  defaultRegion  The default region to use if the record's
-   *                                 country is not specified.
    * @param {object}  [options = {}] a list of options for this method
    * @param {boolean} [options.ignoreInvalid = true]  Whether to ignore invalid address
    *                                 fields in the AddressComponent object. If set to true,
    *                                 invalid fields will be ignored.
    */
-  constructor(
-    record,
-    defaultRegion = FormAutofill.DEFAULT_REGION,
-    { ignoreInvalid = false } = {}
-  ) {
-    const fieldValue = this.#recordToFieldValue(record);
+  constructor(record, { ignoreInvalid = true } = {}) {
+    this.record = {};
 
     // Get country code first so we can use it to parse other fields
-    const country = new Country(fieldValue.country, defaultRegion);
-    this.#fields[Country.name] = country;
-    const region = country.isEmpty() ? defaultRegion : country.country_code;
+    const country = Country.fromRecord(record, FormAutofill.DEFAULT_REGION);
+    const region =
+      country.country_code ||
+      lazy.FormAutofillUtils.identifyCountryCode(FormAutofill.DEFAULT_REGION);
 
-    this.#fields[State.name] = new State(fieldValue.state, region);
-    this.#fields[City.name] = new City(fieldValue.city, region);
-    this.#fields[PostalCode.name] = new PostalCode(
-      fieldValue.postal_code,
-      region
-    );
-    this.#fields[Tel.name] = new Tel(fieldValue.tel, region);
-    this.#fields[StreetAddress.name] = new StreetAddress(
-      fieldValue.street_address,
-      region
-    );
-    this.#fields[Name.name] = new Name(fieldValue.name, region);
-    this.#fields[Organization.name] = new Organization(
-      fieldValue.organization,
-      region
-    );
-    this.#fields[Email.name] = new Email(fieldValue.email, region);
+    let fields = [
+      country,
+      StreetAddress.fromRecord(record, region),
+      PostalCode.fromRecord(record, region),
+      State.fromRecord(record, region),
+      City.fromRecord(record, region),
+      Name.fromRecord(record, region),
+      Tel.fromRecord(record, region),
+      Organization.fromRecord(record, region),
+      Email.fromRecord(record, region),
+    ];
 
-    if (ignoreInvalid) {
-      // TODO: We have to reset it or ignore non-existing fields while comparing
-      this.#fields.filter(f => f.IsValid());
+    for (const field of fields) {
+      if (field.isEmpty() || (ignoreInvalid && !field.isValid())) {
+        continue;
+      }
+      this.#fields[field.constructor.ac] = field;
+
+      if (field.constructor.ac == "name") {
+        this.record["given-name"] = record["given-name"] ?? "";
+        this.record["additional-name"] = record["additional-name"] ?? "";
+        this.record["family-name"] = record["family-name"] ?? "";
+      } else {
+        this.record[field.constructor.ac] = record[field.constructor.ac];
+      }
     }
   }
 
-  /**
-   * Converts address record to a field value object.
-   *
-   * @param  {object} record The record object containing address data.
-   * @returns {object}       A value object with keys corresponding to specific
-   *                         address fields and their respective values.
-   */
-  #recordToFieldValue(record) {
-    let value = {};
-
-    if (record.name) {
-      value.name = record.name;
-    } else {
-      value.name = lazy.FormAutofillNameUtils.joinNameParts({
-        given: record["given-name"],
-        middle: record["additional-name"],
-        family: record["family-name"],
-      });
-    }
-
-    value.email = record.email ?? "";
-    value.organization = record.organization ?? "";
-    value.street_address = record["street-address"] ?? "";
-    value.state = record["address-level1"] ?? "";
-    value.city = record["address-level2"] ?? "";
-    value.country = record.country ?? "";
-    value.postal_code = record["postal-code"] ?? "";
-    value.tel = record.tel ?? "";
-
-    return value;
-  }
-
   /**
    * Retrieves all the address fields.
    *

toolkit/components/formautofill/shared/AddressParser.sys.mjs

@@ -195,6 +195,10 @@ export class AddressParser {
    * @returns {StructuredStreetAddress}
    */
   static parseStreetAddress(address) {
+    if (!address) {
+      return null;
+    }
+
     const separator = "(\\s|,|$)";
 
     const regexpes = [