fix: Unknown Slack users should be able to search team accessible docs (#1049)

* fix: Unknown Slack users should be able to search team accessible docs * test: fix flaky test * test: remove obsolete snapshot * lint * flow * fix: Spelling mistake
2019-09-22 11:52:15 -07:00
parent d46530a4a0
commit b1a1d24f9c
6 changed files with 179 additions and 111 deletions
--- a/server/api/snapshots/users.test.js.snap
+++ b/server/api/snapshots/users.test.js.snap
@@ -70,72 +70,6 @@ Object {
 }
 `;

-exports[`#users.list should require admin for detailed info 1`] = `
-Object {
-  "data": Array [
-    Object {
-      "avatarUrl": "http://example.com/avatar.png",
-      "createdAt": "2018-01-02T00:00:00.000Z",
-      "id": "46fde1d4-0050-428f-9f0b-0bf77f4bdf61",
-      "isAdmin": false,
-      "isSuspended": false,
-      "lastActiveAt": null,
-      "name": "User 1",
-    },
-    Object {
-      "avatarUrl": "http://example.com/avatar.png",
-      "createdAt": "2018-01-01T00:00:00.000Z",
-      "id": "fa952cff-fa64-4d42-a6ea-6955c9689046",
-      "isAdmin": true,
-      "isSuspended": false,
-      "lastActiveAt": null,
-      "name": "Admin User",
-    },
-  ],
-  "ok": true,
-  "pagination": Object {
-    "limit": 15,
-    "nextPath": "/api/users.list?limit=15&offset=15",
-    "offset": 0,
-  },
-  "status": 200,
-}
-`;
-
-exports[`#users.list should return teams paginated user list 1`] = `
-Object {
-  "data": Array [
-    Object {
-      "avatarUrl": "http://example.com/avatar.png",
-      "createdAt": "2018-01-02T00:00:00.000Z",
-      "email": "user1@example.com",
-      "id": "46fde1d4-0050-428f-9f0b-0bf77f4bdf61",
-      "isAdmin": false,
-      "isSuspended": false,
-      "lastActiveAt": null,
-      "name": "User 1",
-    },
-    Object {
-      "avatarUrl": "http://example.com/avatar.png",
-      "createdAt": "2018-01-01T00:00:00.000Z",
-      "email": "admin@example.com",
-      "id": "fa952cff-fa64-4d42-a6ea-6955c9689046",
-      "isAdmin": true,
-      "isSuspended": false,
-      "lastActiveAt": null,
-      "name": "Admin User",
-    },
-  ],
-  "ok": true,
-  "pagination": Object {
-    "limit": 15,
-    "nextPath": "/api/users.list?limit=15&offset=15",
-    "offset": 0,
-  },
-  "status": 200,
-}
-`;
-
 exports[`#users.promote should promote a new admin 1`] = `
 Object {
  "data": Object {
@@ -204,4 +138,4 @@ Object {
  "ok": false,
  "status": 401,
 }
-`;
+`;
--- a/server/api/hooks.js
+++ b/server/api/hooks.js
@@ -58,15 +58,18 @@ router.post('hooks.interactive', async ctx => {
  ctx.assertPresent(token, 'token is required');
  ctx.assertPresent(callback_id, 'callback_id is required');

-  if (token !== process.env.SLACK_VERIFICATION_TOKEN)
+  if (token !== process.env.SLACK_VERIFICATION_TOKEN) {
    throw new AuthenticationError('Invalid verification token');
+  }

-  const user = await User.findOne({
-    where: { service: 'slack', serviceId: data.user.id },
+  const team = await Team.findOne({
+    where: { slackId: data.team.id },
  });
-  if (!user) {
+
+  if (!team) {
    ctx.body = {
-      text: 'Sorry, we couldn’t find your user on this team in Outline.',
+      text:
+        'Sorry, we couldn’t find an integration for your team. Head to your Outline settings to set one up.',
      response_type: 'ephemeral',
      replace_original: false,
    };
@@ -75,12 +78,13 @@ router.post('hooks.interactive', async ctx => {

  // we find the document based on the users teamId to ensure access
  const document = await Document.findOne({
-    where: { id: data.callback_id, teamId: user.teamId },
+    where: {
+      id: data.callback_id,
+      teamId: team.id,
+    },
  });
  if (!document) throw new InvalidRequestError('Invalid document');

-  const team = await Team.findByPk(user.teamId);
-
  // respond with a public message that will be posted in the original channel
  ctx.body = {
    response_type: 'in_channel',
@@ -93,8 +97,9 @@ router.post('hooks.interactive', async ctx => {

 // triggered by the /outline command in Slack
 router.post('hooks.slack', async ctx => {
-  const { token, user_id, text } = ctx.body;
+  const { token, team_id, user_id, text } = ctx.body;
  ctx.assertPresent(token, 'token is required');
+  ctx.assertPresent(team_id, 'team_id is required');
  ctx.assertPresent(user_id, 'user_id is required');

  if (token !== process.env.SLACK_VERIFICATION_TOKEN) {
@@ -116,25 +121,33 @@ router.post('hooks.slack', async ctx => {
    return;
  }

-  const user = await User.findOne({
-    where: {
-      service: 'slack',
-      serviceId: user_id,
-    },
+  const team = await Team.findOne({
+    where: { slackId: team_id },
  });
-  if (!user) {
+  if (!team) {
    ctx.body = {
      response_type: 'ephemeral',
-      text: 'Sorry, we couldn’t find your user – have you signed into Outline?',
+      text:
+        'Sorry, we couldn’t find an integration for your team. Head to your Outline settings to set one up.',
    };
    return;
  }

-  const team = await Team.findByPk(user.teamId);
-  const results = await Document.searchForUser(user, text, {
-    limit: 5,
+  const user = await User.findOne({
+    where: {
+      teamId: team.id,
+      service: 'slack',
+      serviceId: user_id,
+    },
  });

+  const options = {
+    limit: 5,
+  };
+  const results = user
+    ? await Document.searchForUser(user, text, options)
+    : await Document.searchForTeam(team, text, options);
+
  if (results.length) {
    const attachments = [];
    for (const result of results) {
--- a/server/api/hooks.test.js
+++ b/server/api/hooks.test.js
@@ -3,7 +3,7 @@ import TestServer from 'fetch-test-server';
 import app from '../app';
 import { Authentication } from '../models';
 import { flushdb, seed } from '../test/support';
-import { buildDocument, buildUser } from '../test/factories';
+import { buildDocument } from '../test/factories';
 import * as Slack from '../slack';

 const server = new TestServer(app.callback());
@@ -51,12 +51,13 @@ describe('#hooks.unfurl', async () => {

 describe('#hooks.slack', async () => {
  it('should return no matches', async () => {
-    const { user } = await seed();
+    const { user, team } = await seed();

    const res = await server.post('/api/hooks.slack', {
      body: {
        token: process.env.SLACK_VERIFICATION_TOKEN,
        user_id: user.serviceId,
+        team_id: team.slackId,
        text: 'dsfkndfskndsfkn',
      },
    });
@@ -66,7 +67,7 @@ describe('#hooks.slack', async () => {
  });

  it('should return search results with summary if query is in title', async () => {
-    const user = await buildUser();
+    const { user, team } = await seed();
    const document = await buildDocument({
      title: 'This title contains a search term',
      userId: user.id,
@@ -76,6 +77,7 @@ describe('#hooks.slack', async () => {
      body: {
        token: process.env.SLACK_VERIFICATION_TOKEN,
        user_id: user.serviceId,
+        team_id: team.slackId,
        text: 'contains',
      },
    });
@@ -87,7 +89,7 @@ describe('#hooks.slack', async () => {
  });

  it('should return search results if query is regex-like', async () => {
-    const user = await buildUser();
+    const { user, team } = await seed();
    await buildDocument({
      title: 'This title contains a search term',
      userId: user.id,
@@ -97,6 +99,7 @@ describe('#hooks.slack', async () => {
      body: {
        token: process.env.SLACK_VERIFICATION_TOKEN,
        user_id: user.serviceId,
+        team_id: team.slackId,
        text: '*contains',
      },
    });
@@ -106,7 +109,7 @@ describe('#hooks.slack', async () => {
  });

  it('should return search results with snippet if query is in text', async () => {
-    const user = await buildUser();
+    const { user, team } = await seed();
    const document = await buildDocument({
      text: 'This title contains a search term',
      userId: user.id,
@@ -116,6 +119,7 @@ describe('#hooks.slack', async () => {
      body: {
        token: process.env.SLACK_VERIFICATION_TOKEN,
        user_id: user.serviceId,
+        team_id: team.slackId,
        text: 'contains',
      },
    });
@@ -129,11 +133,12 @@ describe('#hooks.slack', async () => {
  });

  it('should respond with help content for help keyword', async () => {
-    const user = await buildUser();
+    const { user, team } = await seed();
    const res = await server.post('/api/hooks.slack', {
      body: {
        token: process.env.SLACK_VERIFICATION_TOKEN,
        user_id: user.serviceId,
+        team_id: team.slackId,
        text: 'help',
      },
    });
@@ -143,11 +148,12 @@ describe('#hooks.slack', async () => {
  });

  it('should respond with help content for no keyword', async () => {
-    const user = await buildUser();
+    const { user, team } = await seed();
    const res = await server.post('/api/hooks.slack', {
      body: {
        token: process.env.SLACK_VERIFICATION_TOKEN,
        user_id: user.serviceId,
+        team_id: team.slackId,
        text: '',
      },
    });
@@ -156,26 +162,46 @@ describe('#hooks.slack', async () => {
    expect(body.text.includes('How to use')).toEqual(true);
  });

-  it('should respond with error if unknown user', async () => {
+  it('should return search results with snippet for unknown user', async () => {
+    const { user, team } = await seed();
+
+    // unpublished document will not be returned
+    await buildDocument({
+      text: 'This title contains a search term',
+      userId: user.id,
+      teamId: user.teamId,
+      publishedAt: null,
+    });
+
+    const document = await buildDocument({
+      text: 'This title contains a search term',
+      userId: user.id,
+      teamId: user.teamId,
+    });
    const res = await server.post('/api/hooks.slack', {
      body: {
        token: process.env.SLACK_VERIFICATION_TOKEN,
-        user_id: 'not-a-user-id',
-        text: 'Welcome',
+        user_id: 'unknown-slack-user-id',
+        team_id: team.slackId,
+        text: 'contains',
      },
    });
    const body = await res.json();
    expect(res.status).toEqual(200);
-    expect(body.text.includes('Sorry')).toEqual(true);
-    expect(body.attachments).toEqual(undefined);
+    expect(body.attachments.length).toEqual(1);
+    expect(body.attachments[0].title).toEqual(document.title);
+    expect(body.attachments[0].text).toEqual(
+      'This title *contains* a search term'
+    );
  });

  it('should error if incorrect verification token', async () => {
-    const { user } = await seed();
+    const { user, team } = await seed();

    const res = await server.post('/api/hooks.slack', {
      body: {
        token: 'wrong-verification-token',
+        team_id: team.slackId,
        user_id: user.serviceId,
        text: 'Welcome',
      },
@@ -186,7 +212,7 @@ describe('#hooks.slack', async () => {

 describe('#hooks.interactive', async () => {
  it('should respond with replacement message', async () => {
-    const user = await buildUser();
+    const { user, team } = await seed();
    const document = await buildDocument({
      title: 'This title contains a search term',
      userId: user.id,
@@ -195,7 +221,8 @@ describe('#hooks.interactive', async () => {

    const payload = JSON.stringify({
      token: process.env.SLACK_VERIFICATION_TOKEN,
-      user: { id: user.serviceId, name: user.name },
+      user: { id: user.serviceId },
+      team: { id: team.slackId },
      callback_id: document.id,
    });
    const res = await server.post('/api/hooks.interactive', {
@@ -208,19 +235,28 @@ describe('#hooks.interactive', async () => {
    expect(body.attachments[0].title).toEqual(document.title);
  });

-  it('should respond with error if unknown user', async () => {
+  it('should respond with replacement message if unknown user', async () => {
+    const { user, team } = await seed();
+    const document = await buildDocument({
+      title: 'This title contains a search term',
+      userId: user.id,
+      teamId: user.teamId,
+    });
+
    const payload = JSON.stringify({
      token: process.env.SLACK_VERIFICATION_TOKEN,
-      user: { id: 'not-a-user-id', name: 'unknown' },
-      callback_id: 'doesnt-matter',
+      user: { id: 'unknown-slack-user-id' },
+      team: { id: team.slackId },
+      callback_id: document.id,
    });
    const res = await server.post('/api/hooks.interactive', {
      body: { payload },
    });
    const body = await res.json();
    expect(res.status).toEqual(200);
-    expect(body.text.includes('Sorry')).toEqual(true);
-    expect(body.attachments).toEqual(undefined);
+    expect(body.response_type).toEqual('in_channel');
+    expect(body.attachments.length).toEqual(1);
+    expect(body.attachments[0].title).toEqual(document.title);
  });

  it('should error if incorrect verification token', async () => {
--- a/server/api/users.test.js
+++ b/server/api/users.test.js
@@ -12,7 +12,7 @@ afterAll(server.close);

 describe('#users.list', async () => {
  it('should return teams paginated user list', async () => {
-    const { admin } = await seed();
+    const { admin, user } = await seed();

    const res = await server.post('/api/users.list', {
      body: { token: admin.getJwtToken() },
@@ -20,18 +20,24 @@ describe('#users.list', async () => {
    const body = await res.json();

    expect(res.status).toEqual(200);
-    expect(body).toMatchSnapshot();
+    expect(body.data.length).toEqual(2);
+    expect(body.data[0].id).toEqual(user.id);
+    expect(body.data[1].id).toEqual(admin.id);
  });

  it('should require admin for detailed info', async () => {
-    const { user } = await seed();
+    const { user, admin } = await seed();
    const res = await server.post('/api/users.list', {
      body: { token: user.getJwtToken() },
    });
    const body = await res.json();

    expect(res.status).toEqual(200);
-    expect(body).toMatchSnapshot();
+    expect(body.data.length).toEqual(2);
+    expect(body.data[0].email).toEqual(undefined);
+    expect(body.data[1].email).toEqual(undefined);
+    expect(body.data[0].id).toEqual(user.id);
+    expect(body.data[1].id).toEqual(admin.id);
  });
 });