Merge pull request #606 from outline/validation-improves

Validation Improvements

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,

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,

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();
   });
 });

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,

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<void>
@@ -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 <token>"`
-          );
-        }
+        throw new AuthenticationError(
+          `Bad Authorization header format. Format is "Authorization: Bearer <token>"`
+        );
       }
       // $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');
         }
       }
 

server/api/middlewares/pagination.js

@@ -1,6 +1,6 @@
 // @flow
-import httpErrors from 'http-errors';
 import querystring from 'querystring';
+import { InvalidRequestError } from '../../errors';
 import { type Context } from 'koa';
 
 export default function pagination(options?: Object) {
@@ -24,7 +24,7 @@ export default function pagination(options?: Object) {
     offset = isNaN(offset) ? 0 : offset;
 
     if (limit > opts.maxLimit) {
-      throw httpErrors.BadRequest(
+      throw new InvalidRequestError(
         `Pagination limit is too large (max ${opts.maxLimit})`
       );
     }

server/emails/index.js

@@ -1,7 +1,7 @@
 // @flow
 import Koa from 'koa';
 import Router from 'koa-router';
-import httpErrors from 'http-errors';
+import { NotFoundError } from '../errors';
 import { Mailer } from '../mailer';
 
 const emailPreviews = new Koa();
@@ -22,7 +22,7 @@ router.get('/:type/:format', async ctx => {
       if (Object.getOwnPropertyNames(previewMailer).includes(ctx.params.type)) {
         // $FlowIssue flow doesn't like this but we're ok with it
         previewMailer[ctx.params.type]('user@example.com');
-      } else throw httpErrors.NotFound();
+      } else throw new NotFoundError('Email template could not be found');
   }
 
   if (!mailerOutput) return;

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' });
+}

server/routes.js

@@ -2,7 +2,6 @@
 import React from 'react';
 import path from 'path';
 import fs from 'fs-extra';
-import httpErrors from 'http-errors';
 import Koa from 'koa';
 import Router from 'koa-router';
 import sendfile from 'koa-sendfile';
@@ -11,6 +10,7 @@ import subdomainRedirect from './middlewares/subdomainRedirect';
 import renderpage from './utils/renderpage';
 import { slackAuth } from '../shared/utils/routeHelpers';
 import { robotsResponse } from './utils/robots';
+import { NotFoundError } from './errors';
 
 import Home from './pages/Home';
 import About from './pages/About';
@@ -88,7 +88,7 @@ router.get('/robots.txt', ctx => (ctx.body = robotsResponse(ctx)));
 // catch all for react app
 router.get('*', async ctx => {
   await renderapp(ctx);
-  if (!ctx.status) ctx.throw(httpErrors.NotFound());
+  if (!ctx.status) ctx.throw(new NotFoundError());
 });
 
 // middleware

server/slack.js

@@ -1,14 +1,15 @@
 // @flow
 import fetch from 'isomorphic-fetch';
 import querystring from 'querystring';
-import httpErrors from 'http-errors';
+import { InvalidRequestError } from './errors';
 
 const SLACK_API_URL = 'https://slack.com/api';
 
 export async function post(endpoint: string, body: Object) {
   let data;
+
+  const token = body.token;
   try {
-    const token = body.token;
     const response = await fetch(`${SLACK_API_URL}/${endpoint}`, {
       method: 'POST',
       headers: {
@@ -18,10 +19,10 @@ export async function post(endpoint: string, body: Object) {
       body: JSON.stringify(body),
     });
     data = await response.json();
-  } catch (e) {
-    throw httpErrors.BadRequest();
+  } catch (err) {
+    throw new InvalidRequestError(err.message);
   }
-  if (!data.ok) throw httpErrors.BadRequest(data.error);
+  if (!data.ok) throw new InvalidRequestError(data.error);
 
   return data;
 }
@@ -33,10 +34,10 @@ export async function request(endpoint: string, body: Object) {
       `${SLACK_API_URL}/${endpoint}?${querystring.stringify(body)}`
     );
     data = await response.json();
-  } catch (e) {
-    throw httpErrors.BadRequest();
+  } catch (err) {
+    throw new InvalidRequestError(err.message);
   }
-  if (!data.ok) throw httpErrors.BadRequest(data.error);
+  if (!data.ok) throw new InvalidRequestError(data.error);
 
   return data;
 }