diff --git a/server/api/__snapshots__/documents.test.js.snap b/server/api/__snapshots__/documents.test.js.snap index 1aec00e8..747d364a 100644 --- a/server/api/__snapshots__/documents.test.js.snap +++ b/server/api/__snapshots__/documents.test.js.snap @@ -1,11 +1,10 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`#documents.create should create as a child 1`] = ` +exports[`#documents.create should error with invalid parentDocument 1`] = ` Object { - "error": "invalid_parent_document", - "message": "Invalid parentDocument", + "error": "authorization_error", + "message": "Authorization error", "ok": false, - "status": 400, } `; @@ -56,7 +55,7 @@ Object { exports[`#documents.update should fail if document lastRevision does not match 1`] = ` Object { - "error": "document_has_changed_since_last_revision", + "error": "invalid_request", "message": "Document has changed since last revision", "ok": false, "status": 400, diff --git a/server/api/documents.js b/server/api/documents.js index 219a783d..34598a65 100644 --- a/server/api/documents.js +++ b/server/api/documents.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 { presentDocument, presentRevision } from '../presenters'; import { Document, Collection, Star, View, Revision } from '../models'; +import { ValidationError, InvalidRequestError } from '../errors'; import policy from '../policies'; const { authorize } = policy; @@ -208,8 +208,6 @@ router.post('documents.create', auth(), async ctx => { }); authorize(user, 'publish', ownerCollection); - if (!ownerCollection) throw httpErrors.BadRequest(); - let parentDocumentObj = {}; if (parentDocument && ownerCollection.type === 'atlas') { parentDocumentObj = await Document.findOne({ @@ -218,8 +216,7 @@ router.post('documents.create', auth(), async ctx => { atlasId: ownerCollection.id, }, }); - if (!parentDocumentObj) - throw httpErrors.BadRequest('Invalid parentDocument'); + authorize(user, 'read', parentDocumentObj); } const newDocument = await Document.create({ @@ -258,7 +255,7 @@ router.post('documents.update', auth(), async ctx => { authorize(ctx.state.user, 'update', document); if (lastRevision && lastRevision !== document.revisionCount) { - throw httpErrors.BadRequest('Document has changed since last revision'); + throw new InvalidRequestError('Document has changed since last revision'); } // Update document @@ -283,27 +280,25 @@ router.post('documents.move', auth(), async ctx => { const { id, parentDocument, index } = ctx.body; ctx.assertPresent(id, 'id is required'); if (parentDocument) - ctx.assertUuid(parentDocument, 'parentDocument must be an uuid'); + ctx.assertUuid(parentDocument, 'parentDocument must be a uuid'); if (index) ctx.assertPositiveInteger(index, 'index must be an integer (>=0)'); + const user = ctx.state.user; const document = await Document.findById(id); - authorize(ctx.state.user, 'update', document); + authorize(user, 'update', document); const collection = document.collection; if (collection.type !== 'atlas') - throw httpErrors.BadRequest("This document can't be moved"); + throw new InvalidRequestError('This document can’t be moved'); // Set parent document if (parentDocument) { const parent = await Document.findById(parentDocument); - if (!parent || parent.atlasId !== document.atlasId) - throw httpErrors.BadRequest( - 'Invalid parentDocument (must be same collection)' - ); + authorize(user, 'update', parent); } if (parentDocument === id) - throw httpErrors.BadRequest('Infinite loop detected and prevented!'); + throw new InvalidRequestError('Infinite loop detected and prevented!'); // If no parent document is provided, set it as null (move to root level) document.parentDocumentId = parentDocument; @@ -327,27 +322,11 @@ router.post('documents.delete', auth(), async ctx => { const collection = document.collection; if (collection.type === 'atlas') { - // Don't allow deletion of root docs - if (collection.documentStructure.length === 1) { - throw httpErrors.BadRequest( - "Unable to delete collection's only document" - ); - } - // Delete document and all of its children - try { - await collection.removeDocument(document); - } catch (e) { - throw httpErrors.BadRequest('Error while deleting'); - } + await collection.removeDocument(document); } - // Delete the actual document - try { - await document.destroy(); - } catch (e) { - throw httpErrors.BadRequest('Error while deleting document'); - } + await document.destroy(); ctx.body = { success: true, diff --git a/server/api/documents.test.js b/server/api/documents.test.js index 17eab993..a86d910d 100644 --- a/server/api/documents.test.js +++ b/server/api/documents.test.js @@ -312,7 +312,7 @@ describe('#documents.create', async () => { expect(body.data.collection.documents[0].children[0].id).toBe(body.data.id); }); - it('should create as a child', async () => { + it('should error with invalid parentDocument', async () => { const { user, collection } = await seed(); const res = await server.post('/api/documents.create', { body: { @@ -325,7 +325,7 @@ describe('#documents.create', async () => { }); const body = await res.json(); - expect(res.status).toEqual(400); + expect(res.status).toEqual(403); expect(body).toMatchSnapshot(); }); }); diff --git a/server/api/hooks.js b/server/api/hooks.js index 46de6be5..962080e1 100644 --- a/server/api/hooks.js +++ b/server/api/hooks.js @@ -1,6 +1,6 @@ // @flow import Router from 'koa-router'; -import httpErrors from 'http-errors'; +import { AuthenticationError, InvalidRequestError } from '../errors'; import { Authentication, Document, User } from '../models'; import * as Slack from '../slack'; const router = new Router(); @@ -10,7 +10,7 @@ router.post('hooks.unfurl', async ctx => { if (challenge) return (ctx.body = ctx.body.challenge); if (token !== process.env.SLACK_VERIFICATION_TOKEN) - throw httpErrors.BadRequest('Invalid token'); + throw new AuthenticationError('Invalid token'); // TODO: Everything from here onwards will get moved to an async job const user = await User.find({ where: { slackId: event.user } }); @@ -50,7 +50,7 @@ router.post('hooks.slack', async ctx => { ctx.assertPresent(text, 'text is required'); if (token !== process.env.SLACK_VERIFICATION_TOKEN) - throw httpErrors.Unauthorized('Invalid token'); + throw new AuthenticationError('Invalid token'); const user = await User.find({ where: { @@ -58,7 +58,7 @@ router.post('hooks.slack', async ctx => { }, }); - if (!user) throw httpErrors.BadRequest('Invalid user'); + if (!user) throw new InvalidRequestError('Invalid user'); const documents = await Document.searchForUser(user, text, { limit: 5, diff --git a/server/api/middlewares/authentication.js b/server/api/middlewares/authentication.js index cde5c53e..050f0e0f 100644 --- a/server/api/middlewares/authentication.js +++ b/server/api/middlewares/authentication.js @@ -1,20 +1,10 @@ // @flow -import httpErrors from 'http-errors'; import JWT from 'jsonwebtoken'; import { type Context } from 'koa'; - import { User, ApiKey } from '../../models'; +import { AuthenticationError } from '../../errors'; -type AuthOptions = { - require?: boolean, -}; - -export default function auth(options: AuthOptions = {}) { - options = { - require: true, - ...options, - }; - +export default function auth() { return async function authMiddleware( ctx: Context, next: () => Promise @@ -32,11 +22,9 @@ export default function auth(options: AuthOptions = {}) { token = credentials; } } else { - if (require) { - throw httpErrors.Unauthorized( - `Bad Authorization header format. Format is "Authorization: Bearer "` - ); - } + throw new AuthenticationError( + `Bad Authorization header format. Format is "Authorization: Bearer "` + ); } // $FlowFixMe } else if (ctx.body.token) { @@ -45,9 +33,7 @@ export default function auth(options: AuthOptions = {}) { token = ctx.request.query.token; } - if (!token && require) { - throw httpErrors.Unauthorized('Authentication required'); - } + if (!token) throw new AuthenticationError('Authentication required'); if (token) { let user; @@ -62,16 +48,13 @@ export default function auth(options: AuthOptions = {}) { }, }); } catch (e) { - throw httpErrors.Unauthorized('Invalid API key'); + throw new AuthenticationError('Invalid API key'); } - if (!apiKey) throw httpErrors.Unauthorized('Invalid API key'); + if (!apiKey) throw new AuthenticationError('Invalid API key'); - user = await User.findOne({ - where: { id: apiKey.userId }, - }); - - if (!user) throw httpErrors.Unauthorized('Invalid API key'); + user = await User.findById(apiKey.userId); + if (!user) throw new AuthenticationError('Invalid API key'); } else { // JWT // Get user without verifying payload signature @@ -79,19 +62,17 @@ export default function auth(options: AuthOptions = {}) { try { payload = JWT.decode(token); } catch (e) { - throw httpErrors.Unauthorized('Unable to decode JWT token'); + throw new AuthenticationError('Unable to decode JWT token'); } - if (!payload) throw httpErrors.Unauthorized('Invalid token'); + if (!payload) throw new AuthenticationError('Invalid token'); - user = await User.findOne({ - where: { id: payload.id }, - }); + user = await User.findById(payload.id); try { JWT.verify(token, user.jwtSecret); } catch (e) { - throw httpErrors.Unauthorized('Invalid token'); + throw new AuthenticationError('Invalid token'); } } diff --git a/server/errors.js b/server/errors.js index e21867c5..e9df32d2 100644 --- a/server/errors.js +++ b/server/errors.js @@ -1,14 +1,10 @@ // @flow import httpErrors from 'http-errors'; -export function ValidationError(message: string = 'Validation failed') { - return httpErrors(400, message, { id: 'validation_error' }); -} - -export function ParamRequiredError( - message: string = 'Required parameter missing' +export function AuthenticationError( + message: string = 'Invalid authentication' ) { - return httpErrors(400, message, { id: 'param_required' }); + return httpErrors(401, message, { id: 'authentication_required' }); } export function AuthorizationError( @@ -23,6 +19,20 @@ export function AdminRequiredError( return httpErrors(403, message, { id: 'admin_required' }); } +export function InvalidRequestError(message: string = 'Request invalid') { + return httpErrors(400, message, { id: 'invalid_request' }); +} + export function NotFoundError(message: string = 'Resource not found') { return httpErrors(404, message, { id: 'not_found' }); } + +export function ParamRequiredError( + message: string = 'Required parameter missing' +) { + return httpErrors(400, message, { id: 'param_required' }); +} + +export function ValidationError(message: string = 'Validation failed') { + return httpErrors(400, message, { id: 'validation_error' }); +}