diff --git a/app/app.js b/app/app.js index 9e7aa05d..e8255560 100644 --- a/app/app.js +++ b/app/app.js @@ -3,7 +3,6 @@ const compression = require('compression'); const config = require('config'); const cors = require('cors'); const express = require('express'); -const { ValidationError } = require('express-validation'); const { AuthMode, DEFAULTCORS } = require('./src/components/constants'); const log = require('./src/components/log')(module.filename); @@ -72,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, { details: 'Server is shutting down' }).send(res); + throw new Problem(503, { detail: 'Server is shutting down' }); } else if (!state.ready) { - new Problem(503, { details: 'Server is not ready' }).send(res); + throw new Problem(503, { detail: 'Server is not ready' }); } else { next(); } @@ -108,31 +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 if (err instanceof ValidationError) { - log.debug(err); - return res.status(err.statusCode).json(err); } else { - // Only log unexpected errors - if (err.stack) log.error(err); - - new Problem(500, 'Server Error', { - detail: (err.message) ? 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, 'Page Not Found', { - detail: 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/package-lock.json b/app/package-lock.json index 680d5ab3..043b0461 100644 --- a/app/package-lock.json +++ b/app/package-lock.json @@ -20,8 +20,8 @@ "date-fns": "^2.30.0", "express": "^4.18.2", "express-basic-auth": "^1.2.1", - "express-validation": "^4.1.0", "express-winston": "^4.2.0", + "joi": "^17.10.2", "js-yaml": "^4.1.0", "jsonwebtoken": "^9.0.1", "knex": "^2.5.1", @@ -3836,50 +3836,12 @@ "@babel/types": "^7.3.0" } }, - "node_modules/@types/body-parser": { - "version": "1.19.2", - "resolved": "https://registry.npmjs.org/@types/body-parser/-/body-parser-1.19.2.tgz", - "integrity": "sha512-ALYone6pm6QmwZoAgeyNksccT9Q4AWZQ6PvfwR37GT6r6FWUPguq6sUmNGSMV2Wr761oQoBxwGGa6DR5o1DC9g==", - "dependencies": { - "@types/connect": "*", - "@types/node": "*" - } - }, "node_modules/@types/color-name": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/@types/color-name/-/color-name-1.1.1.tgz", "integrity": "sha512-rr+OQyAjxze7GgWrSaJwydHStIhHq2lvY3BOC2Mj7KnzI7XK0Uw1TOOdI9lDoajEbSWLiYgoo4f1R51erQfhPQ==", "dev": true }, - "node_modules/@types/connect": { - "version": "3.4.35", - "resolved": "https://registry.npmjs.org/@types/connect/-/connect-3.4.35.tgz", - "integrity": "sha512-cdeYyv4KWoEgpBISTxWvqYsVy444DOqehiF3fM3ne10AmJ62RSyNkUnxMJXHQWRQQX2eR94m5y1IZyDwBjV9FQ==", - "dependencies": { - "@types/node": "*" - } - }, - "node_modules/@types/express": { - "version": "4.17.13", - "resolved": "https://registry.npmjs.org/@types/express/-/express-4.17.13.tgz", - "integrity": "sha512-6bSZTPaTIACxn48l50SR+axgrqm6qXFIxrdAKaG6PaJk3+zuUr35hBlgT7vOmJcum+OEaIBLtHV/qloEAFITeA==", - "dependencies": { - "@types/body-parser": "*", - "@types/express-serve-static-core": "^4.17.18", - "@types/qs": "*", - "@types/serve-static": "*" - } - }, - "node_modules/@types/express-serve-static-core": { - "version": "4.17.28", - "resolved": "https://registry.npmjs.org/@types/express-serve-static-core/-/express-serve-static-core-4.17.28.tgz", - "integrity": "sha512-P1BJAEAW3E2DJUlkgq4tOL3RyMunoWXqbSCygWo5ZIWTjUgN1YnaXWW4VWl/oc8vs/XoYibEGBKP0uZyF4AHig==", - "dependencies": { - "@types/node": "*", - "@types/qs": "*", - "@types/range-parser": "*" - } - }, "node_modules/@types/graceful-fs": { "version": "4.1.5", "resolved": "https://registry.npmjs.org/@types/graceful-fs/-/graceful-fs-4.1.5.tgz", @@ -3889,11 +3851,6 @@ "@types/node": "*" } }, - "node_modules/@types/hapi__joi": { - "version": "16.0.12", - "resolved": "https://registry.npmjs.org/@types/hapi__joi/-/hapi__joi-16.0.12.tgz", - "integrity": "sha512-xJYifuz59jXdWY5JMS15uvA3ycS3nQYOGqoIIE0+fwQ0qI3/4CxBc6RHsOTp6wk9M0NWEdpcTl02lOQOKMifbQ==" - }, "node_modules/@types/istanbul-lib-coverage": { "version": "2.0.4", "resolved": "https://registry.npmjs.org/@types/istanbul-lib-coverage/-/istanbul-lib-coverage-2.0.4.tgz", @@ -3934,15 +3891,11 @@ "integrity": "sha1-7ihweulOEdK4J7y+UnC86n8+ce4=", "dev": true }, - "node_modules/@types/mime": { - "version": "1.3.2", - "resolved": "https://registry.npmjs.org/@types/mime/-/mime-1.3.2.tgz", - "integrity": "sha512-YATxVxgRqNH6nHEIsvg6k2Boc1JHI9ZbH5iWFFv/MTkchz3b1ieGDa5T0a9RznNdI0KhVbdbWSN+KWWrQZRxTw==" - }, "node_modules/@types/node": { "version": "14.0.23", "resolved": "https://registry.npmjs.org/@types/node/-/node-14.0.23.tgz", - "integrity": "sha512-Z4U8yDAl5TFkmYsZdFPdjeMa57NOvnaf1tljHzhouaPEp7LCj2JKkejpI1ODviIAQuW4CcQmxkQ77rnLsOOoKw==" + "integrity": "sha512-Z4U8yDAl5TFkmYsZdFPdjeMa57NOvnaf1tljHzhouaPEp7LCj2JKkejpI1ODviIAQuW4CcQmxkQ77rnLsOOoKw==", + "dev": true }, "node_modules/@types/prettier": { "version": "2.7.1", @@ -3950,25 +3903,6 @@ "integrity": "sha512-ri0UmynRRvZiiUJdiz38MmIblKK+oH30MztdBVR95dv/Ubw6neWSb8u1XpRb72L4qsZOhz+L+z9JD40SJmfWow==", "dev": true }, - "node_modules/@types/qs": { - "version": "6.9.7", - "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.9.7.tgz", - "integrity": "sha512-FGa1F62FT09qcrueBA6qYTrJPVDzah9a+493+o2PCXsesWHIn27G98TsSMs3WPNbZIEj4+VJf6saSFpvD+3Zsw==" - }, - "node_modules/@types/range-parser": { - "version": "1.2.4", - "resolved": "https://registry.npmjs.org/@types/range-parser/-/range-parser-1.2.4.tgz", - "integrity": "sha512-EEhsLsD6UsDM1yFhAvy0Cjr6VwmpMWqFBCb9w07wVugF7w9nfajxLuVmngTIpgS6svCnm6Vaw+MZhoDCKnOfsw==" - }, - "node_modules/@types/serve-static": { - "version": "1.13.10", - "resolved": "https://registry.npmjs.org/@types/serve-static/-/serve-static-1.13.10.tgz", - "integrity": "sha512-nCkHGI4w7ZgAdNkrEu0bv+4xNV/XDqW+DydknebMOQwkpDGx8G+HTlj7R7ABI8i8nKxVw0wtKPi1D+lPOkh4YQ==", - "dependencies": { - "@types/mime": "^1", - "@types/node": "*" - } - }, "node_modules/@types/sinon": { "version": "10.0.10", "resolved": "https://registry.npmjs.org/@types/sinon/-/sinon-10.0.10.tgz", @@ -7482,16 +7416,6 @@ "basic-auth": "^2.0.1" } }, - "node_modules/express-validation": { - "version": "4.1.0", - "resolved": "https://registry.npmjs.org/express-validation/-/express-validation-4.1.0.tgz", - "integrity": "sha512-bAGRVdaJZ7yzCxELX29zvvJk0OUP/tw6tCtZtGnpK1O+yLY9qN8btwtclRQMGfIluZYIuxe++U/yPMlgEpU12A==", - "dependencies": { - "@types/express": "^4.17.13", - "@types/hapi__joi": "16.x.x", - "joi": "^17.6.0" - } - }, "node_modules/express-winston": { "version": "4.2.0", "resolved": "https://registry.npmjs.org/express-winston/-/express-winston-4.2.0.tgz", @@ -10604,14 +10528,14 @@ } }, "node_modules/joi": { - "version": "17.6.0", - "resolved": "https://registry.npmjs.org/joi/-/joi-17.6.0.tgz", - "integrity": "sha512-OX5dG6DTbcr/kbMFj0KGYxuew69HPcAE3K/sZpEV2nP6e/j/C0HV+HNiBPCASxdx5T7DMoa0s8UeHWMnb6n2zw==", + "version": "17.10.2", + "resolved": "https://registry.npmjs.org/joi/-/joi-17.10.2.tgz", + "integrity": "sha512-hcVhjBxRNW/is3nNLdGLIjkgXetkeGc2wyhydhz8KumG23Aerk4HPjU5zaPAMRqXQFc0xNqXTC7+zQjxr0GlKA==", "dependencies": { "@hapi/hoek": "^9.0.0", "@hapi/topo": "^5.0.0", "@sideway/address": "^4.1.3", - "@sideway/formula": "^3.0.0", + "@sideway/formula": "^3.0.1", "@sideway/pinpoint": "^2.0.0" } }, @@ -16669,50 +16593,12 @@ "@babel/types": "^7.3.0" } }, - "@types/body-parser": { - "version": "1.19.2", - "resolved": "https://registry.npmjs.org/@types/body-parser/-/body-parser-1.19.2.tgz", - "integrity": "sha512-ALYone6pm6QmwZoAgeyNksccT9Q4AWZQ6PvfwR37GT6r6FWUPguq6sUmNGSMV2Wr761oQoBxwGGa6DR5o1DC9g==", - "requires": { - "@types/connect": "*", - "@types/node": "*" - } - }, "@types/color-name": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/@types/color-name/-/color-name-1.1.1.tgz", "integrity": "sha512-rr+OQyAjxze7GgWrSaJwydHStIhHq2lvY3BOC2Mj7KnzI7XK0Uw1TOOdI9lDoajEbSWLiYgoo4f1R51erQfhPQ==", "dev": true }, - "@types/connect": { - "version": "3.4.35", - "resolved": "https://registry.npmjs.org/@types/connect/-/connect-3.4.35.tgz", - "integrity": "sha512-cdeYyv4KWoEgpBISTxWvqYsVy444DOqehiF3fM3ne10AmJ62RSyNkUnxMJXHQWRQQX2eR94m5y1IZyDwBjV9FQ==", - "requires": { - "@types/node": "*" - } - }, - "@types/express": { - "version": "4.17.13", - "resolved": "https://registry.npmjs.org/@types/express/-/express-4.17.13.tgz", - "integrity": "sha512-6bSZTPaTIACxn48l50SR+axgrqm6qXFIxrdAKaG6PaJk3+zuUr35hBlgT7vOmJcum+OEaIBLtHV/qloEAFITeA==", - "requires": { - "@types/body-parser": "*", - "@types/express-serve-static-core": "^4.17.18", - "@types/qs": "*", - "@types/serve-static": "*" - } - }, - "@types/express-serve-static-core": { - "version": "4.17.28", - "resolved": "https://registry.npmjs.org/@types/express-serve-static-core/-/express-serve-static-core-4.17.28.tgz", - "integrity": "sha512-P1BJAEAW3E2DJUlkgq4tOL3RyMunoWXqbSCygWo5ZIWTjUgN1YnaXWW4VWl/oc8vs/XoYibEGBKP0uZyF4AHig==", - "requires": { - "@types/node": "*", - "@types/qs": "*", - "@types/range-parser": "*" - } - }, "@types/graceful-fs": { "version": "4.1.5", "resolved": "https://registry.npmjs.org/@types/graceful-fs/-/graceful-fs-4.1.5.tgz", @@ -16722,11 +16608,6 @@ "@types/node": "*" } }, - "@types/hapi__joi": { - "version": "16.0.12", - "resolved": "https://registry.npmjs.org/@types/hapi__joi/-/hapi__joi-16.0.12.tgz", - "integrity": "sha512-xJYifuz59jXdWY5JMS15uvA3ycS3nQYOGqoIIE0+fwQ0qI3/4CxBc6RHsOTp6wk9M0NWEdpcTl02lOQOKMifbQ==" - }, "@types/istanbul-lib-coverage": { "version": "2.0.4", "resolved": "https://registry.npmjs.org/@types/istanbul-lib-coverage/-/istanbul-lib-coverage-2.0.4.tgz", @@ -16767,15 +16648,11 @@ "integrity": "sha1-7ihweulOEdK4J7y+UnC86n8+ce4=", "dev": true }, - "@types/mime": { - "version": "1.3.2", - "resolved": "https://registry.npmjs.org/@types/mime/-/mime-1.3.2.tgz", - "integrity": "sha512-YATxVxgRqNH6nHEIsvg6k2Boc1JHI9ZbH5iWFFv/MTkchz3b1ieGDa5T0a9RznNdI0KhVbdbWSN+KWWrQZRxTw==" - }, "@types/node": { "version": "14.0.23", "resolved": "https://registry.npmjs.org/@types/node/-/node-14.0.23.tgz", - "integrity": "sha512-Z4U8yDAl5TFkmYsZdFPdjeMa57NOvnaf1tljHzhouaPEp7LCj2JKkejpI1ODviIAQuW4CcQmxkQ77rnLsOOoKw==" + "integrity": "sha512-Z4U8yDAl5TFkmYsZdFPdjeMa57NOvnaf1tljHzhouaPEp7LCj2JKkejpI1ODviIAQuW4CcQmxkQ77rnLsOOoKw==", + "dev": true }, "@types/prettier": { "version": "2.7.1", @@ -16783,25 +16660,6 @@ "integrity": "sha512-ri0UmynRRvZiiUJdiz38MmIblKK+oH30MztdBVR95dv/Ubw6neWSb8u1XpRb72L4qsZOhz+L+z9JD40SJmfWow==", "dev": true }, - "@types/qs": { - "version": "6.9.7", - "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.9.7.tgz", - "integrity": "sha512-FGa1F62FT09qcrueBA6qYTrJPVDzah9a+493+o2PCXsesWHIn27G98TsSMs3WPNbZIEj4+VJf6saSFpvD+3Zsw==" - }, - "@types/range-parser": { - "version": "1.2.4", - "resolved": "https://registry.npmjs.org/@types/range-parser/-/range-parser-1.2.4.tgz", - "integrity": "sha512-EEhsLsD6UsDM1yFhAvy0Cjr6VwmpMWqFBCb9w07wVugF7w9nfajxLuVmngTIpgS6svCnm6Vaw+MZhoDCKnOfsw==" - }, - "@types/serve-static": { - "version": "1.13.10", - "resolved": "https://registry.npmjs.org/@types/serve-static/-/serve-static-1.13.10.tgz", - "integrity": "sha512-nCkHGI4w7ZgAdNkrEu0bv+4xNV/XDqW+DydknebMOQwkpDGx8G+HTlj7R7ABI8i8nKxVw0wtKPi1D+lPOkh4YQ==", - "requires": { - "@types/mime": "^1", - "@types/node": "*" - } - }, "@types/sinon": { "version": "10.0.10", "resolved": "https://registry.npmjs.org/@types/sinon/-/sinon-10.0.10.tgz", @@ -19474,16 +19332,6 @@ "basic-auth": "^2.0.1" } }, - "express-validation": { - "version": "4.1.0", - "resolved": "https://registry.npmjs.org/express-validation/-/express-validation-4.1.0.tgz", - "integrity": "sha512-bAGRVdaJZ7yzCxELX29zvvJk0OUP/tw6tCtZtGnpK1O+yLY9qN8btwtclRQMGfIluZYIuxe++U/yPMlgEpU12A==", - "requires": { - "@types/express": "^4.17.13", - "@types/hapi__joi": "16.x.x", - "joi": "^17.6.0" - } - }, "express-winston": { "version": "4.2.0", "resolved": "https://registry.npmjs.org/express-winston/-/express-winston-4.2.0.tgz", @@ -21828,14 +21676,14 @@ } }, "joi": { - "version": "17.6.0", - "resolved": "https://registry.npmjs.org/joi/-/joi-17.6.0.tgz", - "integrity": "sha512-OX5dG6DTbcr/kbMFj0KGYxuew69HPcAE3K/sZpEV2nP6e/j/C0HV+HNiBPCASxdx5T7DMoa0s8UeHWMnb6n2zw==", + "version": "17.10.2", + "resolved": "https://registry.npmjs.org/joi/-/joi-17.10.2.tgz", + "integrity": "sha512-hcVhjBxRNW/is3nNLdGLIjkgXetkeGc2wyhydhz8KumG23Aerk4HPjU5zaPAMRqXQFc0xNqXTC7+zQjxr0GlKA==", "requires": { "@hapi/hoek": "^9.0.0", "@hapi/topo": "^5.0.0", "@sideway/address": "^4.1.3", - "@sideway/formula": "^3.0.0", + "@sideway/formula": "^3.0.1", "@sideway/pinpoint": "^2.0.0" } }, diff --git a/app/package.json b/app/package.json index d05f88ea..d8f6b4fc 100644 --- a/app/package.json +++ b/app/package.json @@ -40,8 +40,8 @@ "date-fns": "^2.30.0", "express": "^4.18.2", "express-basic-auth": "^1.2.1", - "express-validation": "^4.1.0", "express-winston": "^4.2.0", + "joi": "^17.10.2", "js-yaml": "^4.1.0", "jsonwebtoken": "^9.0.1", "knex": "^2.5.1", diff --git a/app/src/components/errorToProblem.js b/app/src/components/errorToProblem.js index f9a60402..8aed7c92 100644 --- a/app/src/components/errorToProblem.js +++ b/app/src/components/errorToProblem.js @@ -40,8 +40,10 @@ function errorToProblem(service, e) { if (e.$response && e.$response.body) delete e.$response.body; return new Problem(e.$metadata.httpStatusCode, { detail: e }); } else { - log.error(`Unknown error calling ${service}: ${e.message}`, { function: 'errorToProblem', status: 502 }); - return new Problem(502, `Unknown ${service} Error`, { detail: e.message }); + // Handle all other errors + const message = `${service} Error: ${e.message}`; + log.error(message, { error: e, function: 'errorToProblem', status: 500 }); + return new Problem(500, { detail: message }); } } diff --git a/app/src/components/utils.js b/app/src/components/utils.js index 49350bce..0c033b12 100644 --- a/app/src/components/utils.js +++ b/app/src/components/utils.js @@ -97,7 +97,7 @@ const utils = { return data; } catch (err) { log.error(err.message, { function: 'getBucket' }); - throw new Problem(404, { details: err.message }); + throw new Problem(404, { detail: err.message }); } }, diff --git a/app/src/controllers/bucket.js b/app/src/controllers/bucket.js index 1458a7e2..1cc31cea 100644 --- a/app/src/controllers/bucket.js +++ b/app/src/controllers/bucket.js @@ -90,7 +90,7 @@ const controller = { function: '_validateCredentials', }); throw new Problem(409, { - details: 'Unable to validate supplied credentials for the bucket', + detail: 'Unable to validate supplied credentials for the bucket', }); } }, @@ -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, { details: permErr.message }).send(res); + next(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/docs/v1.api-spec.yaml b/app/src/docs/v1.api-spec.yaml index b184c40d..3df84916 100644 --- a/app/src/docs/v1.api-spec.yaml +++ b/app/src/docs/v1.api-spec.yaml @@ -2440,26 +2440,20 @@ components: description: What type of problem, link to explanation of problem title: type: string - description: Title of problem, generally the Http Status Code description + description: Title of problem, generally the HTTP Status Code description status: type: string - description: The Http Status code + description: The HTTP Status code detail: type: string - description: Short description of why this problem was raised. - errors: - type: array - items: - type: object - required: - - message - properties: - value: - type: object - description: Contents of the field that was in error. - message: - type: string - description: The error message for the field. + description: >- + A short, human-readable explanation specific to this occurrence of + the problem + instance: + type: string + description: >- + A URI reference that identifies the specific occurrence of the + problem Response-Unauthorized: title: Response Unauthorized type: object diff --git a/app/src/middleware/authentication.js b/app/src/middleware/authentication.js index 2dcd2861..06a62c92 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); + return next(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..9f8aa7d7 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); + return next(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..09520a7d 100644 --- a/app/src/middleware/featureToggle.js +++ b/app/src/middleware/featureToggle.js @@ -7,22 +7,29 @@ const { getAppAuthMode } = require('../components/utils'); * @function requireBasicAuth * Only allows basic authentication requests if application is in the appropriate 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 requireBasicAuth = (req, res, next) => { +const requireBasicAuth = (req, _res, next) => { const authMode = getAppAuthMode(); const authType = req.currentUser ? req.currentUser.authType : undefined; 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(); @@ -32,16 +39,20 @@ const requireBasicAuth = (req, res, next) => { * @function requireSomeAuth * Rejects the request if there is no authorization in the appropriate 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 requireSomeAuth = (req, res, next) => { +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 new file mode 100644 index 00000000..df8f8449 --- /dev/null +++ b/app/src/middleware/validation.js @@ -0,0 +1,34 @@ +const Problem = require('api-problem'); + +/** + * @function validator + * 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) => { + const validationErrors = Object.entries(schema) + .map(([prop, def]) => { + const result = def.validate(req[prop], { abortEarly: false })?.error; + return result ? [prop, result?.details] : undefined; + }) + .filter(error => !!error); + + if (Object.keys(validationErrors).length) { + throw new Problem(422, { + detail: validationErrors + .flatMap(groups => groups[1]?.map(error => error?.message)) + .join('; '), + instance: req.originalUrl, + errors: Object.fromEntries(validationErrors) + }); + } + else next(); + }; +}; + +module.exports = { + validate +}; diff --git a/app/src/routes/v1/object.js b/app/src/routes/v1/object.js index a1943db6..1145200d 100644 --- a/app/src/routes/v1/object.js +++ b/app/src/routes/v1/object.js @@ -10,22 +10,22 @@ const { currentUpload } = require('../../middleware/upload'); router.use(checkAppMode); /** Creates new objects */ -router.put('/', objectValidator.createObject, requireSomeAuth, currentUpload(true), (req, res, next) => { +router.put('/', requireSomeAuth, objectValidator.createObject, currentUpload(true), (req, res, next) => { objectController.createObject(req, res, next); }); /** Search for objects */ -router.get('/', objectValidator.searchObjects, requireSomeAuth, (req, res, next) => { +router.get('/', requireSomeAuth, objectValidator.searchObjects, (req, res, next) => { objectController.searchObjects(req, res, next); }); /** Fetch metadata for specific objects */ -router.get('/metadata', objectValidator.fetchMetadata, requireSomeAuth, (req, res, next) => { +router.get('/metadata', requireSomeAuth, objectValidator.fetchMetadata, (req, res, next) => { objectController.fetchMetadata(req, res, next); }); /** Fetch tags for specific objects */ -router.get('/tagging', objectValidator.fetchTags, requireSomeAuth, (req, res, next) => { +router.get('/tagging', requireSomeAuth, objectValidator.fetchTags, (req, res, next) => { objectController.fetchTags(req, res, next); }); @@ -41,57 +41,57 @@ router.get('/:objectId', objectValidator.readObject, currentObject, hasPermissio }); /** Updates an object */ -router.put('/:objectId', objectValidator.updateObject, requireSomeAuth, currentUpload(), currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { +router.put('/:objectId', requireSomeAuth, objectValidator.updateObject, currentUpload(), currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { objectController.updateObject(req, res, next); }); /** Deletes the object */ -router.delete('/:objectId', objectValidator.deleteObject, requireSomeAuth, currentObject, hasPermission(Permissions.DELETE), (req, res, next) => { +router.delete('/:objectId', requireSomeAuth, objectValidator.deleteObject, currentObject, hasPermission(Permissions.DELETE), (req, res, next) => { objectController.deleteObject(req, res, next); }); /** Returns the object version history */ -router.get('/:objectId/version', objectValidator.listObjectVersion, requireSomeAuth, currentObject, hasPermission(Permissions.READ), (req, res, next) => { +router.get('/:objectId/version', requireSomeAuth, objectValidator.listObjectVersion, currentObject, hasPermission(Permissions.READ), (req, res, next) => { objectController.listObjectVersion(req, res, next); }); /** Sets the public flag of an object */ -router.patch('/:objectId/public', objectValidator.togglePublic, requireSomeAuth, currentObject, hasPermission(Permissions.MANAGE), (req, res, next) => { +router.patch('/:objectId/public', requireSomeAuth, objectValidator.togglePublic, currentObject, hasPermission(Permissions.MANAGE), (req, res, next) => { objectController.togglePublic(req, res, next); }); /** Add metadata to an object */ -router.patch('/:objectId/metadata', objectValidator.addMetadata, requireSomeAuth, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { +router.patch('/:objectId/metadata', requireSomeAuth, objectValidator.addMetadata, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { objectController.addMetadata(req, res, next); }); /** Replace metadata on an object */ -router.put('/:objectId/metadata', objectValidator.replaceMetadata, requireSomeAuth, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { +router.put('/:objectId/metadata', requireSomeAuth, objectValidator.replaceMetadata, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { objectController.replaceMetadata(req, res, next); }); /** Deletes an objects metadata */ -router.delete('/:objectId/metadata', objectValidator.deleteMetadata, requireSomeAuth, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { +router.delete('/:objectId/metadata', requireSomeAuth, objectValidator.deleteMetadata, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { objectController.deleteMetadata(req, res, next); }); /** Synchronizes an object */ -router.get('/:objectId/sync', objectValidator.syncObject, requireSomeAuth, currentObject, hasPermission(Permissions.READ), (req, res, next) => { +router.get('/:objectId/sync', requireSomeAuth, objectValidator.syncObject, currentObject, hasPermission(Permissions.READ), (req, res, next) => { syncController.syncObject(req, res, next); }); /** Add tags to an object */ -router.patch('/:objectId/tagging', objectValidator.addTags, requireSomeAuth, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { +router.patch('/:objectId/tagging', requireSomeAuth, objectValidator.addTags, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { objectController.addTags(req, res, next); }); /** Add tags to an object */ -router.put('/:objectId/tagging', objectValidator.replaceTags, requireSomeAuth, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { +router.put('/:objectId/tagging', requireSomeAuth, objectValidator.replaceTags, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { objectController.replaceTags(req, res, next); }); /** Add tags to an object */ -router.delete('/:objectId/tagging', objectValidator.deleteTags, requireSomeAuth, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { +router.delete('/:objectId/tagging', requireSomeAuth, objectValidator.deleteTags, currentObject, hasPermission(Permissions.UPDATE), (req, res, next) => { objectController.deleteTags(req, res, next); }); diff --git a/app/src/services/storage.js b/app/src/services/storage.js index cb636ea3..aa2c37b0 100644 --- a/app/src/services/storage.js +++ b/app/src/services/storage.js @@ -509,8 +509,7 @@ const objectStorageService = { // TODO: Consider adding API param support for Server Side Encryption // ServerSideEncryption: 'AES256' }, - partSize: utils.calculatePartSize(length), - queueSize: 1 + partSize: utils.calculatePartSize(length) }); upload.on('httpUploadProgress', progress => { diff --git a/app/src/validators/bucket.js b/app/src/validators/bucket.js index 75f11685..212d3c02 100644 --- a/app/src/validators/bucket.js +++ b/app/src/validators/bucket.js @@ -1,6 +1,7 @@ -const { validate, Joi } = require('express-validation'); +const Joi = require('joi'); const { scheme, type } = require('./common'); +const { validate } = require('../middleware/validation'); const schema = { createBucket: { @@ -66,13 +67,13 @@ const schema = { }; const validator = { - createBucket: validate(schema.createBucket, { statusCode: 422 }), - deleteBucket: validate(schema.deleteBucket, { statusCode: 422 }), - headBucket: validate(schema.headBucket, { statusCode: 422 }), - readBucket: validate(schema.readBucket, { statusCode: 422 }), - syncBucket: validate(schema.readBucket, { statusCode: 422 }), - searchBuckets: validate(schema.searchBuckets, { statusCode: 422 }), - updateBucket: validate(schema.updateBucket, { statusCode: 422 }) + createBucket: validate(schema.createBucket), + deleteBucket: validate(schema.deleteBucket), + headBucket: validate(schema.headBucket), + readBucket: validate(schema.readBucket), + syncBucket: validate(schema.readBucket), + searchBuckets: validate(schema.searchBuckets), + updateBucket: validate(schema.updateBucket) }; module.exports = validator; diff --git a/app/src/validators/bucketPermission.js b/app/src/validators/bucketPermission.js index cd0aa0b2..60958a70 100644 --- a/app/src/validators/bucketPermission.js +++ b/app/src/validators/bucketPermission.js @@ -1,7 +1,9 @@ -const { validate, Joi } = require('express-validation'); +const Joi = require('joi'); + const { scheme, type } = require('./common'); const { Permissions } = require('../components/constants'); +const { validate } = require('../middleware/validation'); const schema = { searchPermissions: { @@ -56,10 +58,10 @@ const schema = { }; const validator = { - searchPermissions: validate(schema.searchPermissions, { statusCode: 422 }), - listPermissions: validate(schema.listPermissions, { statusCode: 422 }), - addPermissions: validate(schema.addPermissions, { statusCode: 422 }), - removePermissions: validate(schema.removePermissions, { statusCode: 422 }), + searchPermissions: validate(schema.searchPermissions), + listPermissions: validate(schema.listPermissions), + addPermissions: validate(schema.addPermissions), + removePermissions: validate(schema.removePermissions) }; module.exports = validator; diff --git a/app/src/validators/common.js b/app/src/validators/common.js index c79f9f60..a186677a 100644 --- a/app/src/validators/common.js +++ b/app/src/validators/common.js @@ -1,4 +1,4 @@ -const { Joi: baseJoi } = require('express-validation'); +const baseJoi = require('joi'); const { EMAILREGEX, Permissions } = require('../components/constants'); @@ -6,15 +6,13 @@ const { EMAILREGEX, Permissions } = require('../components/constants'); * @constant Joi * Extend Base Joi with a custom 'csvArray' parser */ -const Joi = baseJoi.extend((joi) => { - return { - type: 'csvArray', - base: joi.array(), - coerce: (value) => ({ - value: value.split ? value.split(',').map(item => item.trim()) : value, - }) - }; -}); +const Joi = baseJoi.extend(joi => ({ + type: 'csvArray', + base: joi.array(), + coerce: (value) => ({ + value: value.split ? value.split(',').map(item => item.trim()) : value, + }) +})); /** * @function oneOrMany diff --git a/app/src/validators/metadata.js b/app/src/validators/metadata.js index a51b992d..4c318253 100644 --- a/app/src/validators/metadata.js +++ b/app/src/validators/metadata.js @@ -1,6 +1,5 @@ -const { validate } = require('express-validation'); - const { type } = require('./common'); +const { validate } = require('../middleware/validation'); const schema = { searchMetadata: { @@ -9,7 +8,7 @@ const schema = { }; const validator = { - searchMetadata: validate(schema.searchMetadata, { statusCode: 422 }) + searchMetadata: validate(schema.searchMetadata) }; module.exports = validator; diff --git a/app/src/validators/object.js b/app/src/validators/object.js index c20448e0..bfd66caa 100644 --- a/app/src/validators/object.js +++ b/app/src/validators/object.js @@ -1,7 +1,8 @@ -const { validate, Joi } = require('express-validation'); +const Joi = require('joi'); const { scheme, type } = require('./common'); const { DownloadMode } = require('../components/constants'); +const { validate } = require('../middleware/validation'); const schema = { addMetadata: { @@ -26,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({ @@ -177,23 +176,23 @@ const schema = { }; const validator = { - addMetadata: validate(schema.addMetadata, { statusCode: 422 }), - addTags: validate(schema.addTags, { statusCode: 422 }), - createObject: validate(schema.createObject, { statusCode: 422 }), - deleteMetadata: validate(schema.deleteMetadata, { statusCode: 422 }), - deleteObject: validate(schema.deleteObject, { statusCode: 422 }), - deleteTags: validate(schema.deleteTags, { statusCode: 422 }), - fetchMetadata: validate(schema.fetchMetadata, { statusCode: 422 }), - fetchTags: validate(schema.fetchTags, { statusCode: 422 }), - headObject: validate(schema.headObject, { statusCode: 422 }), - listObjectVersion: validate(schema.listObjectVersion, { statusCode: 422 }), - readObject: validate(schema.readObject, { statusCode: 422 }), - replaceMetadata: validate(schema.replaceMetadata, { statusCode: 422 }), - replaceTags: validate(schema.replaceTags, { statusCode: 422 }), - searchObjects: validate(schema.searchObjects, { statusCode: 422 }), - syncObject: validate(schema.syncObject, { statusCode: 422 }), - togglePublic: validate(schema.togglePublic, { statusCode: 422 }), - updateObject: validate(schema.updateObject, { statusCode: 422 }) + addMetadata: validate(schema.addMetadata), + addTags: validate(schema.addTags), + createObject: validate(schema.createObject), + deleteMetadata: validate(schema.deleteMetadata), + deleteObject: validate(schema.deleteObject), + deleteTags: validate(schema.deleteTags), + fetchMetadata: validate(schema.fetchMetadata), + fetchTags: validate(schema.fetchTags), + headObject: validate(schema.headObject), + listObjectVersion: validate(schema.listObjectVersion), + readObject: validate(schema.readObject), + replaceMetadata: validate(schema.replaceMetadata), + replaceTags: validate(schema.replaceTags), + searchObjects: validate(schema.searchObjects), + syncObject: validate(schema.syncObject), + togglePublic: validate(schema.togglePublic), + updateObject: validate(schema.updateObject) }; module.exports = validator; diff --git a/app/src/validators/objectPermission.js b/app/src/validators/objectPermission.js index a075427c..3ce99b05 100644 --- a/app/src/validators/objectPermission.js +++ b/app/src/validators/objectPermission.js @@ -1,6 +1,8 @@ -const { validate, Joi } = require('express-validation'); +const Joi = require('joi'); + const { scheme, type } = require('./common'); const { Permissions } = require('../components/constants'); +const { validate } = require('../middleware/validation'); const schema = { searchPermissions: { @@ -56,10 +58,10 @@ const schema = { }; const validator = { - searchPermissions: validate(schema.searchPermissions, { statusCode: 422 }), - listPermissions: validate(schema.listPermissions, { statusCode: 422 }), - addPermissions: validate(schema.addPermissions, { statusCode: 422 }), - removePermissions: validate(schema.removePermissions, { statusCode: 422 }), + searchPermissions: validate(schema.searchPermissions), + listPermissions: validate(schema.listPermissions), + addPermissions: validate(schema.addPermissions), + removePermissions: validate(schema.removePermissions) }; module.exports = validator; diff --git a/app/src/validators/tag.js b/app/src/validators/tag.js index 04e57549..d0186045 100644 --- a/app/src/validators/tag.js +++ b/app/src/validators/tag.js @@ -1,6 +1,8 @@ -const { validate, Joi } = require('express-validation'); +const Joi = require('joi'); const { type } = require('./common'); +const { validate } = require('../middleware/validation'); + const schema = { searchTags: { @@ -11,7 +13,7 @@ const schema = { }; const validator = { - searchTags: validate(schema.searchTags, { statusCode: 422 }) + searchTags: validate(schema.searchTags) }; module.exports = validator; diff --git a/app/src/validators/user.js b/app/src/validators/user.js index eb22f9e1..61143918 100644 --- a/app/src/validators/user.js +++ b/app/src/validators/user.js @@ -1,6 +1,7 @@ -const { validate, Joi } = require('express-validation'); -const { scheme, type } = require('./common'); +const Joi = require('joi'); +const { scheme, type } = require('./common'); +const { validate } = require('../middleware/validation'); const schema = { searchUsers: { @@ -26,8 +27,8 @@ const schema = { }; const validator = { - searchUsers: validate(schema.searchUsers, { statusCode: 422 }), - listIdps: validate(schema.listIdps, { statusCode: 422 }) + searchUsers: validate(schema.searchUsers), + listIdps: validate(schema.listIdps) }; module.exports = validator; diff --git a/app/src/validators/version.js b/app/src/validators/version.js index fe1a7b93..73263907 100644 --- a/app/src/validators/version.js +++ b/app/src/validators/version.js @@ -1,6 +1,7 @@ -const { validate, Joi } = require('express-validation'); +const Joi = require('joi'); const { scheme, type } = require('./common'); +const { validate } = require('../middleware/validation'); const schema = { @@ -22,8 +23,8 @@ const schema = { }; const validator = { - fetchMetadata: validate(schema.fetchMetadata, { statusCode: 422 }), - fetchTags: validate(schema.fetchTags, { statusCode: 422 }), + fetchMetadata: validate(schema.fetchMetadata), + fetchTags: validate(schema.fetchTags) }; module.exports = validator; diff --git a/app/tests/common/helper.js b/app/tests/common/helper.js index 8ef17d69..89ec5dac 100644 --- a/app/tests/common/helper.js +++ b/app/tests/common/helper.js @@ -1,6 +1,5 @@ const express = require('express'); const Problem = require('api-problem'); -const { ValidationError } = require('express-validation'); /** * @class helper @@ -23,25 +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 if (err instanceof ValidationError) { - return res.status(err.statusCode).json(err); } else { - new Problem(500, { - details: (err.message) ? 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/components/errorToProblem.spec.js b/app/tests/unit/components/errorToProblem.spec.js index ad418e9b..c0a5daff 100644 --- a/app/tests/unit/components/errorToProblem.spec.js +++ b/app/tests/unit/components/errorToProblem.spec.js @@ -101,7 +101,7 @@ describe('errorToProblem', () => { expect(result.errors).toBeUndefined(); }); - it('should throw a 502 problem', () => { + it('should throw a 500 problem', () => { const e = { message: 'msg' }; @@ -109,8 +109,8 @@ describe('errorToProblem', () => { expect(result).toBeTruthy(); expect(result instanceof Problem).toBeTruthy(); - expect(result.title).toMatch(`Unknown ${SERVICE} Error`); - expect(result.status).toBe(502); + expect(result.title).toMatch('Internal Server Error'); + expect(result.status).toBe(500); expect(result.detail).toMatch(e.message); }); }); diff --git a/app/tests/unit/controllers/bucket.spec.js b/app/tests/unit/controllers/bucket.spec.js index d9257a80..f33b6b06 100644 --- a/app/tests/unit/controllers/bucket.spec.js +++ b/app/tests/unit/controllers/bucket.spec.js @@ -171,9 +171,7 @@ describe('createBucket', () => { expect(createSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID }); expect(next).toHaveBeenCalledTimes(1); - expect(next).toHaveBeenCalledWith( - new Problem(502, 'Unknown BucketService Error') - ); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); // Skipping until someone can figure out the instanceof issue in the catch block @@ -203,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, @@ -214,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/controllers/object.spec.js b/app/tests/unit/controllers/object.spec.js index aa75dda5..a61dbf1b 100644 --- a/app/tests/unit/controllers/object.spec.js +++ b/app/tests/unit/controllers/object.spec.js @@ -55,7 +55,7 @@ describe('addMetadata', () => { storageHeadObjectSpy.mockReturnValue(BadResponse); await controller.addMetadata(req, res, next); - expect(next).toHaveBeenCalledWith(new Problem(502, 'Unknown ObjectService Error')); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); it('should add the metadata', async () => { @@ -228,7 +228,7 @@ describe('deleteMetadata', () => { storageHeadObjectSpy.mockReturnValue(BadResponse); await controller.deleteMetadata(req, res, next); - expect(next).toHaveBeenCalledWith(new Problem(502, 'Unknown ObjectService Error')); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); it('should delete the requested metadata', async () => { @@ -400,7 +400,7 @@ describe('deleteObject', () => { await controller.deleteObject(req, res, next); expect(storageDeleteObjectSpy).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledTimes(1); - expect(next).toHaveBeenCalledWith(new Problem(502, 'Unknown ObjectService Error')); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); }); @@ -518,7 +518,7 @@ describe('replaceMetadata', () => { storageHeadObjectSpy.mockReturnValue(BadResponse); await controller.replaceMetadata(req, res, next); - expect(next).toHaveBeenCalledWith(new Problem(502, 'Unknown ObjectService Error')); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); it('should replace the metadata', async () => { diff --git a/app/tests/unit/controllers/objectPermission.spec.js b/app/tests/unit/controllers/objectPermission.spec.js index 25528b64..40001770 100644 --- a/app/tests/unit/controllers/objectPermission.spec.js +++ b/app/tests/unit/controllers/objectPermission.spec.js @@ -47,7 +47,7 @@ describe('searchPermissions', () => { await controller.searchPermissions(req, res, next); expect(searchPermissionsSpy).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledTimes(1); - expect(next).toHaveBeenCalledWith(new Problem(502, 'Unknown ObjectPermissionService Error')); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); }); @@ -83,7 +83,7 @@ describe('listPermissions', () => { await controller.listPermissions(req, res, next); expect(searchPermissionsSpy).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledTimes(1); - expect(next).toHaveBeenCalledWith(new Problem(502, 'Unknown ObjectPermissionService Error')); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); }); @@ -121,7 +121,7 @@ describe('addPermissions', () => { await controller.addPermissions(req, res, next); expect(addPermissionsSpy).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledTimes(1); - expect(next).toHaveBeenCalledWith(new Problem(502, 'Unknown ObjectPermissionService Error')); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); }); @@ -156,6 +156,6 @@ describe('removePermissions', () => { await controller.removePermissions(req, res, next); expect(removePermissionsSpy).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledTimes(1); - expect(next).toHaveBeenCalledWith(new Problem(502, 'Unknown ObjectPermissionService Error')); + expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error')); }); }); diff --git a/app/tests/unit/middleware/authentication.spec.js b/app/tests/unit/middleware/authentication.spec.js index 87997193..75447a7d 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 () => { const authorization = 'bearer '; - problemSendSpy.mockImplementation(() => { }); config.has .mockReturnValueOnce(false) // basicAuth.enabled .mockReturnValueOnce(true) // keycloak.enabled @@ -224,9 +219,8 @@ describe('currentUser', () => { issuer: `${serverUrl}/realms/${realm}` })); expect(loginSpy).toHaveBeenCalledTimes(0); - expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(1); - expect(problemSendSpy).toHaveBeenCalledWith(res); + expect(next).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledWith(expect.any(Object)); }); it('short circuits without keycloak.publicKey', async () => { @@ -250,9 +244,8 @@ describe('currentUser', () => { expect(checkBasicAuthSpy).toHaveBeenCalledTimes(0); expect(jwtVerifySpy).toHaveBeenCalledTimes(0); expect(loginSpy).toHaveBeenCalledTimes(0); - expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(1); - expect(problemSendSpy).toHaveBeenCalledWith(res); + expect(next).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledWith(expect.any(Object)); }); }); }); diff --git a/app/tests/unit/middleware/authorization.spec.js b/app/tests/unit/middleware/authorization.spec.js index 8853f887..5ce5aafc 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(); @@ -258,18 +247,16 @@ describe('hasPermission', () => { [1, AuthMode.BASICAUTH], [0, AuthMode.OIDCAUTH], [0, AuthMode.FULLAUTH] - ])('should call next %i times given authMode %s', (nextCount, mode) => { - const sendCount = 1 - nextCount; + ])('should call next %i times given authMode %s', async (nextCount, mode) => { getAppAuthModeSpy.mockReturnValue(mode); const result = mw.hasPermission(Permissions.READ); expect(result).toBeInstanceOf(Function); - result(req, res, next); + await result(req, res, next); - expect(next).toHaveBeenCalledTimes(nextCount); + expect(next).toHaveBeenCalledTimes(1); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); + else expect(next).toHaveBeenCalledWith(expect.any(Object)); expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(0); }); }); @@ -286,7 +273,6 @@ describe('hasPermission', () => { [0, AuthMode.OIDCAUTH], [0, AuthMode.FULLAUTH] ])('should call next %i times given authMode %s', async (nextCount, mode) => { - const sendCount = 1 - nextCount; getAppAuthModeSpy.mockReturnValue(mode); getCurrentUserIdSpy.mockResolvedValue(SYSTEM_USER); getCurrentIdentitySpy.mockReturnValue(SYSTEM_USER); @@ -295,10 +281,9 @@ describe('hasPermission', () => { expect(result).toBeInstanceOf(Function); await result(req, res, next); - expect(next).toHaveBeenCalledTimes(nextCount); + expect(next).toHaveBeenCalledTimes(1); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); + else expect(next).toHaveBeenCalledWith(expect.any(Object)); expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(0); }); }); @@ -323,9 +308,8 @@ describe('hasPermission', () => { expect(result).toBeInstanceOf(Function); await result(req, res, next); - expect(next).toHaveBeenCalledTimes(0); - expect(problemSendSpy).toHaveBeenCalledTimes(1); - expect(problemSendSpy).toHaveBeenCalledWith(res); + expect(next).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledWith(expect.any(Object)); }); }); @@ -342,7 +326,6 @@ describe('hasPermission', () => { [0, AuthMode.OIDCAUTH], [1, AuthMode.FULLAUTH] ])('should call next %i times when authType BASIC and authMode %s', async (nextCount, mode) => { - const sendCount = 1 - nextCount; getAppAuthModeSpy.mockReturnValue(mode); checkPermissionSpy.mockResolvedValue(false); req.currentUser.authType = AuthType.BASIC; @@ -351,10 +334,9 @@ describe('hasPermission', () => { expect(result).toBeInstanceOf(Function); await result(req, res, next); - expect(next).toHaveBeenCalledTimes(nextCount); + expect(next).toHaveBeenCalledTimes(1); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); + else expect(next).toHaveBeenCalledWith(expect.any(Object)); expect(checkPermissionSpy).toHaveBeenCalledTimes(0); }); @@ -370,7 +352,6 @@ describe('hasPermission', () => { [0, true, Permissions.DELETE], [0, true, Permissions.MANAGE] ])('should call next %i times when public %s and permission %s', async (nextCount, isPublic, perm) => { - const sendCount = 1 - nextCount; getAppAuthModeSpy.mockReturnValue(AuthMode.OIDCAUTH); getCurrentUserIdSpy.mockResolvedValue(SYSTEM_USER); req.currentUser.authType = AuthType.OIDC; @@ -380,10 +361,9 @@ describe('hasPermission', () => { expect(result).toBeInstanceOf(Function); await result(req, res, next); - expect(next).toHaveBeenCalledTimes(nextCount); + expect(next).toHaveBeenCalledTimes(1); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); + else expect(next).toHaveBeenCalledWith(expect.any(Object)); expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(0); }); }); @@ -405,7 +385,6 @@ describe('hasPermission', () => { [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) => { - const sendCount = 1 - nextCount; const searchPermCount = +(type === AuthType.BEARER && !!userId); getAppAuthModeSpy.mockReturnValue(AuthMode.OIDCAUTH); getCurrentUserIdSpy.mockResolvedValue(userId); @@ -418,10 +397,10 @@ describe('hasPermission', () => { expect(result).toBeInstanceOf(Function); await result(req, res, next); - expect(next).toHaveBeenCalledTimes(nextCount); + expect(next).toHaveBeenCalledTimes(1); if (nextCount) expect(next).toHaveBeenCalledWith(); - expect(problemSendSpy).toHaveBeenCalledTimes(sendCount); - if (sendCount) expect(problemSendSpy).toHaveBeenCalledWith(res); + else expect(next).toHaveBeenCalledWith(expect.any(Object)); + expect(objSearchPermissionsSpy).toHaveBeenCalledTimes(searchPermCount); if (searchPermCount) { expect(objSearchPermissionsSpy).toHaveBeenCalledWith(expect.objectContaining({ objId: SYSTEM_USER })); 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 new file mode 100644 index 00000000..df1fc994 --- /dev/null +++ b/app/tests/unit/middleware/validation.spec.js @@ -0,0 +1,64 @@ +const { validate } = require('../../../src/middleware/validation'); + +beforeEach(() => { + jest.resetAllMocks(); +}); + +afterAll(() => { + jest.restoreAllMocks(); +}); + +describe('validate', () => { + let req, res, next; + + beforeEach(() => { + req = { + originalUrl: 'originalUrl', + params: { id: 'id' }, + query: { foo: 'bar', bool: false } + }; + res = {}; + next = jest.fn(); + }); + + it('should call next when no validation errors', () => { + const errors = undefined; + const schema = { query: { validate: jest.fn().mockReturnValue(errors) } }; + + const result = validate(schema); + expect(result).toBeInstanceOf(Function); + expect(() => result(req, res, next)).not.toThrow(); + + expect(schema.query.validate).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledWith(); + }); + + it('should respond with 422 with one validation error', () => { + const errors = { error: { details: [{ message: 'message' }] } }; + const schema = { query: { validate: jest.fn().mockReturnValue(errors) } }; + + const result = validate(schema); + expect(result).toBeInstanceOf(Function); + expect(() => result(req, res, next)).toThrow(); + + expect(schema.query.validate).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledTimes(0); + }); + + it('should respond with 422 with multiple validation errors', () => { + const errors = { error: { details: [{ message: 'message' }] } }; + const schema = { + params: { validate: jest.fn().mockReturnValue(errors) }, + query: { validate: jest.fn().mockReturnValue(errors) } + }; + + const result = validate(schema); + expect(result).toBeInstanceOf(Function); + expect(() => result(req, res, next)).toThrow(); + + expect(schema.query.validate).toHaveBeenCalledTimes(1); + expect(schema.query.validate).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledTimes(0); + }); +}); diff --git a/app/tests/unit/validators/object.spec.js b/app/tests/unit/validators/object.spec.js index 63ca5fcf..397b1724 100644 --- a/app/tests/unit/validators/object.spec.js +++ b/app/tests/unit/validators/object.spec.js @@ -1,6 +1,5 @@ -const config = require('config'); const crypto = require('crypto'); -const { Joi } = require('express-validation'); +const Joi = require('joi'); const jestJoi = require('jest-joi'); const { DownloadMode } = require('../../../src/components/constants'); expect.extend(jestJoi.matchers); @@ -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({