diff --git a/app/src/forms/auth/middleware/userAccess.js b/app/src/forms/auth/middleware/userAccess.js index 24099068c..1ccc0d00d 100644 --- a/app/src/forms/auth/middleware/userAccess.js +++ b/app/src/forms/auth/middleware/userAccess.js @@ -356,15 +356,9 @@ const hasRoleModifyPermissions = async (req, _res, next) => { if (userRoles.includes(Roles.OWNER)) { // Can't remove a different user's owner role unless you are an owner. - // - // TODO: Remove this if statement and just throw the exception. It's not - // possible for userId === currentUser.id since we're in an if that we - // are !isOwner but also that userRoles.includes(Roles.OWNER). - if (userId !== currentUser.id) { - throw new Problem(401, { - detail: "You can't update an owner's roles.", - }); - } + throw new Problem(401, { + detail: "You can't update an owner's roles.", + }); } else if (futureRoles.includes(Roles.OWNER)) { // Can't add an owner role unless you are an owner. throw new Problem(401, { diff --git a/app/src/forms/common/middleware/validateParameter.js b/app/src/forms/common/middleware/validateParameter.js index 68b02fd51..2132b2794 100644 --- a/app/src/forms/common/middleware/validateParameter.js +++ b/app/src/forms/common/middleware/validateParameter.js @@ -1,9 +1,9 @@ const Problem = require('api-problem'); const uuid = require('uuid'); +const externalApiService = require('../../form/externalApi/service'); const formService = require('../../form/service'); const submissionService = require('../../submission/service'); -const { ExternalAPI } = require('../models'); /** * Throws a 400 problem if the parameter is not a valid UUID. @@ -60,6 +60,32 @@ const validateDocumentTemplateId = async (req, _res, next, documentTemplateId) = } }; +/** + * Validates that the :externalApiId route parameter exists and is a UUID. This + * validator requires that the :formId route parameter also exists. + * + * @param {*} req the Express object representing the HTTP request. + * @param {*} _res the Express object representing the HTTP response - unused. + * @param {*} next the Express chaining function. + * @param {*} externalAPIId the :externalAPIId value from the route. + */ +const validateExternalAPIId = async (req, _res, next, externalAPIId) => { + try { + _validateUuid(externalAPIId, 'externalAPIId'); + + const externalApi = await externalApiService.readExternalAPI(externalAPIId); + if (!externalApi || externalApi.formId !== req.params.formId) { + throw new Problem(404, { + detail: 'externalAPIId does not exist on this form', + }); + } + + next(); + } catch (error) { + next(error); + } +}; + /** * Validates that the :fileId route parameter exists and is a UUID. * @@ -148,37 +174,11 @@ const validateFormVersionId = async (req, _res, next, formVersionId) => { } }; -/** - * Validates that the :externalApiId route parameter exists and is a UUID. This - * validator requires that the :formId route parameter also exists. - * - * @param {*} req the Express object representing the HTTP request. - * @param {*} _res the Express object representing the HTTP response - unused. - * @param {*} next the Express chaining function. - * @param {*} externalAPIId the :externalAPIId value from the route. - */ -const validateExternalAPIId = async (req, _res, next, externalAPIId) => { - try { - _validateUuid(externalAPIId, 'externalAPIId'); - - const externalApi = await ExternalAPI.query().findById(externalAPIId); - if (!externalApi || externalApi.formId !== req.params.formId) { - throw new Problem(404, { - detail: 'externalAPIId does not exist on this form', - }); - } - - next(); - } catch (error) { - next(error); - } -}; - module.exports = { validateDocumentTemplateId, + validateExternalAPIId, validateFileId, validateFormId, - validateFormVersionId, validateFormVersionDraftId, - validateExternalAPIId, + validateFormVersionId, }; diff --git a/app/src/forms/form/externalApi/service.js b/app/src/forms/form/externalApi/service.js index 0da7ae667..ebf9f6cd1 100644 --- a/app/src/forms/form/externalApi/service.js +++ b/app/src/forms/form/externalApi/service.js @@ -99,6 +99,10 @@ const service = { await ExternalAPI.query().modify('findByIdAndFormId', externalAPIId, formId).first().throwIfNotFound(); await ExternalAPI.query().deleteById(externalAPIId); }, + + readExternalAPI: (externalAPIId) => { + return ExternalAPI.query().findById(externalAPIId); + }, }; module.exports = service; diff --git a/app/tests/unit/forms/common/middleware/validateParameter.spec.js b/app/tests/unit/forms/common/middleware/validateParameter.spec.js index 4e8f005fa..8aeb0baf3 100644 --- a/app/tests/unit/forms/common/middleware/validateParameter.spec.js +++ b/app/tests/unit/forms/common/middleware/validateParameter.spec.js @@ -2,6 +2,7 @@ const { getMockReq, getMockRes } = require('@jest-mock/express'); const { v4: uuidv4 } = require('uuid'); const validateParameter = require('../../../../../src/forms/common/middleware/validateParameter'); +const externalApiService = require('../../../../../src/forms/form/externalApi/service'); const formService = require('../../../../../src/forms/form/service'); const submissionService = require('../../../../../src/forms/submission/service'); @@ -198,6 +199,123 @@ describe('validateDocumentTemplateId', () => { }); }); +describe('validateExternalApiId', () => { + const externalApiId = uuidv4(); + + const mockReadExternalApiResponse = { + formId: formId, + id: externalApiId, + }; + + externalApiService.readExternalAPI = jest.fn().mockReturnValue(mockReadExternalApiResponse); + + describe('400 response when', () => { + const expectedStatus = { status: 400 }; + + test('externalAPIId is missing', async () => { + const req = getMockReq({ + params: { + formId: formId, + }, + }); + const { res, next } = getMockRes(); + + await validateParameter.validateExternalAPIId(req, res, next); + + expect(externalApiService.readExternalAPI).toBeCalledTimes(0); + expect(next).toBeCalledWith(expect.objectContaining(expectedStatus)); + }); + + test.each(invalidUuids)('externalAPIId is "%s"', async (eachExternalApiId) => { + const req = getMockReq({ + params: { + formId: formId, + externalAPIId: eachExternalApiId, + }, + }); + const { res, next } = getMockRes(); + + await validateParameter.validateExternalAPIId(req, res, next, eachExternalApiId); + + expect(externalApiService.readExternalAPI).toBeCalledTimes(0); + expect(next).toBeCalledWith(expect.objectContaining(expectedStatus)); + }); + }); + + describe('404 response when', () => { + const expectedStatus = { status: 404 }; + + test('formId is missing', async () => { + const req = getMockReq({ + params: { + externalAPIId: externalApiId, + }, + }); + const { res, next } = getMockRes(); + + await validateParameter.validateExternalAPIId(req, res, next, externalApiId); + + expect(externalApiService.readExternalAPI).toBeCalledTimes(1); + expect(next).toBeCalledWith(expect.objectContaining(expectedStatus)); + }); + + test('formId does not match', async () => { + externalApiService.readExternalAPI.mockReturnValueOnce({ + formId: uuidv4(), + id: externalApiId, + }); + const req = getMockReq({ + params: { + externalAPIId: externalApiId, + formId: formId, + }, + }); + const { res, next } = getMockRes(); + + await validateParameter.validateExternalAPIId(req, res, next, externalApiId); + + expect(externalApiService.readExternalAPI).toBeCalledTimes(1); + expect(next).toBeCalledWith(expect.objectContaining(expectedStatus)); + }); + }); + + describe('handles error thrown by', () => { + test('readVersion', async () => { + const error = new Error(); + externalApiService.readExternalAPI.mockRejectedValueOnce(error); + const req = getMockReq({ + params: { + externalAPIId: externalApiId, + formId: formId, + }, + }); + const { res, next } = getMockRes(); + + await validateParameter.validateExternalAPIId(req, res, next, externalApiId); + + expect(externalApiService.readExternalAPI).toBeCalledTimes(1); + expect(next).toBeCalledWith(error); + }); + }); + + describe('allows', () => { + test('external api with matching form id', async () => { + const req = getMockReq({ + params: { + externalAPIId: externalApiId, + formId: formId, + }, + }); + const { res, next } = getMockRes(); + + await validateParameter.validateExternalAPIId(req, res, next, externalApiId); + + expect(externalApiService.readExternalAPI).toBeCalledTimes(1); + expect(next).toBeCalledWith(); + }); + }); +}); + describe('validateFileId', () => { describe('400 response when', () => { const expectedStatus = { status: 400 };