From 0e60f3d542c0f7dadf796bab5fca671b5a3cfbc7 Mon Sep 17 00:00:00 2001
From: Miguel de la Cruz <miguel@mcrx.me>
Date: Fri, 22 Dec 2023 10:29:51 +0100
Subject: [PATCH] Improves validation in getRolesByName endpoint (#25215)

* Improves validation in getRolesByName endpoint

* Updates the max constant and fixes linter

* Adds a mechanism to split roles in chunks in the webapp client

---------

Co-authored-by: Mattermost Build <build@mattermost.com>
---
 server/channels/api4/role.go                  |  9 +++++++++
 server/channels/api4/role_test.go             | 12 +++++++++++
 server/i18n/en.json                           |  4 ++++
 .../mattermost-redux/src/actions/roles.ts     | 20 ++++++++++++++++++-
 .../mattermost-redux/src/constants/general.ts |  1 +
 5 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/server/channels/api4/role.go b/server/channels/api4/role.go
index f658ebac35..ba499fe09c 100644
--- a/server/channels/api4/role.go
+++ b/server/channels/api4/role.go
@@ -12,6 +12,8 @@ import (
 	"github.com/mattermost/mattermost/server/v8/channels/audit"
 )
 
+const GetRolesByNamesMax = 100
+
 var notAllowedPermissions = []string{
 	model.PermissionSysconsoleWriteUserManagementSystemRoles.Id,
 	model.PermissionSysconsoleReadUserManagementSystemRoles.Id,
@@ -89,6 +91,13 @@ func getRolesByNames(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if len(rolenames) > GetRolesByNamesMax {
+		c.Err = model.NewAppError("getRolesByNames", "api.roles.get_multiple_by_name_too_many.request_error", map[string]any{
+			"MaxNames": GetRolesByNamesMax,
+		}, "", http.StatusBadRequest)
+		return
+	}
+
 	cleanedRoleNames, valid := model.CleanRoleNames(rolenames)
 	if !valid {
 		c.SetInvalidParam("rolename")
diff --git a/server/channels/api4/role_test.go b/server/channels/api4/role_test.go
index 3747be988f..dfcea31ee3 100644
--- a/server/channels/api4/role_test.go
+++ b/server/channels/api4/role_test.go
@@ -184,6 +184,18 @@ func TestGetRolesByNames(t *testing.T) {
 		_, _, err = client.GetRolesByNames(context.Background(), []string{model.NewId(), model.NewId(), "", "    "})
 		require.NoError(t, err)
 	})
+
+	th.TestForAllClients(t, func(t *testing.T, client *model.Client4) {
+		// too many roles should error with bad request
+		roles := []string{}
+		for i := 0; i < GetRolesByNamesMax+10; i++ {
+			roles = append(roles, role1.Name)
+		}
+
+		_, resp, err := client.GetRolesByNames(context.Background(), roles)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+	})
 }
 
 func TestPatchRole(t *testing.T) {
diff --git a/server/i18n/en.json b/server/i18n/en.json
index a88a43e8d8..27441ecf20 100644
--- a/server/i18n/en.json
+++ b/server/i18n/en.json
@@ -2682,6 +2682,10 @@
     "id": "api.restricted_system_admin",
     "translation": "This action is forbidden to a restricted system admin."
   },
+  {
+    "id": "api.roles.get_multiple_by_name_too_many.request_error",
+    "translation": "Unable to get that many roles by name. Only {{.MaxNames}} roles can be requested at once."
+  },
   {
     "id": "api.roles.patch_roles.license.error",
     "translation": "Your license does not support advanced permissions."
diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/roles.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/roles.ts
index 8673624eca..f491ed68e6 100644
--- a/webapp/channels/src/packages/mattermost-redux/src/actions/roles.ts
+++ b/webapp/channels/src/packages/mattermost-redux/src/actions/roles.ts
@@ -10,6 +10,8 @@ import type {DispatchFunc, GetStateFunc, ActionFunc} from 'mattermost-redux/type
 
 import {bindClientFunc} from './helpers';
 
+import {General} from '../constants';
+
 export function getRolesByNames(rolesNames: string[]) {
     return bindClientFunc({
         clientFunc: Client4.getRolesByNames,
@@ -98,8 +100,24 @@ export function loadRolesIfNeeded(roles: Iterable<string>): ActionFunc {
         if (state.entities.roles.pending) {
             await dispatch(setPendingRoles([]));
         }
+
         if (newRoles.size > 0) {
-            return getRolesByNames(Array.from(newRoles))(dispatch, getState);
+            const newRolesArray = Array.from(newRoles);
+            const getRolesRequests = [];
+
+            for (let i = 0; i < newRolesArray.length; i += General.MAX_GET_ROLES_BY_NAMES) {
+                const chunk = newRolesArray.slice(i, i + General.MAX_GET_ROLES_BY_NAMES);
+                getRolesRequests.push(getRolesByNames(chunk)(dispatch, getState));
+            }
+
+            const result = await Promise.all(getRolesRequests);
+            return result.reduce(
+                (acc: Record<string, any>, val: Record<string, any>): Record<string, any> => {
+                    acc.data = acc.data.concat(val.data);
+                    return acc;
+                },
+                {data: []},
+            );
         }
         return {data: state.entities.roles.roles};
     };
diff --git a/webapp/channels/src/packages/mattermost-redux/src/constants/general.ts b/webapp/channels/src/packages/mattermost-redux/src/constants/general.ts
index 6cbcd40ea2..776620d9b2 100644
--- a/webapp/channels/src/packages/mattermost-redux/src/constants/general.ts
+++ b/webapp/channels/src/packages/mattermost-redux/src/constants/general.ts
@@ -76,4 +76,5 @@ export default {
     ALWAYS_ON: 'always_on',
     DEFAULT_GROUP: 'board',
     CUSTOM_GROUP_USER_ROLE: 'custom_group_user',
+    MAX_GET_ROLES_BY_NAMES: 100,
 };