Closed Bug 1359978 Opened 8 years ago Closed 7 years ago

[Form Autofill] Implement the credit-card storage

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3])

Attachments

(2 files, 1 obsolete file)

Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.
59 bytes, text/x-review-board-request
MattN
: review+
steveck
: review+
Details
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.
59 bytes, text/x-review-board-request
MattN
: review+
steveck
: review+
Details 
Let's extend the ProfileStorage.jsm to store credit card information.
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Attachment #8863062 - Flags: review?(schung)
Attachment #8863083 - Flags: review?(schung)
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review139552

::: browser/extensions/formautofill/FormAutofillParent.jsm:157
(Diff revision 1)
>    _getStatus() {
>      if (!Services.prefs.getBoolPref(ENABLED_PREF)) {
>        return false;
>      }
>  
> -    return this._profileStore.getAll().length > 0;
> +    return this._profileStore.profiles.getAll().length > 0;

I thought you were changing this to `.addresses`?
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review139552

> I thought you were changing this to `.addresses`?

Yeah. I'm thinking to put it in part 3 because Juwei hasn't made the final decision on this naming. Besides, some APIs (e.g. `GetProfiles`) will need to be changed as well.
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review139556

I didn't finish reviewing this yet but here are some comments

::: commit-message-2d595:1
(Diff revision 1)
> +Bug 1359978 - (Part 1) Refactor ProfileStorage.jsm. r=MattN

Please include more details in this (first line) of the commit message to explain what is changing.

::: browser/extensions/formautofill/ProfileStorage.jsm:49
(Diff revision 1)
> +/* exported ProfileStorage */
>  this.EXPORTED_SYMBOLS = ["ProfileStorage"];

I thought this was automatic using EXPORTED_SYMBOLS?

::: browser/extensions/formautofill/ProfileStorage.jsm:68
(Diff revision 1)
>  this.log = null;
>  FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]);

Rather than modifying all the log calls to include the collection name, how about moving this to inside the base class constructor and including the collection name in the 2nd argument? Then we would log with `this.log.debug` instead of `log.debug`.

