From 83f32be6f7d3d3e8e3e1b1970b782821bf927ad3 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 18 Feb 2018 10:56:56 -0800 Subject: [PATCH] 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;