Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow granting the mobile-upload-file permission to guests #34191

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
5 changes: 5 additions & 0 deletions .changeset/shiny-singers-cheer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@rocket.chat/meteor": minor
---

Allows granting the `mobile-upload-file` permission to guests
8 changes: 7 additions & 1 deletion apps/meteor/app/authorization/client/restrictedRoles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
]);
}
});
8 changes: 6 additions & 2 deletions apps/meteor/app/authorization/lib/AuthorizationUtils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const restrictedRolePermissions = new Map();
const restrictedRolePermissions = new Map<string, Set<string>>();

export const AuthorizationUtils = class {
static addRolePermissionWhiteList(roleId: string, list: string[]): void {
Expand All @@ -17,7 +17,7 @@ export const AuthorizationUtils = class {
const rules = restrictedRolePermissions.get(roleId);

for (const permissionId of list) {
rules.add(permissionId);
rules?.add(permissionId);
}
}

Expand Down Expand Up @@ -51,4 +51,8 @@ export const AuthorizationUtils = class {

return false;
}

static hasRestrictionsToRole(roleId: string): boolean {
return restrictedRolePermissions.has(roleId);
}
};
2 changes: 2 additions & 0 deletions apps/meteor/app/authorization/server/constant/permissions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'] },
Comment on lines +70 to +71
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add the start-discussion permission to this file so that it could be restored on my new end-to-end tests (with restorePermissionToRoles). We're currently inserting this permission somewhere else (on Meteor.startup) and it was missing here
start-discussion-other-user is not related to my tests, but I decided to bring this here too since it's created in the same Meteor.startup callback (so we avoid one more tech debt)

{ _id: 'unarchive-room', roles: ['admin'] },
{ _id: 'view-c-room', roles: ['admin', 'user', 'bot', 'app', 'anonymous'] },
{ _id: 'user-generate-access-token', roles: ['admin'] },
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -15,6 +16,10 @@ declare module '@rocket.chat/ddp-client' {

Meteor.methods<ServerMethods>({
async 'authorization:addPermissionToRole'(permissionId, role) {
if (role === 'guest' && !AuthorizationUtils.hasRestrictionsToRole(role) && (await License.hasValidLicense())) {
AuthorizationUtils.addRolePermissionWhiteList(role, await License.getGuestPermissions());
}
Comment on lines +19 to +21
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason EE tests were failing on CI because we didn't have any restrictions stored for the guest role (which didn't happen locally) 🤷‍♂️ so I went through all of the places where we populate such restrictions and couldn't find anything off, please tell me if you do
So I decided to reinforce these restrictions in the endpoint/method itself (which I believe is harmless) and create another task to identify what could be causing this issue later


if (AuthorizationUtils.isPermissionRestrictedForRole(permissionId, role)) {
throw new Meteor.Error('error-action-not-allowed', 'Permission is restricted', {
method: 'authorization:addPermissionToRole',
Expand Down
2 changes: 1 addition & 1 deletion apps/meteor/ee/app/authorization/lib/guestPermissions.ts
Original file line number Diff line number Diff line change
@@ -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'];
118 changes: 118 additions & 0 deletions apps/meteor/tests/end-to-end/api/guest-permissions.ts
Original file line number Diff line number Diff line change
@@ -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');
});
});
});
Loading