1468644 - Add tests to verify unmasked card number is sent when using a temporary card for a payment

Status: VERIFIED FIXED

(Core :: DOM: Web Payments, defect, P1)

Description

[Sam Foster [:sfoster] (he/him)]

STR: 
1. Load https://paymentrequest.show/demo and click "Buy"
2. Click "add" next to payment method to add a new card
3. Fill in a valid credit card number e.g. 4111111111111111 and a differentiated card holder name e.g. "Temporary card". 
4. Uncheck the "Save credit card" box and click "Save"
5. Click pay. 
6. Observe the response recieved by the paymentrequest.show demo page is printed to the page and includes the credit card number recieved ("cardNumber": "xxxx")

Expected: 
Card number should be the number entered e.g. "4111111111111111"

Actual: 
Card number has all but the last 4 digits masked as "************1111"

Comment 1

[Sam Foster [:sfoster] (he/him)]

This was largely fixed in bug 1463608, which allowed us to use the same code path for both temporary and saved cards. I don't see any tests verifying the response has the expectded card properties though. We could use this bug to add some.

Comment 2

[Sam Foster [:sfoster] (he/him)]

I'm looking into this. I can confirm manually that the cc-number, security code, billing address etc. is present in the response, but an new assertion to verify in browser_card_edit.js fails for non-persisted cards.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1468644 - cc-numbers are always masked in dialog content; tests to verify this and card details in the payment response.

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

Comment 4

[Sam Foster [:sfoster] (he/him)]

I was able to remove some special-casing for the temporary records to get this working. One of the things bug 1463608 did was pass temp records through FormAutofill's computeFields method, which populates the cc-encrypted-number and masks the cc-number. Unless I'm missing something, I don't see a case where we wouldn't have the cc-encrypted-number field, so paymentDialogWrapper's _convertProfileBasicCardToPaymentMethodData is changed to reflect this. 

I tacked the response checking onto the card-adding tests, so that is more end-end. We now verify the card details in the response as well as ensure there's no un-masked card numbers held in the dialog content state object.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8993131 [details]
Bug 1468644 - cc-numbers are always masked in dialog content; tests to verify this and card details in the payment response.

https://reviewboard.mozilla.org/r/257956/#review264868

Maybe Jared can do a closer final review as I'm dealing with more other stuff again.

::: browser/components/payments/test/browser/browser_card_edit.js:192
(Diff revision 1)
> +      // fill in a CVV value
> +      let securityCodeInput = content.document.querySelector(".security-code");
> +      securityCodeInput.value = testArgs.securityCode || "123";// Create a new 'change' event
> +      // Dispatch a change event to trigger update of the state
> +      securityCodeInput.dispatchEvent(new Event("change"));

Can you use https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/payments/test/PaymentTestUtils.jsm#177-184 instead. I don't like manually dispatching events because it means we're not really testing how a user really interacts. Now that we have EventUtils working in dialog content tasks we can pref synthesizing keys and mouse events rather than the resulting change event but for now setUserInput is better.

::: browser/components/payments/test/browser/browser_card_edit.js:219
(Diff revision 1)
> +    let cardDetails = result.response.details;
> +    is(cardDetails.cardNumber, expectedDetails["cc-number"], "Check cc-number");
> +    is(cardDetails.cardSecurityCode, expectedDetails["cc-security-code"], "Check security code");
> +    is(cardDetails.cardholderName, expectedDetails["cc-name"], "Check cc-name");
> +    is(cardDetails.expiryMonth, expectedDetails["cc-exp-month"], "Check cc-exp-month");
> +    is(cardDetails.expiryYear, expectedDetails["cc-exp-year"], "Check cc-exp-year");
> +
> +    ok(cardDetails.billingAddress, "response details should contain the billingAddress");
> +    ok(cardDetails.billingAddress.recipient.includes(expectedBillingAddress["given-name"]),
> +       "Check given-name matches recipient in response");
> +    ok(cardDetails.billingAddress.recipient.includes(expectedBillingAddress["family-name"]),
> +       "Check family-name matches recipient in response");
> +    is(cardDetails.billingAddress.addressLine[0], expectedBillingAddress["street-address"],
> +       "Check street-address in response");
> +    is(cardDetails.billingAddress.country,
> +       expectedBillingAddress.country, "Check country in response");

Can you use the helpers we have for this? https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/payments/test/browser/head.js#193,216

Comment 6

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8993131 [details]
Bug 1468644 - cc-numbers are always masked in dialog content; tests to verify this and card details in the payment response.

https://reviewboard.mozilla.org/r/257956/#review264868

> Can you use https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/payments/test/PaymentTestUtils.jsm#177-184 instead. I don't like manually dispatching events because it means we're not really testing how a user really interacts. Now that we have EventUtils working in dialog content tasks we can pref synthesizing keys and mouse events rather than the resulting change event but for now setUserInput is better.

Ah new utils. I missed this going in.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8993131 [details]
Bug 1468644 - cc-numbers are always masked in dialog content; tests to verify this and card details in the payment response.

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

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8993131 [details]
Bug 1468644 - cc-numbers are always masked in dialog content; tests to verify this and card details in the payment response.

https://reviewboard.mozilla.org/r/257956/#review265454

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c552e6f6cc3a
cc-numbers are always masked in dialog content; tests to verify this and card details in the payment response. r=jaws

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c552e6f6cc3a

Comment 11

[Timea Cernea [:tbabos][inactive]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Verified fixed on the latest Nightly version 63.0a1 (2018-07-30) on Windows 10, Mac 10.13, Ubuntu 16.04.
The card number has all the digits displayed in the printed payment response as expected.