Closed Bug 1395122 Opened 7 years ago Closed 7 years ago

[Form Autofill] Decrypt credit card numbers before syncing to server and re-encrypt them after syncing back

Categories

(Firefox :: Sync, enhancement)

Product:
Firefox
Component:
Sync
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

Details

(Whiteboard: [form autofill:M4])

Attachments

(1 file, 3 obsolete files)

Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.
59 bytes, text/x-review-board-request
MattN
: review+
lina
: review+
Details 
Since the credit card numbers stored in the storage are encrypted and passwords among devices are different, we should decrypt the number before syncing it to server and re-encrypt it after it's synced back.
Blocks: 1395123
Since the `ProfileStorage` methods are synchronous, and `FormAutofillStore#createRecord` and `applyIncoming` are async, I wonder if we can call `MasterPassword.{decrypt, encrypt}` in those methods.

So `ProfileStorage#get(id, { rawData: true })` would still return encrypted blobs, and we'd decrypt them when we create the Sync record. Conversely, `applyIncoming` would re-encrypt the data with the local master password before calling the `ProfileStorage` methods.

It's not as clean as having `ProfileStorage` return or accept decrypted data for `rawData: true`, but avoids refactoring to make the `ProfileStorage` methods async.
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #1)
> Since the `ProfileStorage` methods are synchronous, and
> `FormAutofillStore#createRecord` and `applyIncoming` are async, I wonder if
> we can call `MasterPassword.{decrypt, encrypt}` in those methods.

Thanks, Kit. Doing encryption and decryption in `FormAutofillStore#createRecord` and `applyIncoming` seems easier. We'll try it first.

BTW, one problem I can think of is that `ProfileStorage#_mergeSyncedRecords` would need decrypted numbers to determine if `isRemoteSame`. However, `localRecord` is also encrypted so I think it can be resolved within `ProfileStorage` anyway.
Assignee: nobody → lchang
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review181716

LGTM
Attachment #8904840 - Flags: review?(schung) → review+
Comment on attachment 8904841 [details]
Bug 1395122 - [Form Autofill] Part 2: Make "reconcile" and "findDuplicateGUID" async.

https://reviewboard.mozilla.org/r/176604/#review181964

::: browser/extensions/formautofill/test/unit/test_reconcile.js:472
(Diff revision 1)
>  add_task(async function test_reconcile_unknown_version() {
>    let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME);
>  
>    // Cross-version reconciliation isn't supported yet. See bug 1377204.
> -  throws(() => {
> -    profileStorage.addresses.reconcile({
> +  try {
> +    await profileStorage.addresses.reconcile({

`await Assert.rejects(profileStorage.addresses.reconcile(...), /Got unknown record version/)` might be a good fit here and elsewhere, but I don't have a strong preference. `try-await-catch` is also fine. :-)
Attachment #8904841 - Flags: review?(kit) → review+
Comment on attachment 8904841 [details]
Bug 1395122 - [Form Autofill] Part 2: Make "reconcile" and "findDuplicateGUID" async.

https://reviewboard.mozilla.org/r/176604/#review181964

> `await Assert.rejects(profileStorage.addresses.reconcile(...), /Got unknown record version/)` might be a good fit here and elsewhere, but I don't have a strong preference. `try-await-catch` is also fine. :-)

Cool, that's much easier. I didn't know that. Will update the patch. Thanks.
Comment on attachment 8904841 [details]
Bug 1395122 - [Form Autofill] Part 2: Make "reconcile" and "findDuplicateGUID" async.

https://reviewboard.mozilla.org/r/176604/#review182168
Attachment #8904841 - Flags: review?(markh) → review+
Comment on attachment 8904842 [details]
Bug 1395122 - [Form Autofill] Part 3: Ensure the credit card number will be encrypted/decrypted upon being accessed by sync.

https://reviewboard.mozilla.org/r/176606/#review182170

This is fine from Sync's perspective and I'll defer to Sean on most of the ProfileStorage changes.

::: browser/extensions/formautofill/FormAutofillSync.jsm:148
(Diff revision 3)
>  
>    async createRecord(id, collection) {
>      this._log.trace("Create record", id);
>      let record = new AutofillRecord(collection, id);
>      let entry = this.storage.get(id, {
>        rawData: true,

It seems it would be slightly nicer to have .get handle this - possibly with a new options flag (eg, decryptFields). However, this is a minor "taste" issue, so I'll leave that to you and/or the other reviewers who may feel strongly either way.
Attachment #8904842 - Flags: review?(markh) → review+
Comment on attachment 8904843 [details]
Bug 1395122 - [Form Autofill] Part 4: Add unit tests about encryption/decryption in credit card sync modules.

https://reviewboard.mozilla.org/r/176608/#review182172

This is a bit of a rubber-stamp, but it LGTM!

::: browser/extensions/formautofill/test/unit/test_reconcile.js:474
(Diff revision 3)
> +    description: "Local change",
> +    parent: {
> +      // So when we last wrote the record to the server, it had these values.
> +      "guid": "2bbd2d8fbc6b",
> +      "version": 1,
> +      "cc-name": "Mark Hammond",

heh - I'm not sure if my name appearing this many times in the source is a good or bad thing ;)
Attachment #8904843 - Flags: review?(markh) → review+
Comment on attachment 8904842 [details]
Bug 1395122 - [Form Autofill] Part 3: Ensure the credit card number will be encrypted/decrypted upon being accessed by sync.

https://reviewboard.mozilla.org/r/176606/#review181972

This looks good, though I'd like to see the patch again with the questions addressed. Thanks!

::: browser/extensions/formautofill/FormAutofillSync.jsm:150
(Diff revision 2)
>      this._log.trace("Create record", id);
>      let record = new AutofillRecord(collection, id);
>      let entry = this.storage.get(id, {
>        rawData: true,
>      });
> +    if (this.storage.decryptCCNumberFields) {

Instead of checking for `decryptCCNumberFields` here, consider overriding `createRecord` in `CreditCardsStore`, and having it call `this.storage.decryptCCNumberFields` instead.

::: browser/extensions/formautofill/ProfileStorage.jsm:633
(Diff revision 2)
> -        isRemoteSame = lastSyncedValue == sha512(remoteRecord[field]);
> +        let remoteValue = remoteRecord[field];
> +
> +        // "cc-number" is stored in a masked format and "lastSyncedValue" is
> +        // calculated based on it so we should get masked numbers before
> +        // comparing them.
> +        if (field == "cc-number") {

So, just to make sure I understand:

* Locally, we store the encrypted credit card number in `cc-number-encrypted`, and the masked CC number in `cc-number`.
* On the Sync server, we store the full unencrypted CC number in `cc-number`.
* On the way in, we call `encryptCCNumberFields` in `_replaceRecordAt`, which encrypts `cc-number`, assigns the result to `cc-number-encrypted`, and masks `cc-number`.
* On the way out, we call `decryptCCNumberFields` in `createRecord`, which decrypts `cc-number-encrypted`, and stores it in `cc-number`.

Changing the meaning of `cc-number` to be "masked locally, unencrypted remotely" confused me at first. What do you think about using `cc-number-masked` and `cc-number-decrypted` instead? So local records would only contain `cc-number-encrypted` and `cc-number-masked`, and remote records would only contain `cc-number-decrypted`.

::: browser/extensions/formautofill/ProfileStorage.jsm:694
(Diff revision 2)
>     */
> -  _replaceRecordAt(index, remoteRecord, {keepSyncMetadata = false} = {}) {
> +  async _replaceRecordAt(index, remoteRecord, {keepSyncMetadata = false} = {}) {
>      let localRecord = this._store.data[this._collectionName][index];
>      let newRecord = this._clone(remoteRecord);
>  
> +    if (this.encryptCCNumberFields) {

What if we called `encryptCCNumber` fields in `applyIncoming` instead of `_replaceRecordAt` and `add`? From the first reading, it wasn't immediately clear to me that we handled https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/FormAutofillSync.jsm#141, though I later saw that we do.

If we call `encryptCCNumber` fields in `applyIncoming`, could we also also drop the special case for `cc-number` in `_mergeSyncedRecords`? `encryptCCNumber` would replace the decrypted CC with the encrypted and masked CC...so we'd always be comparing encrypted to encrypted, instead of decrypting local and comparing decrypted to decrypted.

Do you think that would work?
Attachment #8904842 - Flags: review?(kit)
Comment on attachment 8904842 [details]
Bug 1395122 - [Form Autofill] Part 3: Ensure the credit card number will be encrypted/decrypted upon being accessed by sync.

https://reviewboard.mozilla.org/r/176606/#review182170

> It seems it would be slightly nicer to have .get handle this - possibly with a new options flag (eg, decryptFields). However, this is a minor "taste" issue, so I'll leave that to you and/or the other reviewers who may feel strongly either way.

I actually prefer to use another option, too. However, we have to make `get` asynchronous since the decryption behavior is. It will happen eventually but might not be soon. In the meantime, I can think of two ways to workaround it: one is to decrypt it outside and the other is to return different types according to whether `decryptFields` is set. I think the latter might be weird though.
Comment on attachment 8904842 [details]
Bug 1395122 - [Form Autofill] Part 3: Ensure the credit card number will be encrypted/decrypted upon being accessed by sync.

https://reviewboard.mozilla.org/r/176606/#review181972

> So, just to make sure I understand:
> 
> * Locally, we store the encrypted credit card number in `cc-number-encrypted`, and the masked CC number in `cc-number`.
> * On the Sync server, we store the full unencrypted CC number in `cc-number`.
> * On the way in, we call `encryptCCNumberFields` in `_replaceRecordAt`, which encrypts `cc-number`, assigns the result to `cc-number-encrypted`, and masks `cc-number`.
> * On the way out, we call `decryptCCNumberFields` in `createRecord`, which decrypts `cc-number-encrypted`, and stores it in `cc-number`.
> 
> Changing the meaning of `cc-number` to be "masked locally, unencrypted remotely" confused me at first. What do you think about using `cc-number-masked` and `cc-number-decrypted` instead? So local records would only contain `cc-number-encrypted` and `cc-number-masked`, and remote records would only contain `cc-number-decrypted`.

Yes, you understand correctly.

We had `cc-number-masked` before. However, we later found some paths in the code flow need to check the existence of `cc-number`, an valid @autocomplete attribute, so we ended up to store masked numbers in `cc-number`. To store the decrypted number in `cc-number-decrpyted` after calling `decryptCCNumberField` may be a good idea. i.e. Local records would contain `cc-number` and `cc-number-encrypted`, and remote records would contain `cc-number-decrypted` only.

> What if we called `encryptCCNumber` fields in `applyIncoming` instead of `_replaceRecordAt` and `add`? From the first reading, it wasn't immediately clear to me that we handled https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/FormAutofillSync.jsm#141, though I later saw that we do.
> 
> If we call `encryptCCNumber` fields in `applyIncoming`, could we also also drop the special case for `cc-number` in `_mergeSyncedRecords`? `encryptCCNumber` would replace the decrypted CC with the encrypted and masked CC...so we'd always be comparing encrypted to encrypted, instead of decrypting local and comparing decrypted to decrypted.
> 
> Do you think that would work?

Yeah, you reminded me that I forgot to handle the `add` case in `applyIncoming`. BTW, the problem I ran into is that the crypto will produce two different ciphertexts at different times even the sources are the same.
Hi Mark and Kit, 

Really thanks for your prompt review. I know my patches are quite confusing because I intended to avoid refactoring ProfileStorage. However, on a second thought, I feel it might not be that hard to make APIs asynchronous with a little workaround. After discussing with Matt, I would like to try another way to make it clearer and see if it works better. Sorry for not doing this earlier. I'll update you the status soon.
Attachment #8904841 - Attachment is obsolete: true
Attachment #8904841 - Flags: review?(selee)
Attachment #8904842 - Attachment is obsolete: true
Attachment #8904842 - Flags: review?(selee)
Attachment #8904843 - Attachment is obsolete: true
Attachment #8904843 - Flags: review?(selee)
Attachment #8904843 - Flags: review?(kit)
Hi Matt,

I uploaded the new patch that tries to treat "cc-number-encrypted" as a computed field and compute it in a normal field-computing process. Thus, it will be decrypted upon calling get/getAll with `{rawData: true}` and we don't need to change most of sync-related modules.

I try this way because I think we will eventually refactor the storage APIs to asynchronous functions and it's more reasonable to encrypt data within the storage APIs rather than doing it outside. However, I'm afraid that changing them to async functions at this moment might cause more efforts to uplift address-only patches to beta. So I workaround it by calling synchronous encrypt/decrypt APIs in MasterPassword module and only trigger the MasterPassword dialog outside ProfileStorage when necessary. We'll refactor them to be async in our V2 plan ASAP and we will no longer need to call "prompt" manually.

BTW, I tried to make the storage APIs asynchronous and use them synchronously but found it doesn't work when function needs to return values and we still need to touch a lot of address-related code.
Status: NEW → ASSIGNED
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review183920

Should we also get feedback/review from someone on the Sync team?

r- for the edit dialog save regression.

::: browser/extensions/formautofill/FormAutofillParent.jsm:202
(Diff revision 3)
> -        await this.profileStorage.creditCards.normalizeCCNumberFields(data.creditcard);
> +        // TODO: "MasterPassword.prompt()" can be removed after the storage APIs
> +        //       are refactored to be async functions.

TODOs should always have bug numbers

::: browser/extensions/formautofill/FormAutofillParent.jsm:443
(Diff revision 3)
> -    await this.profileStorage.creditCards.normalizeCCNumberFields(creditCard.record);
> +    // TODO: "MasterPassword.prompt()" can be removed after the storage APIs are
> +    //       refactored to be async functions.

Same here

::: browser/extensions/formautofill/MasterPassword.jsm:102
(Diff revision 3)
>      }
>  
>      return cryptoSDR.decrypt(cipherText);
>    },
>  
> +  decryptSync(cipherText) {

Can you add a JSDoc comment with @deprecated telling consumers to use the async APIs instead

::: browser/extensions/formautofill/MasterPassword.jsm:104
(Diff revision 3)
>      return cryptoSDR.decrypt(cipherText);
>    },
>  
> +  decryptSync(cipherText) {
> +    if (!this.isLoggedIn) {
> +      throw Components.Exception("Master password should be prompted first", Cr.NS_ERROR_ABORT);

Nit: `Cr.NS_ERROR_UNEXPECTED` would be a bit clearer IMO

::: browser/extensions/formautofill/MasterPassword.jsm:123
(Diff revision 3)
> +  encryptSync(plainText) {
> +    if (!this.isLoggedIn) {

Add JSdoc with @deprecated here too

::: browser/extensions/formautofill/ProfileStorage.jsm:71
(Diff revision 3)
>   *       // computed fields (These fields are computed based on the above fields
>   *       // and are not allowed to be modified directly.)
>   *       cc-given-name,
>   *       cc-additional-name,
>   *       cc-family-name,
> + *       cc-number-encrypted,  // encrypted from original "cc-number"

This is confusing since it's kinda implying that it's  the result of encrypting the masked `cc-number` since  `cc-number` has a comment saying "will be stored in masked format"

::: browser/extensions/formautofill/ProfileStorage.jsm:1437
(Diff revision 3)
> +  _getMaskedCCNumber(ccNumber) {
> +    if (ccNumber.length <= 4) {
> +      return "";

We should avoid getting in this state but I think returning an empty string may be confusing if we show an empty string in the UI. Can we return `"*".repeat(ccNumber.length)` instead?

I kinda want this to throw instead so we get bug reports but I'm not sure how well we're guarding about storage never having a number this short. I think we should at least add a log.warn here

::: browser/extensions/formautofill/ProfileStorage.jsm:1475
(Diff revision 3)
> +      if (FormAutofillUtils.isCCNumber(ccNumber)) {
> +        creditCard["cc-number"] = this._getMaskedCCNumber(ccNumber);
> +        creditCard["cc-number-encrypted"] = MasterPassword.encryptSync(ccNumber);
> +      } else {
> +        delete creditCard["cc-number"];
> +        creditCard["cc-number-encrypted"] = "";

It might be worth having a comment about how this empty string gets handled or what it's for. How would we get in this case? Don't we reject invalid credit card numbers?

::: browser/extensions/formautofill/content/editDialog.js:267
(Diff revision 3)
> -    await storage.normalizeCCNumberFields(creditCard);
> +    // TODO: "MasterPassword.prompt()" can be removed after the storage APIs are
> +    //       refactored to be async functions.
> +    if (await MasterPassword.prompt()) {
> -    await this.saveRecord(creditCard, this._record ? this._record.guid : null);
> +      await this.saveRecord(creditCard, this._record ? this._record.guid : null);

For some reason after this patch, I get prompted to enter my MP again after clicking save in the edit dialog which is a regression compared ot without this patch. I already login when the edit dialog opens so things should be logged in… not sure what's going on.

::: browser/extensions/formautofill/test/unit/test_reconcile.js:478
(Diff revision 3)
> +      // The current local record - by comparing against parent we can see that
> +      // only the given-name has changed locally.
> +      "cc-name": "John Doe",
> +      "cc-number": "4444333322221111",

This comment seems stale since it's the cc-number that changed
Attachment #8904840 - Flags: review?(MattN+bmo) → review-
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review183920

> For some reason after this patch, I get prompted to enter my MP again after clicking save in the edit dialog which is a regression compared ot without this patch. I already login when the edit dialog opens so things should be logged in… not sure what's going on.

Oh, the reason is that `MasterPassword.prompt` is to force a re-auth but I don't think we want to force a re-auth in all the places you added it.
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review183920

Yeah, I also think I need Sync team's review for the patch. I was afraid the architecture would change so was thinking of requesting review for the second round.

> Oh, the reason is that `MasterPassword.prompt` is to force a re-auth but I don't think we want to force a re-auth in all the places you added it.

Oh, sorry, my fault! I misunderstood the usage of MasterPassword module. Will update it ASAP.
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review183920

> We should avoid getting in this state but I think returning an empty string may be confusing if we show an empty string in the UI. Can we return `"*".repeat(ccNumber.length)` instead?
> 
> I kinda want this to throw instead so we get bug reports but I'm not sure how well we're guarding about storage never having a number this short. I think we should at least add a log.warn here

Yeah, we can throw here as it's a private method and we actually verify the number beforehand.

> It might be worth having a comment about how this empty string gets handled or what it's for. How would we get in this case? Don't we reject invalid credit card numbers?

I just wanted to follow the convention that computed fields are always present in the storage even it's empty. Though there's no chance to get in this case because we don't allow saving profiles without cc-number.
Hi Kit,

In this new patch I still touched a few parts in sync-related functions. Would you mind taking a look at it again? Thanks.
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review188564

::: browser/extensions/formautofill/FormAutofillParent.jsm:202
(Diff revisions 3 - 4)
> -        // TODO: "MasterPassword.prompt()" can be removed after the storage APIs
> -        //       are refactored to be async functions.
> -        if (await MasterPassword.prompt()) {
> -          this.profileStorage.creditCards.add(data.creditcard);
> +        // TODO: "MasterPassword.waitForExistingDialog()" can be removed after
> +        //       the storage APIs are refactored to be async functions (bug 1399367).
> +        if (MasterPassword.isUIBusy && !await MasterPassword.waitForExistingDialog()) {
> +          log.debug("User canceled master password entry.");

Same here… `waitForExistingDialog` already checks isUIBusy… why do we need it here too?

::: browser/extensions/formautofill/MasterPassword.jsm:38
(Diff revisions 3 - 4)
> -  get isLoggedIn() {
> -    return Services.logins.isLoggedIn;
> +  get isUIBusy() {
> +    return Services.logins.uiBusy;
>    },

I think isLoggedIn is still useful to expose

::: browser/extensions/formautofill/MasterPassword.jsm:91
(Diff revisions 3 - 4)
> -    } else {
> -      loggedIn = await this.waitForExistingDialog();
> +    } else if (this.isUIBusy) {
> +      dialogPassed = await this.waitForExistingDialog();
>      }
>  
> -    if (!loggedIn) {
> +    if (!dialogPassed) {
>        throw Components.Exception("User canceled master password entry", Cr.NS_ERROR_ABORT);

I don't understand why this change is necessary? `waitForExistingDialog` already checks uiBusy

::: browser/extensions/formautofill/content/editDialog.js:267
(Diff revisions 3 - 4)
> -    // TODO: "MasterPassword.prompt()" can be removed after the storage APIs are
> -    //       refactored to be async functions.
> -    if (await MasterPassword.prompt()) {
> +    // TODO: "MasterPassword.waitForExistingDialog()" can be removed after the
> +    //       storage APIs are refactored to be async functions (bug 1399367).
> +    if (!MasterPassword.isUIBusy || await MasterPassword.waitForExistingDialog()) {

`waitForExistingDialog` already handles checking `Services.logins.uiBusy` for you.
Attachment #8904840 - Flags: review?(MattN+bmo)
The main reason I check "isUIBusy" outside "waitForExistingDialog" is that the function currently returns "isLoggedIn" directly if UI isn't busy (and it returns whether the previous dialog succeeds if it's busy). I was also wondering why we check "isUIBusy" twice in "encrypt" function [1]. After consulting with Steve, I realized that we intended not to show dialog twice if the previous dialog is cancelled. To achieve that, we do early-return (by throwing) if "waitForExistingDialog" returns "false" (means the previous dialog is cancelled). However, that causes the dialog would never popup if no previous dialog exists because this case returns "false" (means not "isLoggedIn") as well.

I also feel this design is quite confusing and was thinking we may always return "true" if UI isn't busy. What do you think?


[1] https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/MasterPassword.jsm#105
Flags: needinfo?(MattN+bmo)
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review186412

Thanks, Luke, and sorry for the delay! This is simpler than the previous patch, though I still have some questions. :-) Here's my understanding of how this will work now:

* Getting a CC record calls `_stripComputedFields`, which decrypts `cc-number-encrypted`, then overwrites the masked CC number in `cc-number` with the full plaintext number.

* Saving a CC record calls `_stripComputedFields`, followed by `_computeFields`. When we save, `cc-number` contains the full plaintext, and no `cc-number-encrypted`. In that case, `_stripComputedFields` won't do anything, and `_computeFields` will encrypt the number, store it in `cc-number-encrypted`, then overwrite the plaintext in `cc-number` with the masked CC number.

I have some vague concerns about this:

1. Before, a computed field meant "in memory only, not stored or synced." `cc-number-encrypted` is almost the opposite: we don't sync it, but it *doesn't* exist in memory, and we *do* store it. At first, I wondered if we could treat `cc-number` (decrypted) as a computed field, and treat `cc-number-masked` and `cc-number-encrypted` as internal fields. Sync would then become the exception, though: normally, we sync internal fields and ignore computed fields. In this case, we would sync `cc-number`, and ignore `cc-number-masked` and `cc-number-encrypted`.

2. We overload `cc-number` to mean "masked CC number in storage, unmasked in memory." Is that so we can show something in the autofill UI if the user cancels the master password dialog? Could we consider a separate `cc-number-masked` field and avoid switching between masked and unmasked?

3. Related to 1 and 2, it's a bit hard for me to follow which fields a record contains, and at what times. I think I understand the flow now, but it took me a while. I don't work on this code every day, so my confusion is probably expected, but it would be great to explain how all this works in a comment.

Some other thoughts (I think your patch handles these two cases correctly, but worth keeping in mind):

* Sync stores login credentials in the password manager, so we shouldn't sync if the user cancels the dialog. However, if they do provide the master password for Sync, will we prompt them again when the CC engine syncs?

* Is there any chance we might accidentally sync the masked number, instead of the plaintext, if the user cancels the dialog?

Clearing r? because I'd like to have another look with the questions answered, but I guess my only actionable request is a comment explaining the different CC states. I'll trust yours and Matt's judgment about splitting masked and unmasked into separate fields, or treating `cc-number` instead of `cc-number-encrypted` as a computed field.

::: browser/extensions/formautofill/ProfileStorage.jsm:390
(Diff revision 4)
>        throw new Error("No matching record.");
>      }
> +    this._stripComputedFields(recordFound);
>  
>      // Clone the record by Object assign API to preserve the property with empty string.
>      let recordToUpdate = Object.assign({}, record);

Could we also use `_clone` here, now that it's simpler, for consistency with `add` and `mergeIfPossible`?

::: browser/extensions/formautofill/ProfileStorage.jsm:608
(Diff revision 4)
>     * @returns {Object|null}
>     *          The merged record, or `null` if there are conflicts and the
>     *          records can't be merged.
>     */
>    _mergeSyncedRecords(localRecord, remoteRecord) {
> +    localRecord = this._clone(localRecord);

It's not clear to me why we need `_clone` here. IIUC, `update` strips computed fields directly from `recordFound`; could we just do `_stripComputedFields(localRecord)` here? (Or in `reconcile`, after http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/browser/extensions/formautofill/ProfileStorage.jsm#774).
Attachment #8904840 - Flags: review?(kit)
Kit, really thanks for your comments.

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #33)
> Here's my understanding of how this will work now:
>
> * Getting a CC record calls `_stripComputedFields`, which decrypts
> `cc-number-encrypted`, then overwrites the masked CC number in `cc-number`
> with the full plaintext number.
> 
> * Saving a CC record calls `_stripComputedFields`, followed by
> `_computeFields`. When we save, `cc-number` contains the full plaintext, and
> no `cc-number-encrypted`. In that case, `_stripComputedFields` won't do
> anything, and `_computeFields` will encrypt the number, store it in
> `cc-number-encrypted`, then overwrite the plaintext in `cc-number` with the
> masked CC number.

Your understanding here is correct.

> I have some vague concerns about this:
> 
> 1. Before, a computed field meant "in memory only, not stored or synced."
> `cc-number-encrypted` is almost the opposite: we don't sync it, but it
> *doesn't* exist in memory, and we *do* store it. At first, I wondered if we
> could treat `cc-number` (decrypted) as a computed field, and treat
> `cc-number-masked` and `cc-number-encrypted` as internal fields. Sync would
> then become the exception, though: normally, we sync internal fields and
> ignore computed fields. In this case, we would sync `cc-number`, and ignore
> `cc-number-masked` and `cc-number-encrypted`.

Yes, you're right. The computed fields were not stored initially. We, however, later refactored it to store them in disk for better performance. Though I'm a bit not sure what "`cc-number-encrypted` doesn't exist in memory" means.

> 2. We overload `cc-number` to mean "masked CC number in storage, unmasked in
> memory." Is that so we can show something in the autofill UI if the user
> cancels the master password dialog? Could we consider a separate
> `cc-number-masked` field and avoid switching between masked and unmasked?

In this design, "cc-number" will become unmasked only when "rawData" is specified during public APIs are called. However, your concern is correct. If, for some reasons, we invent an API that only need to return valid fields, we might get decrypted numbers accidentally because of `_stripComputedFields`. 

I'm also considering if we can bring "cc-number-masked" back but looks like it'll lead to bigger refactoring.

> 3. Related to 1 and 2, it's a bit hard for me to follow which fields a
> record contains, and at what times. I think I understand the flow now, but
> it took me a while. I don't work on this code every day, so my confusion is
> probably expected, but it would be great to explain how all this works in a
> comment.

I'll add more comments to explain it. Thanks.

> Some other thoughts (I think your patch handles these two cases correctly,
> but worth keeping in mind):
> 
> * Sync stores login credentials in the password manager, so we shouldn't
> sync if the user cancels the dialog. However, if they do provide the master
> password for Sync, will we prompt them again when the CC engine syncs?

We won't prompt master password dialog again if user has logged in.

> * Is there any chance we might accidentally sync the masked number, instead
> of the plaintext, if the user cancels the dialog?

If `get/getAll` is called with "rawData: true" but a user has never logged in, the master password dialog will be prompted. If the user cancels it, API will throw errors. So "cc-number" should never be masked if "rawData" is specified.
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review186412

> Could we also use `_clone` here, now that it's simpler, for consistency with `add` and `mergeIfPossible`?

Good catch!

> It's not clear to me why we need `_clone` here. IIUC, `update` strips computed fields directly from `recordFound`; could we just do `_stripComputedFields(localRecord)` here? (Or in `reconcile`, after http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/browser/extensions/formautofill/ProfileStorage.jsm#774).

I did `_clone` here becase I noticed `localRecord` will be used later by `_forkLocalRecord` in `reconcile`. I didn't know if `_forkLocalRecord` wants the original version or the stripped version and also don't know if `localRecord` is guaranteed to be replaced in `reconcile` so I intended not to change it in place.

After checking the code again, I found `localRecord` is only used by `_mergeSyncedRecords` and `_forkLocalRecord` and both need it to be stripped. Also, like what `update` does, `localRecord` is always replaced in `reconcile` no matter what path we run into. So changing it in place seems safe. I'll update my patch accordingly. Thanks for pointing it out.
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review186412

> I did `_clone` here becase I noticed `localRecord` will be used later by `_forkLocalRecord` in `reconcile`. I didn't know if `_forkLocalRecord` wants the original version or the stripped version and also don't know if `localRecord` is guaranteed to be replaced in `reconcile` so I intended not to change it in place.
> 
> After checking the code again, I found `localRecord` is only used by `_mergeSyncedRecords` and `_forkLocalRecord` and both need it to be stripped. Also, like what `update` does, `localRecord` is always replaced in `reconcile` no matter what path we run into. So changing it in place seems safe. I'll update my patch accordingly. Thanks for pointing it out.

On the second thought, I decided to also clone `recordFound` in `update` because it can avoid exposing incomplete changes (the decrypted number especially) if errors happen during updating.
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review188564

> `waitForExistingDialog` already handles checking `Services.logins.uiBusy` for you.

According to Comment 32, I changed to use `prompt` instead. Matt, What do you think?
Blocks: 1402963
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review194122

LGTM!

::: browser/extensions/formautofill/ProfileStorage.jsm:89
(Diff revision 5)
>   * }
>   *
> + *
> + * Encrypt-related Credit Card Fields (cc-number & cc-number-encrypted):
> + *
> + * When saving or updating a credit-card record, the storage will encrypt the

This is a great explanation, thanks for adding it!

::: browser/extensions/formautofill/ProfileStorage.jsm:97
(Diff revision 5)
> + * in "_computeFields". We do reverse actions in "_stripComputedFields", which
> + * decrypts "cc-number-encrypted", restores it to "cc-number", and deletes
> + * "cc-number-encrypted". Therefore, calling "_stripComputedFields" followed by
> + * "_computeFields" can make sure the encrypt-related fields are up-to-date.
> + *
> + * In general, you have to decrpyt the number by your own outside ProfileStorage

Nit: s/decrpyt/decrypt
Attachment #8904840 - Flags: review?(kit) → review+
Comment on attachment 8904840 [details]
Bug 1395122 - [Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage.

https://reviewboard.mozilla.org/r/176602/#review198442

::: browser/extensions/formautofill/MasterPassword.jsm:55
(Diff revision 5)
> +   * @param   {boolean} force Prompt the login dialog no matter it's logged in or not.
> +   *                          If it's false, no dialog prompted when it's logged in already.
>     * @returns {Promise<boolean>} True if it's logged in or no password is set and false
>     *                             if it's still not logged in (prompt canceled or other error).
>     */
> -  async prompt() {
> +  async prompt(force = true) {

I think this new implementation is good but I think the name may be confusing since it may not actually show a prompt. How about
```js
async ensureLoggedIn(reauth = false) {
```

I think reauth should default to false since most consumers won't want to force a dialog to appear.
Attachment #8904840 - Flags: review?(MattN+bmo) → review+
Kit and Matt, Thanks a lot for the review.
Flags: needinfo?(MattN+bmo)
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64adf2b4d017
[Form Autofill] Treat "cc-number-encrypted" as a computed field and compute it within the storage. r=kitcambridge,MattN,steveck
https://hg.mozilla.org/mozilla-central/rev/64adf2b4d017
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: