chore: Improve permissions check on LDAP endpoints (#32335)

RocketChat · Jul 26, 2024 · 1c799e2 · 1c799e2
1 parent b3685bb
commit 1c799e2
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 13 deletions.
diff --git a/apps/meteor/app/api/server/v1/ldap.ts b/apps/meteor/app/api/server/v1/ldap.ts
@@ -2,23 +2,18 @@ import { LDAP } from '@rocket.chat/core-services';
 import { Match, check } from 'meteor/check';
 
 import { SystemLogger } from '../../../../server/lib/logger/system';
-import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission';
 import { settings } from '../../../settings/server';
 import { API } from '../api';
 
 API.v1.addRoute(
 	'ldap.testConnection',
-	{ authRequired: true },
+	{ authRequired: true, permissionsRequired: ['test-admin-options'] },
 	{
 		async post() {
 			if (!this.userId) {
 				throw new Error('error-invalid-user');
 			}
 
-			if (!(await hasPermissionAsync(this.userId, 'test-admin-options'))) {
-				throw new Error('error-not-authorized');
-			}
-
 			if (settings.get<boolean>('LDAP_Enable') !== true) {
 				throw new Error('LDAP_disabled');
 			}
@@ -39,7 +34,7 @@ API.v1.addRoute(
 
 API.v1.addRoute(
 	'ldap.testSearch',
-	{ authRequired: true },
+	{ authRequired: true, permissionsRequired: ['test-admin-options'] },
 	{
 		async post() {
 			check(
@@ -53,10 +48,6 @@ API.v1.addRoute(
 				throw new Error('error-invalid-user');
 			}
 
-			if (!(await hasPermissionAsync(this.userId, 'test-admin-options'))) {
-				throw new Error('error-not-authorized');
-			}
-
 			if (settings.get('LDAP_Enable') !== true) {
 				throw new Error('LDAP_disabled');
 			}

diff --git a/apps/meteor/tests/end-to-end/api/LDAP.ts b/apps/meteor/tests/end-to-end/api/LDAP.ts
@@ -1,10 +1,12 @@
 import { expect } from 'chai';
-import { before, describe, it } from 'mocha';
+import { before, after, describe, it } from 'mocha';
 import type { Response } from 'supertest';
 
 import { getCredentials, api, request, credentials } from '../../data/api-data';
+import { updatePermission } from '../../data/permissions.helper';
 
-describe('LDAP', () => {
+describe('LDAP', function () {
+	this.retries(0);
 	before((done) => getCredentials(done));
 
 	describe('[/ldap.syncNow]', () => {
@@ -42,4 +44,53 @@ describe('LDAP', () => {
 				});
 		});
 	});
+
+	describe('[/ldap.testSearch]', () => {
+		before(async () => {
+			return updatePermission('test-admin-options', ['admin']);
+		});
+
+		after(async () => {
+			return updatePermission('test-admin-options', ['admin']);
+		});
+
+		it('should not allow testing LDAP search if user does NOT have the test-admin-options permission', async () => {
+			await updatePermission('test-admin-options', []);
+			await request
+				.post(api('ldap.testSearch'))
+				.set(credentials)
+				.send({
+					username: 'test-search',
+				})
+				.expect('Content-Type', 'application/json')
+				.expect(403)
+				.expect((res: Response) => {
+					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]');
+				});
+		});
+	});
+
+	describe('[/ldap.testConnection]', () => {
+		before(async () => {
+			return updatePermission('test-admin-options', ['admin']);
+		});
+
+		after(async () => {
+			return updatePermission('test-admin-options', ['admin']);
+		});
+
+		it('should not allow testing LDAP connection if user does NOT have the test-admin-options permission', async () => {
+			await updatePermission('test-admin-options', []);
+			await request
+				.post(api('ldap.testConnection'))
+				.set(credentials)
+				.expect('Content-Type', 'application/json')
+				.expect(403)
+				.expect((res: Response) => {
+					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]');
+				});
+		});
+	});
 });