From e84fb5e6babe150893050a4229e06bfaf7f5035c Mon Sep 17 00:00:00 2001
From: Tom Moor <tom.moor@gmail.com>
Date: Sun, 18 Feb 2018 01:14:51 -0800
Subject: [PATCH] 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 cda46a76..1aec00e8 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 bfe0d589..0891a3b8 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 45ddea40..7ad43a51 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 e7964c10..d7a203f8 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 6f6b320c..16df1553 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 3220b727..24992fc5 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 8bab85c3..750427ad 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 a40a684d..cde5c53e 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 4e8d7dc9..734fcd1b 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 98d63dee..7821bee5 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 319d9852..36aa3765 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 d1f61c47..80dd76f1 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 00000000..57ceb7bd
--- /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 0be7d4c3..70212b24 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 637227a4..fa94dec4 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 00000000..8ca2cbdf
--- /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 578bd8ee..60633117 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}`);
   },
 };