From b6b3eac01bb5f3f0b7bc9412cea4929adb1835c0 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 16 Dec 2024 15:21:32 -0300 Subject: [PATCH 1/9] feat: Allow granting the mobile-upload-file permission to guests --- apps/meteor/app/authorization/client/restrictedRoles.ts | 8 +++++++- apps/meteor/ee/app/authorization/lib/guestPermissions.ts | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/meteor/app/authorization/client/restrictedRoles.ts b/apps/meteor/app/authorization/client/restrictedRoles.ts index 5aa5e426c2bd..c947d3018110 100644 --- a/apps/meteor/app/authorization/client/restrictedRoles.ts +++ b/apps/meteor/app/authorization/client/restrictedRoles.ts @@ -7,6 +7,12 @@ Meteor.startup(async () => { const result = await sdk.call('license:isEnterprise'); if (result) { // #ToDo: Load this from the server with an API call instead of having a duplicate list - AuthorizationUtils.addRolePermissionWhiteList('guest', ['view-d-room', 'view-joined-room', 'view-p-room', 'start-discussion']); + AuthorizationUtils.addRolePermissionWhiteList('guest', [ + 'view-d-room', + 'view-joined-room', + 'view-p-room', + 'start-discussion', + 'mobile-upload-file', + ]); } }); diff --git a/apps/meteor/ee/app/authorization/lib/guestPermissions.ts b/apps/meteor/ee/app/authorization/lib/guestPermissions.ts index 7577d6258a5e..ebc2bd9d0526 100644 --- a/apps/meteor/ee/app/authorization/lib/guestPermissions.ts +++ b/apps/meteor/ee/app/authorization/lib/guestPermissions.ts @@ -1,2 +1,2 @@ // This list is currently duplicated on the client code as there's no available API to load it from the server -export const guestPermissions = ['view-d-room', 'view-joined-room', 'view-p-room', 'start-discussion']; +export const guestPermissions = ['view-d-room', 'view-joined-room', 'view-p-room', 'start-discussion', 'mobile-upload-file']; From a67238ae5330926dad0eebcb9075a42dc317a047 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 16 Dec 2024 15:21:52 -0300 Subject: [PATCH 2/9] tests: Add end-to-end tests --- .../server/constant/permissions.ts | 2 + .../tests/end-to-end/api/guest-permissions.ts | 118 ++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 apps/meteor/tests/end-to-end/api/guest-permissions.ts diff --git a/apps/meteor/app/authorization/server/constant/permissions.ts b/apps/meteor/app/authorization/server/constant/permissions.ts index 12ed3eb8d06c..bc0bc45f1b14 100644 --- a/apps/meteor/app/authorization/server/constant/permissions.ts +++ b/apps/meteor/app/authorization/server/constant/permissions.ts @@ -67,6 +67,8 @@ export const permissions = [ { _id: 'set-owner', roles: ['admin', 'owner'] }, { _id: 'send-many-messages', roles: ['admin', 'bot', 'app'] }, { _id: 'set-leader', roles: ['admin', 'owner'] }, + { _id: 'start-discussion', roles: ['admin', 'user', 'guest', 'app'] }, + { _id: 'start-discussion-other-user', roles: ['admin', 'user', 'owner', 'app'] }, { _id: 'unarchive-room', roles: ['admin'] }, { _id: 'view-c-room', roles: ['admin', 'user', 'bot', 'app', 'anonymous'] }, { _id: 'user-generate-access-token', roles: ['admin'] }, diff --git a/apps/meteor/tests/end-to-end/api/guest-permissions.ts b/apps/meteor/tests/end-to-end/api/guest-permissions.ts new file mode 100644 index 000000000000..3afda0ea1571 --- /dev/null +++ b/apps/meteor/tests/end-to-end/api/guest-permissions.ts @@ -0,0 +1,118 @@ +import { expect } from 'chai'; +import { before, describe, it, after } from 'mocha'; + +import { getCredentials, api, request, credentials, methodCall } from '../../data/api-data'; +import { restorePermissionToRoles } from '../../data/permissions.helper'; +import { IS_EE } from '../../e2e/config/constants'; + +(IS_EE ? describe : describe.skip)('[Guest Permissions]', () => { + const guestPermissions = ['view-d-room', 'view-joined-room', 'view-p-room', 'start-discussion', 'mobile-upload-file']; + + before((done) => getCredentials(done)); + + after(() => Promise.all(guestPermissions.map((permissionName) => restorePermissionToRoles(permissionName)))); + + function succeedRemoveGuestPermission(permissionName: string) { + it(`should allow removing the whitelisted permission ${permissionName} from the guest role`, async () => { + const res = await request + .post(methodCall('authorization:removeRoleFromPermission')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'authorization:removeRoleFromPermission', + params: [permissionName, 'guest'], + id: 'id', + msg: 'method', + }), + }) + .expect('Content-Type', 'application/json') + .expect(200); + + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('message'); + const message = JSON.parse(res.body.message); + expect(message).to.not.have.property('error'); + + const permissionsListRes = await request + .get(api('permissions.listAll')) + .set(credentials) + .expect('Content-Type', 'application/json') + .expect(200); + expect(permissionsListRes.body).to.have.property('success', true); + expect(permissionsListRes.body).to.have.property('update').and.to.be.an('array'); + expect(permissionsListRes.body).to.have.property('remove').and.to.be.an('array'); + + const updatedPermission = permissionsListRes.body.update.find((permission: any) => permission._id === permissionName); + expect(updatedPermission).to.have.property('_id', permissionName); + expect(updatedPermission).to.have.property('roles').and.to.be.an('array').that.does.not.include('guest'); + }); + } + + function succeedAddGuestPermission(permissionName: string) { + it(`should allow granting the whitelisted permission ${permissionName} to the guest role`, async () => { + const res = await request + .post(methodCall('authorization:addPermissionToRole')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'authorization:addPermissionToRole', + params: [permissionName, 'guest'], + id: 'id', + msg: 'method', + }), + }) + .expect('Content-Type', 'application/json') + .expect(200); + + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('message'); + const message = JSON.parse(res.body.message); + expect(message).to.not.have.property('error'); + + const permissionsListRes = await request + .get(api('permissions.listAll')) + .set(credentials) + .expect('Content-Type', 'application/json') + .expect(200); + expect(permissionsListRes.body).to.have.property('success', true); + expect(permissionsListRes.body).to.have.property('update').and.to.be.an('array'); + expect(permissionsListRes.body).to.have.property('remove').and.to.be.an('array'); + + const updatedPermission = permissionsListRes.body.update.find((permission: any) => permission._id === permissionName); + expect(updatedPermission).to.have.property('_id', permissionName); + expect(updatedPermission).to.have.property('roles').and.to.be.an('array').that.includes('guest'); + }); + } + + describe('Default guest roles', () => { + guestPermissions.forEach((permissionName) => { + succeedRemoveGuestPermission(permissionName); + }); + + guestPermissions.forEach((permissionName) => { + succeedAddGuestPermission(permissionName); + }); + + it('should not allow adding a non whitelisted permission to the guest role', async () => { + const res = await request + .post(methodCall('authorization:addPermissionToRole')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'authorization:addPermissionToRole', + params: ['create-c', 'guest'], + id: 'id', + msg: 'method', + }), + }) + .expect('Content-Type', 'application/json') + .expect(200); + + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('message'); + const message = JSON.parse(res.body.message); + expect(message).to.have.property('error'); + expect(message.error).to.have.property('reason', 'Permission is restricted'); + }); + }); +}); From 20a1eb4426afc922d0221645ab913a864affb2e5 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Mon, 16 Dec 2024 15:26:46 -0300 Subject: [PATCH 3/9] Create changeset --- .changeset/shiny-singers-cheer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/shiny-singers-cheer.md diff --git a/.changeset/shiny-singers-cheer.md b/.changeset/shiny-singers-cheer.md new file mode 100644 index 000000000000..ad4c03c2402d --- /dev/null +++ b/.changeset/shiny-singers-cheer.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": minor +--- + +Allows granting the `mobile-upload-file` permission to guests From 6652357ad1df540edf0a42c107ca10a1c708e12d Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 16 Dec 2024 21:38:48 -0300 Subject: [PATCH 4/9] clear cache after adding permissions to whitelist --- apps/meteor/server/services/authorization/service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/meteor/server/services/authorization/service.ts b/apps/meteor/server/services/authorization/service.ts index 5e67cf87b372..ba5be41255af 100644 --- a/apps/meteor/server/services/authorization/service.ts +++ b/apps/meteor/server/services/authorization/service.ts @@ -35,6 +35,7 @@ export class Authorization extends ServiceClass implements IAuthorization { this.onEvent('permission.changed', clearCache); this.onEvent('authorization.guestPermissions', (permissions: string[]) => { AuthorizationUtils.addRolePermissionWhiteList('guest', permissions); + clearCache(); }); } From 47876717a05295213976136893e2adfe37909f4a Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 24 Dec 2024 18:08:03 -0300 Subject: [PATCH 5/9] Convert event handler function to async --- apps/meteor/ee/app/license/server/license.internalService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/ee/app/license/server/license.internalService.ts b/apps/meteor/ee/app/license/server/license.internalService.ts index 9998378191d1..30f354dc70aa 100644 --- a/apps/meteor/ee/app/license/server/license.internalService.ts +++ b/apps/meteor/ee/app/license/server/license.internalService.ts @@ -12,7 +12,7 @@ export class LicenseService extends ServiceClassInternal implements ILicense { constructor() { super(); - License.onValidateLicense((): void => { + License.onValidateLicense(async (): Promise => { if (!License.hasValidLicense()) { return; } From 73d7ead4f00d599397831c910931c42c75e047f8 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Thu, 26 Dec 2024 20:33:33 -0300 Subject: [PATCH 6/9] remove not needed license validation --- apps/meteor/ee/app/license/server/license.internalService.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/apps/meteor/ee/app/license/server/license.internalService.ts b/apps/meteor/ee/app/license/server/license.internalService.ts index 30f354dc70aa..87a3d540a10c 100644 --- a/apps/meteor/ee/app/license/server/license.internalService.ts +++ b/apps/meteor/ee/app/license/server/license.internalService.ts @@ -13,10 +13,6 @@ export class LicenseService extends ServiceClassInternal implements ILicense { super(); License.onValidateLicense(async (): Promise => { - if (!License.hasValidLicense()) { - return; - } - void api.broadcast('authorization.guestPermissions', guestPermissions); void resetEnterprisePermissions(); }); From 84cf7bdbf5f284b7c314ed67b4d0a878d01c9f26 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Thu, 26 Dec 2024 21:43:49 -0300 Subject: [PATCH 7/9] ensure guest permissions on addPermissionToRole method --- apps/meteor/app/authorization/lib/AuthorizationUtils.ts | 8 ++++++-- .../authorization/server/methods/addPermissionToRole.ts | 5 +++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/apps/meteor/app/authorization/lib/AuthorizationUtils.ts b/apps/meteor/app/authorization/lib/AuthorizationUtils.ts index 6ad5cab04720..d51de55cc067 100644 --- a/apps/meteor/app/authorization/lib/AuthorizationUtils.ts +++ b/apps/meteor/app/authorization/lib/AuthorizationUtils.ts @@ -1,4 +1,4 @@ -const restrictedRolePermissions = new Map(); +const restrictedRolePermissions = new Map>(); export const AuthorizationUtils = class { static addRolePermissionWhiteList(roleId: string, list: string[]): void { @@ -17,7 +17,7 @@ export const AuthorizationUtils = class { const rules = restrictedRolePermissions.get(roleId); for (const permissionId of list) { - rules.add(permissionId); + rules?.add(permissionId); } } @@ -51,4 +51,8 @@ export const AuthorizationUtils = class { return false; } + + static hasRestrictionsToRole(roleId: string): boolean { + return restrictedRolePermissions.has(roleId); + } }; diff --git a/apps/meteor/app/authorization/server/methods/addPermissionToRole.ts b/apps/meteor/app/authorization/server/methods/addPermissionToRole.ts index 9a336478ca3e..3f5f7bfc1a26 100644 --- a/apps/meteor/app/authorization/server/methods/addPermissionToRole.ts +++ b/apps/meteor/app/authorization/server/methods/addPermissionToRole.ts @@ -1,3 +1,4 @@ +import { License } from '@rocket.chat/core-services'; import type { ServerMethods } from '@rocket.chat/ddp-client'; import { Permissions } from '@rocket.chat/models'; import { Meteor } from 'meteor/meteor'; @@ -15,6 +16,10 @@ declare module '@rocket.chat/ddp-client' { Meteor.methods({ async 'authorization:addPermissionToRole'(permissionId, role) { + if (role === 'guest' && !AuthorizationUtils.hasRestrictionsToRole(role) && (await License.hasValidLicense())) { + AuthorizationUtils.addRolePermissionWhiteList(role, await License.getGuestPermissions()); + } + if (AuthorizationUtils.isPermissionRestrictedForRole(permissionId, role)) { throw new Meteor.Error('error-action-not-allowed', 'Permission is restricted', { method: 'authorization:addPermissionToRole', From 7ff90bc1c7d0ba815db0a1f5d06518211d535565 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Thu, 26 Dec 2024 22:21:14 -0300 Subject: [PATCH 8/9] Remove unrelated changes --- apps/meteor/ee/app/license/server/license.internalService.ts | 4 ++++ apps/meteor/server/services/authorization/service.ts | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/meteor/ee/app/license/server/license.internalService.ts b/apps/meteor/ee/app/license/server/license.internalService.ts index 87a3d540a10c..30f354dc70aa 100644 --- a/apps/meteor/ee/app/license/server/license.internalService.ts +++ b/apps/meteor/ee/app/license/server/license.internalService.ts @@ -13,6 +13,10 @@ export class LicenseService extends ServiceClassInternal implements ILicense { super(); License.onValidateLicense(async (): Promise => { + if (!License.hasValidLicense()) { + return; + } + void api.broadcast('authorization.guestPermissions', guestPermissions); void resetEnterprisePermissions(); }); diff --git a/apps/meteor/server/services/authorization/service.ts b/apps/meteor/server/services/authorization/service.ts index ba5be41255af..5e67cf87b372 100644 --- a/apps/meteor/server/services/authorization/service.ts +++ b/apps/meteor/server/services/authorization/service.ts @@ -35,7 +35,6 @@ export class Authorization extends ServiceClass implements IAuthorization { this.onEvent('permission.changed', clearCache); this.onEvent('authorization.guestPermissions', (permissions: string[]) => { AuthorizationUtils.addRolePermissionWhiteList('guest', permissions); - clearCache(); }); } From 4fe4ee0fc5a8f8ad9f3097943b76cb9e63a95797 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Thu, 26 Dec 2024 22:22:24 -0300 Subject: [PATCH 9/9] remove more unrelated changes --- apps/meteor/ee/app/license/server/license.internalService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/ee/app/license/server/license.internalService.ts b/apps/meteor/ee/app/license/server/license.internalService.ts index 30f354dc70aa..9998378191d1 100644 --- a/apps/meteor/ee/app/license/server/license.internalService.ts +++ b/apps/meteor/ee/app/license/server/license.internalService.ts @@ -12,7 +12,7 @@ export class LicenseService extends ServiceClassInternal implements ILicense { constructor() { super(); - License.onValidateLicense(async (): Promise => { + License.onValidateLicense((): void => { if (!License.hasValidLicense()) { return; }