From 2f81eb5e871384ee87d8cd59d05ee1f6e405a7f0 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 18 Feb 2018 00:11:48 -0800 Subject: [PATCH 1/7] Added more structure and tests to our authorization code --- package.json | 1 + server/api/documents.js | 31 +++++++++------------ server/api/documents.test.js | 53 +++++++++++++++++++++++++++++++++++- server/api/index.js | 5 ++++ server/policies/document.js | 13 +++++++++ server/policies/index.js | 5 ++++ server/policies/policy.js | 3 ++ server/test/factories.js | 34 +++++++++++++++++++++++ yarn.lock | 12 ++++++++ 9 files changed, 138 insertions(+), 19 deletions(-) create mode 100644 server/policies/document.js create mode 100644 server/policies/index.js create mode 100644 server/policies/policy.js create mode 100644 server/test/factories.js diff --git a/package.json b/package.json index 1f7ea5ad6..53ac3b8db 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 949dcb8c1..58939d9c3 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 60751644b..3220b727f 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 a1b4f8e9a..8bab85c36 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 000000000..0be7d4c38 --- /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 000000000..637227a44 --- /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 000000000..a95351760 --- /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 000000000..d88fb83c6 --- /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 ceb7a3232..1db092c92 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" From e84fb5e6babe150893050a4229e06bfaf7f5035c Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 18 Feb 2018 01:14:51 -0800 Subject: [PATCH 2/7] Update team and collection authorization --- .../api/__snapshots__/documents.test.js.snap | 9 ++++ server/api/__snapshots__/team.test.js.snap | 33 +++++++----- server/api/auth.js | 2 +- server/api/collections.js | 43 +++++++-------- server/api/collections.test.js | 35 +++++++++--- server/api/documents.test.js | 8 +-- server/api/index.js | 9 +++- server/api/middlewares/authentication.js | 5 -- server/api/middlewares/authentication.test.js | 39 -------------- server/api/team.js | 54 ++++++++++--------- server/api/team.test.js | 9 ++-- server/models/Team.js | 6 +-- server/policies/collection.js | 25 +++++++++ server/policies/document.js | 6 ++- server/policies/index.js | 2 + server/policies/user.js | 29 ++++++++++ services/slack/index.js | 2 +- 17 files changed, 181 insertions(+), 135 deletions(-) create mode 100644 server/policies/collection.js create mode 100644 server/policies/user.js diff --git a/server/api/__snapshots__/documents.test.js.snap b/server/api/__snapshots__/documents.test.js.snap index cda46a768..1aec00e87 100644 --- a/server/api/__snapshots__/documents.test.js.snap +++ b/server/api/__snapshots__/documents.test.js.snap @@ -63,6 +63,15 @@ Object { } `; +exports[`#documents.update should require authentication 1`] = ` +Object { + "error": "authentication_required", + "message": "Authentication required", + "ok": false, + "status": 401, +} +`; + exports[`#documents.viewed should require authentication 1`] = ` Object { "error": "authentication_required", diff --git a/server/api/__snapshots__/team.test.js.snap b/server/api/__snapshots__/team.test.js.snap index bfe0d5892..0891a3b86 100644 --- a/server/api/__snapshots__/team.test.js.snap +++ b/server/api/__snapshots__/team.test.js.snap @@ -2,29 +2,37 @@ exports[`#team.addAdmin should promote a new admin 1`] = ` Object { - "avatarUrl": "http://example.com/avatar.png", - "email": "user1@example.com", - "id": "46fde1d4-0050-428f-9f0b-0bf77f4bdf61", - "isAdmin": true, - "name": "User 1", + "data": Object { + "avatarUrl": "http://example.com/avatar.png", + "email": "user1@example.com", + "id": "46fde1d4-0050-428f-9f0b-0bf77f4bdf61", + "isAdmin": true, + "name": "User 1", + "username": "user1", + }, "ok": true, "status": 200, - "username": "user1", } `; exports[`#team.addAdmin should require admin 1`] = ` Object { - "error": "only_available_for_admins", - "message": "Only available for admins", + "error": "authorization_error", + "message": "Authorization Required", "ok": false, - "status": 403, } `; exports[`#team.removeAdmin should demote an admin 1`] = ` Object { - "avatarUrl": null, + "data": Object { + "avatarUrl": "http://example.com/avatar.png", + "email": "user1@example.com", + "id": "46fde1d4-0050-428f-9f0b-0bf77f4bdf61", + "isAdmin": false, + "name": "User 1", + "username": "user1", + }, "ok": true, "status": 200, } @@ -32,10 +40,9 @@ Object { exports[`#team.removeAdmin should require admin 1`] = ` Object { - "error": "only_available_for_admins", - "message": "Only available for admins", + "error": "authorization_error", + "message": "Authorization Required", "ok": false, - "status": 403, } `; diff --git a/server/api/auth.js b/server/api/auth.js index 45ddea403..7ad43a51b 100644 --- a/server/api/auth.js +++ b/server/api/auth.js @@ -9,7 +9,7 @@ const router = new Router(); router.post('auth.info', auth(), async ctx => { const user = ctx.state.user; - const team = await Team.findOne({ where: { id: user.teamId } }); + const team = await Team.findById(user.teamId); ctx.body = { data: { diff --git a/server/api/collections.js b/server/api/collections.js index e7964c105..d7a203f82 100644 --- a/server/api/collections.js +++ b/server/api/collections.js @@ -1,13 +1,14 @@ // @flow import Router from 'koa-router'; import httpErrors from 'http-errors'; -import _ from 'lodash'; import auth from './middlewares/authentication'; import pagination from './middlewares/pagination'; import { presentCollection } from '../presenters'; import { Collection } from '../models'; +import policy from '../policies'; +const { authorize } = policy; const router = new Router(); router.post('collections.create', auth(), async ctx => { @@ -32,6 +33,18 @@ router.post('collections.create', auth(), async ctx => { }; }); +router.post('collections.info', auth(), async ctx => { + const { id } = ctx.body; + ctx.assertPresent(id, 'id is required'); + + const collection = await Collection.scope('withRecentDocuments').findById(id); + authorize(ctx.state.user, 'read', collection); + + ctx.body = { + data: await presentCollection(ctx, collection), + }; +}); + router.post('collections.update', auth(), async ctx => { const { id, name, color } = ctx.body; ctx.assertPresent(name, 'name is required'); @@ -39,6 +52,8 @@ router.post('collections.update', auth(), async ctx => { ctx.assertHexColor(color, 'Invalid hex value (please use format #FFFFFF)'); const collection = await Collection.findById(id); + authorize(ctx.state.user, 'update', collection); + collection.name = name; collection.color = color; await collection.save(); @@ -48,25 +63,6 @@ router.post('collections.update', auth(), async ctx => { }; }); -router.post('collections.info', auth(), async ctx => { - const { id } = ctx.body; - ctx.assertPresent(id, 'id is required'); - - const user = ctx.state.user; - const collection = await Collection.scope('withRecentDocuments').findOne({ - where: { - id, - teamId: user.teamId, - }, - }); - - if (!collection) throw httpErrors.NotFound(); - - ctx.body = { - data: await presentCollection(ctx, collection), - }; -}); - router.post('collections.list', auth(), pagination(), async ctx => { const user = ctx.state.user; const collections = await Collection.findAll({ @@ -94,15 +90,12 @@ router.post('collections.delete', auth(), async ctx => { const { id } = ctx.body; ctx.assertPresent(id, 'id is required'); - const user = ctx.state.user; const collection = await Collection.findById(id); + authorize(ctx.state.user, 'delete', collection); + const total = await Collection.count(); - if (total === 1) throw httpErrors.BadRequest('Cannot delete last collection'); - if (!collection || collection.teamId !== user.teamId) - throw httpErrors.BadRequest(); - try { await collection.destroy(); } catch (e) { diff --git a/server/api/collections.test.js b/server/api/collections.test.js index 6f6b320c8..16df1553c 100644 --- a/server/api/collections.test.js +++ b/server/api/collections.test.js @@ -2,6 +2,7 @@ import TestServer from 'fetch-test-server'; import app from '..'; import { flushdb, seed } from '../test/support'; +import { buildUser } from '../test/factories'; import Collection from '../models/Collection'; const server = new TestServer(app.callback()); @@ -31,14 +32,6 @@ describe('#collections.list', async () => { }); describe('#collections.info', async () => { - it('should require authentication', async () => { - const res = await server.post('/api/collections.info'); - const body = await res.json(); - - expect(res.status).toEqual(401); - expect(body).toMatchSnapshot(); - }); - it('should return collection', async () => { const { user, collection } = await seed(); const res = await server.post('/api/collections.info', { @@ -49,6 +42,23 @@ describe('#collections.info', async () => { expect(res.status).toEqual(200); expect(body.data.id).toEqual(collection.id); }); + + it('should require authentication', async () => { + const res = await server.post('/api/collections.info'); + const body = await res.json(); + + expect(res.status).toEqual(401); + expect(body).toMatchSnapshot(); + }); + + it('should require authorization', async () => { + const { collection } = await seed(); + const user = await buildUser(); + const res = await server.post('/api/collections.info', { + body: { token: user.getJwtToken(), id: collection.id }, + }); + expect(res.status).toEqual(404); + }); }); describe('#collections.create', async () => { @@ -82,6 +92,15 @@ describe('#collections.delete', async () => { expect(body).toMatchSnapshot(); }); + it('should require authorization', async () => { + const { collection } = await seed(); + const user = await buildUser(); + const res = await server.post('/api/collections.delete', { + body: { token: user.getJwtToken(), id: collection.id }, + }); + expect(res.status).toEqual(403); + }); + it('should not delete last collection', async () => { const { user, collection } = await seed(); const res = await server.post('/api/collections.delete', { diff --git a/server/api/documents.test.js b/server/api/documents.test.js index 3220b727f..24992fc50 100644 --- a/server/api/documents.test.js +++ b/server/api/documents.test.js @@ -81,7 +81,7 @@ describe('#documents.revision', async () => { id: document.id, }, }); - expect(res.status).toEqual(404); + expect(res.status).toEqual(403); }); }); @@ -219,7 +219,7 @@ describe('#documents.star', async () => { const res = await server.post('/api/documents.star', { body: { token: user.getJwtToken(), id: document.id }, }); - expect(res.status).toEqual(404); + expect(res.status).toEqual(403); }); }); @@ -251,7 +251,7 @@ describe('#documents.unstar', async () => { const res = await server.post('/api/documents.unstar', { body: { token: user.getJwtToken(), id: document.id }, }); - expect(res.status).toEqual(404); + expect(res.status).toEqual(403); }); }); @@ -442,6 +442,6 @@ describe('#documents.update', async () => { const res = await server.post('/api/documents.update', { body: { token: user.getJwtToken(), id: document.id, text: 'Updated' }, }); - expect(res.status).toEqual(404); + expect(res.status).toEqual(403); }); }); diff --git a/server/api/index.js b/server/api/index.js index 8bab85c36..750427ade 100644 --- a/server/api/index.js +++ b/server/api/index.js @@ -39,8 +39,13 @@ api.use(async (ctx, next) => { } if (message.match('Authorization error')) { - ctx.status = 404; - message = 'Not Found'; + if (ctx.path.match('info')) { + ctx.status = 404; + message = 'Not Found'; + } else { + ctx.status = 403; + message = 'Authorization Required'; + } } if (ctx.status === 500) { diff --git a/server/api/middlewares/authentication.js b/server/api/middlewares/authentication.js index a40a684d5..cde5c53e1 100644 --- a/server/api/middlewares/authentication.js +++ b/server/api/middlewares/authentication.js @@ -7,7 +7,6 @@ import { User, ApiKey } from '../../models'; type AuthOptions = { require?: boolean, - adminOnly?: boolean, }; export default function auth(options: AuthOptions = {}) { @@ -96,10 +95,6 @@ export default function auth(options: AuthOptions = {}) { } } - if (options.adminOnly && !user.isAdmin) { - throw httpErrors.Forbidden('Only available for admins'); - } - ctx.state.token = token; ctx.state.user = user; // $FlowFixMe diff --git a/server/api/middlewares/authentication.test.js b/server/api/middlewares/authentication.test.js index 4e8d7dc9f..734fcd1b8 100644 --- a/server/api/middlewares/authentication.test.js +++ b/server/api/middlewares/authentication.test.js @@ -91,45 +91,6 @@ describe('Authentication middleware', async () => { }); }); - describe('adminOnly', () => { - it('should work if user is an admin', async () => { - const state = {}; - const { user } = await seed(); - const authMiddleware = auth({ adminOnly: true }); - user.isAdmin = true; - await user.save(); - - await authMiddleware( - { - request: { - get: jest.fn(() => `Bearer ${user.getJwtToken()}`), - }, - state, - cache: {}, - }, - jest.fn() - ); - expect(state.user.id).toEqual(user.id); - }); - - it('should raise 403 if user is not an admin', async () => { - const { user } = await seed(); - const authMiddleware = auth({ adminOnly: true }); - user.idAdmin = true; - await user.save(); - - try { - await authMiddleware({ - request: { - get: jest.fn(() => `Bearer ${user.getJwtToken()}`), - }, - }); - } catch (e) { - expect(e.message).toBe('Only available for admins'); - } - }); - }); - it('should return error message if no auth token is available', async () => { const state = {}; const authMiddleware = auth(); diff --git a/server/api/team.js b/server/api/team.js index 98d63dee5..7821bee55 100644 --- a/server/api/team.js +++ b/server/api/team.js @@ -2,13 +2,15 @@ import Router from 'koa-router'; import httpErrors from 'http-errors'; -import User from '../models/User'; import Team from '../models/Team'; +import User from '../models/User'; import auth from './middlewares/authentication'; import pagination from './middlewares/pagination'; import { presentUser } from '../presenters'; +import policy from '../policies'; +const { authorize } = policy; const router = new Router(); router.post('team.users', auth(), pagination(), async ctx => { @@ -31,41 +33,41 @@ router.post('team.users', auth(), pagination(), async ctx => { }; }); -router.post('team.addAdmin', auth({ adminOnly: true }), async ctx => { - const { user } = ctx.body; - const admin = ctx.state.user; - ctx.assertPresent(user, 'id is required'); +router.post('team.addAdmin', auth(), async ctx => { + const userId = ctx.body.user; + const teamId = ctx.state.user.teamId; + ctx.assertPresent(userId, 'id is required'); - const team = await Team.findById(admin.teamId); - const promotedUser = await User.findOne({ - where: { id: user, teamId: admin.teamId }, - }); + const user = await User.findById(userId); + authorize(ctx.state.user, 'promote', user); - if (!promotedUser) throw httpErrors.NotFound(); + const team = await Team.findById(teamId); + await team.addAdmin(user); - await team.addAdmin(promotedUser); - - ctx.body = presentUser(ctx, promotedUser, { includeDetails: true }); + ctx.body = { + data: presentUser(ctx, user, { includeDetails: true }), + }; }); -router.post('team.removeAdmin', auth({ adminOnly: true }), async ctx => { - const { user } = ctx.body; - const admin = ctx.state.user; - ctx.assertPresent(user, 'id is required'); +router.post('team.removeAdmin', auth(), async ctx => { + const userId = ctx.body.user; + const teamId = ctx.state.user.teamId; + ctx.assertPresent(userId, 'id is required'); - const team = await Team.findById(admin.teamId); - const demotedUser = await User.findOne({ - where: { id: user, teamId: admin.teamId }, - }); + const user = await User.findById(userId); + authorize(ctx.state.user, 'demote', user); - if (!demotedUser) throw httpErrors.NotFound(); + const team = await Team.findById(teamId); try { - await team.removeAdmin(demotedUser); - ctx.body = presentUser(ctx, user, { includeDetails: true }); - } catch (e) { - throw httpErrors.BadRequest(e.message); + await team.removeAdmin(user); + } catch (err) { + throw httpErrors.BadRequest(err.message); } + + ctx.body = { + data: presentUser(ctx, user, { includeDetails: true }), + }; }); export default router; diff --git a/server/api/team.test.js b/server/api/team.test.js index 319d98521..36aa3765d 100644 --- a/server/api/team.test.js +++ b/server/api/team.test.js @@ -40,10 +40,7 @@ describe('#team.addAdmin', async () => { const { admin, user } = await seed(); const res = await server.post('/api/team.addAdmin', { - body: { - token: admin.getJwtToken(), - user: user.id, - }, + body: { token: admin.getJwtToken(), user: user.id }, }); const body = await res.json(); @@ -54,7 +51,7 @@ describe('#team.addAdmin', async () => { it('should require admin', async () => { const { user } = await seed(); const res = await server.post('/api/team.addAdmin', { - body: { token: user.getJwtToken() }, + body: { token: user.getJwtToken(), user: user.id }, }); const body = await res.json(); @@ -98,7 +95,7 @@ describe('#team.removeAdmin', async () => { it('should require admin', async () => { const { user } = await seed(); const res = await server.post('/api/team.addAdmin', { - body: { token: user.getJwtToken() }, + body: { token: user.getJwtToken(), user: user.id }, }); const body = await res.json(); diff --git a/server/models/Team.js b/server/models/Team.js index d1f61c47a..80dd76f12 100644 --- a/server/models/Team.js +++ b/server/models/Team.js @@ -42,13 +42,13 @@ Team.prototype.createFirstCollection = async function(userId) { }; Team.prototype.addAdmin = async function(user: User) { - return await user.update({ isAdmin: true }); + return user.update({ isAdmin: true }); }; Team.prototype.removeAdmin = async function(user: User) { const res = await User.findAndCountAll({ where: { - teamId: user.teamId, + teamId: this.id, isAdmin: true, id: { [Op.ne]: user.id, @@ -57,7 +57,7 @@ Team.prototype.removeAdmin = async function(user: User) { limit: 1, }); if (res.count >= 1) { - return await user.update({ isAdmin: false }); + return user.update({ isAdmin: false }); } else { throw new Error('At least one admin is required'); } diff --git a/server/policies/collection.js b/server/policies/collection.js new file mode 100644 index 000000000..57ceb7bd6 --- /dev/null +++ b/server/policies/collection.js @@ -0,0 +1,25 @@ +// @flow +import policy from './policy'; +import Collection from '../models/Collection'; +import User from '../models/User'; + +const { allow } = policy; + +allow(User, 'create', Collection); + +allow( + User, + ['read', 'update'], + Collection, + (user, collection) => collection && user.teamId === collection.teamId +); + +allow( + User, + 'delete', + Collection, + (user, collection) => + collection && + user.teamId === collection.teamId && + (user.id === collection.creatorId || user.isAdmin) +); diff --git a/server/policies/document.js b/server/policies/document.js index 0be7d4c38..70212b24a 100644 --- a/server/policies/document.js +++ b/server/policies/document.js @@ -5,9 +5,11 @@ import User from '../models/User'; const { allow } = policy; +allow(User, 'create', Document); + allow( User, - ['create', 'read', 'update', 'delete'], + ['read', 'update', 'delete'], Document, - (user, doc) => user.teamId === doc.teamId + (user, document) => user.teamId === document.teamId ); diff --git a/server/policies/index.js b/server/policies/index.js index 637227a44..fa94dec4d 100644 --- a/server/policies/index.js +++ b/server/policies/index.js @@ -1,5 +1,7 @@ // @flow import policy from './policy'; import './document'; +import './collection'; +import './user'; export default policy; diff --git a/server/policies/user.js b/server/policies/user.js new file mode 100644 index 000000000..8ca2cbdfb --- /dev/null +++ b/server/policies/user.js @@ -0,0 +1,29 @@ +// @flow +import policy from './policy'; +import User from '../models/User'; + +const { allow } = policy; + +allow( + User, + 'read', + User, + (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, + ['promote', 'demote'], + User, + (actor, user) => user && user.teamId === actor.teamId && actor.isAdmin +); diff --git a/services/slack/index.js b/services/slack/index.js index 578bd8ee7..606331179 100644 --- a/services/slack/index.js +++ b/services/slack/index.js @@ -3,7 +3,7 @@ import type { Event } from '../../server/events'; const Slack = { on: (event: Event) => { - console.log(`Slack service received ${event.name}, id: ${event.model.id}`); + // console.log(`Slack service received ${event.name}, id: ${event.model.id}`); }, }; From 83f32be6f7d3d3e8e3e1b1970b782821bf927ad3 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 18 Feb 2018 10:56:56 -0800 Subject: [PATCH 3/7] Add missing authorization on views endpoints Updated ApiKeys authorization to match elsewhere --- server/api/__snapshots__/views.test.js.snap | 10 +++ server/api/apiKeys.js | 11 ++- server/api/collections.test.js | 2 +- server/api/documents.test.js | 3 +- server/api/hooks.test.js | 2 +- server/api/middlewares/authentication.test.js | 2 +- server/api/team.js | 3 +- server/api/user.js | 2 +- server/api/views.js | 20 +++-- server/api/views.test.js | 73 +++++++++++++++++++ server/events.js | 3 +- server/models/Collection.test.js | 3 +- server/policies/apiKey.js | 14 ++++ server/policies/collection.js | 3 +- server/policies/document.js | 3 +- server/policies/index.js | 3 +- server/policies/user.js | 2 +- server/presenters/user.js | 2 +- server/test/factories.js | 3 +- 19 files changed, 129 insertions(+), 35 deletions(-) create mode 100644 server/api/__snapshots__/views.test.js.snap create mode 100644 server/api/views.test.js create mode 100644 server/policies/apiKey.js diff --git a/server/api/__snapshots__/views.test.js.snap b/server/api/__snapshots__/views.test.js.snap new file mode 100644 index 000000000..f3da465fc --- /dev/null +++ b/server/api/__snapshots__/views.test.js.snap @@ -0,0 +1,10 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`#views.list should require authentication 1`] = ` +Object { + "error": "authentication_required", + "message": "Authentication required", + "ok": false, + "status": 401, +} +`; diff --git a/server/api/apiKeys.js b/server/api/apiKeys.js index 4b6e5c841..3de47baad 100644 --- a/server/api/apiKeys.js +++ b/server/api/apiKeys.js @@ -6,7 +6,9 @@ import auth from './middlewares/authentication'; import pagination from './middlewares/pagination'; import { presentApiKey } from '../presenters'; import { ApiKey } from '../models'; +import policy from '../policies'; +const { authorize } = policy; const router = new Router(); router.post('apiKeys.create', auth(), async ctx => { @@ -14,6 +16,7 @@ router.post('apiKeys.create', auth(), async ctx => { ctx.assertPresent(name, 'name is required'); const user = ctx.state.user; + authorize(user, 'create', ApiKey); const key = await ApiKey.create({ name, @@ -36,9 +39,7 @@ router.post('apiKeys.list', auth(), pagination(), async ctx => { limit: ctx.state.pagination.limit, }); - const data = keys.map(key => { - return presentApiKey(ctx, key); - }); + const data = keys.map(key => presentApiKey(ctx, key)); ctx.body = { pagination: ctx.state.pagination, @@ -52,10 +53,8 @@ router.post('apiKeys.delete', auth(), async ctx => { const user = ctx.state.user; const key = await ApiKey.findById(id); + authorize(user, 'delete', ApiKey); - if (!key || key.userId !== user.id) throw httpErrors.BadRequest(); - - // Delete the actual document try { await key.destroy(); } catch (e) { diff --git a/server/api/collections.test.js b/server/api/collections.test.js index 16df1553c..58b79e9d3 100644 --- a/server/api/collections.test.js +++ b/server/api/collections.test.js @@ -3,7 +3,7 @@ import TestServer from 'fetch-test-server'; import app from '..'; import { flushdb, seed } from '../test/support'; import { buildUser } from '../test/factories'; -import Collection from '../models/Collection'; +import { Collection } from '../models'; const server = new TestServer(app.callback()); beforeEach(flushdb); diff --git a/server/api/documents.test.js b/server/api/documents.test.js index 24992fc50..17eab993b 100644 --- a/server/api/documents.test.js +++ b/server/api/documents.test.js @@ -1,10 +1,9 @@ /* eslint-disable flowtype/require-valid-file-annotation */ import TestServer from 'fetch-test-server'; import app from '..'; -import { View, Star } from '../models'; +import { Document, 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()); diff --git a/server/api/hooks.test.js b/server/api/hooks.test.js index 29c3053e7..b02835335 100644 --- a/server/api/hooks.test.js +++ b/server/api/hooks.test.js @@ -1,7 +1,7 @@ /* eslint-disable flowtype/require-valid-file-annotation */ import TestServer from 'fetch-test-server'; import app from '..'; -import Authentication from '../models/Authentication'; +import { Authentication } from '../models'; import { flushdb, seed } from '../test/support'; import * as Slack from '../slack'; diff --git a/server/api/middlewares/authentication.test.js b/server/api/middlewares/authentication.test.js index 734fcd1b8..2fec3842e 100644 --- a/server/api/middlewares/authentication.test.js +++ b/server/api/middlewares/authentication.test.js @@ -1,6 +1,6 @@ /* eslint-disable flowtype/require-valid-file-annotation */ import { flushdb, seed } from '../../test/support'; -import ApiKey from '../../models/ApiKey'; +import { ApiKey } from '../../models'; import randomstring from 'randomstring'; import auth from './authentication'; diff --git a/server/api/team.js b/server/api/team.js index 7821bee55..864dc2c36 100644 --- a/server/api/team.js +++ b/server/api/team.js @@ -2,8 +2,7 @@ import Router from 'koa-router'; import httpErrors from 'http-errors'; -import Team from '../models/Team'; -import User from '../models/User'; +import { Team, User } from '../models'; import auth from './middlewares/authentication'; import pagination from './middlewares/pagination'; diff --git a/server/api/user.js b/server/api/user.js index 77fdf41af..252bc9e94 100644 --- a/server/api/user.js +++ b/server/api/user.js @@ -2,7 +2,7 @@ import uuid from 'uuid'; import Router from 'koa-router'; import { makePolicy, signPolicy, publicS3Endpoint } from '../utils/s3'; -import Event from '../models/Event'; +import { Event } from '../models'; import auth from './middlewares/authentication'; import { presentUser } from '../presenters'; diff --git a/server/api/views.js b/server/api/views.js index 84a26b212..7e59794a9 100644 --- a/server/api/views.js +++ b/server/api/views.js @@ -1,24 +1,26 @@ // @flow import Router from 'koa-router'; -import httpErrors from 'http-errors'; import auth from './middlewares/authentication'; import { presentView } from '../presenters'; import { View, Document } from '../models'; +import policy from '../policies'; +const { authorize } = policy; const router = new Router(); router.post('views.list', auth(), async ctx => { const { id } = ctx.body; ctx.assertPresent(id, 'id is required'); + const user = ctx.state.user; + const document = await Document.findById(id); + authorize(user, 'read', document); + const views = await View.findAll({ - where: { - documentId: id, - }, + where: { documentId: id }, order: [['updatedAt', 'DESC']], }); - // Collectiones let users = []; let count = 0; await Promise.all( @@ -42,11 +44,13 @@ router.post('views.create', auth(), async ctx => { const user = ctx.state.user; const document = await Document.findById(id); - - if (!document || document.teamId !== user.teamId) - throw httpErrors.BadRequest(); + authorize(user, 'read', document); await View.increment({ documentId: document.id, userId: user.id }); + + ctx.body = { + success: true, + }; }); export default router; diff --git a/server/api/views.test.js b/server/api/views.test.js new file mode 100644 index 000000000..ae22fde94 --- /dev/null +++ b/server/api/views.test.js @@ -0,0 +1,73 @@ +/* eslint-disable flowtype/require-valid-file-annotation */ +import TestServer from 'fetch-test-server'; +import app from '..'; +import { flushdb, seed } from '../test/support'; +import { buildUser } from '../test/factories'; + +const server = new TestServer(app.callback()); + +beforeEach(flushdb); +afterAll(server.close); + +describe('#views.list', async () => { + it('should return views for a document', async () => { + const { user, document } = await seed(); + const res = await server.post('/api/views.list', { + body: { token: user.getJwtToken(), id: document.id }, + }); + expect(res.status).toEqual(200); + }); + + it('should require authentication', async () => { + const { document } = await seed(); + const res = await server.post('/api/views.list', { + body: { id: document.id }, + }); + 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/views.list', { + body: { token: user.getJwtToken(), id: document.id }, + }); + expect(res.status).toEqual(403); + }); +}); + +describe('#views.create', async () => { + it('should allow creating a view record for document', async () => { + const { user, document } = await seed(); + const res = await server.post('/api/views.create', { + body: { token: user.getJwtToken(), id: document.id }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.success).toBe(true); + }); + + it('should require authentication', async () => { + const { document } = await seed(); + const res = await server.post('/api/views.create', { + body: { id: document.id }, + }); + 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/views.create', { + body: { token: user.getJwtToken(), id: document.id }, + }); + expect(res.status).toEqual(403); + }); +}); diff --git a/server/events.js b/server/events.js index 26bf6cf5b..d2a28d601 100644 --- a/server/events.js +++ b/server/events.js @@ -2,8 +2,7 @@ import Queue from 'bull'; import debug from 'debug'; import services from '../services'; -import Document from './models/Document'; -import Collection from './models/Collection'; +import { Collection, Document } from './models'; type DocumentEvent = { name: 'documents.create', diff --git a/server/models/Collection.test.js b/server/models/Collection.test.js index 1327b0aeb..d728f9f86 100644 --- a/server/models/Collection.test.js +++ b/server/models/Collection.test.js @@ -1,7 +1,6 @@ /* eslint-disable flowtype/require-valid-file-annotation */ import { flushdb, seed } from '../test/support'; -import Collection from '../models/Collection'; -import Document from '../models/Document'; +import { Collection, Document } from '../models'; beforeEach(flushdb); beforeEach(jest.resetAllMocks); diff --git a/server/policies/apiKey.js b/server/policies/apiKey.js new file mode 100644 index 000000000..68c49d087 --- /dev/null +++ b/server/policies/apiKey.js @@ -0,0 +1,14 @@ +// @flow +import policy from './policy'; +import { ApiKey, User } from '../models'; + +const { allow } = policy; + +allow(User, 'create', ApiKey); + +allow( + User, + ['read', 'update', 'delete'], + ApiKey, + (user, apiKey) => user && user.id === apiKey.userId +); diff --git a/server/policies/collection.js b/server/policies/collection.js index 57ceb7bd6..ffc9a4226 100644 --- a/server/policies/collection.js +++ b/server/policies/collection.js @@ -1,7 +1,6 @@ // @flow import policy from './policy'; -import Collection from '../models/Collection'; -import User from '../models/User'; +import { Collection, User } from '../models'; const { allow } = policy; diff --git a/server/policies/document.js b/server/policies/document.js index 70212b24a..60f6afbbf 100644 --- a/server/policies/document.js +++ b/server/policies/document.js @@ -1,7 +1,6 @@ // @flow import policy from './policy'; -import Document from '../models/Document'; -import User from '../models/User'; +import { Document, User } from '../models'; const { allow } = policy; diff --git a/server/policies/index.js b/server/policies/index.js index fa94dec4d..94289a001 100644 --- a/server/policies/index.js +++ b/server/policies/index.js @@ -1,7 +1,8 @@ // @flow import policy from './policy'; -import './document'; +import './apiKey'; import './collection'; +import './document'; import './user'; export default policy; diff --git a/server/policies/user.js b/server/policies/user.js index 8ca2cbdfb..3e7361a4b 100644 --- a/server/policies/user.js +++ b/server/policies/user.js @@ -1,6 +1,6 @@ // @flow import policy from './policy'; -import User from '../models/User'; +import { User } from '../models'; const { allow } = policy; diff --git a/server/presenters/user.js b/server/presenters/user.js index 2b6ff1f74..99c6b17d8 100644 --- a/server/presenters/user.js +++ b/server/presenters/user.js @@ -1,5 +1,5 @@ // @flow -import User from '../models/User'; +import { User } from '../models'; type Options = { includeDetails?: boolean, diff --git a/server/test/factories.js b/server/test/factories.js index d88fb83c6..92e6fb5b1 100644 --- a/server/test/factories.js +++ b/server/test/factories.js @@ -1,6 +1,5 @@ // @flow -import Team from '../models/Team'; -import User from '../models/User'; +import { Team, User } from '../models'; import uuid from 'uuid'; let count = 0; From e3e084130c09fc1a8c10173db6648e8a2fcab449 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 18 Feb 2018 10:58:54 -0800 Subject: [PATCH 4/7] Add missing snap --- server/api/__snapshots__/views.test.js.snap | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/server/api/__snapshots__/views.test.js.snap b/server/api/__snapshots__/views.test.js.snap index f3da465fc..fa753dccf 100644 --- a/server/api/__snapshots__/views.test.js.snap +++ b/server/api/__snapshots__/views.test.js.snap @@ -1,5 +1,14 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`#views.create should require authentication 1`] = ` +Object { + "error": "authentication_required", + "message": "Authentication required", + "ok": false, + "status": 401, +} +`; + exports[`#views.list should require authentication 1`] = ` Object { "error": "authentication_required", From 7a0aa0ecf871eb89648727613534565fb4db59d3 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 18 Feb 2018 11:08:43 -0800 Subject: [PATCH 5/7] Add additional future-proofing auth checks for creation --- server/api/collections.js | 1 + server/api/documents.js | 3 +++ server/policies/collection.js | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/server/api/collections.js b/server/api/collections.js index d7a203f82..44b2add50 100644 --- a/server/api/collections.js +++ b/server/api/collections.js @@ -18,6 +18,7 @@ router.post('collections.create', auth(), async ctx => { ctx.assertHexColor(color, 'Invalid hex value (please use format #FFFFFF)'); const user = ctx.state.user; + authorize(user, 'create', Collection); const collection = await Collection.create({ name, diff --git a/server/api/documents.js b/server/api/documents.js index 58939d9c3..219a783da 100644 --- a/server/api/documents.js +++ b/server/api/documents.js @@ -198,12 +198,15 @@ router.post('documents.create', auth(), async ctx => { if (index) ctx.assertPositiveInteger(index, 'index must be an integer (>=0)'); const user = ctx.state.user; + authorize(user, 'create', Document); + const ownerCollection = await Collection.findOne({ where: { id: collection, teamId: user.teamId, }, }); + authorize(user, 'publish', ownerCollection); if (!ownerCollection) throw httpErrors.BadRequest(); diff --git a/server/policies/collection.js b/server/policies/collection.js index ffc9a4226..5bdb091a0 100644 --- a/server/policies/collection.js +++ b/server/policies/collection.js @@ -8,7 +8,7 @@ allow(User, 'create', Collection); allow( User, - ['read', 'update'], + ['read', 'publish', 'update'], Collection, (user, collection) => collection && user.teamId === collection.teamId ); From d73196594d30de32bec2d2b61d9631ce404d1df5 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 18 Feb 2018 13:10:17 -0800 Subject: [PATCH 6/7] Correct ApiKey delete auth check --- server/api/apiKeys.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/api/apiKeys.js b/server/api/apiKeys.js index 3de47baad..1dd33ba41 100644 --- a/server/api/apiKeys.js +++ b/server/api/apiKeys.js @@ -53,7 +53,7 @@ router.post('apiKeys.delete', auth(), async ctx => { const user = ctx.state.user; const key = await ApiKey.findById(id); - authorize(user, 'delete', ApiKey); + authorize(user, 'delete', key); try { await key.destroy(); From 5b6c90821505fe667c045afb97c9d0be7c7f74f3 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Mon, 19 Feb 2018 23:31:18 -0800 Subject: [PATCH 7/7] More granular error responses --- server/api/__snapshots__/team.test.js.snap | 12 +++++---- server/api/apiKeys.js | 7 +----- server/api/collections.js | 10 +++----- server/api/collections.test.js | 2 +- server/api/index.js | 8 +----- server/api/middlewares/validation.js | 18 +++++++------- server/api/team.js | 5 ++-- server/errors.js | 29 ++++++++++++++++++---- server/policies/collection.js | 15 +++++------ server/policies/user.js | 27 +++++++++----------- server/slack.js | 2 +- 11 files changed, 67 insertions(+), 68 deletions(-) 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';