diff --git a/apps/meteor/app/lib/server/functions/addUserToRoom.ts b/apps/meteor/app/lib/server/functions/addUserToRoom.ts index 881afb11812c7..4cae4acdb3394 100644 --- a/apps/meteor/app/lib/server/functions/addUserToRoom.ts +++ b/apps/meteor/app/lib/server/functions/addUserToRoom.ts @@ -12,6 +12,7 @@ import { getSubscriptionAutotranslateDefaultConfig } from '../../../../server/li import { roomCoordinator } from '../../../../server/lib/rooms/roomCoordinator'; import { settings } from '../../../settings/server'; import { getDefaultSubscriptionPref } from '../../../utils/lib/getDefaultSubscriptionPref'; +import { beforeAddUserToRoom as beforeAddUserToRoomPatch } from '../lib/beforeAddUserToRoom'; import { notifyOnRoomChangedById, notifyOnSubscriptionChangedById } from '../lib/notifyListener'; /** @@ -57,7 +58,10 @@ export const addUserToRoom = async ( } try { - await beforeAddUserToRoom.run({ user: userToBeAdded, inviter: (inviter && (await Users.findOneById(inviter._id))) || undefined }, room); + const inviterUser = inviter && ((await Users.findOneById(inviter._id)) || undefined); + // Not "duplicated": we're moving away from callbacks so this is a patch function. We should migrate the next one to be a patch or use this same patch, instead of calling both + await beforeAddUserToRoomPatch([userToBeAdded.username!], room, inviterUser); + await beforeAddUserToRoom.run({ user: userToBeAdded, inviter: inviterUser }, room); } catch (error) { throw new Meteor.Error((error as any)?.message); } diff --git a/apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts b/apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts new file mode 100644 index 0000000000000..984dc34a22a12 --- /dev/null +++ b/apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts @@ -0,0 +1,6 @@ +import type { IRoom, IUser } from '@rocket.chat/core-typings'; +import { makeFunction } from '@rocket.chat/patch-injection'; + +export const beforeAddUserToRoom = makeFunction(async (_users: IUser['username'][], _room: IRoom, _actor?: IUser) => { + // no op on CE +}); diff --git a/apps/meteor/app/slashcommands-invite/server/server.ts b/apps/meteor/app/slashcommands-invite/server/server.ts index 24ad0484fddcc..1079088aba60d 100644 --- a/apps/meteor/app/slashcommands-invite/server/server.ts +++ b/apps/meteor/app/slashcommands-invite/server/server.ts @@ -1,4 +1,4 @@ -import { api } from '@rocket.chat/core-services'; +import { api, isMeteorError } from '@rocket.chat/core-services'; import type { IUser, SlashCommandCallbackParams } from '@rocket.chat/core-typings'; import { Subscriptions, Users } from '@rocket.chat/models'; import { Meteor } from 'meteor/meteor'; @@ -8,6 +8,11 @@ import { addUsersToRoomMethod, sanitizeUsername } from '../../lib/server/methods import { settings } from '../../settings/server'; import { slashCommands } from '../../utils/server/slashCommand'; +// Type guards for the error +function isStringError(error: unknown): error is { error: string } { + return typeof (error as any)?.error === 'string'; +} + /* * Invite is a named function that will replace /invite commands * @param {Object} message - The message object @@ -73,23 +78,31 @@ slashCommands.add({ }, inviter, ); - } catch ({ error }: any) { - if (typeof error !== 'string') { - return; - } + } catch (e: unknown) { + if (isMeteorError(e)) { + const details = Array.isArray(e.details) ? e.details.join(', ') : ''; - if (error === 'error-federated-users-in-non-federated-rooms') { - void api.broadcast('notify.ephemeralMessage', userId, message.rid, { - msg: i18n.t('You_cannot_add_external_users_to_non_federated_room', { lng: settings.get('Language') || 'en' }), - }); - } else if (error === 'cant-invite-for-direct-room') { void api.broadcast('notify.ephemeralMessage', userId, message.rid, { - msg: i18n.t('Cannot_invite_users_to_direct_rooms', { lng: settings.get('Language') || 'en' }), - }); - } else { - void api.broadcast('notify.ephemeralMessage', userId, message.rid, { - msg: i18n.t(error, { lng: settings.get('Language') || 'en' }), + msg: i18n.t(e.message, { lng: settings.get('Language') || 'en', details: `\`${details}\`` }), }); + return; + } + + if (isStringError(e)) { + const { error } = e; + if (error === 'error-federated-users-in-non-federated-rooms') { + void api.broadcast('notify.ephemeralMessage', userId, message.rid, { + msg: i18n.t('You_cannot_add_external_users_to_non_federated_room', { lng: settings.get('Language') || 'en' }), + }); + } else if (error === 'cant-invite-for-direct-room') { + void api.broadcast('notify.ephemeralMessage', userId, message.rid, { + msg: i18n.t('Cannot_invite_users_to_direct_rooms', { lng: settings.get('Language') || 'en' }), + }); + } else { + void api.broadcast('notify.ephemeralMessage', userId, message.rid, { + msg: i18n.t(error, { lng: settings.get('Language') || 'en' }), + }); + } } } }), diff --git a/apps/meteor/ee/server/configuration/abac.ts b/apps/meteor/ee/server/configuration/abac.ts index ae4d67258fda0..138746572a4a4 100644 --- a/apps/meteor/ee/server/configuration/abac.ts +++ b/apps/meteor/ee/server/configuration/abac.ts @@ -7,5 +7,7 @@ Meteor.startup(async () => { await addSettings(); await createPermissions(); + + await import('../hooks/abac'); }); }); diff --git a/apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts b/apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts new file mode 100644 index 0000000000000..9a351b548468e --- /dev/null +++ b/apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts @@ -0,0 +1,22 @@ +import { Abac } from '@rocket.chat/core-services'; +import { License } from '@rocket.chat/license'; + +import { beforeAddUserToRoom } from '../../../../app/lib/server/lib/beforeAddUserToRoom'; +import { settings } from '../../../../app/settings/server'; + +beforeAddUserToRoom.patch(async (prev, users, room, actor) => { + await prev(users, room, actor); + + const validUsers = users.filter(Boolean); + if ( + !room?.abacAttributes?.length || + !validUsers.length || + !License.hasModule('abac') || + room.t !== 'p' || + !settings.get('ABAC_Enabled') + ) { + return; + } + + await Abac.checkUsernamesMatchAttributes(validUsers as string[], room.abacAttributes); +}); diff --git a/apps/meteor/ee/server/hooks/abac/index.ts b/apps/meteor/ee/server/hooks/abac/index.ts new file mode 100644 index 0000000000000..8f93423047ad8 --- /dev/null +++ b/apps/meteor/ee/server/hooks/abac/index.ts @@ -0,0 +1 @@ +import './beforeAddUserToRoom'; diff --git a/apps/meteor/server/methods/addAllUserToRoom.ts b/apps/meteor/server/methods/addAllUserToRoom.ts index 530ea9cfdda2f..1edc776ecf8aa 100644 --- a/apps/meteor/server/methods/addAllUserToRoom.ts +++ b/apps/meteor/server/methods/addAllUserToRoom.ts @@ -6,6 +6,7 @@ import { check } from 'meteor/check'; import { Meteor } from 'meteor/meteor'; import { hasPermissionAsync } from '../../app/authorization/server/functions/hasPermission'; +import { beforeAddUserToRoom } from '../../app/lib/server/lib/beforeAddUserToRoom'; import { notifyOnSubscriptionChangedById } from '../../app/lib/server/lib/notifyListener'; import { settings } from '../../app/settings/server'; import { getDefaultSubscriptionPref } from '../../app/utils/lib/getDefaultSubscriptionPref'; @@ -50,6 +51,11 @@ export const addAllUserToRoomFn = async (userId: string, rid: IRoom['_id'], acti }); } + await beforeAddUserToRoom( + users.map((u) => u.username!), + room, + ); + const now = new Date(); for await (const user of users) { const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, user._id); diff --git a/apps/meteor/tests/end-to-end/api/abac.ts b/apps/meteor/tests/end-to-end/api/abac.ts index fea3da2b08c5b..5a8b9b25371c4 100644 --- a/apps/meteor/tests/end-to-end/api/abac.ts +++ b/apps/meteor/tests/end-to-end/api/abac.ts @@ -1411,4 +1411,76 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I }); }); }); + + describe('Room access (invite, addition)', () => { + let roomWithoutAttr: IRoom; + let roomWithAttr: IRoom; + const accessAttrKey = `access_attr_${Date.now()}`; + + before(async () => { + await updateSetting('ABAC_Enabled', true); + + await request + .post(`${v1}/abac/attributes`) + .set(credentials) + .send({ key: accessAttrKey, values: ['v1'] }) + .expect(200); + + // We have to add them directly cause otherwise the abac engine would kick the user from the room after the attribute is added + await addAbacAttributesToUserDirectly(credentials['X-User-Id'], [{ key: accessAttrKey, values: ['v1'] }]); + + // Create two private rooms: one will stay without attributes, the other will get the attribute + roomWithoutAttr = (await createRoom({ type: 'p', name: `abac-access-noattr-${Date.now()}` })).body.group; + roomWithAttr = (await createRoom({ type: 'p', name: `abac-access-withattr-${Date.now()}` })).body.group; + + // Assign the attribute to the second room + await request + .post(`${v1}/abac/rooms/${roomWithAttr._id}/attributes/${accessAttrKey}`) + .set(credentials) + .send({ values: ['v1'] }) + .expect(200); + }); + + after(async () => { + await deleteRoom({ type: 'p', roomId: roomWithoutAttr._id }); + await deleteRoom({ type: 'p', roomId: roomWithAttr._id }); + }); + + it('INVITE: user without attributes invited to room without attributes succeeds', async () => { + await request + .post(`${v1}/groups.invite`) + .set(credentials) + .send({ roomId: roomWithoutAttr._id, usernames: [unauthorizedUser.username] }) + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + }); + }); + + it('INVITE: user without attributes invited to room with attributes should fail', async () => { + await request + .post(`${v1}/groups.invite`) + .set(credentials) + .send({ roomId: roomWithAttr._id, usernames: [unauthorizedUser.username] }) + .expect(400) + .expect((res) => { + expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('error').that.includes('error-usernames-not-matching-abac-attributes'); + }); + }); + + it('INVITE: after room loses attributes user without attributes can be invited', async () => { + await request.delete(`${v1}/abac/rooms/${roomWithAttr._id}/attributes/${accessAttrKey}`).set(credentials).expect(200); + + // Try inviting again - should now succeed + await request + .post(`${v1}/groups.invite`) + .set(credentials) + .send({ roomId: roomWithAttr._id, usernames: [unauthorizedUser.username] }) + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + }); + }); + }); }); diff --git a/ee/packages/abac/src/index.spec.ts b/ee/packages/abac/src/index.spec.ts index ef397b80d0628..1a55e284bfc80 100644 --- a/ee/packages/abac/src/index.spec.ts +++ b/ee/packages/abac/src/index.spec.ts @@ -14,6 +14,7 @@ const mockUpdateSingleAbacAttributeValuesById = jest.fn(); const mockUpdateAbacAttributeValuesArrayFilteredById = jest.fn(); const mockRemoveAbacAttributeByRoomIdAndKey = jest.fn(); const mockInsertAbacAttributeIfNotExistsById = jest.fn(); +const mockUsersFind = jest.fn(); jest.mock('@rocket.chat/models', () => ({ Rooms: { @@ -37,12 +38,22 @@ jest.mock('@rocket.chat/models', () => ({ removeById: (...args: any[]) => mockAbacDeleteOne(...args), find: (...args: any[]) => mockAbacFind(...args), }, + Users: { + find: (...args: any[]) => mockUsersFind(...args), + }, })); -// Minimal mock for ServiceClass (we don't need its real behavior in unit scope) -jest.mock('@rocket.chat/core-services', () => ({ - ServiceClass: class {}, -})); +// Partial mock for @rocket.chat/core-services: keep real MeteorError, override ServiceClass and Room +jest.mock('@rocket.chat/core-services', () => { + const actual = jest.requireActual('@rocket.chat/core-services'); + return { + ...actual, + ServiceClass: class {}, + Room: { + removeUserFromRoom: jest.fn(), + }, + }; +}); describe('AbacService (unit)', () => { let service: AbacService; @@ -815,4 +826,121 @@ describe('AbacService (unit)', () => { }); }); }); + + describe('checkUsernamesMatchAttributes', () => { + beforeEach(() => { + mockUsersFind.mockReset(); + }); + + const attributes = [{ key: 'dept', values: ['eng'] }]; + + it('returns early (no query) when usernames array is empty', async () => { + await expect(service.checkUsernamesMatchAttributes([], attributes as any)).resolves.toBeUndefined(); + expect(mockUsersFind).not.toHaveBeenCalled(); + }); + + it('returns early (no query) when attributes array is empty', async () => { + await expect(service.checkUsernamesMatchAttributes(['alice'], [])).resolves.toBeUndefined(); + expect(mockUsersFind).not.toHaveBeenCalled(); + }); + + it('resolves when all provided usernames are compliant (query returns empty)', async () => { + const usernames = ['alice', 'bob']; + mockUsersFind.mockImplementationOnce(() => ({ + map: () => ({ + toArray: async () => [], + }), + })); + + await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any)).resolves.toBeUndefined(); + + expect(mockUsersFind).toHaveBeenCalledWith( + { + username: { $in: usernames }, + $or: [ + { + abacAttributes: { + $not: { + $elemMatch: { + key: 'dept', + values: { $all: ['eng'] }, + }, + }, + }, + }, + ], + }, + { projection: { username: 1 } }, + ); + }); + + it('rejects with error-usernames-not-matching-abac-attributes and details for non-compliant users', async () => { + const usernames = ['alice', 'bob', 'charlie']; + const nonCompliantDocs = [{ username: 'bob' }, { username: 'charlie' }]; + mockUsersFind.mockImplementationOnce(() => ({ + map: (fn: (u: any) => string) => ({ + toArray: async () => nonCompliantDocs.map(fn), + }), + })); + + await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any)).rejects.toMatchObject({ + error: 'error-usernames-not-matching-abac-attributes', + message: expect.stringContaining('[error-usernames-not-matching-abac-attributes]'), + details: expect.arrayContaining(['bob', 'charlie']), + }); + }); + }); + describe('buildNonCompliantConditions (private)', () => { + it('returns empty array for empty attributes list', () => { + const result = (service as any).buildNonCompliantConditions([]); + expect(result).toEqual([]); + }); + + it('maps single attribute to $not $elemMatch query', () => { + const attrs = [{ key: 'dept', values: ['eng', 'sales'] }]; + const result = (service as any).buildNonCompliantConditions(attrs); + expect(result).toEqual([ + { + abacAttributes: { + $not: { + $elemMatch: { + key: 'dept', + values: { $all: ['eng', 'sales'] }, + }, + }, + }, + }, + ]); + }); + + it('maps multiple attributes preserving order', () => { + const attrs = [ + { key: 'dept', values: ['eng'] }, + { key: 'region', values: ['emea', 'apac'] }, + ]; + const result = (service as any).buildNonCompliantConditions(attrs); + expect(result).toEqual([ + { + abacAttributes: { + $not: { + $elemMatch: { + key: 'dept', + values: { $all: ['eng'] }, + }, + }, + }, + }, + { + abacAttributes: { + $not: { + $elemMatch: { + key: 'region', + values: { $all: ['emea', 'apac'] }, + }, + }, + }, + }, + ]); + }); + }); }); diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index b11c7992d72b5..4c1084f68b315 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -1,4 +1,4 @@ -import { Room, ServiceClass } from '@rocket.chat/core-services'; +import { MeteorError, Room, ServiceClass } from '@rocket.chat/core-services'; import type { IAbacService } from '@rocket.chat/core-services'; import type { IAbacAttribute, IAbacAttributeDefinition, IRoom, AtLeast } from '@rocket.chat/core-typings'; import { Logger } from '@rocket.chat/logger'; @@ -442,6 +442,38 @@ export class AbacService extends ServiceClass implements IAbacService { })); } + async checkUsernamesMatchAttributes(usernames: string[], attributes: IAbacAttributeDefinition[]): Promise { + if (!usernames.length || !attributes.length) { + return; + } + + const nonComplianceConditions = this.buildNonCompliantConditions(attributes); + const nonCompliantUsersFromList = await Users.find( + { + username: { $in: usernames }, + $or: nonComplianceConditions, + }, + { projection: { username: 1 } }, + ) + .map((u) => u.username as string) + .toArray(); + + const nonCompliantSet = new Set(nonCompliantUsersFromList); + + if (nonCompliantSet.size) { + throw new MeteorError( + 'error-usernames-not-matching-abac-attributes', + 'Some usernames do not comply with the ABAC attributes for the room', + Array.from(nonCompliantSet), + ); + } + + this.logger.debug({ + msg: 'User list complied with ABAC attributes for room', + usernames, + }); + } + protected async onRoomAttributesChanged( room: AtLeast, newAttributes: IAbacAttributeDefinition[], diff --git a/packages/core-services/src/types/IAbacService.ts b/packages/core-services/src/types/IAbacService.ts index 357e9f55b1eab..8dda2f7f17727 100644 --- a/packages/core-services/src/types/IAbacService.ts +++ b/packages/core-services/src/types/IAbacService.ts @@ -17,4 +17,5 @@ export interface IAbacService { removeRoomAbacAttribute(rid: string, key: string): Promise; addRoomAbacAttributeByKey(rid: string, key: string, values: string[]): Promise; replaceRoomAbacAttributeByKey(rid: string, key: string, values: string[]): Promise; + checkUsernamesMatchAttributes(usernames: string[], attributes: IAbacAttributeDefinition[]): Promise; }