From 5afb927ed90cbd486eab71a9c82e438ed7188d41 Mon Sep 17 00:00:00 2001 From: sp-watson <77974320+sp-watson@users.noreply.github.com> Date: Thu, 19 May 2022 14:57:28 +0100 Subject: [PATCH] NN-4114: Caching of auth client credentials (again) (#2155) * Revert "Revert "NN-4114: Caching of auth client credentials (#2150)" (#2154)" This reverts commit c934a21c844c3136ef04b6de539dd445fd67a8f3. * NN-4114: Use system client for client creds cache Co-authored-by: sp-watson --- backend/api/oauthApi.ts | 3 +- backend/api/systemOauthClient.test.ts | 123 ++++++++++++++++++++++++++ backend/api/systemOauthClient.ts | 108 +++++++++++++++++++--- backend/index.ts | 7 ++ backend/middleware/requestLimiter.ts | 2 +- backend/setupWebSession.ts | 2 +- integration-tests/mockApis/auth.js | 5 ++ package-lock.json | 16 ++++ package.json | 1 + 9 files changed, 250 insertions(+), 17 deletions(-) create mode 100644 backend/api/systemOauthClient.test.ts diff --git a/backend/api/oauthApi.ts b/backend/api/oauthApi.ts index efdb2b7e1..07a90fe2d 100644 --- a/backend/api/oauthApi.ts +++ b/backend/api/oauthApi.ts @@ -35,9 +35,10 @@ export const oauthApiFactory = (client, { clientId, clientSecret, url }) => { }) // eslint-disable-next-line camelcase - const parseOauthTokens = ({ access_token, refresh_token }) => ({ + const parseOauthTokens = ({ access_token, refresh_token, expires_in }) => ({ access_token, refresh_token, + expires_in, }) const translateAuthClientError = (error) => { diff --git a/backend/api/systemOauthClient.test.ts b/backend/api/systemOauthClient.test.ts new file mode 100644 index 000000000..aa40e3fac --- /dev/null +++ b/backend/api/systemOauthClient.test.ts @@ -0,0 +1,123 @@ +import { doesNotMatch } from 'assert' +import redisMock from 'redis-mock' +import { promisify } from 'util' +import systemOauthClient, { + clientCredsSetup, + enableLogDebugStatements, + getClientCredentialsTokens, +} from './systemOauthClient' + +describe('system oauth client tests', () => { + const oauthApi = { makeTokenRequest: {} } + + describe('Without Redis cache', () => { + beforeEach(() => { + oauthApi.makeTokenRequest = jest.fn().mockResolvedValue({ + access_token: '123', + refresh_token: '456', + expires_in: 600, + }) + clientCredsSetup(null, oauthApi, true) + }) + + it('Gets and returns token', async () => { + const clientCreds = await getClientCredentialsTokens(null) + expect(clientCreds).toEqual({ + access_token: '123', + refresh_token: '456', + expires_in: 600, + }) + }) + + it('Makes oauth requests each time', async () => { + await getClientCredentialsTokens(null) + await getClientCredentialsTokens(null) + expect(oauthApi.makeTokenRequest).toHaveBeenCalledTimes(2) + }) + }) + + describe('With Redis cache', () => { + let mockRedis + + beforeEach(() => { + oauthApi.makeTokenRequest = jest.fn().mockResolvedValue({ + access_token: '123', + refresh_token: '456', + expires_in: 600, + }) + mockRedis = redisMock.createClient() + clientCredsSetup(mockRedis, oauthApi, true) + }) + + it('Gets and returns token on the first call with correct expiry', async () => { + const clientCreds = await getClientCredentialsTokens('USER1') + expect(clientCreds).toEqual({ + access_token: '123', + refresh_token: '456', + expires_in: 600, + }) + expect(oauthApi.makeTokenRequest).toHaveBeenCalledTimes(1) + }) + + it('Returns the stored token the second call', async () => { + await getClientCredentialsTokens('USER1') + const clientCreds = await getClientCredentialsTokens('USER1') + expect(clientCreds).toEqual({ + access_token: '123', + refresh_token: null, + }) + expect(oauthApi.makeTokenRequest).toHaveBeenCalledTimes(0) + }) + + it('Returns the stored token per user', async () => { + await getClientCredentialsTokens('USER1') + const secondUserClientCreds = await getClientCredentialsTokens('USER2') + expect(secondUserClientCreds).toEqual({ + access_token: '123', + refresh_token: '456', + expires_in: 600, + }) + expect(oauthApi.makeTokenRequest).toHaveBeenCalledWith( + 'grant_type=client_credentials&username=USER2', + 'PSH-client_credentials' + ) + }) + + it('Works when no username provided', async () => { + const noUserClientCreds = await getClientCredentialsTokens(null) + expect(noUserClientCreds).toEqual({ + access_token: '123', + refresh_token: '456', + expires_in: 600, + }) + }) + + it('Expires cached value 1 minute before token expiry', async () => { + await getClientCredentialsTokens('USER1') + + const getTtl = promisify(mockRedis.ttl).bind(mockRedis) + const expiryTime = await getTtl('CC_USER1') + + const tokenExpiry = 600 + expect(expiryTime).toBeLessThan(tokenExpiry - 59) + expect(expiryTime).toBeGreaterThan(tokenExpiry - 120) // Allow 1 minute + }) + }) + + describe('Logging debug statements', () => { + it('Will happen in t3', async () => { + const logDebug = enableLogDebugStatements({ app: { production: true }, phaseName: 'DEV' }) + expect(logDebug).toEqual(true) + }) + + it('Will not happen in production', async () => { + const logDebug = enableLogDebugStatements({ app: { production: true } }) + expect(logDebug).toEqual(false) + }) + + it('Will happen if no config', async () => { + const logDebug = enableLogDebugStatements({}) + expect(logDebug).toEqual(true) + }) + }) +}) diff --git a/backend/api/systemOauthClient.ts b/backend/api/systemOauthClient.ts index da18552e0..bea63cbd9 100644 --- a/backend/api/systemOauthClient.ts +++ b/backend/api/systemOauthClient.ts @@ -1,31 +1,111 @@ import querystring from 'querystring' -import clientFactory from './oauthEnabledClient' -import { oauthApiFactory } from './oauthApi' -import config from '../config' +import { createClient } from 'redis' +import { promisify } from 'util' +import clientFactory from './oauthEnabledClient' import logger from '../log' +import { oauthApiFactory } from './oauthApi' -export const getClientCredentialsTokens = async (username) => { - const oauthRequest = username - ? querystring.stringify({ grant_type: 'client_credentials', username }) - : querystring.stringify({ grant_type: 'client_credentials' }) +let getRedisAsync +let setRedisAsync +let oauthClient +let logDebug - const oauthResult = await oauthApiFactory( +export const getSystemOauthApiClient = (configData) => { + return oauthApiFactory( clientFactory({ - baseUrl: config.apis.oauth2.url, - timeout: config.apis.oauth2.timeoutSeconds * 1000, + baseUrl: configData.apis.oauth2.url, + timeout: configData.apis.oauth2.timeoutSeconds * 1000, }), { - clientId: config.apis.oauth2.systemClientId, - clientSecret: config.apis.oauth2.systemClientSecret, - url: config.apis.oauth2.url, + clientId: configData.apis.oauth2.systemClientId, + clientSecret: configData.apis.oauth2.systemClientSecret, + url: configData.apis.oauth2.url, } - ).makeTokenRequest(oauthRequest, 'PSH-client_credentials') + ) +} + +export const getTokenStore = (configData) => { + const { enabled, host, port, password } = configData.redis + if (!enabled || !host) return null + + const client = createClient({ + host, + port, + password, + tls: configData.app.production ? {} : false, + }) + + client.on('error', (e: Error) => logger.error(e, 'Redis client error')) + + logger.info(`Oauth token store created`) + return client +} + +export const enableLogDebugStatements = (configData) => { + return !configData.app?.production || configData.phaseName === 'DEV' +} + +export const clientCredsSetup = (tokenStore, oauthApi, logDebugStatements) => { + const redisTokenStore = tokenStore + getRedisAsync = redisTokenStore ? promisify(redisTokenStore?.get).bind(redisTokenStore) : (key) => {} + setRedisAsync = redisTokenStore + ? promisify(redisTokenStore?.set).bind(redisTokenStore) + : (key, value, command, expiry) => {} + + oauthClient = oauthApi + + logDebug = logDebugStatements +} + +const requestClientCredentials = async (username) => { + const oauthRequest = username + ? querystring.stringify({ grant_type: 'client_credentials', username }) + : querystring.stringify({ grant_type: 'client_credentials' }) + + const oauthResult = await oauthClient.makeTokenRequest(oauthRequest, 'PSH-client_credentials') logger.debug(`Oauth request for grant type 'client_credentials', result status: successful`) return oauthResult } +// Remove this when we are confident caching is working +const debug = (operation: string, username: string) => { + if (logDebug) logger.info(`OAUTH CLIENT CREDS ${operation} FOR ${username}`) +} + +const getKey = (username: string): string => { + const baseKey = username || '%ANONYMOUS%' + return `CC_${baseKey}` +} + +export const getClientCredentialsTokens = async (username) => { + const key = getKey(username) + + debug('GET', key) + const token = await getRedisAsync(key) + if (token) { + debug('GOT', key) + // We need to preserve the oauth result to avoid changing all the code and esp. tests (we use axios). + // According to axios-config-decorators.ts we only use the auth token, custom request headers and pagination. + // For client creds, pagination and custom headers are not relevant when getting client creds. + return { + // Need only access token - refresh token and authSource (as per useApiClientCreds.ts) are actually not used + // for client creds (the access functions are in contextProperties.ts) + access_token: token, + refresh_token: null, + } + } + + const oauthResult = await requestClientCredentials(username) + + // set TTL slightly less than expiry of token. Async but no need to wait + await setRedisAsync(key, oauthResult.access_token, 'EX', oauthResult.expires_in - 60) + debug(`SET-${oauthResult.expires_in}`, key) + + return oauthResult +} + export default { getClientCredentialsTokens, } diff --git a/backend/index.ts b/backend/index.ts index b0be6c3fe..70f4405a4 100644 --- a/backend/index.ts +++ b/backend/index.ts @@ -16,6 +16,12 @@ import setupHealthChecks from './setupHealthChecks' import setupBodyParsers from './setupBodyParsers' import setupWebSecurity from './setupWebSecurity' import setupAuth from './setupAuth' +import { + clientCredsSetup, + getTokenStore, + getSystemOauthApiClient, + enableLogDebugStatements, +} from './api/systemOauthClient' import setupStaticContent from './setupStaticContent' import nunjucksSetup from './utils/nunjucksSetup' import setupRedirects from './setupRedirects' @@ -47,6 +53,7 @@ app.set('view engine', 'njk') nunjucksSetup(app) phaseNameSetup(app, config) +clientCredsSetup(getTokenStore(config), getSystemOauthApiClient(config), enableLogDebugStatements(config)) app.use(cookieParser()) app.use(setupBodyParsers()) diff --git a/backend/middleware/requestLimiter.ts b/backend/middleware/requestLimiter.ts index 929066e11..cbbff34fc 100644 --- a/backend/middleware/requestLimiter.ts +++ b/backend/middleware/requestLimiter.ts @@ -31,6 +31,6 @@ const initRedisStoreIfEnabled = ({ host, port, password, production }) => { password, tls: production ? {} : false, }) - client.on('error', (e: Error) => logger.error('Redis client error', e)) + client.on('error', (e: Error) => logger.error(e, 'Redis client error')) return { store: new RedisStore({ client }) } } diff --git a/backend/setupWebSession.ts b/backend/setupWebSession.ts index 97bf74eb7..2520552c1 100644 --- a/backend/setupWebSession.ts +++ b/backend/setupWebSession.ts @@ -21,7 +21,7 @@ export default () => { tls: config.app.production ? {} : false, }) - client.on('error', (e: Error) => logger.error('Redis client error', e)) + client.on('error', (e: Error) => logger.error(e, 'Redis client error')) return new RedisStore({ client }) } diff --git a/integration-tests/mockApis/auth.js b/integration-tests/mockApis/auth.js index d4b53220d..4dc613cff 100644 --- a/integration-tests/mockApis/auth.js +++ b/integration-tests/mockApis/auth.js @@ -203,6 +203,11 @@ const stubClientCredentialsRequest = () => }, response: { status: 200, + jsonBody: { + access_token: 'EXAMPLE_ACCESS_TOKEN', + refresh_token: 'EXAMPLE_REFRESH_TOKEN', + expires_in: 43200, + }, }, }) diff --git a/package-lock.json b/package-lock.json index 754457d20..34ee47236 100644 --- a/package-lock.json +++ b/package-lock.json @@ -167,6 +167,7 @@ "process": "0.11.10", "raf": "^3.4.0", "react-test-renderer": "16.8.6", + "redis-mock": "^0.56.3", "redux-mock-store": "^1.5.3", "sass-loader": "^12.1.0", "set-cookie-parser": "^2.4.8", @@ -19681,6 +19682,15 @@ "node": ">=4" } }, + "node_modules/redis-mock": { + "version": "0.56.3", + "resolved": "https://registry.npmjs.org/redis-mock/-/redis-mock-0.56.3.tgz", + "integrity": "sha512-ynaJhqk0Qf3Qajnwvy4aOjS4Mdf9IBkELWtjd+NYhpiqu4QCNq6Vf3Q7c++XRPGiKiwRj9HWr0crcwy7EiPjYQ==", + "dev": true, + "engines": { + "node": ">=6" + } + }, "node_modules/redis-parser": { "version": "3.0.0", "resolved": "https://registry.npmjs.org/redis-parser/-/redis-parser-3.0.0.tgz", @@ -38144,6 +38154,12 @@ "resolved": "https://registry.npmjs.org/redis-errors/-/redis-errors-1.2.0.tgz", "integrity": "sha1-62LSrbFeTq9GEMBK/hUpOEJQq60=" }, + "redis-mock": { + "version": "0.56.3", + "resolved": "https://registry.npmjs.org/redis-mock/-/redis-mock-0.56.3.tgz", + "integrity": "sha512-ynaJhqk0Qf3Qajnwvy4aOjS4Mdf9IBkELWtjd+NYhpiqu4QCNq6Vf3Q7c++XRPGiKiwRj9HWr0crcwy7EiPjYQ==", + "dev": true + }, "redis-parser": { "version": "3.0.0", "resolved": "https://registry.npmjs.org/redis-parser/-/redis-parser-3.0.0.tgz", diff --git a/package.json b/package.json index f42fead69..65275ea56 100644 --- a/package.json +++ b/package.json @@ -167,6 +167,7 @@ "process": "0.11.10", "raf": "^3.4.0", "react-test-renderer": "16.8.6", + "redis-mock": "^0.56.3", "redux-mock-store": "^1.5.3", "sass-loader": "^12.1.0", "set-cookie-parser": "^2.4.8",