Added more structure and tests to our authorization code

package.json

@@ -87,6 +87,7 @@
     "boundless-popover": "^1.0.4",
     "bugsnag": "^1.7.0",
     "bull": "^3.3.7",
+    "cancan": "3.1.0",
     "copy-to-clipboard": "^3.0.6",
     "css-loader": "^0.28.7",
     "debug": "2.2.0",

server/api/documents.js

@@ -6,12 +6,9 @@ import auth from './middlewares/authentication';
 import pagination from './middlewares/pagination';
 import { presentDocument, presentRevision } from '../presenters';
 import { Document, Collection, Star, View, Revision } from '../models';
+import policy from '../policies';
 
-const authDocumentForUser = (ctx, document) => {
-  const user = ctx.state.user;
-  if (!document || document.teamId !== user.teamId) throw httpErrors.NotFound();
-};
-
+const { authorize } = policy;
 const router = new Router();
 
 router.post('documents.list', auth(), pagination(), async ctx => {
@@ -110,7 +107,7 @@ router.post('documents.info', auth(), async ctx => {
   ctx.assertPresent(id, 'id is required');
   const document = await Document.findById(id);
 
-  authDocumentForUser(ctx, document);
+  authorize(ctx.state.user, 'read', document);
 
   ctx.body = {
     data: await presentDocument(ctx, document),
@@ -123,7 +120,7 @@ router.post('documents.revisions', auth(), pagination(), async ctx => {
   ctx.assertPresent(id, 'id is required');
   const document = await Document.findById(id);
 
-  authDocumentForUser(ctx, document);
+  authorize(ctx.state.user, 'read', document);
 
   const revisions = await Revision.findAll({
     where: { documentId: id },
@@ -170,7 +167,7 @@ router.post('documents.star', auth(), async ctx => {
   const user = await ctx.state.user;
   const document = await Document.findById(id);
 
-  authDocumentForUser(ctx, document);
+  authorize(ctx.state.user, 'read', document);
 
   await Star.findOrCreate({
     where: { documentId: document.id, userId: user.id },
@@ -183,7 +180,7 @@ router.post('documents.unstar', auth(), async ctx => {
   const user = await ctx.state.user;
   const document = await Document.findById(id);
 
-  authDocumentForUser(ctx, document);
+  authorize(ctx.state.user, 'read', document);
 
   await Star.destroy({
     where: { documentId: document.id, userId: user.id },
@@ -254,20 +251,20 @@ router.post('documents.update', auth(), async ctx => {
 
   const user = ctx.state.user;
   const document = await Document.findById(id);
-  const collection = document.collection;
+
+  authorize(ctx.state.user, 'update', document);
 
   if (lastRevision && lastRevision !== document.revisionCount) {
     throw httpErrors.BadRequest('Document has changed since last revision');
   }
 
-  authDocumentForUser(ctx, document);
-
   // Update document
   if (title) document.title = title;
   if (text) document.text = text;
   document.lastModifiedById = user.id;
 
   await document.save();
+  const collection = document.collection;
   if (collection.type === 'atlas') {
     await collection.updateDocument(document);
   }
@@ -287,10 +284,9 @@ router.post('documents.move', auth(), async ctx => {
   if (index) ctx.assertPositiveInteger(index, 'index must be an integer (>=0)');
 
   const document = await Document.findById(id);
-  const collection = await Collection.findById(document.atlasId);
-
-  authDocumentForUser(ctx, document);
+  authorize(ctx.state.user, 'update', document);
 
+  const collection = document.collection;
   if (collection.type !== 'atlas')
     throw httpErrors.BadRequest("This document can't be moved");
 
@@ -324,10 +320,9 @@ router.post('documents.delete', auth(), async ctx => {
   ctx.assertPresent(id, 'id is required');
 
   const document = await Document.findById(id);
-  const collection = await Collection.findById(document.atlasId);
-
-  authDocumentForUser(ctx, document);
+  authorize(ctx.state.user, 'delete', document);
 
+  const collection = document.collection;
   if (collection.type === 'atlas') {
     // Don't allow deletion of root docs
     if (collection.documentStructure.length === 1) {

server/api/documents.test.js

@@ -3,6 +3,7 @@ import TestServer from 'fetch-test-server';
 import app from '..';
 import { View, Star } from '../models';
 import { flushdb, seed } from '../test/support';
+import { buildUser } from '../test/factories';
 import Document from '../models/Document';
 
 const server = new TestServer(app.callback());
@@ -55,7 +56,7 @@ describe('#documents.list', async () => {
 });
 
 describe('#documents.revision', async () => {
-  it("should return document's revisions", async () => {
+  it("should return a document's revisions", async () => {
     const { user, document } = await seed();
     const res = await server.post('/api/documents.revisions', {
       body: {
@@ -70,6 +71,18 @@ describe('#documents.revision', async () => {
     expect(body.data[0].id).not.toEqual(document.id);
     expect(body.data[0].title).toEqual(document.title);
   });
+
+  it('should require authorization', async () => {
+    const { document } = await seed();
+    const user = await buildUser();
+    const res = await server.post('/api/documents.revisions', {
+      body: {
+        token: user.getJwtToken(),
+        id: document.id,
+      },
+    });
+    expect(res.status).toEqual(404);
+  });
 });
 
 describe('#documents.search', async () => {
@@ -199,6 +212,15 @@ describe('#documents.star', async () => {
     expect(res.status).toEqual(401);
     expect(body).toMatchSnapshot();
   });
+
+  it('should require authorization', async () => {
+    const { document } = await seed();
+    const user = await buildUser();
+    const res = await server.post('/api/documents.star', {
+      body: { token: user.getJwtToken(), id: document.id },
+    });
+    expect(res.status).toEqual(404);
+  });
 });
 
 describe('#documents.unstar', async () => {
@@ -222,6 +244,15 @@ describe('#documents.unstar', async () => {
     expect(res.status).toEqual(401);
     expect(body).toMatchSnapshot();
   });
+
+  it('should require authorization', async () => {
+    const { document } = await seed();
+    const user = await buildUser();
+    const res = await server.post('/api/documents.unstar', {
+      body: { token: user.getJwtToken(), id: document.id },
+    });
+    expect(res.status).toEqual(404);
+  });
 });
 
 describe('#documents.create', async () => {
@@ -393,4 +424,24 @@ describe('#documents.update', async () => {
       'Updated title'
     );
   });
+
+  it('should require authentication', async () => {
+    const { document } = await seed();
+    const res = await server.post('/api/documents.update', {
+      body: { id: document.id, text: 'Updated' },
+    });
+    const body = await res.json();
+
+    expect(res.status).toEqual(401);
+    expect(body).toMatchSnapshot();
+  });
+
+  it('should require authorization', async () => {
+    const { document } = await seed();
+    const user = await buildUser();
+    const res = await server.post('/api/documents.update', {
+      body: { token: user.getJwtToken(), id: document.id, text: 'Updated' },
+    });
+    expect(res.status).toEqual(404);
+  });
 });

server/api/index.js

@@ -38,6 +38,11 @@ api.use(async (ctx, next) => {
       }
     }
 
+    if (message.match('Authorization error')) {
+      ctx.status = 404;
+      message = 'Not Found';
+    }
+
     if (ctx.status === 500) {
       message = 'Internal Server Error';
       ctx.app.emit('error', err, ctx);

server/policies/document.js

@@ -0,0 +1,13 @@
+// @flow
+import policy from './policy';
+import Document from '../models/Document';
+import User from '../models/User';
+
+const { allow } = policy;
+
+allow(
+  User,
+  ['create', 'read', 'update', 'delete'],
+  Document,
+  (user, doc) => user.teamId === doc.teamId
+);

server/policies/index.js

@@ -0,0 +1,5 @@
+// @flow
+import policy from './policy';
+import './document';
+
+export default policy;

server/policies/policy.js

@@ -0,0 +1,3 @@
+// @flow
+import CanCan from 'cancan';
+export default new CanCan();

server/test/factories.js

@@ -0,0 +1,34 @@
+// @flow
+import Team from '../models/Team';
+import User from '../models/User';
+import uuid from 'uuid';
+
+let count = 0;
+
+export function buildTeam(overrides: Object = {}) {
+  count++;
+
+  return Team.create({
+    name: `Team ${count}`,
+    slackId: uuid.v4(),
+    ...overrides,
+  });
+}
+
+export async function buildUser(overrides: Object = {}) {
+  count++;
+
+  if (!overrides.teamId) {
+    const team = await buildTeam();
+    overrides.teamId = team.id;
+  }
+
+  return User.create({
+    email: `user${count}@example.com`,
+    username: `user${count}`,
+    name: `User ${count}`,
+    password: 'test123!',
+    slackId: uuid.v4(),
+    ...overrides,
+  });
+}

yarn.lock

@@ -423,6 +423,10 @@ attr-accept@^1.0.3:
   version "1.1.0"
   resolved "https://registry.yarnpkg.com/attr-accept/-/attr-accept-1.1.0.tgz#b5cd35227f163935a8f1de10ed3eba16941f6be6"
 
+auto-bind@^1.1.0:
+  version "1.2.0"
+  resolved "https://registry.yarnpkg.com/auto-bind/-/auto-bind-1.2.0.tgz#8b7e318aad53d43ba8a8ecaf0066d85d5f798cd6"
+
 autoprefixer@^6.3.1:
   version "6.7.7"
   resolved "https://registry.yarnpkg.com/autoprefixer/-/autoprefixer-6.7.7.tgz#1dbd1c835658e35ce3f9984099db00585c782014"
@@ -1531,6 +1535,14 @@ camelize@1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/camelize/-/camelize-1.0.0.tgz#164a5483e630fa4321e5af07020e531831b2609b"
 
+cancan@3.1.0:
+  version "3.1.0"
+  resolved "https://registry.yarnpkg.com/cancan/-/cancan-3.1.0.tgz#4d148e73795324f689a9b1002e61839c17ea821e"
+  dependencies:
+    arrify "^1.0.1"
+    auto-bind "^1.1.0"
+    is-plain-obj "^1.1.0"
+
 caniuse-api@^1.5.2:
   version "1.6.1"
   resolved "https://registry.yarnpkg.com/caniuse-api/-/caniuse-api-1.6.1.tgz#b534e7c734c4f81ec5fbe8aca2ad24354b962c6c"