Skip to content

Commit

Permalink
fix: [ENTERPRISE] Guest users can join more than maxRoomsPerGuest roo…
Browse files Browse the repository at this point in the history
  • Loading branch information
carlosrodrigues94 authored Jun 15, 2023
1 parent 0d00dba commit 916c0dc
Show file tree
Hide file tree
Showing 13 changed files with 291 additions and 3 deletions.
6 changes: 6 additions & 0 deletions .changeset/shiny-donkeys-prove.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@rocket.chat/meteor": patch
"@rocket.chat/model-typings": patch
---

fix: [ENTERPRISE] Guest users can join more than maxRoomsPerGuest rooms
7 changes: 7 additions & 0 deletions apps/meteor/app/lib/server/functions/addUserToRoom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ export const addUserToRoom = async function (

const userToBeAdded = typeof user !== 'string' ? user : await Users.findOneByUsername(user.replace('@', ''));
const roomDirectives = roomCoordinator.getRoomDirectives(room.t);

if (!userToBeAdded) {
throw new Meteor.Error('user-not-found');
}

if (
!(await roomDirectives.allowMemberAction(room, RoomMemberActions.JOIN, userToBeAdded._id)) &&
!(await roomDirectives.allowMemberAction(room, RoomMemberActions.INVITE, userToBeAdded._id))
Expand All @@ -39,6 +44,8 @@ export const addUserToRoom = async function (
throw new Meteor.Error((error as any)?.message);
}

await callbacks.run('beforeAddedToRoom', { user: userToBeAdded, inviter: userToBeAdded });

// Check if user is already in room
const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, userToBeAdded._id);
if (subscription || !userToBeAdded) {
Expand Down
3 changes: 2 additions & 1 deletion apps/meteor/app/lib/server/functions/createRoom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,15 @@ export const createRoom = async <T extends RoomType>(
} else {
for await (const username of [...new Set(members)]) {
const member = await Users.findOneByUsername(username, {
projection: { 'username': 1, 'settings.preferences': 1, 'federated': 1 },
projection: { 'username': 1, 'settings.preferences': 1, 'federated': 1, 'roles': 1 },
});
if (!member) {
continue;
}

try {
await callbacks.run('federation.beforeAddUserToARoom', { user: member, inviter: owner }, room);
await callbacks.run('beforeAddedToRoom', { user: member, inviter: owner });
} catch (error) {
continue;
}
Expand Down
9 changes: 9 additions & 0 deletions apps/meteor/ee/app/license/server/license.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ interface IValidLicense {
}

let maxGuestUsers = 0;
let maxRoomsPerGuest = 0;
let maxActiveUsers = 0;

class LicenseClass {
Expand Down Expand Up @@ -192,6 +193,10 @@ class LicenseClass {
maxGuestUsers = license.maxGuestUsers;
}

if (license.maxRoomsPerGuest > maxRoomsPerGuest) {
maxRoomsPerGuest = license.maxRoomsPerGuest;
}

if (license.maxActiveUsers > maxActiveUsers) {
maxActiveUsers = license.maxActiveUsers;
}
Expand Down Expand Up @@ -323,6 +328,10 @@ export function getMaxGuestUsers(): number {
return maxGuestUsers;
}

export function getMaxRoomsPerGuest(): number {
return maxRoomsPerGuest;
}

export function getMaxActiveUsers(): number {
return maxActiveUsers;
}
Expand Down
1 change: 1 addition & 0 deletions apps/meteor/ee/server/startup/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import './apps';
import './audit';
import './deviceManagement';
import './engagementDashboard';
import './maxRoomsPerGuest';
import './seatsCap';
import './services';
import './upsell';
Expand Down
21 changes: 21 additions & 0 deletions apps/meteor/ee/server/startup/maxRoomsPerGuest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { Meteor } from 'meteor/meteor';
import { Subscriptions } from '@rocket.chat/models';

import { callbacks } from '../../../lib/callbacks';
import { getMaxRoomsPerGuest } from '../../app/license/server/license';
import { i18n } from '../../../server/lib/i18n';

callbacks.add(
'beforeAddedToRoom',
async ({ user }) => {
if (user.roles?.includes('guest')) {
const totalSubscriptions = await Subscriptions.countByUserId(user._id);

if (totalSubscriptions >= getMaxRoomsPerGuest()) {
throw new Meteor.Error('error-max-rooms-per-guest-reached', i18n.t('error-max-rooms-per-guest-reached'));
}
}
},
callbacks.priority.MEDIUM,
'check-max-rooms-per-guest',
);
1 change: 1 addition & 0 deletions apps/meteor/packages/rocketchat-i18n/i18n/en.i18n.json
Original file line number Diff line number Diff line change
Expand Up @@ -1993,6 +1993,7 @@
"error-max-departments-number-reached": "You reached the maximum number of departments allowed by your license. Contact sale@rocket.chat for a new license.",
"error-max-guests-number-reached": "You reached the maximum number of guest users allowed by your license. Contact sale@rocket.chat for a new license.",
"error-max-number-simultaneous-chats-reached": "The maximum number of simultaneous chats per agent has been reached.",
"error-max-rooms-per-guest-reached": "The maximum number of rooms per guest has been reached.",
"error-message-deleting-blocked": "Message deleting is blocked",
"error-message-editing-blocked": "Message editing is blocked",
"error-message-size-exceeded": "Message size exceeds Message_MaxAllowedSize",
Expand Down
6 changes: 6 additions & 0 deletions apps/meteor/server/models/raw/Subscriptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,12 @@ export class SubscriptionsRaw extends BaseRaw<ISubscription> implements ISubscri
return this.col.countDocuments(query);
}

countByUserId(userId: string): Promise<number> {
const query = { 'u._id': userId };

return this.col.countDocuments(query);
}

countByRoomId(roomId: string): Promise<number> {
const query = {
rid: roomId,
Expand Down
1 change: 1 addition & 0 deletions apps/meteor/tests/data/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const CI_MAX_ROOMS_PER_GUEST = 10;
63 changes: 62 additions & 1 deletion apps/meteor/tests/end-to-end/api/02-channels.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import { expect } from 'chai';

import { getCredentials, api, request, credentials, apiPublicChannelName, channel, reservedWords } from '../../data/api-data.js';
import { adminUsername, password } from '../../data/user';
import { createUser, login } from '../../data/users.helper';
import { createUser, login, deleteUser } from '../../data/users.helper';
import { updatePermission, updateSetting } from '../../data/permissions.helper';
import { createRoom } from '../../data/rooms.helper';
import { createIntegration, removeIntegration } from '../../data/integration.helper';
import { testFileUploads } from '../../data/uploads.helper';
import { CI_MAX_ROOMS_PER_GUEST as maxRoomsPerGuest } from '../../data/constants';

function getRoomInfo(roomId) {
return new Promise((resolve /* , reject*/) => {
Expand Down Expand Up @@ -48,6 +49,66 @@ describe('[Channels]', function () {
.end(done);
});

describe('[/channels.create]', () => {
let guestUser;
let room;

before(async () => {
guestUser = await createUser({ roles: ['guest'] });
});
after(async () => {
await deleteUser(guestUser);
});

it('should not add guest users to more rooms than defined in the license', async function () {
// TODO this is not the right way to do it. We're doing this way for now just because we have separate CI jobs for EE and CE,
// ideally we should have a single CI job that adds a license and runs both CE and EE tests.
if (!process.env.IS_EE) {
this.skip();
}

const promises = [];
for (let i = 0; i < maxRoomsPerGuest; i++) {
promises.push(
createRoom({
type: 'c',
name: `channel.test.${Date.now()}-${Math.random()}`,
members: [guestUser.username],
}),
);
}
await Promise.all(promises);

request
.post(api('channels.create'))
.set(credentials)
.send({
name: `channel.test.${Date.now()}-${Math.random()}`,
members: [guestUser.username],
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
room = res.body.group;
})
.then(() => {
request
.get(api('channels.members'))
.set(credentials)
.query({
roomId: room._id,
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('members').and.to.be.an('array');
expect(res.body.members).to.have.lengthOf(1);
});
});
});
});
describe('[/channels.info]', () => {
let testChannel = {};
let channelMessage = {};
Expand Down
63 changes: 62 additions & 1 deletion apps/meteor/tests/end-to-end/api/03-groups.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import { expect } from 'chai';

import { getCredentials, api, request, credentials, group, apiPrivateChannelName } from '../../data/api-data.js';
import { adminUsername, password } from '../../data/user';
import { createUser, login } from '../../data/users.helper';
import { createUser, login, deleteUser } from '../../data/users.helper';
import { updatePermission, updateSetting } from '../../data/permissions.helper';
import { createRoom } from '../../data/rooms.helper';
import { createIntegration, removeIntegration } from '../../data/integration.helper';
import { testFileUploads } from '../../data/uploads.helper';
import { CI_MAX_ROOMS_PER_GUEST as maxRoomsPerGuest } from '../../data/constants';

function getRoomInfo(roomId) {
return new Promise((resolve /* , reject*/) => {
Expand Down Expand Up @@ -47,6 +48,66 @@ describe('[Groups]', function () {
})
.end(done);
});
describe('[/groups.create]', () => {
let guestUser;
let room;

before(async () => {
guestUser = await createUser({ roles: ['guest'] });
});
after(async () => {
await deleteUser(guestUser);
});

it('should not add guest users to more rooms than defined in the license', async function () {
// TODO this is not the right way to do it. We're doing this way for now just because we have separate CI jobs for EE and CE,
// ideally we should have a single CI job that adds a license and runs both CE and EE tests.
if (!process.env.IS_EE) {
this.skip();
}
const promises = [];

for (let i = 0; i < maxRoomsPerGuest; i++) {
promises.push(
createRoom({
type: 'p',
name: `channel.test.${Date.now()}-${Math.random()}`,
members: [guestUser.username],
}),
);
}
await Promise.all(promises);

request
.post(api('groups.create'))
.set(credentials)
.send({
name: `channel.test.${Date.now()}-${Math.random()}`,
members: [guestUser.username],
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
room = res.body.group;
})
.then(() => {
request
.get(api('groups.members'))
.set(credentials)
.query({
roomId: room._id,
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('members').and.to.be.an('array');
expect(res.body.members).to.have.lengthOf(1);
});
});
});
});
describe('/groups.create (encrypted)', () => {
it('should create a new encrypted group', async () => {
await request
Expand Down
Loading

0 comments on commit 916c0dc

Please sign in to comment.