From 0c72d96bc60a33f37fd82897cf3537ef859072fb 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 1/2] 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 07b03494900f..d6aabb9a60f3 100644 --- a/apps/meteor/tests/end-to-end/api/03-groups.js +++ b/apps/meteor/tests/end-to-end/api/03-groups.js @@ -1070,33 +1070,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]'); + }); }); }); @@ -1260,7 +1264,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]'); }); }); }); From c68e1e803dd522f69e19ab4b26f5c2058d0cf74b Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Tue, 7 May 2024 15:13:42 -0300 Subject: [PATCH 2/2] chore!: Improve permissions check on integrations endpoints (#32355) --- apps/meteor/app/api/server/v1/integrations.ts | 54 ++++++++------- .../api/06-outgoing-integrations.js | 69 +++++++++---------- .../api/07-incoming-integrations.js | 41 +++++------ 3 files changed, 81 insertions(+), 83 deletions(-) diff --git a/apps/meteor/app/api/server/v1/integrations.ts b/apps/meteor/app/api/server/v1/integrations.ts index f7eb9e5a1467..5306e7fbbea3 100644 --- a/apps/meteor/app/api/server/v1/integrations.ts +++ b/apps/meteor/app/api/server/v1/integrations.ts @@ -11,7 +11,6 @@ import { Match, check } from 'meteor/check'; import { Meteor } from 'meteor/meteor'; import type { Filter } from 'mongodb'; -import { hasAtLeastOnePermissionAsync } from '../../../authorization/server/functions/hasPermission'; import { mountIntegrationHistoryQueryBasedOnPermissions, mountIntegrationQueryBasedOnPermissions, @@ -43,15 +42,17 @@ API.v1.addRoute( API.v1.addRoute( 'integrations.history', - { authRequired: true, validateParams: isIntegrationsHistoryProps }, + { + authRequired: true, + validateParams: isIntegrationsHistoryProps, + permissionsRequired: { + GET: { permissions: ['manage-outgoing-integrations', 'manage-own-outgoing-integrations'], operation: 'hasAny' }, + }, + }, { async get() { const { userId, queryParams } = this; - if (!(await hasAtLeastOnePermissionAsync(userId, ['manage-outgoing-integrations', 'manage-own-outgoing-integrations']))) { - return API.v1.unauthorized(); - } - if (!queryParams.id || queryParams.id.trim() === '') { return API.v1.failure('Invalid integration id.'); } @@ -83,20 +84,22 @@ API.v1.addRoute( API.v1.addRoute( 'integrations.list', - { 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 { offset, count } = await getPaginationItems(this.queryParams); const { sort, fields: projection, query } = await this.parseJsonQuery(); @@ -124,20 +127,23 @@ API.v1.addRoute( API.v1.addRoute( 'integrations.remove', - { authRequired: true, validateParams: isIntegrationsRemoveProps }, { - async post() { - if ( - !(await hasAtLeastOnePermissionAsync(this.userId, [ + authRequired: true, + validateParams: isIntegrationsRemoveProps, + permissionsRequired: { + POST: { + permissions: [ 'manage-outgoing-integrations', 'manage-own-outgoing-integrations', 'manage-incoming-integrations', 'manage-own-incoming-integrations', - ])) - ) { - return API.v1.unauthorized(); - } - + ], + operation: 'hasAny', + }, + }, + }, + { + async post() { const { bodyParams } = this; let integration: IIntegration | null = null; diff --git a/apps/meteor/tests/end-to-end/api/06-outgoing-integrations.js b/apps/meteor/tests/end-to-end/api/06-outgoing-integrations.js index 120b467ec1b8..c14cfde70817 100644 --- a/apps/meteor/tests/end-to-end/api/06-outgoing-integrations.js +++ b/apps/meteor/tests/end-to-end/api/06-outgoing-integrations.js @@ -284,47 +284,42 @@ describe('[Outgoing Integrations]', function () { }); }); - it('should return unauthorized error when the user does not have any integrations permissions', (done) => { - updatePermission('manage-incoming-integrations', []).then(() => { - updatePermission('manage-own-incoming-integrations', []).then(() => { - updatePermission('manage-outgoing-integrations', []).then(() => { - updatePermission('manage-outgoing-integrations', []).then(() => { - request - .get(api('integrations.list')) - .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); - }); - }); + it('should return unauthorized error when the user does not have any integrations permissions', async () => { + await Promise.all([ + updatePermission('manage-incoming-integrations', []), + updatePermission('manage-own-incoming-integrations', []), + updatePermission('manage-outgoing-integrations', []), + updatePermission('manage-outgoing-integrations', []), + ]); + + await request + .get(api('integrations.list')) + .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]'); }); - }); }); }); describe('[/integrations.history]', () => { - it('should return an error when the user DOES NOT the necessary permission', (done) => { - updatePermission('manage-outgoing-integrations', []).then(() => { - updatePermission('manage-own-outgoing-integrations', []).then(() => { - request - .get(api('integrations.history')) - .set(credentials) - .query({ - id: integration._id, - }) - .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); + it('should return an error when the user DOES NOT the necessary permission', async () => { + await updatePermission('manage-outgoing-integrations', []); + await updatePermission('manage-own-outgoing-integrations', []); + await request + .get(api('integrations.history')) + .set(credentials) + .query({ + id: integration._id, + }) + .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]'); }); - }); }); it('should return the history of outgoing integrations', (done) => { @@ -457,7 +452,7 @@ describe('[Outgoing Integrations]', 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]'); }) .end(done); }); @@ -476,7 +471,7 @@ describe('[Outgoing Integrations]', 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]'); }) .end(done); }); diff --git a/apps/meteor/tests/end-to-end/api/07-incoming-integrations.js b/apps/meteor/tests/end-to-end/api/07-incoming-integrations.js index 37ce98ade6c5..b935015dc534 100644 --- a/apps/meteor/tests/end-to-end/api/07-incoming-integrations.js +++ b/apps/meteor/tests/end-to-end/api/07-incoming-integrations.js @@ -309,7 +309,7 @@ describe('[Incoming Integrations]', function () { }); describe('[/integrations.history]', () => { - it('should return an error when trying to get history of incoming integrations', (done) => { + it('should return an error when trying to get history of incoming integrations if user does NOT have enough permissions', (done) => { request .get(api('integrations.history')) .set(credentials) @@ -320,7 +320,7 @@ describe('[Incoming Integrations]', 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]'); }) .end(done); }); @@ -395,25 +395,22 @@ describe('[Incoming Integrations]', function () { }); }); - it('should return unauthorized error when the user does not have any integrations permissions', (done) => { - updatePermission('manage-incoming-integrations', []).then(() => { - updatePermission('manage-own-incoming-integrations', []).then(() => { - updatePermission('manage-outgoing-integrations', []).then(() => { - updatePermission('manage-outgoing-integrations', []).then(() => { - request - .get(api('integrations.list')) - .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); - }); - }); + it('should return unauthorized error when the user does not have any integrations permissions', async () => { + await Promise.all([ + updatePermission('manage-incoming-integrations', []), + updatePermission('manage-own-incoming-integrations', []), + updatePermission('manage-outgoing-integrations', []), + updatePermission('manage-outgoing-integrations', []), + ]); + await request + .get(api('integrations.list')) + .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]'); }); - }); }); }); @@ -570,7 +567,7 @@ describe('[Incoming Integrations]', 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]'); }) .end(done); }); @@ -589,7 +586,7 @@ describe('[Incoming Integrations]', 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]'); }) .end(done); });