From 05c81e0f04a3f0e9024af308357abb050d9da9de Mon Sep 17 00:00:00 2001 From: Mark Bridgett Date: Fri, 17 Aug 2018 11:28:51 +0100 Subject: [PATCH] PP-4132 Added sub-segmenting to instrument the card_check request path, middleware and controller - modified tests to add a @global stub for x-ray and continuation-local-storage that is not required in unit testing --- app/controllers/charge_controller.js | 25 ++++++-- app/middleware/retrieve_charge.js | 30 +++++++--- app/middleware/x_ray.js | 2 +- app/models/card.js | 78 +++++++++++++++---------- app/models/charge.js | 4 +- app/router.js | 23 ++++---- app/utils/base_client.js | 10 ++-- app/utils/base_client2.js | 2 +- app/utils/cardid_client.js | 2 +- aws-xray.rules | 2 +- package-lock.json | 2 +- test/integration/charge_ft_tests.js | 68 ++++++++++++++------- test/middleware/retrieve_charge_test.js | 20 ++++++- test/models/card_test.js | 20 ++++++- test/test_helpers/test_helpers.js | 14 +++-- 15 files changed, 206 insertions(+), 96 deletions(-) diff --git a/app/controllers/charge_controller.js b/app/controllers/charge_controller.js index 1181b8bad..1d31ed0e8 100644 --- a/app/controllers/charge_controller.js +++ b/app/controllers/charge_controller.js @@ -4,6 +4,8 @@ const logger = require('winston') const _ = require('lodash') const i18n = require('i18n') +const {getNamespace} = require('continuation-local-storage') +const AWSXRay = require('aws-xray-sdk') // local dependencies const logging = require('../utils/logging.js') @@ -35,6 +37,7 @@ const AUTH_3DS_EPDQ_RESULTS = { declined: 'DECLINED', error: 'ERROR' } +const clsXrayConfig = require('../../config/xray-cls') function appendChargeForNewView (charge, req, chargeId) { const cardModel = Card(charge.gatewayAccount.cardTypes, req.headers[CORRELATION_HEADER]) @@ -170,12 +173,22 @@ module.exports = { }) }, checkCard: (req, res) => { - Card(req.chargeData.gateway_account.card_types, req.headers[CORRELATION_HEADER]) - .checkCard(normalise.creditCard(req.body.cardNo)) - .then( - () => res.json({'accepted': true}), - message => res.json({'accepted': false, message}) - ) + const namespace = getNamespace(clsXrayConfig.nameSpaceName) + const clsSegment = namespace.get(clsXrayConfig.segmentKeyName) + AWSXRay.captureAsyncFunc('Card_checkCard', function (subsegment) { + Card(req.chargeData.gateway_account.card_types, req.headers[CORRELATION_HEADER]) + .checkCard(normalise.creditCard(req.body.cardNo), subsegment) + .then( + () => { + subsegment.close() + return res.json({'accepted': true}) + }, + message => { + subsegment.close(message) + return res.json({'accepted': false, message}) + } + ) + }, clsSegment) }, authWaiting: (req, res) => { const charge = normalise.charge(req.chargeData, req.chargeId) diff --git a/app/middleware/retrieve_charge.js b/app/middleware/retrieve_charge.js index 5d0fdc14a..ba39fc2a3 100644 --- a/app/middleware/retrieve_charge.js +++ b/app/middleware/retrieve_charge.js @@ -1,5 +1,9 @@ 'use strict' +// NPM dependencies +const AWSXRay = require('aws-xray-sdk') +const {getNamespace} = require('continuation-local-storage') + // local dependencies const views = require('../utils/views.js') const Charge = require('../models/charge.js') @@ -7,20 +11,28 @@ const chargeParam = require('../services/charge_param_retriever.js') const CORRELATION_HEADER = require('../utils/correlation_header.js').CORRELATION_HEADER const withAnalyticsError = require('../utils/analytics.js').withAnalyticsError +// constants +const clsXrayConfig = require('../../config/xray-cls') + module.exports = (req, res, next) => { const chargeId = chargeParam.retrieve(req) - + const namespace = getNamespace(clsXrayConfig.nameSpaceName) + const clsSegment = namespace.get(clsXrayConfig.segmentKeyName) if (!chargeId) { views.display(res, 'UNAUTHORISED', withAnalyticsError()) } else { req.chargeId = chargeId - Charge(req.headers[CORRELATION_HEADER]).find(chargeId) - .then(data => { - req.chargeData = data - next() - }) - .catch(() => { - views.display(res, 'SYSTEM_ERROR', withAnalyticsError()) - }) + AWSXRay.captureAsyncFunc('Charge_find', function (subsegment) { + Charge(req.headers[CORRELATION_HEADER]).find(chargeId) + .then(data => { + subsegment.close() + req.chargeData = data + next() + }) + .catch(() => { + subsegment.close('error') + views.display(res, 'SYSTEM_ERROR', withAnalyticsError()) + }) + }, clsSegment) } } diff --git a/app/middleware/x_ray.js b/app/middleware/x_ray.js index 10126912a..a49d60163 100644 --- a/app/middleware/x_ray.js +++ b/app/middleware/x_ray.js @@ -1,7 +1,7 @@ 'use strict' // NPM dependencies -const getNamespace = require('continuation-local-storage').getNamespace +const {getNamespace} = require('continuation-local-storage') // Local dependencies const clsXrayConfig = require('../../config/xray-cls') diff --git a/app/models/card.js b/app/models/card.js index e38378f66..a9f76d91e 100644 --- a/app/models/card.js +++ b/app/models/card.js @@ -5,51 +5,65 @@ const _ = require('lodash') const q = require('q') const changeCase = require('change-case') const logger = require('winston') +const AWSXRay = require('aws-xray-sdk') +const {getNamespace} = require('continuation-local-storage') // local dependencies const cardIdClient = require('../utils/cardid_client') -const checkCard = function (cardNo, allowed, correlationId) { - const defer = q.defer() +// Constants +const clsXrayConfig = require('../../config/xray-cls') +const checkCard = function (cardNo, allowed, correlationId, subSegment) { + const defer = q.defer() const startTime = new Date() const data = {'cardNumber': parseInt(cardNo)} - cardIdClient.post({data: data, correlationId: correlationId}, function (data, response) { - logger.info(`[${correlationId}] - %s to %s ended - total time %dms`, 'POST', cardIdClient.CARD_URL, new Date() - startTime) + // Use a subSegment if passed, otherwise get our main segment + if (!subSegment) { + const namespace = getNamespace(clsXrayConfig.nameSpaceName) + subSegment = namespace.get(clsXrayConfig.segmentKeyName) + } - if (response.statusCode === 404) { - return defer.reject('Your card is not supported') - } - // if the server is down, or returns non 500, just continue - if (response.statusCode !== 200) { - return defer.resolve() - } + AWSXRay.captureAsyncFunc('cardIdClient_post', function (postSubsegment) { + cardIdClient.post({data: data, correlationId: correlationId}, function (data, response) { + postSubsegment.close() + logger.info(`[${correlationId}] - %s to %s ended - total time %dms`, 'POST', cardIdClient.CARD_URL, new Date() - startTime) - const cardBrand = changeCase.paramCase(data.brand) - const cardType = normaliseCardType(data.type) + if (response.statusCode === 404) { + return defer.reject('Your card is not supported') + } + // if the server is down, or returns non 500, just continue + if (response.statusCode !== 200) { + return defer.resolve() + } + + const cardBrand = changeCase.paramCase(data.brand) + const cardType = normaliseCardType(data.type) - logger.debug(`[${correlationId}] Checking card brand - `, {'cardBrand': cardBrand, 'cardType': cardType}) + logger.debug(`[${correlationId}] Checking card brand - `, {'cardBrand': cardBrand, 'cardType': cardType}) - const brandExists = _.filter(allowed, {brand: cardBrand}).length > 0 - if (!brandExists) defer.reject(changeCase.titleCase(cardBrand) + ' is not supported') + const brandExists = _.filter(allowed, {brand: cardBrand}).length > 0 + if (!brandExists) defer.reject(changeCase.titleCase(cardBrand) + ' is not supported') - const cardObject = _.find(allowed, {brand: cardBrand, type: cardType}) + const cardObject = _.find(allowed, {brand: cardBrand, type: cardType}) - if (!cardObject) { - switch (cardType) { - case 'DEBIT': - return defer.reject(changeCase.titleCase(cardBrand) + ' debit cards are not supported') - case 'CREDIT': - return defer.reject(changeCase.titleCase(cardBrand) + ' credit cards are not supported') + if (!cardObject) { + switch (cardType) { + case 'DEBIT': + return defer.reject(changeCase.titleCase(cardBrand) + ' debit cards are not supported') + case 'CREDIT': + return defer.reject(changeCase.titleCase(cardBrand) + ' credit cards are not supported') + } } - } - return defer.resolve(cardBrand) - }).on('error', function (error) { - logger.error(`[${correlationId}] ERROR CALLING CARDID AT ${cardIdClient.CARD_URL}`, error) - logger.info(`[${correlationId}] - %s to %s ended - total time %dms`, 'POST', cardIdClient.cardUrl, new Date() - startTime) - defer.resolve() - }) + return defer.resolve(cardBrand) + }, postSubsegment).on('error', function (error) { + postSubsegment.close(error) + logger.error(`[${correlationId}] ERROR CALLING CARDID AT ${cardIdClient.CARD_URL}`, error) + logger.info(`[${correlationId}] - %s to %s ended - total time %dms`, 'POST', cardIdClient.cardUrl, new Date() - startTime) + defer.resolve() + }) + }, subSegment) return defer.promise } @@ -74,6 +88,8 @@ module.exports = function (allowedCards, correlationId) { return { withdrawalTypes: withdrawalTypes, allowed: _.clone(allowed), - checkCard: (cardNo) => { return checkCard(cardNo, allowed, correlationId) } + checkCard: (cardNo, subSegment) => { + return checkCard(cardNo, allowed, correlationId, subSegment) + } } } diff --git a/app/models/charge.js b/app/models/charge.js index f1b5aa0cd..fb46eae65 100644 --- a/app/models/charge.js +++ b/app/models/charge.js @@ -53,7 +53,7 @@ module.exports = function (correlationId) { return defer.promise } - const find = function (chargeId) { + const find = function (chargeId, subSegment) { const defer = q.defer() const url = connectorurl('show', {chargeId: chargeId}) @@ -78,7 +78,7 @@ module.exports = function (correlationId) { return defer.reject(new Error('GET_FAILED')) } defer.resolve(data) - }).on('error', function (err) { + }, subSegment).on('error', function (err) { logger.info('[%s] - %s to %s ended - total time %dms', correlationId, 'GET', url, new Date() - startTime) logger.error('[%s] Calling connector to get charge threw exception -', correlationId, { service: 'connector', diff --git a/app/router.js b/app/router.js index b601ab02f..fa4009782 100644 --- a/app/router.js +++ b/app/router.js @@ -59,7 +59,6 @@ exports.bind = function (app) { const middlewareStack = [ xraySegmentCls, - resolveLanguage, csrfCheck, csrfTokenGeneration, actionName, @@ -73,28 +72,30 @@ exports.bind = function (app) { app.get(card.authWaiting.path, middlewareStack, charge.authWaiting) app.get(card.auth3dsRequired.path, middlewareStack, charge.auth3dsRequired) app.get(card.auth3dsRequiredOut.path, middlewareStack, charge.auth3dsRequiredOut) - app.post(card.auth3dsRequiredInEpdq.path, [csrfTokenGeneration, retrieveCharge], charge.auth3dsRequiredInEpdq) - app.get(card.auth3dsRequiredInEpdq.path, [csrfTokenGeneration, retrieveCharge], charge.auth3dsRequiredInEpdq) - app.post(card.auth3dsRequiredIn.path, [csrfTokenGeneration, retrieveCharge], charge.auth3dsRequiredIn) - app.get(card.auth3dsRequiredIn.path, [csrfTokenGeneration, retrieveCharge], charge.auth3dsRequiredIn) + app.post(card.auth3dsRequiredInEpdq.path, [xraySegmentCls, csrfTokenGeneration, retrieveCharge], charge.auth3dsRequiredInEpdq) + app.get(card.auth3dsRequiredInEpdq.path, [xraySegmentCls, csrfTokenGeneration, retrieveCharge], charge.auth3dsRequiredInEpdq) + app.post(card.auth3dsRequiredIn.path, [xraySegmentCls, csrfTokenGeneration, retrieveCharge], charge.auth3dsRequiredIn) + app.get(card.auth3dsRequiredIn.path, [xraySegmentCls, csrfTokenGeneration, retrieveCharge], charge.auth3dsRequiredIn) app.post(card.auth3dsHandler.path, middlewareStack, charge.auth3dsHandler) app.get(card.captureWaiting.path, middlewareStack, charge.captureWaiting) app.post(card.create.path, middlewareStack, charge.create) app.get(card.confirm.path, middlewareStack, charge.confirm) app.post(card.capture.path, middlewareStack, charge.capture) app.post(card.cancel.path, middlewareStack, charge.cancel) - app.post(card.checkCard.path, retrieveCharge, charge.checkCard) - app.get(card.return.path, retrieveCharge, returnCont.return) + app.post(card.checkCard.path, xraySegmentCls, retrieveCharge, charge.checkCard) + app.get(card.return.path, xraySegmentCls, retrieveCharge, returnCont.return) // secure controller - app.get(paths.secure.get.path, secure.new) - app.post(paths.secure.post.path, secure.new) + app.get(paths.secure.get.path, xraySegmentCls, secure.new) + app.post(paths.secure.post.path, xraySegmentCls, secure.new) // static controller - app.get(paths.static.humans.path, statik.humans) - app.all(paths.static.naxsi_error.path, statik.naxsi_error) + app.get(paths.static.humans.path, xraySegmentCls, statik.humans) + app.all(paths.static.naxsi_error.path, xraySegmentCls, statik.naxsi_error) // route to gov.uk 404 page // this has to be the last route registered otherwise it will redirect other routes app.all('*', (req, res) => res.redirect('https://www.gov.uk/404')) + + app.use(AWSXRay.express.closeSegment()) } diff --git a/app/utils/base_client.js b/app/utils/base_client.js index 2d75556e0..91c6f59c5 100644 --- a/app/utils/base_client.js +++ b/app/utils/base_client.js @@ -11,7 +11,7 @@ const urlParse = require('url') const logger = require('winston') const https = setHttpClient() const _ = require('lodash') -const getNamespace = require('continuation-local-storage').getNamespace +const {getNamespace} = require('continuation-local-storage') const AWSXRay = require('aws-xray-sdk') // Local dependencies @@ -131,8 +131,8 @@ module.exports = { * * @returns {OutgoingMessage} */ - get: function (url, args, callback, subsegment) { - return _request('GET', url, args, callback, subsegment) + get: function (url, args, callback, subSegment) { + return _request('GET', url, args, callback, subSegment) }, /** @@ -143,8 +143,8 @@ module.exports = { * * @returns {OutgoingMessage} */ - post: function (url, args, callback) { - return _request('POST', url, args, callback) + post: function (url, args, callback, subSegment) { + return _request('POST', url, args, callback, subSegment) }, /** diff --git a/app/utils/base_client2.js b/app/utils/base_client2.js index cda5cc526..7bac9cdc9 100644 --- a/app/utils/base_client2.js +++ b/app/utils/base_client2.js @@ -7,7 +7,7 @@ const urlParse = require('url').parse const _ = require('lodash') const logger = require('winston') const request = require('requestretry') -const getNamespace = require('continuation-local-storage').getNamespace +const {getNamespace} = require('continuation-local-storage') const AWSXRay = require('aws-xray-sdk') // Local dependencies diff --git a/app/utils/cardid_client.js b/app/utils/cardid_client.js index 126df24a6..42df06086 100644 --- a/app/utils/cardid_client.js +++ b/app/utils/cardid_client.js @@ -13,5 +13,5 @@ const CARD_URL = process.env.CARDID_HOST + '/v1/api/card' * * @returns {Request} */ -exports.post = (args, callBack) => baseClient.post(CARD_URL, args, callBack) +exports.post = (args, callBack, subSegment) => baseClient.post(CARD_URL, args, callBack, subSegment) exports.CARD_URL = CARD_URL diff --git a/aws-xray.rules b/aws-xray.rules index 93928c65b..f48625dc4 100644 --- a/aws-xray.rules +++ b/aws-xray.rules @@ -19,7 +19,7 @@ ], "default": { "fixed_target": 1, - "rate": 0.1 + "rate": 0.01 }, "version": 1 } \ No newline at end of file diff --git a/package-lock.json b/package-lock.json index cc23e8744..4916d0a2d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2772,7 +2772,7 @@ }, "lodash": { "version": "3.10.1", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-3.10.1.tgz", + "resolved": "http://registry.npmjs.org/lodash/-/lodash-3.10.1.tgz", "integrity": "sha1-W/Rejkm6QYnhfUgnid/RW9FAt7Y=", "dev": true }, diff --git a/test/integration/charge_ft_tests.js b/test/integration/charge_ft_tests.js index 212652acd..76f4d3f4e 100644 --- a/test/integration/charge_ft_tests.js +++ b/test/integration/charge_ft_tests.js @@ -1,40 +1,58 @@ -const EMPTY_BODY = '' +'use strict' +// NPM dependencies const _ = require('lodash') const request = require('supertest') const nock = require('nock') -const app = require('../../server.js').getApp() const chai = require('chai') const cheerio = require('cheerio') +const winston = require('winston') const expect = chai.expect - +const proxyquire = require('proxyquire') +const AWSXRay = require('aws-xray-sdk') const should = chai.should() +// Local dependencies +const app = proxyquire('../../server.js', { + 'aws-xray-sdk': { + enableManualMode: () => {}, + setLogger: () => {}, + middleware: { + setSamplingRules: () => {} + }, + config: () => {}, + express: { + openSegment: () => (req, res, next) => next(), + closeSegment: () => (req, rest, next) => next() + }, + captureAsyncFunc: (name, callback) => callback(new AWSXRay.Segment('stub-subsegment')), + '@global': true + }, + 'continuation-local-storage': { + getNamespace: function () { + return { + get: () => new AWSXRay.Segment('stub-segment'), + bindEmitter: () => {}, + run: callback => callback(), + set: () => {} + } + }, + '@global': true + } +}).getApp() const cookie = require('../test_helpers/session.js') const helper = require('../test_helpers/test_helpers.js') - -const winston = require('winston') - const {getChargeRequest, postChargeRequest} = require('../test_helpers/test_helpers.js') const connectorResponseForPutCharge = require('../test_helpers/test_helpers.js').connectorResponseForPutCharge const {defaultConnectorResponseForGetCharge, defaultAdminusersResponseForGetService} = require('../test_helpers/test_helpers.js') const State = require('../../app/models/state.js') const serviceFixtures = require('../fixtures/service_fixtures') -let mockServer - -let defaultCardID = function () { - nock(process.env.CARDID_HOST) - .post('/v1/api/card', () => { - return true - }) - .reply(200, {brand: 'visa', label: 'visa', type: 'D'}) -} - +// Constants +const EMPTY_BODY = '' let defaultCorrelationHeader = { reqheaders: {'x-request-id': 'some-unique-id'} } - const gatewayAccount = { gatewayAccountId: '12345', paymentProvider: 'sandbox', @@ -42,6 +60,16 @@ const gatewayAccount = { type: 'test' } +let mockServer + +let defaultCardID = function () { + nock(process.env.CARDID_HOST) + .post('/v1/api/card', () => { + return true + }) + .reply(200, {brand: 'visa', label: 'visa', type: 'D'}) +} + describe('chargeTests', function () { let localServer = process.env.CONNECTOR_HOST let adminUsersHost = process.env.ADMINUSERS_URL @@ -180,7 +208,7 @@ describe('chargeTests', function () { .expect(200) .expect(function (res) { const $ = cheerio.load(res.text) - expect($('#card-details #csrf').attr('value')).to.not.be.empty // eslint-disable-line + expect($('#card-details #csrf').attr('value')).to.not.be.empty // eslint-disable-line expect($('.payment-summary #amount').text()).to.eql('£23.45') expect($('#govuk-script-charge')[0].children[0].data).to.contains(chargeId) expect($('.payment-summary #payment-description').text()).to.contain('Payment Description') @@ -687,7 +715,7 @@ describe('chargeTests', function () { .expect(function (res) { const $ = cheerio.load(res.text) expect($('#govuk-script-charge')[0].children[0].data).to.contains(chargeId) - expect($('#card-details #csrf').attr('value')).to.not.be.empty // eslint-disable-line + expect($('#card-details #csrf').attr('value')).to.not.be.empty // eslint-disable-line expect($('.payment-summary #amount').text()).to.eql('£23.45') expect($('.payment-summary #payment-description').text()).to.contain('Payment Description') expect($('#card-details').attr('action')).to.eql(frontendCardDetailsPostPath) @@ -787,7 +815,7 @@ describe('chargeTests', function () { .expect(200) .expect(function (res) { const $ = cheerio.load(res.text) - expect($('#confirmation #csrf').attr('value')).to.not.be.empty // eslint-disable-line + expect($('#confirmation #csrf').attr('value')).to.not.be.empty // eslint-disable-line expect($('#card-number').text()).to.contains('************1234') expect($('#expiry-date').text()).to.contains('11/99') expect($('#cardholder-name').text()).to.contains('Test User') diff --git a/test/middleware/retrieve_charge_test.js b/test/middleware/retrieve_charge_test.js index a5f5ab37c..42f3a5036 100644 --- a/test/middleware/retrieve_charge_test.js +++ b/test/middleware/retrieve_charge_test.js @@ -6,9 +6,25 @@ const assert = require('assert') const sinon = require('sinon') const {expect} = require('chai') const nock = require('nock') +const proxyquire = require('proxyquire') +const AWSXRay = require('aws-xray-sdk') -// local dependencies -const retrieveCharge = require(path.join(__dirname, '/../../app/middleware/retrieve_charge.js')) +const retrieveCharge = proxyquire(path.join(__dirname, '/../../app/middleware/retrieve_charge.js'), { + 'aws-xray-sdk': { + captureAsyncFunc: function (name, callback) { + callback(new AWSXRay.Segment('stub-subsegment')) + } + }, + 'continuation-local-storage': { + getNamespace: function () { + return { + get: function () { + return new AWSXRay.Segment('stub-segment') + } + } + } + } +}) const ANALYTICS_ERROR = { analytics: { diff --git a/test/models/card_test.js b/test/models/card_test.js index 627498758..482a4991c 100644 --- a/test/models/card_test.js +++ b/test/models/card_test.js @@ -5,10 +5,28 @@ const path = require('path') const assert = require('assert') const nock = require('nock') const {unexpectedPromise} = require(path.join(__dirname, '/../test_helpers/test_helpers.js')) +const proxyquire = require('proxyquire') +const AWSXRay = require('aws-xray-sdk') // local dependencies require(path.join(__dirname, '/../test_helpers/html_assertions.js')) -const CardModel = require(path.join(__dirname, '/../../app/models/card.js')) + +const CardModel = proxyquire(path.join(__dirname, '/../../app/models/card.js'), { + 'aws-xray-sdk': { + captureAsyncFunc: function (name, callback) { + callback(new AWSXRay.Segment('stub-subsegment')) + } + }, + 'continuation-local-storage': { + getNamespace: function () { + return { + get: function () { + return new AWSXRay.Segment('stub-segment') + } + } + } + } +}) // constants const aRequestId = 'unique-request-id' diff --git a/test/test_helpers/test_helpers.js b/test/test_helpers/test_helpers.js index de59b5007..f34ed7b6b 100644 --- a/test/test_helpers/test_helpers.js +++ b/test/test_helpers/test_helpers.js @@ -1,14 +1,20 @@ +'use strict' + +// NPM dependencies const request = require('supertest') const _ = require('lodash') +const chaiExpect = require('chai').expect +const csrf = require('csrf') +const nock = require('nock') + +// local dependencies const serviceFixtures = require('../fixtures/service_fixtures') const Service = require('../../app/models/Service.class') const frontendCardDetailsPath = '/card_details' + +// constants const connectorChargePath = '/v1/frontend/charges/' const adminusersServicePath = '/v1/api/services' -const chaiExpect = require('chai').expect -const csrf = require('csrf') -const nock = require('nock') - const defaultCorrelationId = 'some-unique-id' const defaultGatewayAccountId = '12345'