diff --git a/app/app.js b/app/app.js index 212948c0..e8255560 100644 --- a/app/app.js +++ b/app/app.js @@ -71,11 +71,11 @@ if (config.has('server.privacyMask')) { } // Block requests until service is ready -app.use((_req, res, next) => { +app.use((_req, _res, next) => { if (state.shutdown) { - new Problem(503, { detail: 'Server is shutting down' }).send(res); + throw new Problem(503, { detail: 'Server is shutting down' }); } else if (!state.ready) { - new Problem(503, { detail: 'Server is not ready' }).send(res); + throw new Problem(503, { detail: 'Server is not ready' }); } else { next(); } @@ -107,24 +107,21 @@ apiRouter.use('/v1', v1Router); // Root level Router app.use(/(\/api)?/, apiRouter); -// Handle ValidationError & 500 -// eslint-disable-next-line no-unused-vars -app.use((err, _req, res, _next) => { +// Handle 404 +app.use((req, _res) => { // eslint-disable-line no-unused-vars + throw new Problem(404, { instance: req.originalUrl }); +}); + +// Handle Problem Responses +app.use((err, req, res, _next) => { // eslint-disable-line no-unused-vars if (err instanceof Problem) { err.send(res); } else { - // Only log unexpected errors - if (err.stack) log.error(err); - - new Problem(500, { detail: err.message ?? err }).send(res); + if (err.stack) log.error(err); // Only log unexpected errors + new Problem(500, { detail: err.message ?? err, instance: req.originalUrl }).send(res); } }); -// Handle 404 -app.use((req, res) => { - new Problem(404, { instance: req.originalUrl }).send(res); -}); - // Ensure unhandled errors gracefully shut down the application process.on('unhandledRejection', err => { log.error(`Unhandled Rejection: ${err.message ?? err}`, { function: 'onUnhandledRejection' }); diff --git a/app/src/controllers/bucket.js b/app/src/controllers/bucket.js index ea3e0796..ef9f4a01 100644 --- a/app/src/controllers/bucket.js +++ b/app/src/controllers/bucket.js @@ -102,6 +102,7 @@ const controller = { * @param {object} res Express response object * @param {function} next The next callback function * @returns {function} Express middleware function + * @throws The error encountered upon failure */ async createBucket(req, res, next) { const data = { @@ -122,7 +123,7 @@ const controller = { if (e instanceof UniqueViolationError) { // Grant all permissions if credentials precisely match response = await bucketService.checkGrantPermissions(data).catch(permErr => { - throw new Problem(403, { detail: permErr.message }).send(res); + throw new Problem(403, { detail: permErr.message, instance: req.originalUrl }); }); } else { next(errorToProblem(SERVICE, e)); diff --git a/app/src/controllers/object.js b/app/src/controllers/object.js index c94b30b5..fe87613b 100644 --- a/app/src/controllers/object.js +++ b/app/src/controllers/object.js @@ -164,6 +164,7 @@ const controller = { * @param {object} res Express response object * @param {function} next The next callback function * @returns {function} Express middleware function + * @throws The error encountered upon failure */ async addTags(req, res, next) { try { @@ -190,7 +191,10 @@ const controller = { if (newSet.length > 10) { // 409 when total tag limit exceeded - return new Problem(409, { detail: 'Request exceeds maximum of 9 user-defined tag sets allowed' }).send(res); + throw new Problem(409, { + detail: 'Request exceeds maximum of 9 user-defined tag sets allowed', + instance: req.originalUrl + }); } const data = { @@ -233,7 +237,10 @@ const controller = { if (bucketId && userId) { const permission = await bucketPermissionService.searchPermissions({ userId: userId, bucketId: bucketId, permCode: 'CREATE' }); if (!permission.length) { - throw new Problem(403, { detail: 'User lacks permission to complete this action' }); + throw new Problem(403, { + detail: 'User lacks permission to complete this action', + instance: req.originalUrl + }); } } @@ -263,18 +270,27 @@ const controller = { }); // Hard short circuit skip file as the object already exists on bucket - throw new Problem(409, { detail: 'Bucket already contains object' }); + throw new Problem(409, { + detail: 'Bucket already contains object', + instance: req.originalUrl + }); } catch (err) { if (err instanceof Problem) throw err; // Rethrow Problem type errors // Object is soft deleted from the bucket if (err.$response?.headers['x-amz-delete-marker']) { - throw new Problem(409, { detail: 'Bucket already contains object' }); + throw new Problem(409, { + detail: 'Bucket already contains object', + instance: req.originalUrl + }); } // Skip upload in the unlikely event we get an unexpected error from headObject if (err.$metadata?.httpStatusCode !== 404) { - throw new Problem(502, { detail: 'Bucket communication error' }); + throw new Problem(502, { + detail: 'Bucket communication error', + instance: req.originalUrl + }); } // Object does not exist on bucket @@ -293,7 +309,10 @@ const controller = { }); s3Response = await storageService.upload({ ...data, stream: req }); } else { - throw new Problem(413, { detail: 'File exceeds maximum 50GB limit' }); + throw new Problem(413, { + detail: 'File exceeds maximum 50GB limit', + instance: req.originalUrl + }); } } @@ -932,7 +951,10 @@ const controller = { // Skip upload in the unlikely event we get an unexpected response from headObject if (headResponse.$metadata?.httpStatusCode !== 200) { - throw new Problem(502, { detail: 'Bucket communication error' }); + throw new Problem(502, { + detail: 'Bucket communication error', + instance: req.originalUrl + }); } // Object exists on bucket @@ -951,20 +973,32 @@ const controller = { }); s3Response = await storageService.upload({ ...data, stream: req }); } else { - throw new Problem(413, { detail: 'File exceeds maximum 50GB limit' }); + throw new Problem(413, { + detail: 'File exceeds maximum 50GB limit', + instance: req.originalUrl + }); } } catch (err) { if (err instanceof Problem) throw err; // Rethrow Problem type errors else if (err.$metadata?.httpStatusCode !== 404) { // An unexpected response from headObject - throw new Problem(502, { detail: 'Bucket communication error' }); + throw new Problem(502, { + detail: 'Bucket communication error', + instance: req.originalUrl + }); } else { if (err.$response?.headers['x-amz-delete-marker']) { // Object is soft deleted from the bucket - throw new Problem(409, { detail: 'Unable to update soft deleted object' }); + throw new Problem(409, { + detail: 'Unable to update soft deleted object', + instance: req.originalUrl + }); } else { // Bucket is missing the existing object - throw new Problem(409, { detail: 'Bucket does not contain existing object' }); + throw new Problem(409, { + detail: 'Bucket does not contain existing object', + instance: req.originalUrl + }); } // TODO: Add in sync operation to update object record in COMS DB? } diff --git a/app/src/middleware/authentication.js b/app/src/middleware/authentication.js index 2dcd2861..140a836d 100644 --- a/app/src/middleware/authentication.js +++ b/app/src/middleware/authentication.js @@ -44,6 +44,7 @@ const _spkiWrapper = (spki) => `-----BEGIN PUBLIC KEY-----\n${spki}\n-----END PU * @param {object} res Express response object * @param {function} next The next callback function * @returns {function} Express middleware function + * @throws The error encountered upon failure */ const currentUser = async (req, res, next) => { const authorization = req.get('Authorization'); @@ -84,7 +85,7 @@ const currentUser = async (req, res, next) => { throw new Error('Invalid authorization token'); } } catch (err) { - return new Problem(403, { detail: err.message }).send(res); + throw new Problem(403, { detail: err.message, instance: req.originalUrl }); } } } diff --git a/app/src/middleware/authorization.js b/app/src/middleware/authorization.js index b079573d..55a32ee4 100644 --- a/app/src/middleware/authorization.js +++ b/app/src/middleware/authorization.js @@ -49,11 +49,12 @@ const _checkPermission = async ({ currentObject, currentUser, params }, permissi * @function checkAppMode * Rejects the request if the incoming authentication mode does not match the application mode * @param {object} req Express request object - * @param {object} res Express response object + * @param {object} _res Express response object * @param {function} next The next callback function * @returns {function} Express middleware function + * @throws The error encountered upon failure */ -const checkAppMode = (req, res, next) => { +const checkAppMode = (req, _res, next) => { const authMode = getAppAuthMode(); const authType = req.currentUser ? req.currentUser.authType : undefined; @@ -65,11 +66,12 @@ const checkAppMode = (req, res, next) => { } } catch (err) { log.verbose(err.message, { function: 'checkAppMode', authMode: authMode, authType: authType }); - return new Problem(501, { + throw new Problem(501, { detail: 'Current application mode does not support incoming authentication type', + instance: req.originalUrl, authMode: authMode, authType: authType - }).send(res); + }); } next(); @@ -107,9 +109,10 @@ const currentObject = async (req, _res, next) => { * - if passed permission exists for current user on object or bucket (see: _checkPermission) * @param {string} permission a permission code (eg: READ) * @returns {function} Express middleware function + * @throws The error encountered upon failure */ const hasPermission = (permission) => { - return async (req, res, next) => { + return async (req, _res, next) => { const authMode = getAppAuthMode(); const authType = req.currentUser ? req.currentUser.authType : undefined; @@ -133,7 +136,7 @@ const hasPermission = (permission) => { } } catch (err) { log.verbose(err.message, { function: 'hasPermission' }); - return new Problem(403, { detail: 'User lacks permission to complete this action' }).send(res); + throw new Problem(403, { detail: 'User lacks permission to complete this action', instance: req.originalUrl }); } next(); diff --git a/app/src/middleware/featureToggle.js b/app/src/middleware/featureToggle.js index 18eb8741..cf883d9f 100644 --- a/app/src/middleware/featureToggle.js +++ b/app/src/middleware/featureToggle.js @@ -10,6 +10,7 @@ const { getAppAuthMode } = require('../components/utils'); * @param {object} res Express response object * @param {function} next The next callback function * @returns {function} Express middleware function + * @throws The error encountered upon failure */ const requireBasicAuth = (req, res, next) => { const authMode = getAppAuthMode(); @@ -18,11 +19,17 @@ const requireBasicAuth = (req, res, next) => { const canBasicMode = (mode) => [AuthMode.BASICAUTH, AuthMode.FULLAUTH].includes(mode); if (authMode === AuthMode.OIDCAUTH) { - return new Problem(501, { detail: 'This action is not supported in the current authentication mode' }).send(res); + throw new Problem(501, { + detail: 'This action is not supported in the current authentication mode', + instance: req.originalUrl + }); } if (canBasicMode(authMode) && authType !== AuthType.BASIC) { - return new Problem(403, { detail: 'User lacks permission to complete this action' }).send(res); + throw new Problem(403, { + detail: 'User lacks permission to complete this action', + instance: req.originalUrl + }); } next(); @@ -35,13 +42,17 @@ const requireBasicAuth = (req, res, next) => { * @param {object} res Express response object * @param {function} next The next callback function * @returns {function} Express middleware function + * @throws The error encountered upon failure */ const requireSomeAuth = (req, res, next) => { const authMode = getAppAuthMode(); const authType = req.currentUser ? req.currentUser.authType : undefined; if (authMode !== AuthMode.NOAUTH && (!authType || authType === AuthType.NONE)) { - return new Problem(403, { detail: 'User lacks permission to complete this action' }).send(res); + throw new Problem(403, { + detail: 'User lacks permission to complete this action', + instance: req.originalUrl + }); } next(); diff --git a/app/src/middleware/upload.js b/app/src/middleware/upload.js index 4901ccc9..0c052d30 100644 --- a/app/src/middleware/upload.js +++ b/app/src/middleware/upload.js @@ -8,13 +8,17 @@ const contentDisposition = require('content-disposition'); * @param {object} _res Express response object * @param {function} next The next callback function * @returns {function} Express middleware function + * @throws The error encountered upon failure */ const currentUpload = (strict = false) => { - return (req, res, next) => { + return (req, _res, next) => { // Check Content-Length Header const contentLength = parseInt(req.get('Content-Length')); // TODO: Figure out what's killing and returning a 400 in response stack - if (!contentLength) return new Problem(411, { detail: 'Content-Length must be greater than 0' }).send(res); + if (!contentLength) throw new Problem(411, { + detail: 'Content-Length must be greater than 0', + instance: req.originalUrl + }); // Check Content-Disposition Header let filename; @@ -27,10 +31,16 @@ const currentUpload = (strict = false) => { filename = parameters?.filename; } catch (e) { // Ignore improperly formatted Content-Disposition when not in strict mode - if (strict) return new Problem(400, { detail: `Content-Disposition header error: ${e.message}` }).send(res); + if (strict) throw new Problem(400, { + detail: `Content-Disposition header error: ${e.message}`, + instance: req.originalUrl + }); } } else { - if (strict) return new Problem(415, { detail: 'Content-Disposition header missing' }).send(res); + if (strict) throw new Problem(415, { + detail: 'Content-Disposition header missing', + instance: req.originalUrl + }); } // Check Content-Type Header diff --git a/app/src/middleware/validation.js b/app/src/middleware/validation.js index b3128a68..df8f8449 100644 --- a/app/src/middleware/validation.js +++ b/app/src/middleware/validation.js @@ -5,9 +5,10 @@ const Problem = require('api-problem'); * Performs express request validation against a specified `schema` * @param {object} schema An object containing Joi validation schema definitions * @returns {function} Express middleware function + * @throws The error encountered upon failure */ const validate = (schema) => { - return (req, res, next) => { + return (req, _res, next) => { const validationErrors = Object.entries(schema) .map(([prop, def]) => { const result = def.validate(req[prop], { abortEarly: false })?.error; @@ -16,13 +17,13 @@ const validate = (schema) => { .filter(error => !!error); if (Object.keys(validationErrors).length) { - return new Problem(422, { + throw new Problem(422, { detail: validationErrors .flatMap(groups => groups[1]?.map(error => error?.message)) .join('; '), instance: req.originalUrl, errors: Object.fromEntries(validationErrors) - }).send(res); + }); } else next(); }; diff --git a/app/src/validators/object.js b/app/src/validators/object.js index 6141ed92..bfd66caa 100644 --- a/app/src/validators/object.js +++ b/app/src/validators/object.js @@ -27,8 +27,6 @@ const schema = { }).nand('s3VersionId', 'versionId') }, - // TODO: Make this schema definition unit-testable - // bucketId property was undefined in unit test createObject: { headers: type.metadata(), query: Joi.object({ diff --git a/app/tests/common/helper.js b/app/tests/common/helper.js index 97b8ce41..89ec5dac 100644 --- a/app/tests/common/helper.js +++ b/app/tests/common/helper.js @@ -22,21 +22,21 @@ const helper = { })); app.use(basePath, router); + // Handle 404 + app.use((req, _res) => { // eslint-disable-line no-unused-vars + throw new Problem(404, { instance: req.originalUrl }); + }); + // Handle 500 // eslint-disable-next-line no-unused-vars - app.use((err, _req, res, _next) => { + app.use((err, req, res, _next) => { if (err instanceof Problem) { err.send(res); } else { - new Problem(500, { detail: err.message ?? err }).send(res); + new Problem(500, { detail: err.message ?? err, instance: req.originalUrl }).send(res); } }); - // Handle 404 - app.use((_req, res) => { - new Problem(404).send(res); - }); - return app; }, diff --git a/app/tests/unit/controllers/bucket.spec.js b/app/tests/unit/controllers/bucket.spec.js index 56e54953..f33b6b06 100644 --- a/app/tests/unit/controllers/bucket.spec.js +++ b/app/tests/unit/controllers/bucket.spec.js @@ -201,7 +201,7 @@ describe('createBucket', () => { await controller.createBucket(req, res, next); expect(headBucketSpy).toHaveBeenCalledTimes(1); - expect(headBucketSpy).toHaveBeenCalledWith(req.body); + expect(headBucketSpy).toHaveBeenCalledWith(expect.objectContaining(req.body)); expect(getCurrentIdentitySpy).toHaveBeenCalledTimes(1); expect(getCurrentIdentitySpy).toHaveBeenCalledWith( CURRENT_USER, @@ -212,7 +212,7 @@ describe('createBucket', () => { expect(createSpy).toHaveBeenCalledTimes(1); expect(createSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID }); expect(checkGrantPermissionsSpy).toHaveBeenCalledTimes(1); - expect(checkGrantPermissionsSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID }); + expect(checkGrantPermissionsSpy).toHaveBeenCalledWith(expect.objectContaining({ ...req.body, userId: USR_ID })); expect(res.status).toHaveBeenCalledWith(201); }); diff --git a/app/tests/unit/middleware/authentication.spec.js b/app/tests/unit/middleware/authentication.spec.js index 87997193..47e8772d 100644 --- a/app/tests/unit/middleware/authentication.spec.js +++ b/app/tests/unit/middleware/authentication.spec.js @@ -86,7 +86,6 @@ describe('currentUser', () => { const checkBasicAuthSpy = jest.spyOn(mw, '_checkBasicAuth'); const jwtVerifySpy = jest.spyOn(jwt, 'verify'); const loginSpy = jest.spyOn(userService, 'login'); - const problemSendSpy = jest.spyOn(Problem.prototype, 'send'); let req, res, next; @@ -117,7 +116,6 @@ describe('currentUser', () => { expect(checkBasicAuthSpy).toHaveBeenCalledTimes(0); expect(next).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(0); }); }); @@ -141,7 +139,6 @@ describe('currentUser', () => { expect(checkBasicAuthSpy).toHaveBeenCalledTimes(1); expect(checkBasicAuthSpy).toHaveBeenCalledWith(req, res, next); expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(0); }); }); @@ -192,13 +189,11 @@ describe('currentUser', () => { expect(loginSpy).toHaveBeenCalledWith(expect.objectContaining({ sub: 'sub' })); expect(next).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(0); }); - it('short circuits with invalid auth token', async () => { + it('short circuits with invalid auth token', () => { const authorization = 'bearer '; - problemSendSpy.mockImplementation(() => { }); config.has .mockReturnValueOnce(false) // basicAuth.enabled .mockReturnValueOnce(true) // keycloak.enabled @@ -209,7 +204,7 @@ describe('currentUser', () => { .mockReturnValueOnce(realm); // keycloak.realm req.get.mockReturnValueOnce(authorization); - await mw.currentUser(req, res, next); + expect(() => mw.currentUser(req, res, next)).rejects.toThrow(); expect(req.currentUser).toBeFalsy(); expect(req.get).toHaveBeenCalledTimes(1); @@ -225,11 +220,9 @@ describe('currentUser', () => { })); expect(loginSpy).toHaveBeenCalledTimes(0); expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(1); - expect(problemSendSpy).toHaveBeenCalledWith(res); }); - it('short circuits without keycloak.publicKey', async () => { + it('short circuits without keycloak.publicKey', () => { jwtVerifySpy.mockReturnValue({ sub: 'sub' }); loginSpy.mockImplementation(() => { }); config.has @@ -238,7 +231,7 @@ describe('currentUser', () => { .mockReturnValueOnce(false); // keycloak.publicKey req.get.mockReturnValueOnce(authorization); - await mw.currentUser(req, res, next); + expect(() => mw.currentUser(req, res, next)).rejects.toThrow(); expect(req.currentUser).toBeFalsy(); expect(req.get).toHaveBeenCalledTimes(1); @@ -251,8 +244,6 @@ describe('currentUser', () => { expect(jwtVerifySpy).toHaveBeenCalledTimes(0); expect(loginSpy).toHaveBeenCalledTimes(0); expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(1); - expect(problemSendSpy).toHaveBeenCalledWith(res); }); }); }); diff --git a/app/tests/unit/middleware/authorization.spec.js b/app/tests/unit/middleware/authorization.spec.js index 8853f887..62b23aa0 100644 --- a/app/tests/unit/middleware/authorization.spec.js +++ b/app/tests/unit/middleware/authorization.spec.js @@ -1,4 +1,3 @@ -const Problem = require('api-problem'); const { NIL: SYSTEM_USER } = require('uuid'); const mw = require('../../../src/middleware/authorization'); @@ -131,7 +130,6 @@ describe('_checkPermission', () => { describe('checkAppMode', () => { const getAppAuthModeSpy = jest.spyOn(utils, 'getAppAuthMode'); - const problemSendSpy = jest.spyOn(Problem.prototype, 'send'); let req, res, next; @@ -161,23 +159,20 @@ describe('checkAppMode', () => { ])('should call next %i times given authMode %s and authType %s', (nextCount, mode, type) => { const sendCount = 1 - nextCount; getAppAuthModeSpy.mockReturnValue(mode); - problemSendSpy.mockImplementation(() => { }); if (type) req.currentUser = { authType: type }; - mw.checkAppMode(req, res, next); + const result = () => mw.checkAppMode(req, res, next); + if (sendCount) expect(result).toThrow(); + else expect(result).not.toThrow(); expect(getAppAuthModeSpy).toHaveBeenCalledTimes(1); expect(getAppAuthModeSpy).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); expect(next).toHaveBeenCalledTimes(nextCount); }); }); -// TODO: Determine if storage call is still part of the injected object describe('currentObject', () => { const objectReadSpy = jest.spyOn(objectService, 'read'); - // const storageListObjectVersionSpy = jest.spyOn(storageService, 'listObjectVersion'); let req, res, next; @@ -205,7 +200,6 @@ describe('currentObject', () => { const objectId = '1234'; req.params = { objectId: objectId }; objectReadSpy.mockImplementation(() => { throw new Error('test'); }); - // storageListObjectVersionSpy.mockResolvedValue({}); mw.currentObject(req, res, next); @@ -219,7 +213,6 @@ describe('currentObject', () => { it('injects the current object based on the service results', async () => { const objectId = '1234'; const testRecord = { a: 1 }; - // const testStorage = { b: 2 }; req.params = { objectId: objectId }; objectReadSpy.mockResolvedValue(testRecord); @@ -227,7 +220,6 @@ describe('currentObject', () => { expect(req.currentObject).toBeTruthy(); expect(req.currentObject).toEqual(expect.objectContaining(testRecord)); - // expect(req.currentObject).toEqual(expect.objectContaining(testStorage)); expect(objectReadSpy).toHaveBeenCalledTimes(1); expect(objectReadSpy).toHaveBeenCalledWith(objectId); expect(next).toHaveBeenCalledTimes(1); @@ -240,13 +232,10 @@ describe('hasPermission', () => { const getCurrentIdentitySpy = jest.spyOn(utils, 'getCurrentIdentity'); const getCurrentUserIdSpy = jest.spyOn(userService, 'getCurrentUserId'); const objSearchPermissionsSpy = jest.spyOn(objectPermissionService, 'searchPermissions'); - const problemSendSpy = jest.spyOn(Problem.prototype, 'send'); let req, res, next; beforeEach(() => { - problemSendSpy.mockImplementation(() => { }); - req = {}; res = {}; next = jest.fn(); @@ -264,12 +253,11 @@ describe('hasPermission', () => { const result = mw.hasPermission(Permissions.READ); expect(result).toBeInstanceOf(Function); - result(req, res, next); + if (sendCount) expect(result(req, res, next)).rejects.toThrow(); + else expect(result(req, res, next)).resolves.not.toThrow(); expect(next).toHaveBeenCalledTimes(nextCount); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(0); }); }); @@ -285,7 +273,7 @@ describe('hasPermission', () => { [1, AuthMode.BASICAUTH], [0, AuthMode.OIDCAUTH], [0, AuthMode.FULLAUTH] - ])('should call next %i times given authMode %s', async (nextCount, mode) => { + ])('should call next %i times given authMode %s', (nextCount, mode) => { const sendCount = 1 - nextCount; getAppAuthModeSpy.mockReturnValue(mode); getCurrentUserIdSpy.mockResolvedValue(SYSTEM_USER); @@ -293,12 +281,11 @@ describe('hasPermission', () => { const result = mw.hasPermission(Permissions.READ); expect(result).toBeInstanceOf(Function); - await result(req, res, next); + if (sendCount) expect(result(req, res, next)).rejects.toThrow(); + else expect(result(req, res, next)).resolves.not.toThrow(); expect(next).toHaveBeenCalledTimes(nextCount); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(0); }); }); @@ -313,7 +300,7 @@ describe('hasPermission', () => { [{ bucketId: SYSTEM_USER }], [{ objectId: SYSTEM_USER }], [{ bucketId: SYSTEM_USER, objectId: SYSTEM_USER }] - ])('should call next 0 times with params %j', async (params) => { + ])('should call next 0 times with params %j', (params) => { req.params = params; getAppAuthModeSpy.mockReturnValue(AuthMode.FULLAUTH); getCurrentUserIdSpy.mockResolvedValue(SYSTEM_USER); @@ -321,11 +308,9 @@ describe('hasPermission', () => { const result = mw.hasPermission(Permissions.READ); expect(result).toBeInstanceOf(Function); - await result(req, res, next); + expect(result(req, res, next)).rejects.toThrow(); expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(1); - expect(problemSendSpy).toHaveBeenCalledWith(res); }); }); @@ -341,7 +326,7 @@ describe('hasPermission', () => { [1, AuthMode.BASICAUTH], [0, AuthMode.OIDCAUTH], [1, AuthMode.FULLAUTH] - ])('should call next %i times when authType BASIC and authMode %s', async (nextCount, mode) => { + ])('should call next %i times when authType BASIC and authMode %s', (nextCount, mode) => { const sendCount = 1 - nextCount; getAppAuthModeSpy.mockReturnValue(mode); checkPermissionSpy.mockResolvedValue(false); @@ -349,12 +334,11 @@ describe('hasPermission', () => { const result = mw.hasPermission(Permissions.READ); expect(result).toBeInstanceOf(Function); - await result(req, res, next); + if (sendCount) expect(result(req, res, next)).rejects.toThrow(); + else expect(result(req, res, next)).resolves.not.toThrow(); expect(next).toHaveBeenCalledTimes(nextCount); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); expect(checkPermissionSpy).toHaveBeenCalledTimes(0); }); @@ -369,7 +353,7 @@ describe('hasPermission', () => { [0, true, Permissions.UPDATE], [0, true, Permissions.DELETE], [0, true, Permissions.MANAGE] - ])('should call next %i times when public %s and permission %s', async (nextCount, isPublic, perm) => { + ])('should call next %i times when public %s and permission %s', (nextCount, isPublic, perm) => { const sendCount = 1 - nextCount; getAppAuthModeSpy.mockReturnValue(AuthMode.OIDCAUTH); getCurrentUserIdSpy.mockResolvedValue(SYSTEM_USER); @@ -378,12 +362,11 @@ describe('hasPermission', () => { const result = mw.hasPermission(perm); expect(result).toBeInstanceOf(Function); - await result(req, res, next); + if (sendCount) expect(result(req, res, next)).rejects.toThrow(); + else expect(result(req, res, next)).resolves.not.toThrow(); expect(next).toHaveBeenCalledTimes(nextCount); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(0); }); }); @@ -404,7 +387,7 @@ describe('hasPermission', () => { [0, AuthType.BEARER, SYSTEM_USER, []], [0, AuthType.BEARER, SYSTEM_USER, [{ permCode: Permissions.UPDATE }]], [1, AuthType.BEARER, SYSTEM_USER, [{ permCode: Permissions.READ }, { permCode: Permissions.UPDATE }]] - ])('should call next %i times when authType %s, userId %o and have permissions %j', async (nextCount, type, userId, perms) => { + ])('should call next %i times when authType %s, userId %o and have permissions %j', (nextCount, type, userId, perms) => { const sendCount = 1 - nextCount; const searchPermCount = +(type === AuthType.BEARER && !!userId); getAppAuthModeSpy.mockReturnValue(AuthMode.OIDCAUTH); @@ -416,17 +399,20 @@ describe('hasPermission', () => { const result = mw.hasPermission(Permissions.READ); expect(result).toBeInstanceOf(Function); - await result(req, res, next); - - expect(next).toHaveBeenCalledTimes(nextCount); - if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); - expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(searchPermCount); - if (searchPermCount) { - expect(objSearchPermissionsSpy).toHaveBeenCalledWith(expect.objectContaining({ objId: SYSTEM_USER })); - expect(objSearchPermissionsSpy).toHaveBeenCalledWith(expect.objectContaining({ userId: userId })); - } + result(req, res, next).then(() => { + expect(sendCount).toEqual(0); + }).catch(() => { + expect(sendCount).toEqual(1); + }).finally(() => { + expect(next).toHaveBeenCalledTimes(nextCount); + if (nextCount) expect(next).toHaveBeenCalledWith(); + + expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(searchPermCount); + if (searchPermCount) { + expect(objSearchPermissionsSpy).toHaveBeenCalledWith(expect.objectContaining({ objId: SYSTEM_USER })); + expect(objSearchPermissionsSpy).toHaveBeenCalledWith(expect.objectContaining({ userId: userId })); + } + }); }); }); }); diff --git a/app/tests/unit/middleware/featureToggle.spec.js b/app/tests/unit/middleware/featureToggle.spec.js index eae17db7..6eb30ac9 100644 --- a/app/tests/unit/middleware/featureToggle.spec.js +++ b/app/tests/unit/middleware/featureToggle.spec.js @@ -1,5 +1,3 @@ -const Problem = require('api-problem'); - const { requireBasicAuth, requireSomeAuth } = require('../../../src/middleware/featureToggle'); const { AuthMode, AuthType } = require('../../../src/components/constants'); const utils = require('../../../src/components/utils'); @@ -19,13 +17,10 @@ afterAll(() => { describe('requireBasicAuth', () => { const getAppAuthModeSpy = jest.spyOn(utils, 'getAppAuthMode'); - const problemSendSpy = jest.spyOn(Problem.prototype, 'send'); let req, res, next; beforeEach(() => { - problemSendSpy.mockImplementation(() => { }); - req = {}; res = {}; next = jest.fn(); @@ -53,24 +48,20 @@ describe('requireBasicAuth', () => { getAppAuthModeSpy.mockReturnValue(mode); if (type) req.currentUser = { authType: type }; - requireBasicAuth(req, res, next); + if (sendCount) expect(() => requireBasicAuth(req, res, next)).toThrow(); + else expect(() => requireBasicAuth(req, res, next)).not.toThrow(); expect(next).toHaveBeenCalledTimes(nextCount); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); }); }); describe('requireSomeAuth', () => { const getAppAuthModeSpy = jest.spyOn(utils, 'getAppAuthMode'); - const problemSendSpy = jest.spyOn(Problem.prototype, 'send'); let req, res, next; beforeEach(() => { - problemSendSpy.mockImplementation(() => { }); - req = {}; res = {}; next = jest.fn(); @@ -98,11 +89,10 @@ describe('requireSomeAuth', () => { getAppAuthModeSpy.mockReturnValue(mode); if (type) req.currentUser = { authType: type }; - requireSomeAuth(req, res, next); + if (sendCount) expect(() => requireSomeAuth(req, res, next)).toThrow(); + else expect(() => requireSomeAuth(req, res, next)).not.toThrow(); expect(next).toHaveBeenCalledTimes(nextCount); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); }); }); diff --git a/app/tests/unit/middleware/upload.spec.js b/app/tests/unit/middleware/upload.spec.js index c5d429bf..8af29685 100644 --- a/app/tests/unit/middleware/upload.spec.js +++ b/app/tests/unit/middleware/upload.spec.js @@ -1,5 +1,3 @@ -const Problem = require('api-problem'); - const { currentUpload } = require('../../../src/middleware/upload'); beforeEach(() => { @@ -11,13 +9,9 @@ afterAll(() => { }); describe('currentUpload', () => { - const problemSendSpy = jest.spyOn(Problem.prototype, 'send'); - let req, res, next; beforeEach(() => { - problemSendSpy.mockImplementation(() => { }); - req = { get: jest.fn() }; res = {}; next = jest.fn(); @@ -53,13 +47,12 @@ describe('currentUpload', () => { const result = currentUpload(strict); expect(result).toBeInstanceOf(Function); - result(req, res, next); + if (sendCount) expect(() => result(req, res, next)).toThrow(); + else expect(() => result(req, res, next)).not.toThrow(); expect(req.currentUpload).toEqual(current); expect(next).toHaveBeenCalledTimes(nextCount); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); }); }); diff --git a/app/tests/unit/middleware/validation.spec.js b/app/tests/unit/middleware/validation.spec.js index 966954b9..df1fc994 100644 --- a/app/tests/unit/middleware/validation.spec.js +++ b/app/tests/unit/middleware/validation.spec.js @@ -1,5 +1,3 @@ -const Problem = require('api-problem'); - const { validate } = require('../../../src/middleware/validation'); beforeEach(() => { @@ -11,13 +9,9 @@ afterAll(() => { }); describe('validate', () => { - const problemSendSpy = jest.spyOn(Problem.prototype, 'send'); - let req, res, next; beforeEach(() => { - problemSendSpy.mockImplementation(() => { }); - req = { originalUrl: 'originalUrl', params: { id: 'id' }, @@ -33,12 +27,11 @@ describe('validate', () => { const result = validate(schema); expect(result).toBeInstanceOf(Function); - result(req, res, next); + expect(() => result(req, res, next)).not.toThrow(); expect(schema.query.validate).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(0); }); it('should respond with 422 with one validation error', () => { @@ -47,12 +40,10 @@ describe('validate', () => { const result = validate(schema); expect(result).toBeInstanceOf(Function); - result(req, res, next); + expect(() => result(req, res, next)).toThrow(); expect(schema.query.validate).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(1); - expect(problemSendSpy).toHaveBeenCalledWith(res); }); it('should respond with 422 with multiple validation errors', () => { @@ -64,12 +55,10 @@ describe('validate', () => { const result = validate(schema); expect(result).toBeInstanceOf(Function); - result(req, res, next); + expect(() => result(req, res, next)).toThrow(); expect(schema.query.validate).toHaveBeenCalledTimes(1); expect(schema.query.validate).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(1); - expect(problemSendSpy).toHaveBeenCalledWith(res); }); }); diff --git a/app/tests/unit/validators/object.spec.js b/app/tests/unit/validators/object.spec.js index ebc8f03e..397b1724 100644 --- a/app/tests/unit/validators/object.spec.js +++ b/app/tests/unit/validators/object.spec.js @@ -1,4 +1,3 @@ -const config = require('config'); const crypto = require('crypto'); const Joi = require('joi'); const jestJoi = require('jest-joi'); @@ -96,18 +95,11 @@ describe('createObject', () => { describe('query', () => { describe('bucketId', () => { + const bucketId = schema.createObject.query.describe().keys.bucketId; - it.skip('is in schema when DB mode enabled', () => { - config.has.mockReturnValueOnce(true); - const bucketId = schema.createObject.query.describe().keys.bucketId; + it('is the expected schema', () => { expect(bucketId).toEqual(type.uuidv4.describe()); }); - - it.skip('is not in schema when DB mode not enabled', () => { - config.has.mockReturnValueOnce(false); - const bucketId = schema.createObject.query.describe().keys.bucketId; - expect(bucketId).toEqual(undefined); - }); }); describe('tagset', () => { @@ -396,8 +388,7 @@ describe('searchObjects', () => { })); }); - // TODO: define expected schema - it.skip('enforces general metadata pattern', () => { + it('enforces general metadata pattern', () => { expect(headers.patterns).toEqual(expect.arrayContaining([ expect.objectContaining({ regex: '/^x-amz-meta-.{1,255}$/i', @@ -407,7 +398,7 @@ describe('searchObjects', () => { expect.objectContaining({ name: 'min', args: expect.objectContaining({ - limit: 0 + limit: 1 }) }), expect.objectContaining({