Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NON-270: Improve REST client #238

Merged
merged 3 commits into from
Oct 4, 2023
Merged

NON-270: Improve REST client #238

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6458a3f
Improve REST client typing information and add PATCH, PUT and DELETE …
ushkarev Oct 3, 2023
bd70e88
Propagate user types into `res.locals` in request handlers
ushkarev Oct 3, 2023
c650de9
Update change log
ushkarev Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions server/@types/express/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { UserDetails } from '../../services/userService'

export default {}

declare module 'express-session' {
Expand All @@ -10,8 +12,7 @@ declare module 'express-session' {

export declare global {
namespace Express {
interface User {
username: string
interface User extends Partial<UserDetails> {
token: string
authSource: string
}
Expand All @@ -21,5 +22,9 @@ export declare global {
id: string
logout(done: (err: unknown) => void): void
}

interface Locals {
user: Express.User
}
Comment on lines +26 to +28
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setUpCurrentUser middleware always populates res.locals.user via the user service

}
}
4 changes: 2 additions & 2 deletions server/data/hmppsAuthClient.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ describe('hmppsAuthClient', () => {
tokenStore.getToken.mockResolvedValue(null)

fakeHmppsAuthApi
.post(`/oauth/token`, 'grant_type=client_credentials&username=Bob')
.post('/oauth/token', 'grant_type=client_credentials&username=Bob')
.basicAuth({ user: config.apis.hmppsAuth.systemClientId, pass: config.apis.hmppsAuth.systemClientSecret })
.matchHeader('Content-Type', 'application/x-www-form-urlencoded')
.reply(200, token)
Expand All @@ -82,7 +82,7 @@ describe('hmppsAuthClient', () => {
tokenStore.getToken.mockResolvedValue(null)

fakeHmppsAuthApi
.post(`/oauth/token`, 'grant_type=client_credentials')
.post('/oauth/token', 'grant_type=client_credentials')
.basicAuth({ user: config.apis.hmppsAuth.systemClientId, pass: config.apis.hmppsAuth.systemClientSecret })
.matchHeader('Content-Type', 'application/x-www-form-urlencoded')
.reply(200, token)
Expand Down
19 changes: 13 additions & 6 deletions server/data/hmppsAuthClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { URLSearchParams } from 'url'

import superagent from 'superagent'

import type TokenStore from './tokenStore'
Expand Down Expand Up @@ -32,8 +33,14 @@ function getSystemClientTokenFromHmppsAuth(username?: string): Promise<superagen
}

export interface User {
name: string
activeCaseLoadId: string
username: string
name?: string
active?: boolean
authSource?: string
uuid?: string
userId?: string
staffId?: number // deprecated, use userId
activeCaseLoadId?: string // deprecated, use user roles api
Comment on lines +36 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf. hmpps-auth#UserDetail

Copy link
Contributor

@andrewrlee andrewrlee Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly odd that some of these fields are prison specific - though activeCaseLoadId was here before anyway

Copy link
Contributor

@andrewrlee andrewrlee Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now as it matches the auth response. I guess auth ideally would expose this as a union type

Copy link
Contributor Author

@ushkarev ushkarev Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it took me quite a while to unpick what user-type info comes from where (the stuff in non-associations is more complex as it also loads caseloads) and this is the simplest thing i felt was meaningful… it's what hmpps-auth (allegedly) returns. in practice, i dunno whether all "auth sources" provide some of these fields. it felt like a sensible thing to mark the nullable ones in hmpps-auth as possibly-undefined here. the link to the UserDetail object above suggests getting caseload stuff from other apis.

still, whether this set of fields is good or good enough, i think it's nice to wire it up into res.locals – right?

auth ideally would expose this as a union type

<3

}

export interface UserRole {
Expand All @@ -48,14 +55,14 @@ export default class HmppsAuthClient {
}

getUser(token: string): Promise<User> {
logger.info(`Getting user details: calling HMPPS Auth`)
return HmppsAuthClient.restClient(token).get({ path: '/api/user/me' }) as Promise<User>
logger.info('Getting user details: calling HMPPS Auth')
return HmppsAuthClient.restClient(token).get<User>({ path: '/api/user/me' })
}

getUserRoles(token: string): Promise<string[]> {
return HmppsAuthClient.restClient(token)
.get({ path: '/api/user/me/roles' })
.then(roles => (<UserRole[]>roles).map(role => role.roleCode))
.get<UserRole[]>({ path: '/api/user/me/roles' })
.then(roles => roles.map(role => role.roleCode))
}

async getSystemClientToken(username?: string): Promise<string> {
Expand Down
122 changes: 73 additions & 49 deletions server/data/restClient.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import nock from 'nock'
import RestClient from './restClient'

import { AgentConfig } from '../config'
import RestClient from './restClient'

const restClient = new RestClient(
'name-1',
'api-name',
{
url: 'http://localhost:8080/api',
timeout: {
Expand All @@ -15,15 +16,15 @@ const restClient = new RestClient(
'token-1',
)

describe('POST', () => {
it('Should return response body', async () => {
describe.each(['get', 'patch', 'post', 'put', 'delete'] as const)('Method: %s', method => {
it('should return response body', async () => {
nock('http://localhost:8080', {
reqheaders: { authorization: 'Bearer token-1' },
})
.post('/api/test')
[method]('/api/test')
.reply(200, { success: true })

const result = await restClient.post({
const result = await restClient[method]({
path: '/test',
})

Expand All @@ -34,14 +35,14 @@ describe('POST', () => {
})
})

it('Should return raw response body', async () => {
it('should return raw response body', async () => {
nock('http://localhost:8080', {
reqheaders: { authorization: 'Bearer token-1' },
})
.post('/api/test')
[method]('/api/test')
.reply(200, { success: true })

const result = await restClient.post({
const result = await restClient[method]({
path: '/test',
headers: { header1: 'headerValue1' },
raw: true,
Expand All @@ -50,63 +51,86 @@ describe('POST', () => {
expect(nock.isDone()).toBe(true)

expect(result).toMatchObject({
req: { method: 'POST' },
req: { method: method.toUpperCase() },
status: 200,
text: '{"success":true}',
})
})

it('Should not retry by default', async () => {
nock('http://localhost:8080', {
reqheaders: { authorization: 'Bearer token-1' },
if (method === 'get' || method === 'delete') {
it('should retry by default', async () => {
nock('http://localhost:8080', {
reqheaders: { authorization: 'Bearer token-1' },
})
[method]('/api/test')
.reply(500)
[method]('/api/test')
.reply(500)
[method]('/api/test')
.reply(500)

await expect(
restClient[method]({
path: '/test',
headers: { header1: 'headerValue1' },
}),
).rejects.toThrow('Internal Server Error')

expect(nock.isDone()).toBe(true)
})
.post('/api/test')
.reply(500)

await expect(
restClient.post({
path: '/test',
headers: { header1: 'headerValue1' },
}),
).rejects.toThrow('Internal Server Error')

expect(nock.isDone()).toBe(true)
})

it('retries if configured to do so', async () => {
nock('http://localhost:8080', {
reqheaders: { authorization: 'Bearer token-1' },
} else {
it('should not retry by default', async () => {
nock('http://localhost:8080', {
reqheaders: { authorization: 'Bearer token-1' },
})
[method]('/api/test')
.reply(500)

await expect(
restClient[method]({
path: '/test',
headers: { header1: 'headerValue1' },
}),
).rejects.toThrow('Internal Server Error')

expect(nock.isDone()).toBe(true)
})
.post('/api/test')
.reply(500)
.post('/api/test')
.reply(500)
.post('/api/test')
.reply(500)

await expect(
restClient.post({
path: '/test',
headers: { header1: 'headerValue1' },
retry: true,
}),
).rejects.toThrow('Internal Server Error')

expect(nock.isDone()).toBe(true)
})
it('should retry if configured to do so', async () => {
nock('http://localhost:8080', {
reqheaders: { authorization: 'Bearer token-1' },
})
[method]('/api/test')
.reply(500)
[method]('/api/test')
.reply(500)
[method]('/api/test')
.reply(500)

await expect(
restClient[method]({
path: '/test',
headers: { header1: 'headerValue1' },
retry: true,
}),
).rejects.toThrow('Internal Server Error')

expect(nock.isDone()).toBe(true)
})
}

it('can recover through retries', async () => {
nock('http://localhost:8080', {
reqheaders: { authorization: 'Bearer token-1' },
})
.post('/api/test')
[method]('/api/test')
.reply(500)
.post('/api/test')
[method]('/api/test')
.reply(500)
.post('/api/test')
[method]('/api/test')
.reply(200, { success: true })

const result = await restClient.post({
const result = await restClient[method]({
path: '/test',
headers: { header1: 'headerValue1' },
retry: true,
Expand Down
Loading