HPS: add Apple Pay raw cryptogram support #3209

slogsdon · 2019-05-07T13:04:32Z

No description provided.

shasum · 2019-06-19T15:07:36Z

Just a heads up. We are looking into this at Spreedly. I'll coordinate through the review and merge process. Thanks! for submitting this feature.

shasum

Looks good overall. Marked some minor feedback separately.

As a potentially sensitive field, it's recommended to scrub out the cryptogram. It'd be good to add it to the scrub method and verify it in the tests.

shasum · 2019-06-20T19:11:19Z

lib/active_merchant/billing/gateways/hps.rb

@@ -46,6 +65,17 @@ def purchase(money, card_or_token, options={})
          add_details(xml, options)
          add_descriptor_name(xml, options)
          add_payment(xml, card_or_token, options)
+
+          if card_or_token.is_a?(NetworkTokenizationCreditCard)


Could this be factored out as a separate method that gets invoked from both authorize and purchase?

Not a problem. I'll get this implemented.

Done via 0beea4d1aee082c944c4c9e6e44946d3000023f5.

shasum · 2019-06-20T20:47:06Z

lib/active_merchant/billing/gateways/hps.rb

+          xml.hps :PaymentData, three_d_secure[:cavv] if three_d_secure[:cavv]
+          # the gateway only allows a single character for the ECI
+          xml.hps :ECommerceIndicator, strip_leading_zero(three_d_secure[:eci]) if three_d_secure[:eci]
+          xml.hps :XID, three_d_secure[:xid] if three_d_secure[:xid]


Does it make sense to map transaction_id to XID, if it's available on a NetworkTokenizationCreditCard?

Ah. I must have missed that on the NetworkTokenizationCreditCard object. I'll refactor.

Done via 8fe3ace525e0818e5398e3b7be7c0b8ebc6e8942.

Yeah, that attribute is not immediately apparent, kinda buried in there. Refactor looks good.

shasum · 2019-06-20T20:56:42Z

test/remote/gateways/remote_hps_test.rb

+    assert_equal 'Success', response.message
+  end
+
+  def test_successful_auth_with_apple_pay_raw_cryptogram_with_eci


Would it be useful to also cover the capture part of an Apple Pay authorization?

I had copied the test cases from the tests for Stripe. I can add an additional test for capturing an Apple Pay transaction, but it will perform a nearly identical capture as a normal open authorization.

Cool. Just wanted to check if it provides additional coverage specific to this payment method. Sounds like that's not the case, I'm fine as is.

shasum · 2019-06-21T18:12:22Z

@slogsdon Thank you for pushing the updates. They look good. I think this previous comment of mine got buried among other stuff. Could you please also address this.

As a potentially sensitive field, it's recommended to scrub out the cryptogram. It'd be good to add it to the scrub method and verify it in the tests.

slogsdon · 2019-06-21T18:20:21Z

@shasum Apologies for missing that. Done via d0bdc85f39c8264c9c93ce70e68d5ed230e6756c.

shasum · 2019-06-21T19:30:59Z

@slogsdon Awesome. Sorry to nitpick, there's also a scrub test buried under remote tests, do you mind adding an assertion there. Thanks.

slogsdon · 2019-06-21T20:52:14Z

@shasum Not a problem. I've added an additional remote test with the assertion in dc60c6d3d99666239d28ee507c472612540c701b.

shasum

@slogsdon Thank you. Everything looks good to me. I'm going to check with our internal team that works on Active Merchant, in terms of next steps to have this merged.

bayprogrammer

Have a question about whether we should add some tests for the 3DS options hash, but otherwise looks good to me.

bayprogrammer · 2019-06-26T17:27:02Z

lib/active_merchant/billing/gateways/hps.rb

+            eci: card_or_token.eci,
+            xid: card_or_token.transaction_id,
+          })
+        elsif options[:three_d_secure]


Should we test the :three_d_secure options hash path as well? I didn't see any new or existing tests exercising this logic.

Good catch. That'd be a good thing to add. @slogsdon Thoughts?

Done as of 76859d924f9d05cea7aa0115bc219c2a8a0ddacc.

Looks great! Thank you @slogsdon

@bayprogrammer Are we good to go on this?

bayprogrammer

I noticed one more improvement we could make to strengthen the assertions in the new tests (left suggested changes in comment), but otherwise I think this is good to go.

