diff --git a/server/api/__snapshots__/team.test.js.snap b/server/api/__snapshots__/team.test.js.snap index 0891a3b86..917e0d182 100644 --- a/server/api/__snapshots__/team.test.js.snap +++ b/server/api/__snapshots__/team.test.js.snap @@ -17,9 +17,10 @@ Object { exports[`#team.addAdmin should require admin 1`] = ` Object { - "error": "authorization_error", - "message": "Authorization Required", + "error": "admin_required", + "message": "An admin role is required to access this resource", "ok": false, + "status": 403, } `; @@ -40,15 +41,16 @@ Object { exports[`#team.removeAdmin should require admin 1`] = ` Object { - "error": "authorization_error", - "message": "Authorization Required", + "error": "admin_required", + "message": "An admin role is required to access this resource", "ok": false, + "status": 403, } `; exports[`#team.removeAdmin shouldn't demote admins if only one available 1`] = ` Object { - "error": "at_least_one_admin_is_required", + "error": "validation_error", "message": "At least one admin is required", "ok": false, "status": 400, diff --git a/server/api/apiKeys.js b/server/api/apiKeys.js index 1dd33ba41..2452030bd 100644 --- a/server/api/apiKeys.js +++ b/server/api/apiKeys.js @@ -1,6 +1,5 @@ // @flow import Router from 'koa-router'; -import httpErrors from 'http-errors'; import auth from './middlewares/authentication'; import pagination from './middlewares/pagination'; @@ -55,11 +54,7 @@ router.post('apiKeys.delete', auth(), async ctx => { const key = await ApiKey.findById(id); authorize(user, 'delete', key); - try { - await key.destroy(); - } catch (e) { - throw httpErrors.BadRequest('Error while deleting key'); - } + await key.destroy(); ctx.body = { success: true, diff --git a/server/api/collections.js b/server/api/collections.js index 44b2add50..86b447644 100644 --- a/server/api/collections.js +++ b/server/api/collections.js @@ -1,11 +1,11 @@ // @flow import Router from 'koa-router'; -import httpErrors from 'http-errors'; import auth from './middlewares/authentication'; import pagination from './middlewares/pagination'; import { presentCollection } from '../presenters'; import { Collection } from '../models'; +import { ValidationError } from '../errors'; import policy from '../policies'; const { authorize } = policy; @@ -95,13 +95,9 @@ router.post('collections.delete', auth(), async ctx => { authorize(ctx.state.user, 'delete', collection); const total = await Collection.count(); - if (total === 1) throw httpErrors.BadRequest('Cannot delete last collection'); + if (total === 1) throw new ValidationError('Cannot delete last collection'); - try { - await collection.destroy(); - } catch (e) { - throw httpErrors.BadRequest('Error while deleting collection'); - } + await collection.destroy(); ctx.body = { success: true, diff --git a/server/api/collections.test.js b/server/api/collections.test.js index 58b79e9d3..fcc7248ac 100644 --- a/server/api/collections.test.js +++ b/server/api/collections.test.js @@ -57,7 +57,7 @@ describe('#collections.info', async () => { const res = await server.post('/api/collections.info', { body: { token: user.getJwtToken(), id: collection.id }, }); - expect(res.status).toEqual(404); + expect(res.status).toEqual(403); }); }); diff --git a/server/api/index.js b/server/api/index.js index 750427ade..096a8c3e2 100644 --- a/server/api/index.js +++ b/server/api/index.js @@ -39,13 +39,7 @@ api.use(async (ctx, next) => { } if (message.match('Authorization error')) { - if (ctx.path.match('info')) { - ctx.status = 404; - message = 'Not Found'; - } else { - ctx.status = 403; - message = 'Authorization Required'; - } + ctx.status = 403; } if (ctx.status === 500) { diff --git a/server/api/middlewares/validation.js b/server/api/middlewares/validation.js index 231595cf1..5ac0ef360 100644 --- a/server/api/middlewares/validation.js +++ b/server/api/middlewares/validation.js @@ -1,43 +1,43 @@ // @flow -import apiError from '../../errors'; import validator from 'validator'; +import { ParamRequiredError, ValidationError } from '../../errors'; import { validateColorHex } from '../../../shared/utils/color'; export default function validation() { return function validationMiddleware(ctx: Object, next: Function) { - ctx.assertPresent = function assertPresent(value, message) { + ctx.assertPresent = (value, message) => { if (value === undefined || value === null || value === '') { - throw apiError(400, 'validation_error', message); + throw new ParamRequiredError(message); } }; - ctx.assertNotEmpty = function assertNotEmpty(value, message) { + ctx.assertNotEmpty = (value, message) => { if (value === '') { - throw apiError(400, 'validation_error', message); + throw new ValidationError(message); } }; ctx.assertEmail = (value, message) => { if (!validator.isEmail(value)) { - throw apiError(400, 'validation_error', message); + throw new ValidationError(message); } }; ctx.assertUuid = (value, message) => { if (!validator.isUUID(value)) { - throw apiError(400, 'validation_error', message); + throw new ValidationError(message); } }; ctx.assertPositiveInteger = (value, message) => { if (!validator.isInt(value, { min: 0 })) { - throw apiError(400, 'validation_error', message); + throw new ValidationError(message); } }; ctx.assertHexColor = (value, message) => { if (!validateColorHex(value)) { - throw apiError(400, 'validation_error', message); + throw new ValidationError(message); } }; diff --git a/server/api/team.js b/server/api/team.js index 864dc2c36..a812de8dc 100644 --- a/server/api/team.js +++ b/server/api/team.js @@ -1,7 +1,6 @@ // @flow import Router from 'koa-router'; -import httpErrors from 'http-errors'; - +import { ValidationError } from '../errors'; import { Team, User } from '../models'; import auth from './middlewares/authentication'; @@ -61,7 +60,7 @@ router.post('team.removeAdmin', auth(), async ctx => { try { await team.removeAdmin(user); } catch (err) { - throw httpErrors.BadRequest(err.message); + throw new ValidationError(err.message); } ctx.body = { diff --git a/server/errors.js b/server/errors.js index 03bfe2f30..e21867c51 100644 --- a/server/errors.js +++ b/server/errors.js @@ -1,9 +1,28 @@ // @flow import httpErrors from 'http-errors'; -const apiError = (code: number, id: string, message: string) => { - return httpErrors(code, message, { id }); -}; +export function ValidationError(message: string = 'Validation failed') { + return httpErrors(400, message, { id: 'validation_error' }); +} -export default apiError; -export { httpErrors }; +export function ParamRequiredError( + message: string = 'Required parameter missing' +) { + return httpErrors(400, message, { id: 'param_required' }); +} + +export function AuthorizationError( + message: string = 'You do not have permission to access this resource' +) { + return httpErrors(403, message, { id: 'permission_required' }); +} + +export function AdminRequiredError( + message: string = 'An admin role is required to access this resource' +) { + return httpErrors(403, message, { id: 'admin_required' }); +} + +export function NotFoundError(message: string = 'Resource not found') { + return httpErrors(404, message, { id: 'not_found' }); +} diff --git a/server/policies/collection.js b/server/policies/collection.js index 5bdb091a0..29ae2dbf5 100644 --- a/server/policies/collection.js +++ b/server/policies/collection.js @@ -1,6 +1,7 @@ // @flow import policy from './policy'; import { Collection, User } from '../models'; +import { AdminRequiredError } from '../errors'; const { allow } = policy; @@ -13,12 +14,8 @@ allow( (user, collection) => collection && user.teamId === collection.teamId ); -allow( - User, - 'delete', - Collection, - (user, collection) => - collection && - user.teamId === collection.teamId && - (user.id === collection.creatorId || user.isAdmin) -); +allow(User, 'delete', Collection, (user, collection) => { + if (!collection || user.teamId !== collection.teamId) return false; + if (user.id === collection.creatorId) return true; + if (!user.isAdmin) throw new AdminRequiredError(); +}); diff --git a/server/policies/user.js b/server/policies/user.js index 3e7361a4b..4e8336b28 100644 --- a/server/policies/user.js +++ b/server/policies/user.js @@ -1,6 +1,7 @@ // @flow import policy from './policy'; import { User } from '../models'; +import { AdminRequiredError } from '../errors'; const { allow } = policy; @@ -11,19 +12,15 @@ allow( (actor, user) => user && user.teamId === actor.teamId ); -allow( - User, - ['update', 'delete'], - User, - (actor, user) => - user && - user.teamId === actor.teamId && - (user.id === actor.id || actor.isAdmin) -); +allow(User, ['update', 'delete'], User, (actor, user) => { + if (!user || user.teamId !== actor.teamId) return false; + if (user.id === actor.id) return true; + if (actor.isAdmin) return true; + throw new AdminRequiredError(); +}); -allow( - User, - ['promote', 'demote'], - User, - (actor, user) => user && user.teamId === actor.teamId && actor.isAdmin -); +allow(User, ['promote', 'demote'], User, (actor, user) => { + if (!user || user.teamId !== actor.teamId) return false; + if (actor.isAdmin) return true; + throw new AdminRequiredError(); +}); diff --git a/server/slack.js b/server/slack.js index 7ca4b78ca..098b6f5af 100644 --- a/server/slack.js +++ b/server/slack.js @@ -1,7 +1,7 @@ // @flow import fetch from 'isomorphic-fetch'; import querystring from 'querystring'; -import { httpErrors } from './errors'; +import httpErrors from 'http-errors'; const SLACK_API_URL = 'https://slack.com/api';