From cebc93b9b0cf17c389fd27dd33b5b1d4ccabcad9 Mon Sep 17 00:00:00 2001
From: Matheus Barbosa Silva
 <36537004+matheusbsilva137@users.noreply.github.com>
Date: Tue, 7 May 2024 12:54:20 -0300
Subject: [PATCH] chore!: Improve permissions check on groups endpoints
 (#32332)

---
 apps/meteor/app/api/server/v1/groups.ts       | 31 +++++-----
 apps/meteor/tests/end-to-end/api/03-groups.js | 58 ++++++++++---------
 2 files changed, 44 insertions(+), 45 deletions(-)

diff --git a/apps/meteor/app/api/server/v1/groups.ts b/apps/meteor/app/api/server/v1/groups.ts
index 34deb57304fc..df03e38dc147 100644
--- a/apps/meteor/app/api/server/v1/groups.ts
+++ b/apps/meteor/app/api/server/v1/groups.ts
@@ -9,11 +9,7 @@ import { findUsersOfRoom } from '../../../../server/lib/findUsersOfRoom';
 import { hideRoomMethod } from '../../../../server/methods/hideRoom';
 import { removeUserFromRoomMethod } from '../../../../server/methods/removeUserFromRoom';
 import { canAccessRoomAsync, roomAccessAttributes } from '../../../authorization/server';
-import {
-	hasAllPermissionAsync,
-	hasAtLeastOnePermissionAsync,
-	hasPermissionAsync,
-} from '../../../authorization/server/functions/hasPermission';
+import { hasAllPermissionAsync, hasPermissionAsync } from '../../../authorization/server/functions/hasPermission';
 import { saveRoomSettings } from '../../../channel-settings/server/methods/saveRoomSettings';
 import { mountIntegrationQueryBasedOnPermissions } from '../../../integrations/server/lib/mountQueriesBasedOnPermission';
 import { createPrivateGroupMethod } from '../../../lib/server/methods/createPrivateGroup';
@@ -412,20 +408,22 @@ API.v1.addRoute(
 
 API.v1.addRoute(
 	'groups.getIntegrations',
-	{ authRequired: true },
 	{
-		async get() {
-			if (
-				!(await hasAtLeastOnePermissionAsync(this.userId, [
+		authRequired: true,
+		permissionsRequired: {
+			GET: {
+				permissions: [
 					'manage-outgoing-integrations',
 					'manage-own-outgoing-integrations',
 					'manage-incoming-integrations',
 					'manage-own-incoming-integrations',
-				]))
-			) {
-				return API.v1.unauthorized();
-			}
-
+				],
+				operation: 'hasAny',
+			},
+		},
+	},
+	{
+		async get() {
 			const findResult = await findPrivateGroupByIdOrName({
 				params: this.queryParams,
 				userId: this.userId,
@@ -670,12 +668,9 @@ API.v1.addRoute(
 
 API.v1.addRoute(
 	'groups.listAll',
-	{ authRequired: true },
+	{ authRequired: true, permissionsRequired: ['view-room-administration'] },
 	{
 		async get() {
-			if (!(await hasPermissionAsync(this.userId, 'view-room-administration'))) {
-				return API.v1.unauthorized();
-			}
 			const { offset, count } = await getPaginationItems(this.queryParams);
 			const { sort, fields, query } = await this.parseJsonQuery();
 			const ourQuery = Object.assign({}, query, { t: 'p' as RoomType });
diff --git a/apps/meteor/tests/end-to-end/api/03-groups.js b/apps/meteor/tests/end-to-end/api/03-groups.js
index 2bf6bc774999..0c025ef3b485 100644
--- a/apps/meteor/tests/end-to-end/api/03-groups.js
+++ b/apps/meteor/tests/end-to-end/api/03-groups.js
@@ -1143,33 +1143,37 @@ describe('[Groups]', function () {
 	});
 
 	describe('/groups.listAll', () => {
-		it('should fail if the user doesnt have view-room-administration permission', (done) => {
-			updatePermission('view-room-administration', []).then(() => {
-				request
-					.get(api('groups.listAll'))
-					.set(credentials)
-					.expect('Content-Type', 'application/json')
-					.expect(403)
-					.expect((res) => {
-						expect(res.body).to.have.property('success', false);
-						expect(res.body).to.have.property('error', 'unauthorized');
-					})
-					.end(done);
-			});
+		before(async () => {
+			return updatePermission('view-room-administration', ['admin']);
 		});
-		it('should succeed if user has view-room-administration permission', (done) => {
-			updatePermission('view-room-administration', ['admin']).then(() => {
-				request
-					.get(api('groups.listAll'))
-					.set(credentials)
-					.expect('Content-Type', 'application/json')
-					.expect(200)
-					.expect((res) => {
-						expect(res.body).to.have.property('success', true);
-						expect(res.body).to.have.property('groups').and.to.be.an('array');
-					})
-					.end(done);
-			});
+
+		after(async () => {
+			return updatePermission('view-room-administration', ['admin']);
+		});
+
+		it('should succeed if user has view-room-administration permission', async () => {
+			await request
+				.get(api('groups.listAll'))
+				.set(credentials)
+				.expect('Content-Type', 'application/json')
+				.expect(200)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', true);
+					expect(res.body).to.have.property('groups').and.to.be.an('array');
+				});
+		});
+
+		it('should fail if the user doesnt have view-room-administration permission', async () => {
+			await updatePermission('view-room-administration', []);
+			await request
+				.get(api('groups.listAll'))
+				.set(credentials)
+				.expect('Content-Type', 'application/json')
+				.expect(403)
+				.expect((res) => {
+					expect(res.body).to.have.property('success', false);
+					expect(res.body).to.have.property('error', 'User does not have the permissions required for this action [error-unauthorized]');
+				});
 		});
 	});
 
@@ -1333,7 +1337,7 @@ describe('[Groups]', function () {
 				.expect(403)
 				.expect((res) => {
 					expect(res.body).to.have.property('success', false);
-					expect(res.body).to.have.property('error', 'unauthorized');
+					expect(res.body).to.have.property('error', 'User does not have the permissions required for this action [error-unauthorized]');
 				});
 		});
 	});