bayprogrammer · 2019-07-03T14:46:34Z

test/unit/gateways/hps_test.rb

+    end.check_request do |method, endpoint, data, headers|
+      assert_match(/<hps:SecureECommerce>(.*)<\/hps:SecureECommerce>/, data)
+      assert_match(/<hps:PaymentDataSource>Visa 3DSecure<\/hps:PaymentDataSource>/, data)
+    end.respond_with(successful_charge_response)


I would expand the assertions for the new 3DS tests to add something like the following (which verifies the ECI leading-zero-stripping is happening as expected and that the other values from the 3DS options are correctly input into the resultant XML request document):

assert_match(/<hps:PaymentData>#{options[:three_d_secure][:cavv]}<\/hps:PaymentData>/, data) assert_match(/<hps:ECommerceIndicator>5<\/hps:ECommerceIndicator>/, data) assert_match(/<hps:XID>#{options[:three_d_secure][:xid]}<\/hps:XID>/, data)

Sounds a good improvement to add coverage around ECI zero stripping and 3DS. @slogsdon Could we have that updated pls. Looks like we are almost there.

Done via cc218668e61a6c53ab0f415de156455e20411e74.

bayprogrammer

🎉 @slogsdon @shasum Thanks so much for the revisions. Looks great!

Unit: 34 tests, 171 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 40 tests, 102 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Closes activemerchant#3209

shasum assigned slogsdon Jun 19, 2019

shasum self-requested a review June 19, 2019 15:08

shasum reviewed Jun 20, 2019

View reviewed changes

shasum added the gateway/feature Adds a new feature to an existing gateway label Jun 20, 2019

jknipp requested a review from a team June 21, 2019 16:05

shasum approved these changes Jun 24, 2019

View reviewed changes

bayprogrammer reviewed Jun 26, 2019

View reviewed changes

bayprogrammer suggested changes Jul 3, 2019

View reviewed changes

bayprogrammer approved these changes Jul 8, 2019

View reviewed changes

shasum force-pushed the hps-add-apple-pay-raw branch from cc21866 to d14c276 Compare July 9, 2019 19:42

HPS: Add Apple Pay raw cryptogram support

c387d6f

Unit: 34 tests, 171 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 40 tests, 102 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Closes activemerchant#3209

shasum force-pushed the hps-add-apple-pay-raw branch from d14c276 to c387d6f Compare July 9, 2019 19:56

shasum merged commit c387d6f into activemerchant:master Jul 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPS: add Apple Pay raw cryptogram support #3209

HPS: add Apple Pay raw cryptogram support #3209

slogsdon commented May 7, 2019

shasum commented Jun 19, 2019 •

edited

Loading

shasum left a comment

shasum Jun 20, 2019

slogsdon Jun 21, 2019

slogsdon Jun 21, 2019

shasum Jun 21, 2019

shasum Jun 20, 2019

slogsdon Jun 21, 2019

slogsdon Jun 21, 2019

shasum Jun 21, 2019

shasum Jun 20, 2019

slogsdon Jun 21, 2019

shasum Jun 21, 2019

shasum commented Jun 21, 2019

slogsdon commented Jun 21, 2019

shasum commented Jun 21, 2019

slogsdon commented Jun 21, 2019

shasum left a comment

bayprogrammer left a comment

bayprogrammer Jun 26, 2019

shasum Jun 26, 2019

slogsdon Jul 1, 2019

slogsdon Jul 2, 2019

shasum Jul 2, 2019

bayprogrammer left a comment

bayprogrammer Jul 3, 2019

shasum Jul 3, 2019

slogsdon Jul 8, 2019

bayprogrammer left a comment

HPS: add Apple Pay raw cryptogram support #3209

HPS: add Apple Pay raw cryptogram support #3209

Conversation

slogsdon commented May 7, 2019

shasum commented Jun 19, 2019 • edited Loading

shasum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shasum commented Jun 21, 2019

slogsdon commented Jun 21, 2019

shasum commented Jun 21, 2019

slogsdon commented Jun 21, 2019

shasum left a comment

Choose a reason for hiding this comment

bayprogrammer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bayprogrammer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bayprogrammer left a comment

Choose a reason for hiding this comment

shasum commented Jun 19, 2019 •

edited

Loading