From 2312a433a901db15650a11a45a4805eb05ab28bf Mon Sep 17 00:00:00 2001 From: Marco Tranchino Date: Fri, 9 Aug 2024 14:56:01 +0100 Subject: [PATCH] Remove code path for Apple Pay validation with request retry With a recent change[1], we have enabled Apple Pay Merchant Validaton with `axios` and `hpagent`. The change has been successfully verified on the Test environment, where the code path with `axios` was switched on using an environment variable[2]. Now that the code path with `axios` has been switched ON also on Production, then we are now able to stop using `requestretry` in pay-frontend by completely deleting the code path using it for the Apple Pay Merchant Validation. Note that whilst working on this code, we have also improved the Pay CLI, and that now it is possible to run Pay locally --with-egress-proxy. For this reason, we are also removing the code path using `axios` without a proxy URL, since it is not production like and it is not needed for local development either. Further information in JIRA[3]. [1] https://github.com/alphagov/pay-frontend/pull/3891 [2] https://github.com/alphagov/pay-infra/pull/4913 [3] https://payments-platform.atlassian.net/browse/PP-12853 --- .secrets.baseline | 4 +- .../merchant-validation.controller.js | 118 +++-------- package-lock.json | 22 --- package.json | 1 - .../merchant-validation.controller.test.js | 36 ---- ...old-merchant-validation.controller.test.js | 184 ------------------ 6 files changed, 28 insertions(+), 337 deletions(-) delete mode 100644 test/controllers/web-payments/apple-pay/old-merchant-validation.controller.test.js diff --git a/.secrets.baseline b/.secrets.baseline index 8f76e74bd..1afdd2a02 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -118,7 +118,7 @@ "filename": "app/controllers/web-payments/apple-pay/merchant-validation.controller.js", "hashed_secret": "1348b145fa1a555461c1b790a2f66614781091e9", "is_verified": false, - "line_number": 19 + "line_number": 16 } ], "test/controllers/web-payments/apple-pay/normalise-apple-pay-payload.test.js": [ @@ -389,5 +389,5 @@ } ] }, - "generated_at": "2024-08-09T08:40:47Z" + "generated_at": "2024-08-09T13:55:54Z" } diff --git a/app/controllers/web-payments/apple-pay/merchant-validation.controller.js b/app/controllers/web-payments/apple-pay/merchant-validation.controller.js index 2d3d96e37..0d671ceec 100644 --- a/app/controllers/web-payments/apple-pay/merchant-validation.controller.js +++ b/app/controllers/web-payments/apple-pay/merchant-validation.controller.js @@ -1,13 +1,10 @@ 'use strict' -const request = require('requestretry') // to be removed once axios is in use const logger = require('../../../utils/logger')(__filename) const { getLoggingFields } = require('../../../utils/logging-fields-helper') const axios = require('axios') const { HttpsProxyAgent } = require('hpagent') const proxyUrl = process.env.HTTPS_PROXY -const applePayMerchantValidationViaAxios = process.env.APPLE_PAY_MERCHANT_VALIDATION_VIA_AXIOS === 'true' - function getCertificateMultiline (cert) { return `-----BEGIN CERTIFICATE----- @@ -54,104 +51,41 @@ module.exports = async (req, res) => { return res.sendStatus(400) } - const httpsAgent = new HttpsProxyAgent({ - proxy: proxyUrl, - cert: merchantIdentityVars.cert, - key: merchantIdentityVars.key - }) - - const axiosInstance = axios.create({ httpsAgent, proxy: false }); - if (proxyUrl) { logger.info('Using proxy URL') + } else { + logger.info('No proxy URL. Warning: this would only happen when running locally without egress proxy.') } - const options = applePayMerchantValidationViaAxios ? - { - url: url, - method: 'post', - cert: merchantIdentityVars.cert, - key: merchantIdentityVars.key, - headers: { 'Content-Type': 'application/json' }, - data: { - merchantIdentifier: merchantIdentityVars.merchantIdentifier, - displayName: 'GOV.UK Pay', - initiative: 'web', - initiativeContext: process.env.APPLE_PAY_MERCHANT_DOMAIN - }, - httpsAgent: httpsAgent - } : - { - url: url, - cert: merchantIdentityVars.cert, - key: merchantIdentityVars.key, - method: 'post', - body: { - merchantIdentifier: merchantIdentityVars.merchantIdentifier, - displayName: 'GOV.UK Pay', - initiative: 'web', - initiativeContext: process.env.APPLE_PAY_MERCHANT_DOMAIN - }, - json: true - } + logger.info('Generating Apple Pay session via axios and https proxy agent (hpagent)') - if (applePayMerchantValidationViaAxios) { - if (proxyUrl) { - logger.info('Generating Apple Pay session via axios and https proxy agent (hpagent)') - - const data = { - merchantIdentifier: merchantIdentityVars.merchantIdentifier, - displayName: 'GOV.UK Pay', - initiative: 'web', - initiativeContext: process.env.APPLE_PAY_MERCHANT_DOMAIN - } + const data = { + merchantIdentifier: merchantIdentityVars.merchantIdentifier, + displayName: 'GOV.UK Pay', + initiative: 'web', + initiativeContext: process.env.APPLE_PAY_MERCHANT_DOMAIN + } - try { - const response = await axiosInstance.post(url, data, { headers: { 'Content-Type': 'application/json; charset=utf-8' } }) + const httpsAgent = new HttpsProxyAgent({ + proxy: proxyUrl, + cert: merchantIdentityVars.cert, + key: merchantIdentityVars.key + }) - logger.info('Apple Pay session successfully generated via axios and https proxy agent (hpagent)') - res.status(200).send(response.data) - } catch (error) { - logger.info('Error generating Apple Pay session with axios and https proxy agent (hpagent)', { - ...getLoggingFields(req), - error: error.message, - status: error.response ? error.response.status : 'No status' - }) - logger.info('Apple Pay session via axios and https proxy agent (hpagent) failed', 'Apple Pay Error') - res.status(500).send('Apple Pay Error') - } - } else { - logger.info('Generating Apple Pay session via axios and https proxy agent (hpagent) (NO PROXY)') - try { - const response = await axios(options) + const axiosInstance = axios.create({ httpsAgent, proxy: false }); - logger.info('Apple Pay session successfully generated via axios and https proxy agent (hpagent) (NO PROXY)') - res.status(200).send(response.data) - } catch (error) { - logger.info('Error generating Apple Pay session (NO PROXY)', { - ...getLoggingFields(req), - error: error.message - }) - logger.info('Apple Pay session via axios and https proxy agent (hpagent) with no proxy failed', 'Apple Pay Error') - res.status(500).send('Apple Pay Error') - } - } + try { + const response = await axiosInstance.post(url, data, { headers: { 'Content-Type': 'application/json; charset=utf-8' } }) - } else { - logger.info('Generating Apple Pay session via request retry') - request(options, (err, response, body) => { - if (err) { - logger.info('Error generating Apple Pay session', { - ...getLoggingFields(req), - error: err, - response: response, - body: body - }) - logger.info('Apple Pay session via request retry failed', body) - return res.status(500).send(body) - } - logger.info('Apple Pay session successfully generated via request retry') - res.status(200).send(body) + logger.info('Apple Pay session successfully generated via axios and https proxy agent (hpagent)') + res.status(200).send(response.data) + } catch (error) { + logger.info('Error generating Apple Pay session with axios and https proxy agent (hpagent)', { + ...getLoggingFields(req), + error: error.message, + status: error.response ? error.response.status : 'No status' }) + logger.info('Apple Pay session via axios and https proxy agent (hpagent) failed', 'Apple Pay Error') + res.status(500).send('Apple Pay Error') } } diff --git a/package-lock.json b/package-lock.json index bf92e861a..ab447f470 100644 --- a/package-lock.json +++ b/package-lock.json @@ -40,7 +40,6 @@ "punycode": "1.4.1", "randomstring": "^1.3.0", "request": "^2.88.2", - "requestretry": "^7.1.0", "serve-favicon": "2.5.0", "staticify": "5.0.x", "throng": "5.0.x", @@ -13974,18 +13973,6 @@ "uuid": "bin/uuid" } }, - "node_modules/requestretry": { - "version": "7.1.0", - "resolved": "https://registry.npmjs.org/requestretry/-/requestretry-7.1.0.tgz", - "integrity": "sha512-TqVDgp251BW4b8ddQ2ptaj/57Z3LZHLscAUT7v6qs70buqF2/IoOVjYbpjJ6HiW7j5+waqegGI8xKJ/+uzgDmw==", - "dependencies": { - "extend": "^3.0.2", - "lodash": "^4.17.15" - }, - "peerDependencies": { - "request": "2.*.*" - } - }, "node_modules/require-directory": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/require-directory/-/require-directory-2.1.1.tgz", @@ -27772,15 +27759,6 @@ "throttleit": "^1.0.0" } }, - "requestretry": { - "version": "7.1.0", - "resolved": "https://registry.npmjs.org/requestretry/-/requestretry-7.1.0.tgz", - "integrity": "sha512-TqVDgp251BW4b8ddQ2ptaj/57Z3LZHLscAUT7v6qs70buqF2/IoOVjYbpjJ6HiW7j5+waqegGI8xKJ/+uzgDmw==", - "requires": { - "extend": "^3.0.2", - "lodash": "^4.17.15" - } - }, "require-directory": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/require-directory/-/require-directory-2.1.1.tgz", diff --git a/package.json b/package.json index 6b1e31cc6..e03465434 100644 --- a/package.json +++ b/package.json @@ -101,7 +101,6 @@ "punycode": "1.4.1", "randomstring": "^1.3.0", "request": "^2.88.2", - "requestretry": "^7.1.0", "serve-favicon": "2.5.0", "staticify": "5.0.x", "throng": "5.0.x", diff --git a/test/controllers/web-payments/apple-pay/merchant-validation.controller.test.js b/test/controllers/web-payments/apple-pay/merchant-validation.controller.test.js index 9764dfe5d..c98805cce 100644 --- a/test/controllers/web-payments/apple-pay/merchant-validation.controller.test.js +++ b/test/controllers/web-payments/apple-pay/merchant-validation.controller.test.js @@ -38,7 +38,6 @@ describe('Validate with Apple the merchant is legitimate', () => { process.env.STRIPE_APPLE_PAY_MERCHANT_ID = stripeMerchantId process.env.STRIPE_APPLE_PAY_MERCHANT_ID_CERTIFICATE = stripeCertificate process.env.STRIPE_APPLE_PAY_MERCHANT_ID_CERTIFICATE_KEY = stripeKey - process.env.APPLE_PAY_MERCHANT_VALIDATION_VIA_AXIOS = 'true' sendSpy = sinon.spy() res = { @@ -47,41 +46,6 @@ describe('Validate with Apple the merchant is legitimate', () => { } }) - describe('when running locally with no proxy', () => { - it('should return a payload for a Worldpay payment if Merchant is valid', async () => { - const axiosStub = sinon.stub().resolves(appleResponse) - const controller = getControllerWithMocks(axiosStub) - - const req = { - body: { - url, - paymentProvider: 'worldpay' - } - } - await controller(req, res) - - sinon.assert.calledOnce(axiosStub) - const axiosCallArg = axiosStub.getCall(0).args[0] - - sinon.assert.match(axiosCallArg, { - url: url, - method: 'post', - cert: sinon.match(cert => cert.includes(worldpayCertificate)), - key: sinon.match(key => key.includes(worldpayKey)), - headers: { 'Content-Type': 'application/json' }, - data: { - merchantIdentifier: worldpayMerchantId, - displayName: 'GOV.UK Pay', - initiative: 'web', - initiativeContext: merchantDomain - } - }) - - sinon.assert.calledWith(res.status, 200) - sinon.assert.calledWith(sendSpy, appleResponse.data) - }) - }) - describe('when there is a proxy', () => { beforeEach(() => { process.env.HTTPS_PROXY = 'https://fakeproxy.com' diff --git a/test/controllers/web-payments/apple-pay/old-merchant-validation.controller.test.js b/test/controllers/web-payments/apple-pay/old-merchant-validation.controller.test.js deleted file mode 100644 index 3ba68ef8a..000000000 --- a/test/controllers/web-payments/apple-pay/old-merchant-validation.controller.test.js +++ /dev/null @@ -1,184 +0,0 @@ -'use strict' - -const sinon = require('sinon') -const proxyquire = require('proxyquire') - -const merchantDomain = 'www.pymnt.uk' -const worldpayMerchantId = 'worldpay.merchant.id' -const worldpayCertificate = 'A-WORLDPAY-CERTIFICATE' -const worldpayKey = 'A-WORLDPAY-KEY' -const stripeMerchantId = 'stripe.merchant.id' -const stripeCertificate = 'A-STRIPE-CERTIFICATE' -const stripeKey = 'A-STRIPE-KEY' -const url = 'https://fakeapple.url' - -const oldAppleResponse = { status: 200 } -const oldAppleResponseBody = { foo: 'bar' } - -function getControllerWithMocks (requestMock) { - return proxyquire('../../../../app/controllers/web-payments/apple-pay/merchant-validation.controller', { - requestretry: requestMock - }) -} - -describe('Validate with Apple the merchant is legitimate', () => { - let res, sendSpy - - beforeEach(() => { - process.env.APPLE_PAY_MERCHANT_DOMAIN = merchantDomain - process.env.WORLDPAY_APPLE_PAY_MERCHANT_ID = worldpayMerchantId - process.env.WORLDPAY_APPLE_PAY_MERCHANT_ID_CERTIFICATE = worldpayCertificate - process.env.WORLDPAY_APPLE_PAY_MERCHANT_ID_CERTIFICATE_KEY = worldpayKey - process.env.STRIPE_APPLE_PAY_MERCHANT_ID = stripeMerchantId - process.env.STRIPE_APPLE_PAY_MERCHANT_ID_CERTIFICATE = stripeCertificate - process.env.STRIPE_APPLE_PAY_MERCHANT_ID_CERTIFICATE_KEY = stripeKey - process.env.APPLE_PAY_MERCHANT_VALIDATION_VIA_AXIOS = 'false' - - sendSpy = sinon.spy() - res = { - status: sinon.spy(() => ({ send: sendSpy })), - sendStatus: sinon.spy() - } - }) - - it('should return a payload for a Worldpay payment if Merchant is valid', async () => { - const mockRequest = sinon.stub().yields(null, oldAppleResponse, oldAppleResponseBody) - const controller = getControllerWithMocks(mockRequest) - - const req = { - body: { - url, - paymentProvider: 'worldpay' - } - } - await controller(req, res) - - sinon.assert.calledWith(mockRequest, { - url, - body: { - merchantIdentifier: worldpayMerchantId, - displayName: 'GOV.UK Pay', - initiative: 'web', - initiativeContext: merchantDomain - }, - method: 'post', - json: true, - cert: `-----BEGIN CERTIFICATE----- -${worldpayCertificate} ------END CERTIFICATE-----`, - key: `-----BEGIN PRIVATE KEY----- -${worldpayKey} ------END PRIVATE KEY-----` - }) - sinon.assert.calledWith(res.status, 200) - sinon.assert.calledWith(sendSpy, oldAppleResponseBody) - }) - - it('should return a payload for a Stripe payment if Merchant is valid', async () => { - const mockRequest = sinon.stub().yields(null, oldAppleResponse, oldAppleResponseBody) - const controller = getControllerWithMocks(mockRequest) - - const req = { - body: { - url, - paymentProvider: 'stripe' - } - } - await controller(req, res) - - sinon.assert.calledWith(mockRequest, { - url, - body: { - merchantIdentifier: stripeMerchantId, - displayName: 'GOV.UK Pay', - initiative: 'web', - initiativeContext: merchantDomain - }, - method: 'post', - json: true, - cert: `-----BEGIN CERTIFICATE----- -${stripeCertificate} ------END CERTIFICATE-----`, - key: `-----BEGIN PRIVATE KEY----- -${stripeKey} ------END PRIVATE KEY-----` - }) - sinon.assert.calledWith(res.status, 200) - sinon.assert.calledWith(sendSpy, oldAppleResponseBody) - }) - - it('should return 400 if no url is provided', async () => { - const mockRequest = sinon.stub().yields(null, oldAppleResponse, oldAppleResponseBody) - const controller = getControllerWithMocks(mockRequest) - - const req = { - body: { - paymentProvider: 'worldpay' - } - } - await controller(req, res) - sinon.assert.calledWith(res.sendStatus, 400) - }) - - it('should return a payload for a Sandbox payment if Merchant is valid', async () => { - const mockRequest = sinon.stub().yields(null, oldAppleResponse, oldAppleResponseBody) - const controller = getControllerWithMocks(mockRequest) - - const req = { - body: { - url, - paymentProvider: 'sandbox' - } - } - await controller(req, res) - - sinon.assert.calledWith(mockRequest, { - url, - body: { - merchantIdentifier: worldpayMerchantId, - displayName: 'GOV.UK Pay', - initiative: 'web', - initiativeContext: merchantDomain - }, - method: 'post', - json: true, - cert: `-----BEGIN CERTIFICATE----- -${worldpayCertificate} ------END CERTIFICATE-----`, - key: `-----BEGIN PRIVATE KEY----- -${worldpayKey} ------END PRIVATE KEY-----` - }) - sinon.assert.calledWith(res.status, 200) - sinon.assert.calledWith(sendSpy, oldAppleResponseBody) - }) - - it('should return 400 for unexpected payment provider', async () => { - const mockRequest = sinon.stub().yields(null, oldAppleResponse, oldAppleResponseBody) - const controller = getControllerWithMocks(mockRequest) - - const req = { - body: { - url, - paymentProvider: 'mystery' - } - } - await controller(req, res) - sinon.assert.calledWith(res.sendStatus, 400) - }) - - it('should return an error if Apple Pay returns an error', async () => { - const mockRequest = sinon.stub().yields(new Error(), oldAppleResponse, oldAppleResponseBody) - const controller = getControllerWithMocks(mockRequest) - - const req = { - body: { - url, - paymentProvider: 'worldpay' - } - } - await controller(req, res) - sinon.assert.calledWith(res.status, 500) - sinon.assert.calledWith(sendSpy, oldAppleResponseBody) - }) -})