From 1a11c216aed52793746e86961414bba614d3d3bc Mon Sep 17 00:00:00 2001 From: Andrew Lee <1517745+andrewrlee@users.noreply.github.com> Date: Mon, 21 Dec 2020 14:22:07 +0000 Subject: [PATCH] Alee/decoupling auth client from redis (#11) * Decoupling auth client from redis impl --- package-lock.json | 9 +++++ package.json | 1 + server/config.ts | 2 +- server/data/hmppsAuthClient.test.ts | 33 +++++++--------- server/data/hmppsAuthClient.ts | 33 +++++----------- server/data/testutils/hmppsAuthClientSetup.ts | 14 ------- server/data/tokenStore.test.ts | 34 +++++++++++++++++ server/data/tokenStore.ts | 38 +++++++++++++++++++ server/index.ts | 3 +- server/services/userService.test.ts | 10 +++-- 10 files changed, 114 insertions(+), 63 deletions(-) delete mode 100644 server/data/testutils/hmppsAuthClientSetup.ts create mode 100644 server/data/tokenStore.test.ts create mode 100644 server/data/tokenStore.ts diff --git a/package-lock.json b/package-lock.json index 7b29188..562ac28 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1970,6 +1970,15 @@ "integrity": "sha512-ewFXqrQHlFsgc09MK5jP5iR7vumV/BYayNC6PgJO2LPe8vrnNFyjQjSppfEngITi0qvfKtzFvgKymGheFM9UOA==", "dev": true }, + "@types/redis": { + "version": "2.8.28", + "resolved": "https://registry.npmjs.org/@types/redis/-/redis-2.8.28.tgz", + "integrity": "sha512-8l2gr2OQ969ypa7hFOeKqtFoY70XkHxISV0pAwmQ2nm6CSPb1brmTmqJCGGrekCo+pAZyWlNXr+Kvo6L/1wijA==", + "dev": true, + "requires": { + "@types/node": "*" + } + }, "@types/serve-static": { "version": "1.13.6", "resolved": "https://registry.npmjs.org/@types/serve-static/-/serve-static-1.13.6.tgz", diff --git a/package.json b/package.json index bd9768d..4f1b2c7 100644 --- a/package.json +++ b/package.json @@ -121,6 +121,7 @@ "@types/jest": "^26.0.19", "@types/node": "^14.14.13", "@types/passport": "^1.0.4", + "@types/redis": "^2.8.28", "@types/superagent": "^4.1.10", "@types/supertest": "^2.0.10", "@typescript-eslint/eslint-plugin": "^4.10.0", diff --git a/server/config.ts b/server/config.ts index 65d0ad0..9b9188a 100755 --- a/server/config.ts +++ b/server/config.ts @@ -37,7 +37,7 @@ export default { staticResourceCacheDuration: 20, redis: { host: process.env.REDIS_HOST, - port: process.env.REDIS_PORT || 6379, + port: parseInt(process.env.REDIS_PORT, 10) || 6379, password: process.env.REDIS_AUTH_TOKEN, tls_enabled: get('REDIS_TLS_ENABLED', 'false'), }, diff --git a/server/data/hmppsAuthClient.test.ts b/server/data/hmppsAuthClient.test.ts index e277929..106a07a 100644 --- a/server/data/hmppsAuthClient.test.ts +++ b/server/data/hmppsAuthClient.test.ts @@ -1,22 +1,15 @@ import nock from 'nock' -import redis from 'redis' import config from '../config' import HmppsAuthClient from './hmppsAuthClient' +import TokenStore from './tokenStore' -const username = 'Bob' -const token = { access_token: 'token-1', expires_in: 300 } +jest.mock('./tokenStore') -jest.mock('redis', () => ({ - createClient: jest.fn().mockReturnThis(), - on: jest.fn(), - get: jest.fn(), - set: jest.fn().mockImplementation((key, value, command, ttl, callback) => callback(null, null)), -})) +const tokenStore = new TokenStore(null) as jest.Mocked -function givenRedisResponse(storedToken: string) { - redis.get.mockImplementation((key, callback) => callback(null, storedToken)) -} +const username = 'Bob' +const token = { access_token: 'token-1', expires_in: 300 } describe('hmppsAuthClient', () => { let fakeHmppsAuthApi: nock.Scope @@ -24,10 +17,11 @@ describe('hmppsAuthClient', () => { beforeEach(() => { fakeHmppsAuthApi = nock(config.apis.hmppsAuth.url) - hmppsAuthClient = new HmppsAuthClient() + hmppsAuthClient = new HmppsAuthClient(tokenStore) }) afterEach(() => { + jest.resetAllMocks() nock.cleanAll() }) @@ -59,19 +53,18 @@ describe('hmppsAuthClient', () => { describe('getSystemClientToken', () => { it('should instantiate the redis client', async () => { - givenRedisResponse(token.access_token) + tokenStore.getToken.mockResolvedValue(token.access_token) await hmppsAuthClient.getSystemClientToken(username) - expect(redis.createClient).toBeCalledTimes(1) }) it('should return token from redis if one exists', async () => { - givenRedisResponse(token.access_token) + tokenStore.getToken.mockResolvedValue(token.access_token) const output = await hmppsAuthClient.getSystemClientToken(username) expect(output).toEqual(token.access_token) }) it('should return token from HMPPS Auth with username', async () => { - givenRedisResponse(null) + tokenStore.getToken.mockResolvedValue(null) fakeHmppsAuthApi .post(`/oauth/token`, 'grant_type=client_credentials&username=Bob') @@ -82,11 +75,11 @@ describe('hmppsAuthClient', () => { const output = await hmppsAuthClient.getSystemClientToken(username) expect(output).toEqual(token.access_token) - expect(redis.set).toBeCalledWith('Bob', token.access_token, 'EX', 240, expect.any(Function)) + expect(tokenStore.setToken).toBeCalledWith('Bob', token.access_token, 240) }) it('should return token from HMPPS Auth without username', async () => { - givenRedisResponse(null) + tokenStore.getToken.mockResolvedValue(null) fakeHmppsAuthApi .post(`/oauth/token`, 'grant_type=client_credentials') @@ -97,7 +90,7 @@ describe('hmppsAuthClient', () => { const output = await hmppsAuthClient.getSystemClientToken() expect(output).toEqual(token.access_token) - expect(redis.set).toBeCalledWith('Bob', token.access_token, 'EX', 240, expect.any(Function)) + expect(tokenStore.setToken).toBeCalledWith('%ANONYMOUS%', token.access_token, 240) }) }) }) diff --git a/server/data/hmppsAuthClient.ts b/server/data/hmppsAuthClient.ts index b54d8f1..59075e4 100644 --- a/server/data/hmppsAuthClient.ts +++ b/server/data/hmppsAuthClient.ts @@ -1,7 +1,6 @@ import superagent from 'superagent' import querystring from 'querystring' -import redis from 'redis' -import { promisify } from 'util' +import type TokenStore from './tokenStore' import logger from '../../log' import config from '../config' @@ -10,20 +9,6 @@ import RestClient from './restClient' const timeoutSpec = config.apis.hmppsAuth.timeout const hmppsAuthUrl = config.apis.hmppsAuth.url -const redisClient = redis.createClient({ - port: config.redis.port, - password: config.redis.password, - host: config.redis.host, - tls: config.redis.tls_enabled === 'true' ? {} : false, - prefix: 'systemToken:', -}) - -redisClient.on('error', error => { - logger.error(error, `Redis error`) -}) - -const getRedisAsync = promisify(redisClient.get).bind(redisClient) -const setRedisAsync = promisify(redisClient.set).bind(redisClient) function getSystemClientTokenFromHmppsAuth(username?: string): Promise { const clientToken = generateOauthClientToken( @@ -47,16 +32,18 @@ function getSystemClientTokenFromHmppsAuth(username?: string): Promise { - const redisKey = username || '%ANONYMOUS%' + const key = username || '%ANONYMOUS%' - const tokenFromRedis = await getRedisAsync(redisKey) - if (tokenFromRedis) { - return tokenFromRedis + const token = await this.tokenStore.getToken(key) + if (token) { + return token } const newToken = await getSystemClientTokenFromHmppsAuth(username) // set TTL slightly less than expiry of token. Async but no need to wait - await setRedisAsync(redisKey, newToken.body.access_token, 'EX', newToken.body.expires_in - 60) + await this.tokenStore.setToken(key, newToken.body.access_token, newToken.body.expires_in - 60) return newToken.body.access_token } diff --git a/server/data/testutils/hmppsAuthClientSetup.ts b/server/data/testutils/hmppsAuthClientSetup.ts deleted file mode 100644 index 6f21387..0000000 --- a/server/data/testutils/hmppsAuthClientSetup.ts +++ /dev/null @@ -1,14 +0,0 @@ -import HmppsAuthClient from '../hmppsAuthClient' - -const MockedHmppsAuthClient = >HmppsAuthClient - -jest.mock('../hmppsAuthClient') -jest.mock('redis', () => ({ - createClient: jest.fn().mockImplementation(() => ({ - on: jest.fn(), - get: jest.fn(), - set: jest.fn(), - })), -})) - -export default MockedHmppsAuthClient diff --git a/server/data/tokenStore.test.ts b/server/data/tokenStore.test.ts new file mode 100644 index 0000000..4f9d92f --- /dev/null +++ b/server/data/tokenStore.test.ts @@ -0,0 +1,34 @@ +import { RedisClient } from 'redis' +import TokenStore from './tokenStore' + +const redisClient = { + on: jest.fn(), + get: jest.fn(), + set: jest.fn(), +} + +describe('tokenStore', () => { + let tokenStore: TokenStore + + beforeEach(() => { + tokenStore = new TokenStore((redisClient as unknown) as RedisClient) + }) + + afterEach(() => { + jest.resetAllMocks() + }) + + it('Can retrieve token', async () => { + redisClient.get.mockImplementation((key, callback) => callback(null, 'token-1')) + + await expect(tokenStore.getToken('user-1')).resolves.toBe('token-1') + + expect(redisClient.get).toHaveBeenCalledWith('user-1', expect.any(Function)) + }) + + it('Can set token', async () => { + await tokenStore.setToken('user-1', 'token-1', 10) + + expect(redisClient.set).toHaveBeenCalledWith('user-1', 'token-1', 'EX', 10, expect.any(Function)) + }) +}) diff --git a/server/data/tokenStore.ts b/server/data/tokenStore.ts new file mode 100644 index 0000000..b421a85 --- /dev/null +++ b/server/data/tokenStore.ts @@ -0,0 +1,38 @@ +import redis from 'redis' +import { promisify } from 'util' + +import logger from '../../log' +import config from '../config' + +const createRedisClient = () => { + return redis.createClient({ + port: config.redis.port, + password: config.redis.password, + host: config.redis.host, + tls: config.redis.tls_enabled === 'true' ? {} : false, + prefix: 'systemToken:', + }) +} + +export default class TokenStore { + private getRedisAsync: (key: string) => Promise + + private setRedisAsync: (key: string, value: string, mode: string, durationSeconds: number) => Promise + + constructor(redisClient: redis.RedisClient = createRedisClient()) { + redisClient.on('error', error => { + logger.error(error, `Redis error`) + }) + + this.getRedisAsync = promisify(redisClient.get).bind(redisClient) + this.setRedisAsync = promisify(redisClient.set).bind(redisClient) + } + + public async setToken(key: string, token: string, durationSeconds: number): Promise { + this.setRedisAsync(key, token, 'EX', durationSeconds) + } + + public async getToken(key: string): Promise { + return this.getRedisAsync(key) + } +} diff --git a/server/index.ts b/server/index.ts index 9305017..ad2c8e0 100755 --- a/server/index.ts +++ b/server/index.ts @@ -1,8 +1,9 @@ import createApp from './app' import HmppsAuthClient from './data/hmppsAuthClient' +import TokenStore from './data/tokenStore' import UserService from './services/userService' -const hmppsAuthClient = new HmppsAuthClient() +const hmppsAuthClient = new HmppsAuthClient(new TokenStore()) const userService = new UserService(hmppsAuthClient) const app = createApp(userService) diff --git a/server/services/userService.test.ts b/server/services/userService.test.ts index d162835..6cd84f5 100644 --- a/server/services/userService.test.ts +++ b/server/services/userService.test.ts @@ -1,19 +1,21 @@ import UserService from './userService' -import MockedHmppsAuthClient from '../data/testutils/hmppsAuthClientSetup' +import HmppsAuthClient, { User } from '../data/hmppsAuthClient' + +jest.mock('../data/hmppsAuthClient') const token = 'some token' describe('User service', () => { - let hmppsAuthClient + let hmppsAuthClient: jest.Mocked let userService: UserService describe('getUser', () => { beforeEach(() => { - hmppsAuthClient = new MockedHmppsAuthClient() + hmppsAuthClient = new HmppsAuthClient(null) as jest.Mocked userService = new UserService(hmppsAuthClient) }) it('Retrieves and formats user name', async () => { - hmppsAuthClient.getUser.mockResolvedValue({ name: 'john smith' }) + hmppsAuthClient.getUser.mockResolvedValue({ name: 'john smith' } as User) const result = await userService.getUser(token)