diff --git a/package.json b/package.json index 1f7ea5ad..53ac3b8d 100644 --- a/package.json +++ b/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", diff --git a/server/api/documents.js b/server/api/documents.js index 949dcb8c..58939d9c 100644 --- a/server/api/documents.js +++ b/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) { diff --git a/server/api/documents.test.js b/server/api/documents.test.js index 60751644..3220b727 100644 --- a/server/api/documents.test.js +++ b/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); + }); }); diff --git a/server/api/index.js b/server/api/index.js index a1b4f8e9..8bab85c3 100644 --- a/server/api/index.js +++ b/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); diff --git a/server/policies/document.js b/server/policies/document.js new file mode 100644 index 00000000..0be7d4c3 --- /dev/null +++ b/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 +); diff --git a/server/policies/index.js b/server/policies/index.js new file mode 100644 index 00000000..637227a4 --- /dev/null +++ b/server/policies/index.js @@ -0,0 +1,5 @@ +// @flow +import policy from './policy'; +import './document'; + +export default policy; diff --git a/server/policies/policy.js b/server/policies/policy.js new file mode 100644 index 00000000..a9535176 --- /dev/null +++ b/server/policies/policy.js @@ -0,0 +1,3 @@ +// @flow +import CanCan from 'cancan'; +export default new CanCan(); diff --git a/server/test/factories.js b/server/test/factories.js new file mode 100644 index 00000000..d88fb83c --- /dev/null +++ b/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, + }); +} diff --git a/yarn.lock b/yarn.lock index ceb7a323..1db092c9 100644 --- a/yarn.lock +++ b/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"