diff --git a/.changeset/quiet-teams-guard.md b/.changeset/quiet-teams-guard.md new file mode 100644 index 0000000000000..ac9a8af178b0a --- /dev/null +++ b/.changeset/quiet-teams-guard.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Ensures room permission checks are applied consistently regardless of how the room is identified when converting a channel to a team or creating a team from an existing room diff --git a/apps/meteor/app/api/server/v1/channels.ts b/apps/meteor/app/api/server/v1/channels.ts index 44be9f89b54cc..5cd2e327dc17f 100644 --- a/apps/meteor/app/api/server/v1/channels.ts +++ b/apps/meteor/app/api/server/v1/channels.ts @@ -46,7 +46,7 @@ import { removeRoomModerator } from '../../../../server/methods/removeRoomModera import { removeRoomOwner } from '../../../../server/methods/removeRoomOwner'; import { removeUserFromRoomMethod } from '../../../../server/methods/removeUserFromRoom'; import { canAccessRoomAsync } from '../../../authorization/server'; -import { 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 { addUsersToRoomMethod } from '../../../lib/server/methods/addUsersToRoom'; @@ -522,10 +522,6 @@ API.v1.addRoute( return API.v1.failure('The parameter "channelId" or "channelName" is required'); } - if (channelId && !(await hasPermissionAsync(this.userId, 'edit-room', channelId))) { - return API.v1.forbidden(); - } - const room = await findChannelByIdOrName({ params: channelId !== undefined ? { roomId: channelId } : { roomName: channelName }, userId: this.userId, @@ -535,6 +531,10 @@ API.v1.addRoute( return API.v1.failure('Channel not found'); } + if (!(await hasAllPermissionAsync(this.userId, ['create-team', 'edit-room'], room._id))) { + return API.v1.forbidden(); + } + const subscriptions = await Subscriptions.findByRoomId(room._id, { projection: { 'u._id': 1 }, }); diff --git a/apps/meteor/app/api/server/v1/teams.ts b/apps/meteor/app/api/server/v1/teams.ts index fe3de29cf22a4..e53615e44a4f3 100644 --- a/apps/meteor/app/api/server/v1/teams.ts +++ b/apps/meteor/app/api/server/v1/teams.ts @@ -30,7 +30,11 @@ import { escapeRegExp } from '@rocket.chat/string-helpers'; import { eraseRoom } from '../../../../server/lib/eraseRoom'; import { canAccessRoomAsync } from '../../../authorization/server'; -import { hasPermissionAsync, hasAtLeastOnePermissionAsync } from '../../../authorization/server/functions/hasPermission'; +import { + hasPermissionAsync, + hasAtLeastOnePermissionAsync, + hasAllPermissionAsync, +} from '../../../authorization/server/functions/hasPermission'; import { removeUserFromRoom } from '../../../lib/server/functions/removeUserFromRoom'; import { settings } from '../../../settings/server'; import type { ExtractRoutesFromAPI } from '../ApiClass'; @@ -125,11 +129,16 @@ const teamsEndpoints = API.v1 }), 400: validateBadRequestErrorResponse, 401: validateUnauthorizedErrorResponse, + 403: validateForbiddenErrorResponse, }, }, async function action() { const { name, type, members, room, owner } = this.bodyParams; + if (room?.id && !(await hasAllPermissionAsync(this.userId, ['create-team', 'edit-room'], room.id))) { + return API.v1.forbidden(); + } + const team = await Team.create(this.userId, { team: { name, diff --git a/apps/meteor/tests/end-to-end/api/channels.ts b/apps/meteor/tests/end-to-end/api/channels.ts index 16dbbd836f2b4..363f18886aa61 100644 --- a/apps/meteor/tests/end-to-end/api/channels.ts +++ b/apps/meteor/tests/end-to-end/api/channels.ts @@ -3991,6 +3991,36 @@ describe('[Channels]', () => { }); }); + describe('when a user without edit-room permission on the channel tries to convert it to a team', () => { + let outsiderChannel: IRoom; + let outsiderUser: TestUser; + let outsiderCredentials: Credentials; + + before(async () => { + await updatePermission('create-team', ['admin', 'user']); + await updatePermission('edit-room', ['admin', 'owner', 'moderator']); + + outsiderChannel = (await createRoom({ type: 'c', name: `channel.convertToTeam.outsider.test.${Date.now()}` })).body.channel; + outsiderUser = await createUser(); + outsiderCredentials = await login(outsiderUser.username, password); + }); + + after(async () => { + await Promise.all([deleteRoom({ type: 'c', roomId: outsiderChannel._id }), deleteUser(outsiderUser)]); + }); + + it('should return 403 when using channelName', async () => { + await request + .post(api('channels.convertToTeam')) + .set(outsiderCredentials) + .send({ channelName: outsiderChannel.name }) + .expect(403) + .expect((res) => { + expect(res.body).to.have.a.property('success', false); + }); + }); + }); + it(`should return an error when the channel's name and id are sent as parameter`, (done) => { void request .post(api('channels.convertToTeam')) diff --git a/apps/meteor/tests/end-to-end/api/teams.ts b/apps/meteor/tests/end-to-end/api/teams.ts index 35ddbaae0b5d9..d99f2817d28e6 100644 --- a/apps/meteor/tests/end-to-end/api/teams.ts +++ b/apps/meteor/tests/end-to-end/api/teams.ts @@ -249,6 +249,63 @@ describe('[Teams]', () => { }); }); }); + + describe('/teams.create - existing room ownership check', () => { + let roomOwner: TestUser; + let roomOwnerCredentials: Credentials; + let attacker: TestUser; + let attackerCredentials: Credentials; + let targetRoom: IRoom; + const teamName = `test-team-hijack-${Date.now()}`; + + before(async () => { + [roomOwner, attacker] = await Promise.all([createUser(), createUser()]); + [roomOwnerCredentials, attackerCredentials] = await Promise.all([ + login(roomOwner.username, password), + login(attacker.username, password), + ]); + targetRoom = (await createRoom({ type: 'c', name: `test-room-hijack-${Date.now()}`, credentials: roomOwnerCredentials })).body + .channel; + }); + + before(() => updatePermission('create-team', ['admin', 'user'])); + + after(async () => { + await Promise.all([ + deleteRoom({ type: 'c', roomId: targetRoom._id }), + deleteUser(roomOwner), + deleteUser(attacker), + updatePermission('create-team', ['admin', 'user']), + ]); + }); + + it('should not allow a user with no ownership/moderation of a room to hijack it into a new team by passing room.id', async () => { + await request + .post(api('teams.create')) + .set(attackerCredentials) + .send({ + name: teamName, + type: 0, + room: { id: targetRoom._id }, + }) + .expect('Content-Type', 'application/json') + .expect(403) + .expect((res) => { + expect(res.body).to.have.property('success', false); + }); + + await request + .get(api('channels.info')) + .set(credentials) + .query({ roomId: targetRoom._id }) + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body.channel).to.not.have.property('teamId'); + expect(res.body.channel).to.not.have.property('teamMain'); + }); + }); + }); }); describe('/teams.convertToChannel', () => {