Skip to content

Commit

Permalink
PP-12395: Validation when creating API key (#4397)
Browse files Browse the repository at this point in the history
* PP-12395: Validation when creating API key

A description is not valid if it's empty or more than 50 chars. However given the text box has a maxLength of 50, the latter validation error can't occur in practice.
  • Loading branch information
oswaldquek authored Dec 30, 2024
1 parent c760810 commit 5bf46dc
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,40 @@ const { response } = require('@utils/response')
const formatSimplifiedAccountPathsFor = require('@utils/simplified-account/format/format-simplified-account-paths-for')
const paths = require('@root/paths')
const { TOKEN_SOURCE, createApiKey } = require('@services/api-keys.service')
const { body, validationResult } = require('express-validator')
const formatValidationErrors = require('@utils/simplified-account/format/format-validation-errors')

async function get (req, res) {
return response(req, res, 'simplified-account/settings/api-keys/api-key-name', {
backLink: formatSimplifiedAccountPathsFor(paths.simplifiedAccount.settings.apiKeys.index, req.service.externalId, req.account.type)
})
}

const descriptionValidation = [
body('description')
.trim()
.notEmpty()
.withMessage('Name must not be empty')
.isLength({ max: 50 })
.withMessage('Name must be 50 characters or fewer')
]

async function post (req, res) {
const description = req.body.description // TODO validate length - deal with this in another PR
await Promise.all(descriptionValidation.map(validation => validation.run(req)))
const errors = validationResult(req)

if (!errors.isEmpty()) {
const formattedErrors = formatValidationErrors(errors)
return response(req, res, 'simplified-account/settings/api-keys/api-key-name', {
errors: {
summary: formattedErrors.errorSummary,
formErrors: formattedErrors.formErrors
},
backLink: formatSimplifiedAccountPathsFor(paths.simplifiedAccount.settings.apiKeys.index, req.service.externalId, req.account.type)
})
}

const description = req.body.description
const newApiKey = await createApiKey(req.account, description, req.user.email, TOKEN_SOURCE.API)
response(req, res, 'simplified-account/settings/api-keys/new-api-key-details', {
description,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,37 +45,94 @@ describe('Controller: settings/create-api-key', () => {
})

describe('post', () => {
before(() => {
nextRequest({
body: {
description: 'a test api key'
},
user: {
email: '[email protected]'
}
describe('a valid description', () => {
before(() => {
nextRequest({
body: {
description: 'a test api key'
},
user: {
email: '[email protected]'
}
})
call('post')
})
call('post')
})

it('should submit values to the api keys service', () => {
expect(apiKeysService.createApiKey).to.have.been.calledWith(
sinon.match.any,
'a test api key',
'[email protected]',
TOKEN_SOURCE.API
)
})
it('should submit values to the api keys service', () => {
expect(apiKeysService.createApiKey).to.have.been.calledWith(
sinon.match.any,
'a test api key',
'[email protected]',
TOKEN_SOURCE.API
)
})

it('should call the response method', () => {
expect(mockResponse).to.have.been.calledOnce // eslint-disable-line
it('should call the response method', () => {
expect(mockResponse).to.have.been.calledOnce // eslint-disable-line
})

it('should pass context data to the response method', () => {
expect(mockResponse.args[0][2]).to.equal('simplified-account/settings/api-keys/new-api-key-details')
expect(mockResponse.args[0][3]).to.have.property('backToApiKeysLink').to.equal(
formatSimplifiedAccountPathsFor(paths.simplifiedAccount.settings.apiKeys.index, SERVICE_ID, ACCOUNT_TYPE))
expect(mockResponse.args[0][3]).to.have.property('apiKey').to.equal(newApiKey)
expect(mockResponse.args[0][3]).to.have.property('description').to.equal('a test api key')
})
})

it('should pass context data to the response method', () => {
expect(mockResponse.args[0][2]).to.equal('simplified-account/settings/api-keys/new-api-key-details')
expect(mockResponse.args[0][3]).to.have.property('backToApiKeysLink').to.equal(
formatSimplifiedAccountPathsFor(paths.simplifiedAccount.settings.apiKeys.index, SERVICE_ID, ACCOUNT_TYPE))
expect(mockResponse.args[0][3]).to.have.property('apiKey').to.equal(newApiKey)
expect(mockResponse.args[0][3]).to.have.property('description').to.equal('a test api key')
describe('an invalid description', () => {
function assertMockResponseArgs (errorMessage) {
expect(mockResponse.calledOnce).to.be.true // eslint-disable-line
expect(mockResponse.args[0][2]).to.equal('simplified-account/settings/api-keys/api-key-name')
expect(mockResponse.args[0][3].errors.summary[0].text).to.equal(errorMessage)
expect(mockResponse.args[0][3].errors.formErrors.description).to.equal(errorMessage)
expect(mockResponse.args[0][3].backLink).to.equal(
formatSimplifiedAccountPathsFor(paths.simplifiedAccount.settings.apiKeys.index, SERVICE_ID, ACCOUNT_TYPE))
}

describe('empty description', () => {
before(() => {
nextRequest({
body: {
description: ''
},
user: {
email: '[email protected]'
}
})
call('post')
})

it('should not call apiKeysService.createApiKey', () => {
sinon.assert.notCalled(apiKeysService.createApiKey)
})

it('should pass req, res, template path and context to the response method', () => {
assertMockResponseArgs('Name must not be empty')
})
})

describe('description more than 50 chars', () => {
before(() => {
nextRequest({
body: {
description: 'more than fifty chars more than fifty chars more than fifty chars more than fifty chars'
},
user: {
email: '[email protected]'
}
})
call('post')
})

it('should not call apiKeysService.createApiKey', () => {
sinon.assert.notCalled(apiKeysService.createApiKey)
})

it('should pass req, res, template path and context to the response method', () => {
assertMockResponseArgs('Name must be 50 characters or fewer')
})
})
})
})
})
2 changes: 1 addition & 1 deletion app/utils/simplified-account/settings/service-settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ module.exports = (account, service, currentUrl, permissions) => {
.category('developers')
.add({
id: 'api-keys',
name: 'API keys',
name: account.type === 'test' ? 'Test API keys' : 'Live API keys',
path: paths.simplifiedAccount.settings.apiKeys.index,
permission: 'tokens_active_read',
alwaysViewable: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
name: "description",
type: "text",
attributes: {
maxlength: "100"
maxlength: "50"
},
classes: "govuk-input--width-40",
label: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('Settings - API keys', () => {
cy.visit(`/simplified/service/${SERVICE_EXTERNAL_ID}/account/${ACCOUNT_TYPE}/settings/api-keys`)
})
it('should show appropriate buttons and text', () => {
cy.get('#api-keys').should('have.text', 'API keys')
cy.get('#api-keys').should('have.text', 'Test API keys')
cy.get('.service-settings-pane')
.find('a')
.contains('Create a new API key')
Expand All @@ -62,7 +62,7 @@ describe('Settings - API keys', () => {
cy.visit(`/simplified/service/${SERVICE_EXTERNAL_ID}/account/${ACCOUNT_TYPE}/settings/api-keys`)
})
it('should show appropriate buttons and text', () => {
cy.get('#api-keys').should('have.text', 'API keys')
cy.get('#api-keys').should('have.text', 'Test API keys')
cy.get('.service-settings-pane')
.find('a')
.contains('Create a new API key')
Expand Down

0 comments on commit 5bf46dc

Please sign in to comment.