Merge pull request #4305 from weseek/imprv/refactor-slack-integration-checkcommandpermission

Imprv/refactor slack integration checkcommandpermission

packages/app/src/server/routes/apiv3/slack-integration.js

@@ -4,12 +4,13 @@ const express = require('express');
 const mongoose = require('mongoose');
 const urljoin = require('url-join');
 
-const { verifySlackRequest, generateWebClient, getSupportedGrowiActionsRegExps } = require('@growi/slack');
+const { verifySlackRequest, parseSlashCommand } = require('@growi/slack');
 
 const logger = loggerFactory('growi:routes:apiv3:slack-integration');
 const router = express.Router();
 const SlackAppIntegration = mongoose.model('SlackAppIntegration');
 const { respondIfSlackbotError } = require('../../service/slack-command-handler/respond-if-slackbot-error');
+const { checkPermission } = require('../../util/slack-integration');
 
 module.exports = (crowi) => {
   this.app = crowi.express;
@@ -44,32 +45,39 @@ module.exports = (crowi) => {
     next();
   }
 
-  async function checkCommandPermission(req, res, next) {
-    let payload;
-    if (req.body.payload) {
-      payload = JSON.parse(req.body.payload);
-    }
-    if (req.body.text == null && !payload) { // when /relation-test
-      return next();
-    }
-
-    const tokenPtoG = req.headers['x-growi-ptog-tokens'];
-
+  async function extractPermissionsCommands(tokenPtoG) {
     const slackAppIntegration = await SlackAppIntegration.findOne({ tokenPtoG });
     const permissionsForBroadcastUseCommands = slackAppIntegration.permissionsForBroadcastUseCommands;
     const permissionsForSingleUseCommands = slackAppIntegration.permissionsForSingleUseCommands;
 
-    // get command name from req.body
-    let command = '';
+    return { permissionsForBroadcastUseCommands, permissionsForSingleUseCommands };
+  }
+
+
+  async function checkCommandsPermission(req, res, next) {
+    if (req.body.text == null) return next(); // when /relation-test
+
+    const tokenPtoG = req.headers['x-growi-ptog-tokens'];
+    const { permissionsForBroadcastUseCommands, permissionsForSingleUseCommands } = await extractPermissionsCommands(tokenPtoG);
+    const commandPermission = Object.fromEntries([...permissionsForBroadcastUseCommands, ...permissionsForSingleUseCommands]);
+
+    const command = parseSlashCommand(req.body).growiCommandType;
+    const fromChannel = req.body.channel_name;
+    const isPermitted = checkPermission(commandPermission, command, fromChannel);
+    if (isPermitted) return next();
+
+    return res.status(403).send(`It is not allowed to run '${command}' command to this GROWI.`);
+  }
+
+  async function checkInteractionsPermission(req, res, next) {
+    const payload = JSON.parse(req.body.payload);
+    if (payload == null) return next(); // when /relation-test
+
     let actionId = '';
     let callbackId = '';
     let fromChannel = '';
 
-    if (!payload) { // when request is to /commands
-      command = req.body.text.split(' ')[0];
-      fromChannel = req.body.channel_name;
-    }
-    else if (payload.actions) { // when request is to /interactions && block_actions
+    if (payload.actions) { // when request is to /interactions && block_actions
       actionId = payload.actions[0].action_id;
       fromChannel = payload.channel.name;
     }
@@ -78,37 +86,15 @@ module.exports = (crowi) => {
       fromChannel = JSON.parse(payload.view.private_metadata).channelName;
     }
 
-    // code below checks permission at channel level
-    let isPermitted = false;
-    [...permissionsForBroadcastUseCommands.keys(), ...permissionsForSingleUseCommands.keys()].forEach((commandName) => {
-      // boolean or string[]
-      let permission = permissionsForBroadcastUseCommands.get(commandName);
-      if (permission === undefined) {
-        permission = permissionsForSingleUseCommands.get(commandName);
-      }
-
-      // ex. search OR search:handlerName
-      const commandRegExp = new RegExp(`(^${commandName}$)|(^${commandName}:\\w+)`);
-
-      // skip this forEach loop if the requested command is not in permissionsForBroadcastUseCommands key
-      if (!commandRegExp.test(command) && !commandRegExp.test(actionId) && !commandRegExp.test(callbackId)) {
-        return;
-      }
+    const tokenPtoG = req.headers['x-growi-ptog-tokens'];
+    const { permissionsForBroadcastUseCommands, permissionsForSingleUseCommands } = await extractPermissionsCommands(tokenPtoG);
+    const commandPermission = Object.fromEntries([...permissionsForBroadcastUseCommands, ...permissionsForSingleUseCommands]);
+    const callbacIdkOrActionId = callbackId || actionId;
 
-      // permission check
-      if (permission === true) {
-        isPermitted = true;
-        return;
-      }
-      if (Array.isArray(permission) && permission.includes(fromChannel)) {
-        isPermitted = true;
-      }
-    });
+    const isPermitted = checkPermission(commandPermission, callbacIdkOrActionId, fromChannel);
+    if (isPermitted) return next();
 
-    if (isPermitted) {
-      return next();
-    }
-    res.status(403).send(`It is not allowed to run '${command}' command to this GROWI.`);
+    res.status(403).send('It is not allowed to run  command to this GROWI.');
   }
 
   const addSigningSecretToReq = (req, res, next) => {
@@ -151,7 +137,7 @@ module.exports = (crowi) => {
     return handleCommands(req, res, client);
   });
 
-  router.post('/proxied/commands', verifyAccessTokenFromProxy, checkCommandPermission, async(req, res) => {
+  router.post('/proxied/commands', verifyAccessTokenFromProxy, checkCommandsPermission, async(req, res) => {
     const { body } = req;
     // eslint-disable-next-line max-len
     // see: https://api.slack.com/apis/connections/events-api#the-events-api__subscribing-to-event-types__events-api-request-urls__request-url-configuration--verification
@@ -206,7 +192,7 @@ module.exports = (crowi) => {
     return handleInteractions(req, res, client);
   });
 
-  router.post('/proxied/interactions', verifyAccessTokenFromProxy, checkCommandPermission, async(req, res) => {
+  router.post('/proxied/interactions', verifyAccessTokenFromProxy, checkInteractionsPermission, async(req, res) => {
     const tokenPtoG = req.headers['x-growi-ptog-tokens'];
     const client = await slackIntegrationService.generateClientByTokenPtoG(tokenPtoG);
 

packages/app/src/server/util/slack-integration.ts

@@ -0,0 +1,27 @@
+type CommandPermission = { [key:string]: string[] | boolean }
+
+export const checkPermission = (
+    commandPermission:CommandPermission, commandOrActionIdOrCallbackId:string, fromChannel:string,
+):boolean => {
+  let isPermitted = false;
+
+  Object.entries(commandPermission).forEach((entry) => {
+    const [command, value] = entry;
+    const permission = value;
+    const commandRegExp = new RegExp(`(^${command}$)|(^${command}:\\w+)`);
+
+    if (!commandRegExp.test(commandOrActionIdOrCallbackId)) return;
+
+    // permission check
+    if (permission === true) {
+      isPermitted = true;
+      return;
+    }
+    if (Array.isArray(permission) && permission.includes(fromChannel)) {
+      isPermitted = true;
+      return;
+    }
+  });
+
+  return isPermitted;
+};

packages/slackbot-proxy/src/services/RelationsService.ts

@@ -11,14 +11,14 @@ import loggerFactory from '~/utils/logger';
 
 const logger = loggerFactory('slackbot-proxy:services:RelationsService');
 
-type checkPermissionForInteractionsResults = {
+type CheckPermissionForInteractionsResults = {
   allowedRelations:Relation[],
   disallowedGrowiUrls:Set<string>,
   commandName:string,
   rejectedResults:PromiseRejectedResult[]
 }
 
-type checkEachRelationResult = {
+type CheckEachRelationResult = {
   allowedRelation:Relation|null,
   disallowedGrowiUrl:string|null,
   eachRelationCommandName:string,
@@ -122,7 +122,7 @@ export class RelationsService {
 
   async checkPermissionForInteractions(
       relations:Relation[], actionId:string, callbackId:string, channelName:string,
-  ):Promise<checkPermissionForInteractionsResults> {
+  ):Promise<CheckPermissionForInteractionsResults> {
 
     const allowedRelations:Relation[] = [];
     const disallowedGrowiUrls:Set<string> = new Set();
@@ -150,7 +150,7 @@ export class RelationsService {
     };
   }
 
-  checkEachRelation(relation:Relation, actionId:string, callbackId:string, channelName:string):checkEachRelationResult {
+  checkEachRelation(relation:Relation, actionId:string, callbackId:string, channelName:string):CheckEachRelationResult {
 
     let allowedRelation:Relation|null = null;
     let disallowedGrowiUrl:string|null = null;