From 03d82204b7d944101ee76e45fb84ddfafaf4709d Mon Sep 17 00:00:00 2001 From: Steven Date: Thu, 14 Jan 2021 18:33:34 +0000 Subject: [PATCH 1/2] PP-7445: add gateway accounts url structure upgrade util Simple utility to upgrade or redirect account specific URLs from the current method to the proposed URL structure. This will both rederict users who are following older links, allow a seamless release for users actively using the admin tool during a release and log upgrades so that we can see how often this is happening and when legacy code can be removed. --- app/routes.js | 13 +++++++ app/utils/flatten-nested-values.js | 12 ++++++ app/utils/flatten-nested-values.test.js | 19 +++++++++ app/utils/gateway-account-urls.js | 47 +++++++++++++++++++++++ app/utils/gateway-account-urls.js.test.js | 16 ++++++++ test/integration/routes.it.test.js | 22 +++++++++++ test/test-helpers/mock-session.js | 3 +- 7 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 app/utils/flatten-nested-values.js create mode 100644 app/utils/flatten-nested-values.test.js create mode 100644 app/utils/gateway-account-urls.js create mode 100644 app/utils/gateway-account-urls.js.test.js create mode 100644 test/integration/routes.it.test.js diff --git a/app/routes.js b/app/routes.js index 69d4ae4e46..525ab823b1 100644 --- a/app/routes.js +++ b/app/routes.js @@ -7,6 +7,7 @@ const logger = require('./utils/logger')(__filename) const response = require('./utils/response.js').response const generateRoute = require('./utils/generate-route') const paths = require('./paths.js') +const accountUrls = require('./utils/gateway-account-urls') const userIsAuthorised = require('./middleware/user-is-authorised') const getServiceAndAccount = require('./middleware/get-service-and-gateway-account.middleware') @@ -488,6 +489,18 @@ module.exports.bind = function (app) { app.use(paths.account.root, account) app.all('*', (req, res) => { + const currentSessionAccountExternalId = req.gateway_account && req.gateway_account.currentGatewayAccountExternalId + if (accountUrls.isLegacyAccountsUrl(req.url) && currentSessionAccountExternalId) { + const upgradedPath = accountUrls.getUpgradedAccountStructureUrl(req.url, currentSessionAccountExternalId) + logger.info('Accounts URL utility upgraded a request to a legacy account URL', { + url: req.originalUrl, + redirected_url: upgradedPath, + session_has_user: !!req.user, + is_internal_user: req.user && req.user.internalUser + }) + res.redirect(upgradedPath) + return + } logger.info('Page not found', { url: req.originalUrl }) diff --git a/app/utils/flatten-nested-values.js b/app/utils/flatten-nested-values.js new file mode 100644 index 0000000000..53607facb3 --- /dev/null +++ b/app/utils/flatten-nested-values.js @@ -0,0 +1,12 @@ +'use strict' +function flattenNestedValues (target) { + return Object.values(target).reduce((aggregate, value) => { + const valueIsNestedObject = typeof value === 'object' && value !== null + if (valueIsNestedObject) { + return [ ...aggregate, ...flattenNestedValues(value) ] + } + return [ ...aggregate, value ] + }, []) +} + +module.exports = flattenNestedValues diff --git a/app/utils/flatten-nested-values.test.js b/app/utils/flatten-nested-values.test.js new file mode 100644 index 0000000000..bb5a57d1e7 --- /dev/null +++ b/app/utils/flatten-nested-values.test.js @@ -0,0 +1,19 @@ +const { expect } = require('chai') +const flattenNestedValues = require('./flatten-nested-values') +describe('flatten nested values utility', () => { + it('correctly flattens nested values', () => { + const nested = { + one: { + two: { + index: 'path-1', + secondPage: 'path-2' + }, + three: 'path-3' + }, + four: 'path-4' + } + const flat = flattenNestedValues(nested) + expect(flat.length).to.equal(4) + expect(flat).to.have.members(['path-1', 'path-2', 'path-3', 'path-4']) + }) +}) diff --git a/app/utils/gateway-account-urls.js b/app/utils/gateway-account-urls.js new file mode 100644 index 0000000000..815fc3d159 --- /dev/null +++ b/app/utils/gateway-account-urls.js @@ -0,0 +1,47 @@ +'use strict' +// check if a missed URL (404) is a URL that has been upgraded during the +// account URL structure change. When this utility is reporting few or no +// upgrades it can be removed +const urlJoin = require('url-join') +const paths = require('../paths') +const formattedPathFor = require('./replace-params-in-path') +const flattenNestedValues = require('./flatten-nested-values') + +// only flatten paths once given the singleton module export patten, these +// should never change after the server spins up +const allAccountPaths = flattenNestedValues(paths.account) +const templatedAccountPaths = allAccountPaths.filter((path) => path.includes(':')) + +const removeEmptyValues = (value) => !!value + +function isLegacyAccountsUrl (url) { + if (allAccountPaths.includes(url)) { + return true + } else { + // the path isn't directly in the list, check to see if it's a templated value + const numberOfUrlParts = url.split('/').filter(removeEmptyValues).length + return templatedAccountPaths.some((templatedPath) => { + const parts = templatedPath.split('/').filter(removeEmptyValues) + const matches = parts + + // remove variable sections + .filter((part) => !part.startsWith(':')) + + // ensure every part of the url structure is present in the url we're comparing against + .every((part) => url.includes(part)) + + // verify it matches and is not a subset (has less length) + return matches && parts.length === numberOfUrlParts + }) + } +} + +function getUpgradedAccountStructureUrl (url, gatewayAccountExternalId) { + const base = formattedPathFor(paths.account.root, gatewayAccountExternalId) + return urlJoin(base, url) +} + +module.exports = { + isLegacyAccountsUrl, + getUpgradedAccountStructureUrl +} diff --git a/app/utils/gateway-account-urls.js.test.js b/app/utils/gateway-account-urls.js.test.js new file mode 100644 index 0000000000..bb282cb7d0 --- /dev/null +++ b/app/utils/gateway-account-urls.js.test.js @@ -0,0 +1,16 @@ +const { expect } = require('chai') + +const accountsUrl = require('./gateway-account-urls') +describe('account URL checker', () => { + it('correctly identifies an original account URL', () => { + const url = '/billing-address' + const result = accountsUrl.isLegacyAccountsUrl(url) + expect(result).to.be.true //eslint-disable-line + }) + + it('correctly upgrades a URL to the account structure', () => { + const url = '/create-payment-link/manage/some-product-external-id/add-reporting-column/some-metadata-key' + const gatewayAccountExternalId = 'some-account-external-id' + expect(accountsUrl.getUpgradedAccountStructureUrl(url, gatewayAccountExternalId)).to.equal('/account/some-account-external-id/create-payment-link/manage/some-product-external-id/add-reporting-column/some-metadata-key') + }) +}) diff --git a/test/integration/routes.it.test.js b/test/integration/routes.it.test.js new file mode 100644 index 0000000000..211b99d268 --- /dev/null +++ b/test/integration/routes.it.test.js @@ -0,0 +1,22 @@ +const request = require('supertest') + +const { getApp } = require('../../server') +const session = require('../test-helpers/mock-session.js') +const app = session.getAppWithLoggedInUser(getApp(), session.getUser()) + +describe('URL upgrade utility', () => { + it('correctly upgrades URLs in the account specific paths', () => { + return request(app) + .get('/billing-address') + .expect(302) + .then((res) => { + res.header['location'].should.include('/account/external-id-set-by-create-app-with-session/billing-address') // eslint-disable-line + }) + }) + + it('correctly 404s as expected for non account specific paths', () => { + return request(app) + .get('/unknown-address') + .expect(404) + }) +}) diff --git a/test/test-helpers/mock-session.js b/test/test-helpers/mock-session.js index 655253237e..b37272dfb4 100644 --- a/test/test-helpers/mock-session.js +++ b/test/test-helpers/mock-session.js @@ -18,7 +18,8 @@ const createAppWithSession = function (app, sessionData, gatewayAccountCookie, r req.session = req.session || sessionData || {} req.register_invite = registerInviteData || {} req.gateway_account = gatewayAccountCookie || { - currentGatewayAccountId: _.get(sessionData, 'passport.user.serviceRoles[0].service.gatewayAccountIds[0]') + currentGatewayAccountId: _.get(sessionData, 'passport.user.serviceRoles[0].service.gatewayAccountIds[0]'), + currentGatewayAccountExternalId: 'external-id-set-by-create-app-with-session' } next() From 88d9ba244e10b75bc7d60daf537a87b394f8699f Mon Sep 17 00:00:00 2001 From: Steven Date: Thu, 14 Jan 2021 18:43:15 +0000 Subject: [PATCH 2/2] PP-7445: set account external id on session when switching accounts The only valid reason for switching an account with the current service switcher methodology should be through the my services page. This ensures the external ID is set by the service switcher, the external ID is then used when upgrading legacy account URLs to the new structure. With the existing middleware that no longer meets best practice, it is possible to assign a new gateway account by default (the 0th gateway account belonging to a given user). This is difficult to understand both from a user and developer perspective and will be removed with the legacy code when the new structure is in. --- app/controllers/my-services/post-index.controller.js | 10 ++++++---- app/models/GatewayAccount.class.js | 3 ++- app/views/services/_service-switch.njk | 3 ++- .../controller/service-switch.controller.it.test.js | 4 +++- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/controllers/my-services/post-index.controller.js b/app/controllers/my-services/post-index.controller.js index b6712224fa..e3b6c9574f 100644 --- a/app/controllers/my-services/post-index.controller.js +++ b/app/controllers/my-services/post-index.controller.js @@ -9,13 +9,15 @@ const validAccountId = (accountId, user) => { } module.exports = (req, res) => { - let newAccountId = _.get(req, 'body.gatewayAccountId') + const gatewayAccountId = req.body && req.body.gatewayAccountId + const gatewayAccountExternalId = req.body && req.body.gatewayAccountExternalId - if (validAccountId(newAccountId, req.user)) { - req.gateway_account.currentGatewayAccountId = newAccountId + if (validAccountId(gatewayAccountId, req.user)) { + req.gateway_account.currentGatewayAccountId = gatewayAccountId + req.gateway_account.currentGatewayAccountExternalId = gatewayAccountExternalId res.redirect(302, paths.dashboard.index) } else { - logger.warn(`Attempted to switch to invalid account ${newAccountId}`) + logger.warn(`Attempted to switch to invalid account ${gatewayAccountId}`) res.redirect(302, paths.serviceSwitcher.index) } } diff --git a/app/models/GatewayAccount.class.js b/app/models/GatewayAccount.class.js index fd3f0a65a5..203fc53e0b 100644 --- a/app/models/GatewayAccount.class.js +++ b/app/models/GatewayAccount.class.js @@ -23,6 +23,7 @@ class GatewayAccount { **/ constructor (gatewayAccountData) { this.id = gatewayAccountData.gateway_account_id + this.external_id = gatewayAccountData.external_id this.name = gatewayAccountData.service_name this.type = gatewayAccountData.type this.paymentProvider = gatewayAccountData.payment_provider @@ -39,7 +40,7 @@ class GatewayAccount { // until we have external ids for card accounts, the external id is the internal one return { id: this.id, - external_id: this.id, + external_id: this.external_id, payment_provider: this.paymentProvider, service_name: this.name, type: this.type diff --git a/app/views/services/_service-switch.njk b/app/views/services/_service-switch.njk index cfb623c162..75b60ed984 100644 --- a/app/views/services/_service-switch.njk +++ b/app/views/services/_service-switch.njk @@ -1,7 +1,8 @@
  • - + +