::: browser/extensions/formautofill/ProfileStorage.jsm:95
(Diff revision 1)
> -  guid: "test-guid-1",
> -  organization: "Sesame Street",
> -  "street-address": "123 Sesame Street.",
> -  tel: "1-345-345-3456",
> -}, {
> -  guid: "test-guid-2",
> +  "timeLastUsed",
> +  "timeLastModified",
> +  "timesUsed",
> +];
> +
> +class DataHandler {

"AutofillRecord" or "AutofillProfile" would be clearer IMO

::: browser/extensions/formautofill/ProfileStorage.jsm:96
(Diff revision 1)
> -  organization: "Sesame Street",
> -  "street-address": "123 Sesame Street.",
> -  tel: "1-345-345-3456",
> -}, {
> -  guid: "test-guid-2",
> -  organization: "Mozilla",
> +  "timeLastModified",
> +  "timesUsed",
> +];
> +
> +class DataHandler {
> +  constructor(store, dataId, validFields) {

Nit: dataId could maybe be called `collectionName` or just `collection` but that's longer…

"Id" made me think it was something more like a GUID. Thoughts?
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review139556

> Nit: dataId could maybe be called `collectionName` or just `collection` but that's longer…
> 
> "Id" made me think it was something more like a GUID. Thoughts?

Agree. That would be better.
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review139574

I only have some questions but it looks fine basically. I would like to know how it integrate with Bug 1361965

::: commit-message-2d595:1
(Diff revision 1)
> +Bug 1359978 - (Part 1) Refactor ProfileStorage.jsm. r=MattN

Maybe creating more commits for differnt purposes?

::: browser/extensions/formautofill/ProfileStorage.jsm:96
(Diff revision 1)
> -  organization: "Sesame Street",
> -  "street-address": "123 Sesame Street.",
> -  tel: "1-345-345-3456",
> -}, {
> -  guid: "test-guid-2",
> -  organization: "Mozilla",
> +  "timeLastModified",
> +  "timesUsed",
> +];
> +
> +class DataHandler {
> +  constructor(store, dataId, validFields) {

+1 that there could be other better naming than ID

::: browser/extensions/formautofill/ProfileStorage.jsm:120
(Diff revision 1)
>  
> -    let profileToSave = this._clone(profile);
> -    this._normalizeProfile(profileToSave);
> +    let dataToSave = this._clone(data);
> +    this._normalizeData(dataToSave);
>  
> -    profileToSave.guid = gUUIDGenerator.generateUUID().toString()
> +    let guid;
> +    while (!guid || this._findByGUID(guid)) {

Is that necessary to check the duplicate guid, since you already applied `gUUIDGenerator` here?

::: browser/extensions/formautofill/ProfileStorage.jsm:285
(Diff revision 1)
>    _findByGUID(guid) {
> -    return this._store.data.profiles.find(profile => profile.guid == guid);
> -  },
> +    let found = this._findIndexByGUID(guid);
> +    return found < 0 ? undefined : this._store.data[this._dataId][found];
> +  }
> +
> +  _findIndexByGUID(guid) {

Sorry but I'm not sure why we need this `_findIndexByGUID` here...

::: browser/extensions/formautofill/ProfileStorage.jsm:391
(Diff revision 1)
> +    this.INTERNAL_FIELDS = INTERNAL_FIELDS;
> +  }
>  
> -    for (let key in profile) {
> -      if (!VALID_FIELDS.includes(key)) {
> -        throw new Error(`"${key}" is not a valid field.`);
> +  get profiles() {
> +    if (!this._profiles) {
> +      this._store.ensureDataReady();

Is it sufficient to have `ensureDataReady` only here? I thought it would be called once while initializing profile here.
Attachment #8863062 - Flags: review?(schung)
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review139556

> Rather than modifying all the log calls to include the collection name, how about moving this to inside the base class constructor and including the collection name in the 2nd argument? Then we would log with `this.log.debug` instead of `log.debug`.

Good point. Thanks.
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review139574

> Is that necessary to check the duplicate guid, since you already applied `gUUIDGenerator` here?

I'm not sure, either. I'm just afraid the conflicts because we truncate the UUID into 12 characters only.

> Sorry but I'm not sure why we need this `_findIndexByGUID` here...

I noticed that sync team implemented this function in their WIP patch so I prepared this for them.

> Is it sufficient to have `ensureDataReady` only here? I thought it would be called once while initializing profile here.

I think so as everyone who wants to manipulate the records needs to retrieve a `profiles` here. In other words, data should be always ready once the `profiles` object is initialized.
Depends on: 1362934
Attachment #8863062 - Flags: review?(schung)
Attachment #8863062 - Flags: review?(MattN+bmo)
Attachment #8865175 - Flags: review?(scwwu)
Attachment #8865175 - Flags: review?(schung)
Attachment #8865175 - Flags: review?(MattN+bmo)
We're going to address the renaming part in bug 1362911 so clear the review flags.
Attachment #8865175 - Attachment is obsolete: true
Patch has been updated base on bug 1362911.
Depends on: 1362911
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review140504

I only some trivial nits but overall looks fine, thanks.

::: browser/extensions/formautofill/ProfileStorage.jsm:285
(Diff revision 1)
>    _findByGUID(guid) {
> -    return this._store.data.profiles.find(profile => profile.guid == guid);
> -  },
> +    let found = this._findIndexByGUID(guid);
> +    return found < 0 ? undefined : this._store.data[this._dataId][found];
> +  }
> +
> +  _findIndexByGUID(guid) {

Ok, I think having a comment about the main purpose(for sync) might be great.

::: browser/extensions/formautofill/ProfileStorage.jsm:239
(Diff revision 3)
> -   * @returns {Array.<Address>}
> -   *          An array containing clones of all addresses.
> +   * @param   {boolean} config.noComputedFields
> +   *          Returns raw record without those computed fields.
> +   * @returns {Array.<Object>}
> +   *          An array containing clones of all records.
>     */
> -  getAll() {
> +  getAll({noComputedFields} = {}) {

Do we need a test for the case if noComputedFields is provoded?

::: browser/extensions/formautofill/test/unit/test_profileStorage.js:2
(Diff revision 3)
>  /**
> - * Tests ProfileStorage object.
> + * Tests ProfileStorage object with addresses records.

nit: Not sure if you prefer to split the credit card  and addresses records test in different files or into the same file. If you'll create another test file for credit card, maybe you can rename this test to "test_addressesRecords.js". Or rename the task name with more spefic record type like "test_add_address" if you prefer to integrate all the tests in one file.
Attachment #8863062 - Flags: review?(schung) → review+
Comment on attachment 8863083 [details]
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.

https://reviewboard.mozilla.org/r/134918/#review140782

::: browser/extensions/formautofill/ProfileStorage.jsm:51
(Diff revision 5)
> + *     {
> + *       guid,             // 12 characters
> + *
> + *       // credit card fields
> + *       cc-name,
> + *       cc-number,        // encrypted in the disk

Nit: "encrypted on disk"

::: browser/extensions/formautofill/ProfileStorage.jsm:419
(Diff revision 5)
> +  _recordReadProcessor(creditCard, noComputedFields) {
> +    // TODO: Decrypt cc-number here (bug 1337314).

I think it would be better to delay this until it's actually needed so we don't end up prompting for the master password just because we read the file. I think we decided not to filter credit card numbers as you type so ideally we don't actually need to decrypt until the user actually selects a card in autocomplete. The last 4 digits could be stored unencrypted.

To implement this I think we should move to storage returning instances of a CreditCard class rather than vanilla JS objects. Then it can have methods to get the whole number without having to go back to storage but still doing it on-demand.

I think it's a much cleaner architecure that I wanted to move to for addresses too but didn't want to block on switching to ES classes for it.

The "Compute split names" can be done in a lazy getter of this CreditCard class which is cleaner IMO. The constructor could handle raw data from JSONFile and we would still use JSONFile for storage.

What do you think of this approach? I don't think it should be done in this bug but it will affect bug 1337314. This would be similar to nsILoginInfo btw. which has methods like `matches` and `equals` which are also like helpers we're implementing which are better as instance methods.

::: browser/extensions/formautofill/ProfileStorage.jsm:442
(Diff revision 5)
> +      }
> +    }
> +  }
> +
> +  _recordWriteProcessor(creditCard) {
> +    // TODO: Encrypt cc-number here (bug 1337314).

I'm fine with doing this in a follow-up assuming we never save data here before encryption lands.

::: browser/extensions/formautofill/ProfileStorage.jsm:462
(Diff revision 5)
> +    }
> +
> +    // Validate expiry date
> +    if (creditCard["cc-exp-month"]) {
> +      let expMonth = parseInt(creditCard["cc-exp-month"], 10);
> +      if (isNaN(expMonth) || expMonth < 0 || expMonth > 12) {

Don't you want `<= 0`?

::: browser/extensions/formautofill/ProfileStorage.jsm:470
(Diff revision 5)
> +        creditCard["cc-exp-month"] = String(expMonth);
> +      }
> +    }
> +    if (creditCard["cc-exp-year"]) {
> +      let expYear = parseInt(creditCard["cc-exp-year"], 10);
> +      if (isNaN(expYear) || expYear < 0) {

We should probably enforce 4 digits years as a sanity check

::: browser/extensions/formautofill/test/unit/test_creditCardStorage.js:44
(Diff revision 5)
> +let prepareTestCreditCards = Task.async(function* (path) {
> +  let profileStorage = new ProfileStorage(path);

Do we need Task.jsm for this or is there a newer ES way to do this without it? I'm not up-to-date on the async/await stuff.
Attachment #8863083 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review140808
Attachment #8863062 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8863083 [details]
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.

https://reviewboard.mozilla.org/r/134918/#review140782

> I think it would be better to delay this until it's actually needed so we don't end up prompting for the master password just because we read the file. I think we decided not to filter credit card numbers as you type so ideally we don't actually need to decrypt until the user actually selects a card in autocomplete. The last 4 digits could be stored unencrypted.
> 
> To implement this I think we should move to storage returning instances of a CreditCard class rather than vanilla JS objects. Then it can have methods to get the whole number without having to go back to storage but still doing it on-demand.
> 
> I think it's a much cleaner architecure that I wanted to move to for addresses too but didn't want to block on switching to ES classes for it.
> 
> The "Compute split names" can be done in a lazy getter of this CreditCard class which is cleaner IMO. The constructor could handle raw data from JSONFile and we would still use JSONFile for storage.
> 
> What do you think of this approach? I don't think it should be done in this bug but it will affect bug 1337314. This would be similar to nsILoginInfo btw. which has methods like `matches` and `equals` which are also like helpers we're implementing which are better as instance methods.

I've just confirmed with Juwei that we will filter credit card numbers when typing, so we are probably unable to delay the prompt? 

I think there are still benefits of returning instances rather than JS objects, e.g. lazy getter of computed fields. However, I'm not sure if it works as we need to pass the object to the content process via message manager.

> I'm fine with doing this in a follow-up assuming we never save data here before encryption lands.

I'll ensure that encryption will land first.

> Don't you want `<= 0`?

Yup, that's a mistake. Thanks.

> We should probably enforce 4 digits years as a sanity check

Good point!

> Do we need Task.jsm for this or is there a newer ES way to do this without it? I'm not up-to-date on the async/await stuff.

This test file was forked from `test_profileStorage.js` so Task.jsm was kept as-is. I also feel that async/await looks good. Maybe I can try them next time.
Comment on attachment 8863062 [details]
Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records.

https://reviewboard.mozilla.org/r/134900/#review140504

> Do we need a test for the case if noComputedFields is provoded?

Tests added.

> nit: Not sure if you prefer to split the credit card  and addresses records test in different files or into the same file. If you'll create another test file for credit card, maybe you can rename this test to "test_addressesRecords.js". Or rename the task name with more spefic record type like "test_add_address" if you prefer to integrate all the tests in one file.

Good Catch. Thanks.
(In reply to Luke Chang [:lchang] from comment #24)
> I've just confirmed with Juwei that we will filter credit card numbers when
> typing, so we are probably unable to delay the prompt? 
> 
> I think there are still benefits of returning instances rather than JS
> objects, e.g. lazy getter of computed fields. However, I'm not sure if it
> works as we need to pass the object to the content process via message
> manager.

Matt, what do you think?
Flags: needinfo?(MattN+bmo)
Comment on attachment 8863083 [details]
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.

https://reviewboard.mozilla.org/r/134918/#review140782

> I've just confirmed with Juwei that we will filter credit card numbers when typing, so we are probably unable to delay the prompt? 
> 
> I think there are still benefits of returning instances rather than JS objects, e.g. lazy getter of computed fields. However, I'm not sure if it works as we need to pass the object to the content process via message manager.

We had a meeting and decided that we won't filter credit card numbers and will show unmaksed credit card numbers for users with a master password set (regardless of whether it's logged in or not). Upon selecting a row to fill a credit card, the MP dialog will be forced to appear and upon succesful login the credit card will be filled. Users without a master password will have the whole CC number previewed and will support live-filtering of credit card numbers in autocomplete.

For now we can use a vanilla JS object for now to unblock sync engine progress even though serializing the object won't be a lot of work.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8863083 [details]
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.

https://reviewboard.mozilla.org/r/134918/#review141940

::: browser/extensions/formautofill/ProfileStorage.jsm:459
(Diff revision 9)
> +    if (creditCard["cc-number"]) {
> +      let ccNumber = creditCard["cc-number"].replace(/\s/g, "");
> +      delete creditCard["cc-number"];
> +
> +      if (!ccNumber.match(/^\d+$/)) {
> +        throw new Error("Credit card numbers contain invalid characters.");

Nit "Credit cards number contains…" (move the "s")

::: browser/extensions/formautofill/ProfileStorage.jsm:493
(Diff revision 9)
> +    if (creditCard["cc-exp-month"]) {
> +      let expMonth = parseInt(creditCard["cc-exp-month"], 10);
> +      if (isNaN(expMonth) || expMonth < 1 || expMonth > 12) {
> +        delete creditCard["cc-exp-month"];
> +      } else {
> +        creditCard["cc-exp-month"] = String(expMonth);

Why a string here too?

::: browser/extensions/formautofill/ProfileStorage.jsm:500
(Diff revision 9)
> +      } else if (expYear < 100) {
> +        // Enforce 4 digits years.
> +        creditCard["cc-exp-year"] = String(expYear + 2000);

What I meant is that we throw if we are given more or less than 4 digits… I guess this works too. We should  document assumptions in a JSDoc block above "class CreditCards" about what we expect in terms of input to make it clear who is responsible for doing conversions like this.

::: browser/extensions/formautofill/ProfileStorage.jsm:504
(Diff revision 9)
> +        delete creditCard["cc-exp-year"];
> +      } else if (expYear < 100) {
> +        // Enforce 4 digits years.
> +        creditCard["cc-exp-year"] = String(expYear + 2000);
> +      } else {
> +        creditCard["cc-exp-year"] = String(expYear);

Why store a string instead of a number? Are we going to define that we're always going to store strings even for numeric data? I guess it helps with API consistency as long as that's defined and clear.

::: browser/extensions/formautofill/test/unit/test_creditCardRecords.js:79
(Diff revision 9)
> +      do_check_eq(creditCardWithMeta["cc-number-masked"],
> +        "*".repeat(creditCard["cc-number"].length - 4) + creditCard["cc-number"].substr(-4));

Nit: if you're using the same code in the test as the shipping code you're not really testing that this does the right thing…
Comment on attachment 8863083 [details]
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.

https://reviewboard.mozilla.org/r/134918/#review141952

::: browser/extensions/formautofill/ProfileStorage.jsm:468
(Diff revisions 8 - 9)
> +      // e.g. creditCard["cc-number-encrypted"] = Encrypt(creditCard["cc-number"]);
> +
> +      if (ccNumber.length > 4) {
> +        creditCard["cc-number-masked"] = "*".repeat(ccNumber.length - 4) + ccNumber.substr(-4);
> +      } else {
> +        creditCard["cc-number-masked"] = ccNumber;

Nit: I'm not sure if we need to consider such case since there shouldn't be a valid cc-number that less then 4 digits. Maybe we can file a follow up bug for basic credit card number validation(should be a low priority task)?
Attachment #8863083 - Flags: review?(schung) → review+
Comment on attachment 8863083 [details]
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.

https://reviewboard.mozilla.org/r/134918/#review141940

> Nit "Credit cards number contains…" (move the "s")

Nit-of-a-nit: I think Matt accidentally introduced another "s" - I think it should read "Credit card number contains…"
Comment on attachment 8863083 [details]
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.

https://reviewboard.mozilla.org/r/134918/#review141940

> Nit-of-a-nit: I think Matt accidentally introduced another "s" - I think it should read "Credit card number contains…"

Thanks both of you.

> What I meant is that we throw if we are given more or less than 4 digits… I guess this works too. We should  document assumptions in a JSDoc block above "class CreditCards" about what we expect in terms of input to make it clear who is responsible for doing conversions like this.

I personally would like to make `profileStorage` do the normalization for the most part but I'm not sure if it's the right way. I'll document it first anyway. Since `addresses` should have the same assumption, I'll write it above `class AutofillRecords`.

> Why store a string instead of a number? Are we going to define that we're always going to store strings even for numeric data? I guess it helps with API consistency as long as that's defined and clear.

I intended to covert it to the same type at any time but I forgot why I chose a string rather than a number. After revisiting the `_normalizeRecord` function, I think it was designed to accept both strings and numbers. So I'll change it to store numbers. Thanks for pointint it out.

> Nit: if you're using the same code in the test as the shipping code you're not really testing that this does the right thing…

You're right. Fixed.
Comment on attachment 8863083 [details]
Bug 1359978 - (Part 2) Add credit card support to ProfileStorage.jsm.

https://reviewboard.mozilla.org/r/134918/#review141952

> Nit: I'm not sure if we need to consider such case since there shouldn't be a valid cc-number that less then 4 digits. Maybe we can file a follow up bug for basic credit card number validation(should be a low priority task)?

Agree. I'll file a bug to follow up the credit card number validation.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3d9ed00279c9 -d edb50441ee93: rebasing 395763:3d9ed00279c9 "Bug 1359978 - (Part 1) Implement AutofillRecords as a base class for handling address records. r=MattN,steveck"
local [dest] changed browser/extensions/formautofill/test/unit/test_profileStorage.js which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
merging browser/extensions/formautofill/FormAutofillParent.jsm
merging browser/extensions/formautofill/test/unit/test_enabledStatus.js
merging browser/extensions/formautofill/test/unit/test_savedFieldNames.js
merging browser/extensions/formautofill/test/unit/test_transformFields.js
warning: conflicts while merging browser/extensions/formautofill/test/unit/test_transformFields.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/376cb53491aa
(Part 1) Implement AutofillRecords as a base class for handling address records. r=MattN,steveck
https://hg.mozilla.org/integration/autoland/rev/e6f6f7fa0426
(Part 2) Add credit card support to ProfileStorage.jsm. r=MattN,steveck
https://hg.mozilla.org/mozilla-central/rev/376cb53491aa
https://hg.mozilla.org/mozilla-central/rev/e6f6f7fa0426
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: