1394075 - [Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Luke Chang [:lchang] (inactive)]

Since the crypto API can only be used in chrome process, we need to find out a way to decrypt them in FormAutofillParent. My proposal is to decrypt them in "_getRecords" and cache them in records as "cc-number-decrypted".

Comment 1

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1394075 - [Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set.

Review commit: https://reviewboard.mozilla.org/r/172906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172906/

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8901419 [details]
Bug 1394075 - [Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set.

https://reviewboard.mozilla.org/r/172906/#review178396

::: browser/extensions/formautofill/FormAutofillParent.jsm:250
(Diff revision 1)
>      Services.prefs.removeObserver(ENABLED_AUTOFILL_ADDRESSES_PREF, this);
>      Services.prefs.removeObserver(ENABLED_AUTOFILL_CREDITCARDS_PREF, this);
>    },
>  
>    /**
>     * Get the records from profile store and return results back to content

nit: We can add more description about returning the result with decrpyed cc-number only in no mp case.

::: browser/extensions/formautofill/FormAutofillParent.jsm:277
(Diff revision 1)
> +      target.sendAsyncMessage("FormAutofill:Records", recordsInCollection);
> +      return;
> +    }
> +
>      let records = [];
> -    if (info && info.fieldName &&
> +    let isMPEnabled = collectionName == CREDITCARDS_COLLECTION_NAME && MasterPassword.isEnabled;

nit: I guess the purpose of checking `collectionName == CREDITCARDS_COLLECTION_NAME` is for avoiding the the unnecessary MasterPassword.isEnabled API calling, but the name variable `isMPEnabled` is not clear that it's specific to cc only. Maybe `isMPEnabledCC` or something?

::: browser/extensions/formautofill/FormAutofillParent.jsm:294
(Diff revision 1)
> -          //       (Bug 1358941)
> -          let name = record[info.fieldName];
>  
> -          if (!searchString) {
> -            return !!name;
> +      if (info.fieldName == "cc-number") {
> +        // We don't filter "cc-number" when MasterPassword is set.
> +        if (isMPEnabled) {

I might assume that we can return getAll directly once the `isMPEnabled` is true.

Comment 3

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901419 [details]
Bug 1394075 - [Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set.

https://reviewboard.mozilla.org/r/172906/#review178400

::: browser/extensions/formautofill/FormAutofillParent.jsm:294
(Diff revision 1)
> -          //       (Bug 1358941)
> -          let name = record[info.fieldName];
>  
> -          if (!searchString) {
> -            return !!name;
> +      if (info.fieldName == "cc-number") {
> +        // We don't filter "cc-number" when MasterPassword is set.
> +        if (isMPEnabled) {

We can return `getAll` directly if `isMPEnabled` is true, `info.fieldName` is "cc-number" and `record["cc-number"]` isn't empty. It'll look like:

```js
if (isMPEnabled && info.fieldName == "cc-number") {
  return recordsInCollection.filter(record => !!record[cc-number"]);
}
```

I don't have a strong opinion so I'll change to this if you're fine with that.

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901419 [details]
Bug 1394075 - [Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set.

https://reviewboard.mozilla.org/r/172906/#review178402

::: browser/extensions/formautofill/FormAutofillParent.jsm:277
(Diff revision 1)
> +      target.sendAsyncMessage("FormAutofill:Records", recordsInCollection);
> +      return;
> +    }
> +
>      let records = [];
> -    if (info && info.fieldName &&
> +    let isMPEnabled = collectionName == CREDITCARDS_COLLECTION_NAME && MasterPassword.isEnabled;

Yes, I intended to avoid loading MasterPassword.jsm when unnecessary.

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901419 [details]
Bug 1394075 - [Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172906/diff/1-2/

Comment 6

[Luke Chang [:lchang] (inactive)]

Updated. Thanks.

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8901419 [details]
Bug 1394075 - [Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set.

https://reviewboard.mozilla.org/r/172906/#review178410

::: browser/extensions/formautofill/FormAutofillParent.jsm:279
(Diff revisions 1 - 2)
>        return;
>      }
>  
> +    let isCCAndMPEnabled = collectionName == CREDITCARDS_COLLECTION_NAME && MasterPassword.isEnabled;
> +    // We don't filter "cc-number" when MasterPassword is set.
> +    if (isCCAndMPEnabled && info.fieldName == "cc-number") {

nit: Maybe I missed other edge case, but I still think it's not possible to have a record without cc-number since we'll check if the number is valid before saving. Thta's why I'll suggest that we can return recordsInCollection directly without filtering.

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901419 [details]
Bug 1394075 - [Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set.

https://reviewboard.mozilla.org/r/172906/#review178410

> nit: Maybe I missed other edge case, but I still think it's not possible to have a record without cc-number since we'll check if the number is valid before saving. Thta's why I'll suggest that we can return recordsInCollection directly without filtering.

Yeah, there isn't any chance in our workflow that is able to create a record without "cc-number". Though I think it's still worth to check since that limitation happens outside ProfileStorage.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28aa1a0038ce
[Form Autofill] Preview decrypted credit card numbers when MasterPassword isn't set. r=steveck

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/28aa1a0038ce