Closed Bug 1468644 Opened Last year Closed Last year

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox63 --- verified

People

(Reporter: sfoster, Assigned: sfoster)

Details

(Whiteboard: [webpayments])

Attachments

(1 file)

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"
Priority: -- → P2
Whiteboard: [webpayments]
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.
Summary: Masked card number is sent when using a temporary card for a payment → Add tests to verify unmasked card number is sent when using a temporary card for a payment
Priority: P2 → P3
Whiteboard: [webpayments] → [webpayments-reserve]
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.
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [webpayments-reserve] → [webpayments]
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 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
Attachment #8993131 - Flags: review?(MattN+bmo)
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 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
Attachment #8993131 - Flags: review?(jaws) → review+
Pushed by sfoster@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c552e6f6cc3a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Mentor: hani.yacoub
Flags: qe-verify+
Mentor: hani.yacoub
QA Contact: hani.yacoub
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.