Skip to content

Commit d9c7701

Browse files
authored
feat: Update add/join methods to use abac rules (#37339)
1 parent 3f5c8d4 commit d9c7701

File tree

11 files changed

+308
-21
lines changed

11 files changed

+308
-21
lines changed

apps/meteor/app/lib/server/functions/addUserToRoom.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { getSubscriptionAutotranslateDefaultConfig } from '../../../../server/li
1212
import { roomCoordinator } from '../../../../server/lib/rooms/roomCoordinator';
1313
import { settings } from '../../../settings/server';
1414
import { getDefaultSubscriptionPref } from '../../../utils/lib/getDefaultSubscriptionPref';
15+
import { beforeAddUserToRoom as beforeAddUserToRoomPatch } from '../lib/beforeAddUserToRoom';
1516
import { notifyOnRoomChangedById, notifyOnSubscriptionChangedById } from '../lib/notifyListener';
1617

1718
/**
@@ -57,7 +58,10 @@ export const addUserToRoom = async (
5758
}
5859

5960
try {
60-
await beforeAddUserToRoom.run({ user: userToBeAdded, inviter: (inviter && (await Users.findOneById(inviter._id))) || undefined }, room);
61+
const inviterUser = inviter && ((await Users.findOneById(inviter._id)) || undefined);
62+
// 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
63+
await beforeAddUserToRoomPatch([userToBeAdded.username!], room, inviterUser);
64+
await beforeAddUserToRoom.run({ user: userToBeAdded, inviter: inviterUser }, room);
6165
} catch (error) {
6266
throw new Meteor.Error((error as any)?.message);
6367
}
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import type { IRoom, IUser } from '@rocket.chat/core-typings';
2+
import { makeFunction } from '@rocket.chat/patch-injection';
3+
4+
export const beforeAddUserToRoom = makeFunction(async (_users: IUser['username'][], _room: IRoom, _actor?: IUser) => {
5+
// no op on CE
6+
});

apps/meteor/app/slashcommands-invite/server/server.ts

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { api } from '@rocket.chat/core-services';
1+
import { api, isMeteorError } from '@rocket.chat/core-services';
22
import type { IUser, SlashCommandCallbackParams } from '@rocket.chat/core-typings';
33
import { Subscriptions, Users } from '@rocket.chat/models';
44
import { Meteor } from 'meteor/meteor';
@@ -8,6 +8,11 @@ import { addUsersToRoomMethod, sanitizeUsername } from '../../lib/server/methods
88
import { settings } from '../../settings/server';
99
import { slashCommands } from '../../utils/server/slashCommand';
1010

11+
// Type guards for the error
12+
function isStringError(error: unknown): error is { error: string } {
13+
return typeof (error as any)?.error === 'string';
14+
}
15+
1116
/*
1217
* Invite is a named function that will replace /invite commands
1318
* @param {Object} message - The message object
@@ -73,23 +78,31 @@ slashCommands.add({
7378
},
7479
inviter,
7580
);
76-
} catch ({ error }: any) {
77-
if (typeof error !== 'string') {
78-
return;
79-
}
81+
} catch (e: unknown) {
82+
if (isMeteorError(e)) {
83+
const details = Array.isArray(e.details) ? e.details.join(', ') : '';
8084

81-
if (error === 'error-federated-users-in-non-federated-rooms') {
82-
void api.broadcast('notify.ephemeralMessage', userId, message.rid, {
83-
msg: i18n.t('You_cannot_add_external_users_to_non_federated_room', { lng: settings.get('Language') || 'en' }),
84-
});
85-
} else if (error === 'cant-invite-for-direct-room') {
8685
void api.broadcast('notify.ephemeralMessage', userId, message.rid, {
87-
msg: i18n.t('Cannot_invite_users_to_direct_rooms', { lng: settings.get('Language') || 'en' }),
88-
});
89-
} else {
90-
void api.broadcast('notify.ephemeralMessage', userId, message.rid, {
91-
msg: i18n.t(error, { lng: settings.get('Language') || 'en' }),
86+
msg: i18n.t(e.message, { lng: settings.get('Language') || 'en', details: `\`${details}\`` }),
9287
});
88+
return;
89+
}
90+
91+
if (isStringError(e)) {
92+
const { error } = e;
93+
if (error === 'error-federated-users-in-non-federated-rooms') {
94+
void api.broadcast('notify.ephemeralMessage', userId, message.rid, {
95+
msg: i18n.t('You_cannot_add_external_users_to_non_federated_room', { lng: settings.get('Language') || 'en' }),
96+
});
97+
} else if (error === 'cant-invite-for-direct-room') {
98+
void api.broadcast('notify.ephemeralMessage', userId, message.rid, {
99+
msg: i18n.t('Cannot_invite_users_to_direct_rooms', { lng: settings.get('Language') || 'en' }),
100+
});
101+
} else {
102+
void api.broadcast('notify.ephemeralMessage', userId, message.rid, {
103+
msg: i18n.t(error, { lng: settings.get('Language') || 'en' }),
104+
});
105+
}
93106
}
94107
}
95108
}),

apps/meteor/ee/server/configuration/abac.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,7 @@ Meteor.startup(async () => {
77

88
await addSettings();
99
await createPermissions();
10+
11+
await import('../hooks/abac');
1012
});
1113
});
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { Abac } from '@rocket.chat/core-services';
2+
import { License } from '@rocket.chat/license';
3+
4+
import { beforeAddUserToRoom } from '../../../../app/lib/server/lib/beforeAddUserToRoom';
5+
import { settings } from '../../../../app/settings/server';
6+
7+
beforeAddUserToRoom.patch(async (prev, users, room, actor) => {
8+
await prev(users, room, actor);
9+
10+
const validUsers = users.filter(Boolean);
11+
if (
12+
!room?.abacAttributes?.length ||
13+
!validUsers.length ||
14+
!License.hasModule('abac') ||
15+
room.t !== 'p' ||
16+
!settings.get('ABAC_Enabled')
17+
) {
18+
return;
19+
}
20+
21+
await Abac.checkUsernamesMatchAttributes(validUsers as string[], room.abacAttributes);
22+
});
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
import './beforeAddUserToRoom';

apps/meteor/server/methods/addAllUserToRoom.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { check } from 'meteor/check';
66
import { Meteor } from 'meteor/meteor';
77

88
import { hasPermissionAsync } from '../../app/authorization/server/functions/hasPermission';
9+
import { beforeAddUserToRoom } from '../../app/lib/server/lib/beforeAddUserToRoom';
910
import { notifyOnSubscriptionChangedById } from '../../app/lib/server/lib/notifyListener';
1011
import { settings } from '../../app/settings/server';
1112
import { getDefaultSubscriptionPref } from '../../app/utils/lib/getDefaultSubscriptionPref';
@@ -50,6 +51,11 @@ export const addAllUserToRoomFn = async (userId: string, rid: IRoom['_id'], acti
5051
});
5152
}
5253

54+
await beforeAddUserToRoom(
55+
users.map((u) => u.username!),
56+
room,
57+
);
58+
5359
const now = new Date();
5460
for await (const user of users) {
5561
const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, user._id);

apps/meteor/tests/end-to-end/api/abac.ts

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1411,4 +1411,76 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
14111411
});
14121412
});
14131413
});
1414+
1415+
describe('Room access (invite, addition)', () => {
1416+
let roomWithoutAttr: IRoom;
1417+
let roomWithAttr: IRoom;
1418+
const accessAttrKey = `access_attr_${Date.now()}`;
1419+
1420+
before(async () => {
1421+
await updateSetting('ABAC_Enabled', true);
1422+
1423+
await request
1424+
.post(`${v1}/abac/attributes`)
1425+
.set(credentials)
1426+
.send({ key: accessAttrKey, values: ['v1'] })
1427+
.expect(200);
1428+
1429+
// We have to add them directly cause otherwise the abac engine would kick the user from the room after the attribute is added
1430+
await addAbacAttributesToUserDirectly(credentials['X-User-Id'], [{ key: accessAttrKey, values: ['v1'] }]);
1431+
1432+
// Create two private rooms: one will stay without attributes, the other will get the attribute
1433+
roomWithoutAttr = (await createRoom({ type: 'p', name: `abac-access-noattr-${Date.now()}` })).body.group;
1434+
roomWithAttr = (await createRoom({ type: 'p', name: `abac-access-withattr-${Date.now()}` })).body.group;
1435+
1436+
// Assign the attribute to the second room
1437+
await request
1438+
.post(`${v1}/abac/rooms/${roomWithAttr._id}/attributes/${accessAttrKey}`)
1439+
.set(credentials)
1440+
.send({ values: ['v1'] })
1441+
.expect(200);
1442+
});
1443+
1444+
after(async () => {
1445+
await deleteRoom({ type: 'p', roomId: roomWithoutAttr._id });
1446+
await deleteRoom({ type: 'p', roomId: roomWithAttr._id });
1447+
});
1448+
1449+
it('INVITE: user without attributes invited to room without attributes succeeds', async () => {
1450+
await request
1451+
.post(`${v1}/groups.invite`)
1452+
.set(credentials)
1453+
.send({ roomId: roomWithoutAttr._id, usernames: [unauthorizedUser.username] })
1454+
.expect(200)
1455+
.expect((res) => {
1456+
expect(res.body).to.have.property('success', true);
1457+
});
1458+
});
1459+
1460+
it('INVITE: user without attributes invited to room with attributes should fail', async () => {
1461+
await request
1462+
.post(`${v1}/groups.invite`)
1463+
.set(credentials)
1464+
.send({ roomId: roomWithAttr._id, usernames: [unauthorizedUser.username] })
1465+
.expect(400)
1466+
.expect((res) => {
1467+
expect(res.body).to.have.property('success', false);
1468+
expect(res.body).to.have.property('error').that.includes('error-usernames-not-matching-abac-attributes');
1469+
});
1470+
});
1471+
1472+
it('INVITE: after room loses attributes user without attributes can be invited', async () => {
1473+
await request.delete(`${v1}/abac/rooms/${roomWithAttr._id}/attributes/${accessAttrKey}`).set(credentials).expect(200);
1474+
1475+
// Try inviting again - should now succeed
1476+
await request
1477+
.post(`${v1}/groups.invite`)
1478+
.set(credentials)
1479+
.send({ roomId: roomWithAttr._id, usernames: [unauthorizedUser.username] })
1480+
.expect(200)
1481+
.expect((res) => {
1482+
expect(res.body).to.have.property('success', true);
1483+
});
1484+
});
1485+
});
14141486
});

ee/packages/abac/src/index.spec.ts

Lines changed: 132 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ const mockUpdateSingleAbacAttributeValuesById = jest.fn();
1414
const mockUpdateAbacAttributeValuesArrayFilteredById = jest.fn();
1515
const mockRemoveAbacAttributeByRoomIdAndKey = jest.fn();
1616
const mockInsertAbacAttributeIfNotExistsById = jest.fn();
17+
const mockUsersFind = jest.fn();
1718

1819
jest.mock('@rocket.chat/models', () => ({
1920
Rooms: {
@@ -37,12 +38,22 @@ jest.mock('@rocket.chat/models', () => ({
3738
removeById: (...args: any[]) => mockAbacDeleteOne(...args),
3839
find: (...args: any[]) => mockAbacFind(...args),
3940
},
41+
Users: {
42+
find: (...args: any[]) => mockUsersFind(...args),
43+
},
4044
}));
4145

42-
// Minimal mock for ServiceClass (we don't need its real behavior in unit scope)
43-
jest.mock('@rocket.chat/core-services', () => ({
44-
ServiceClass: class {},
45-
}));
46+
// Partial mock for @rocket.chat/core-services: keep real MeteorError, override ServiceClass and Room
47+
jest.mock('@rocket.chat/core-services', () => {
48+
const actual = jest.requireActual('@rocket.chat/core-services');
49+
return {
50+
...actual,
51+
ServiceClass: class {},
52+
Room: {
53+
removeUserFromRoom: jest.fn(),
54+
},
55+
};
56+
});
4657

4758
describe('AbacService (unit)', () => {
4859
let service: AbacService;
@@ -815,4 +826,121 @@ describe('AbacService (unit)', () => {
815826
});
816827
});
817828
});
829+
830+
describe('checkUsernamesMatchAttributes', () => {
831+
beforeEach(() => {
832+
mockUsersFind.mockReset();
833+
});
834+
835+
const attributes = [{ key: 'dept', values: ['eng'] }];
836+
837+
it('returns early (no query) when usernames array is empty', async () => {
838+
await expect(service.checkUsernamesMatchAttributes([], attributes as any)).resolves.toBeUndefined();
839+
expect(mockUsersFind).not.toHaveBeenCalled();
840+
});
841+
842+
it('returns early (no query) when attributes array is empty', async () => {
843+
await expect(service.checkUsernamesMatchAttributes(['alice'], [])).resolves.toBeUndefined();
844+
expect(mockUsersFind).not.toHaveBeenCalled();
845+
});
846+
847+
it('resolves when all provided usernames are compliant (query returns empty)', async () => {
848+
const usernames = ['alice', 'bob'];
849+
mockUsersFind.mockImplementationOnce(() => ({
850+
map: () => ({
851+
toArray: async () => [],
852+
}),
853+
}));
854+
855+
await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any)).resolves.toBeUndefined();
856+
857+
expect(mockUsersFind).toHaveBeenCalledWith(
858+
{
859+
username: { $in: usernames },
860+
$or: [
861+
{
862+
abacAttributes: {
863+
$not: {
864+
$elemMatch: {
865+
key: 'dept',
866+
values: { $all: ['eng'] },
867+
},
868+
},
869+
},
870+
},
871+
],
872+
},
873+
{ projection: { username: 1 } },
874+
);
875+
});
876+
877+
it('rejects with error-usernames-not-matching-abac-attributes and details for non-compliant users', async () => {
878+
const usernames = ['alice', 'bob', 'charlie'];
879+
const nonCompliantDocs = [{ username: 'bob' }, { username: 'charlie' }];
880+
mockUsersFind.mockImplementationOnce(() => ({
881+
map: (fn: (u: any) => string) => ({
882+
toArray: async () => nonCompliantDocs.map(fn),
883+
}),
884+
}));
885+
886+
await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any)).rejects.toMatchObject({
887+
error: 'error-usernames-not-matching-abac-attributes',
888+
message: expect.stringContaining('[error-usernames-not-matching-abac-attributes]'),
889+
details: expect.arrayContaining(['bob', 'charlie']),
890+
});
891+
});
892+
});
893+
describe('buildNonCompliantConditions (private)', () => {
894+
it('returns empty array for empty attributes list', () => {
895+
const result = (service as any).buildNonCompliantConditions([]);
896+
expect(result).toEqual([]);
897+
});
898+
899+
it('maps single attribute to $not $elemMatch query', () => {
900+
const attrs = [{ key: 'dept', values: ['eng', 'sales'] }];
901+
const result = (service as any).buildNonCompliantConditions(attrs);
902+
expect(result).toEqual([
903+
{
904+
abacAttributes: {
905+
$not: {
906+
$elemMatch: {
907+
key: 'dept',
908+
values: { $all: ['eng', 'sales'] },
909+
},
910+
},
911+
},
912+
},
913+
]);
914+
});
915+
916+
it('maps multiple attributes preserving order', () => {
917+
const attrs = [
918+
{ key: 'dept', values: ['eng'] },
919+
{ key: 'region', values: ['emea', 'apac'] },
920+
];
921+
const result = (service as any).buildNonCompliantConditions(attrs);
922+
expect(result).toEqual([
923+
{
924+
abacAttributes: {
925+
$not: {
926+
$elemMatch: {
927+
key: 'dept',
928+
values: { $all: ['eng'] },
929+
},
930+
},
931+
},
932+
},
933+
{
934+
abacAttributes: {
935+
$not: {
936+
$elemMatch: {
937+
key: 'region',
938+
values: { $all: ['emea', 'apac'] },
939+
},
940+
},
941+
},
942+
},
943+
]);
944+
});
945+
});
818946
});

0 commit comments

Comments
 (0)