Skip to content

Commit a932679

Browse files
KevLehmanKevLehman
committed
feat: Remove users from rooms when subject attributes no longer match (#37506)
1 parent 43ff16a commit a932679

File tree

5 files changed

+727
-14
lines changed

5 files changed

+727
-14
lines changed

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

Lines changed: 151 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ const mockRemoveAbacAttributeByRoomIdAndKey = jest.fn();
1818
const mockInsertAbacAttributeIfNotExistsById = jest.fn();
1919
const mockUsersFind = jest.fn();
2020
const mockUsersUpdateOne = jest.fn();
21+
const mockUsersSetAbacAttributesById = jest.fn();
22+
const mockUsersUnsetAbacAttributesById = jest.fn();
2123

2224
jest.mock('@rocket.chat/models', () => ({
2325
Rooms: {
@@ -43,6 +45,9 @@ jest.mock('@rocket.chat/models', () => ({
4345
},
4446
Users: {
4547
find: (...args: any[]) => mockUsersFind(...args),
48+
setAbacAttributesById: (...args: any[]) => mockUsersSetAbacAttributesById(...args),
49+
unsetAbacAttributesById: (...args: any[]) => mockUsersUnsetAbacAttributesById(...args),
50+
findOneAndUpdate: (...args: any[]) => mockUsersUpdateOne(...args),
4651
updateOne: (...args: any[]) => mockUsersUpdateOne(...args),
4752
},
4853
}));
@@ -69,8 +74,8 @@ describe('AbacService (unit)', () => {
6974

7075
describe('addSubjectAttributes (merging behavior)', () => {
7176
const getUpdatedAttributesFromCall = () => {
72-
const call = mockUsersUpdateOne.mock.calls.find((c) => c[1]?.$set?.abacAttributes);
73-
return call?.[1].$set.abacAttributes as any[] | undefined;
77+
const last = mockUsersSetAbacAttributesById.mock.calls.at(-1);
78+
return last?.[1] as any[] | undefined;
7479
};
7580

7681
it('merges values from multiple LDAP keys mapping to the same ABAC key', async () => {
@@ -87,7 +92,7 @@ describe('AbacService (unit)', () => {
8792

8893
await service.addSubjectAttributes(user, ldapUser, map);
8994

90-
expect(mockUsersUpdateOne).toHaveBeenCalledTimes(1);
95+
expect(mockUsersSetAbacAttributesById).toHaveBeenCalledTimes(1);
9196
const final = getUpdatedAttributesFromCall();
9297
expect(final).toBeDefined();
9398
expect(final).toHaveLength(1);
@@ -130,8 +135,7 @@ describe('AbacService (unit)', () => {
130135

131136
await service.addSubjectAttributes(user, ldapUser, map);
132137

133-
const unsetCall = mockUsersUpdateOne.mock.calls.find((c) => c[1]?.$unset?.abacAttributes);
134-
expect(unsetCall).toBeDefined();
138+
expect(mockUsersUnsetAbacAttributesById).toHaveBeenCalledTimes(1);
135139
});
136140

137141
it('does nothing when no LDAP values are found and user had no previous attributes', async () => {
@@ -141,7 +145,8 @@ describe('AbacService (unit)', () => {
141145

142146
await service.addSubjectAttributes(user, ldapUser, map);
143147

144-
expect(mockUsersUpdateOne).not.toHaveBeenCalled();
148+
expect(mockUsersSetAbacAttributesById).not.toHaveBeenCalled();
149+
expect(mockUsersUnsetAbacAttributesById).not.toHaveBeenCalled();
145150
});
146151

147152
it('calls onSubjectAttributesChanged when user loses an attribute value', async () => {
@@ -226,8 +231,7 @@ describe('AbacService (unit)', () => {
226231
const spy = jest.spyOn<any, any>(service as any, 'onSubjectAttributesChanged');
227232
await service.addSubjectAttributes(user, ldapUser, map);
228233

229-
const unsetCall = mockUsersUpdateOne.mock.calls.find((c) => c[1]?.$unset?.abacAttributes);
230-
expect(unsetCall).toBeDefined();
234+
expect(mockUsersUnsetAbacAttributesById).toHaveBeenCalledTimes(1);
231235
expect(spy).toHaveBeenCalledTimes(1);
232236
expect(spy.mock.calls[0][1]).toEqual([]);
233237
});
@@ -1252,4 +1256,143 @@ describe('AbacService (unit)', () => {
12521256
]);
12531257
});
12541258
});
1259+
describe('buildRoomNonCompliantConditionsFromSubject (private)', () => {
1260+
const invoke = (defs: IAbacAttributeDefinition[]) => (service as any).buildRoomNonCompliantConditionsFromSubject(defs) as any[];
1261+
1262+
it('returns a single $nin condition when given no subject attributes', () => {
1263+
const result = invoke([]);
1264+
expect(result).toHaveLength(1);
1265+
expect(result[0]).toEqual({
1266+
abacAttributes: {
1267+
$elemMatch: {
1268+
key: { $nin: [] },
1269+
},
1270+
},
1271+
});
1272+
});
1273+
1274+
it('builds conditions for a single attribute with multiple values', () => {
1275+
const result = invoke([{ key: 'dept', values: ['eng', 'sales'] }]);
1276+
expect(result).toHaveLength(2);
1277+
expect(result[0]).toEqual({
1278+
abacAttributes: {
1279+
$elemMatch: {
1280+
key: { $nin: ['dept'] },
1281+
},
1282+
},
1283+
});
1284+
expect(result[1]).toEqual({
1285+
abacAttributes: {
1286+
$elemMatch: {
1287+
key: 'dept',
1288+
values: { $elemMatch: { $nin: ['eng', 'sales'] } },
1289+
},
1290+
},
1291+
});
1292+
});
1293+
1294+
it('deduplicates attribute values and preserves key insertion order', () => {
1295+
const defs: IAbacAttributeDefinition[] = [
1296+
{ key: 'dept', values: ['eng', 'sales', 'eng'] },
1297+
{ key: 'region', values: ['emea', 'emea', 'apac'] },
1298+
];
1299+
const result = invoke(defs);
1300+
expect(result).toHaveLength(3);
1301+
expect(result[0]).toEqual({
1302+
abacAttributes: {
1303+
$elemMatch: {
1304+
key: { $nin: ['dept', 'region'] },
1305+
},
1306+
},
1307+
});
1308+
expect(result[1]).toEqual({
1309+
abacAttributes: {
1310+
$elemMatch: {
1311+
key: 'dept',
1312+
values: { $elemMatch: { $nin: ['eng', 'sales'] } },
1313+
},
1314+
},
1315+
});
1316+
expect(result[2]).toEqual({
1317+
abacAttributes: {
1318+
$elemMatch: {
1319+
key: 'region',
1320+
values: { $elemMatch: { $nin: ['emea', 'apac'] } },
1321+
},
1322+
},
1323+
});
1324+
});
1325+
1326+
it('overrides duplicated keys using the last occurrence only', () => {
1327+
const defs: IAbacAttributeDefinition[] = [
1328+
{ key: 'dept', values: ['eng', 'sales'] },
1329+
{ key: 'dept', values: ['support'] },
1330+
];
1331+
const result = invoke(defs);
1332+
expect(result).toHaveLength(2);
1333+
expect(result[0]).toEqual({
1334+
abacAttributes: {
1335+
$elemMatch: {
1336+
key: { $nin: ['dept'] },
1337+
},
1338+
},
1339+
});
1340+
expect(result[1]).toEqual({
1341+
abacAttributes: {
1342+
$elemMatch: {
1343+
key: 'dept',
1344+
values: { $elemMatch: { $nin: ['support'] } },
1345+
},
1346+
},
1347+
});
1348+
});
1349+
1350+
it('is resilient to mixed ordering of attributes', () => {
1351+
const defs: IAbacAttributeDefinition[] = [
1352+
{ key: 'b', values: ['2', '1'] },
1353+
{ key: 'a', values: ['x'] },
1354+
{ key: 'c', values: ['z', 'z', 'y'] },
1355+
];
1356+
const result = invoke(defs);
1357+
expect(result).toHaveLength(4);
1358+
expect(result[0]).toEqual({
1359+
abacAttributes: {
1360+
$elemMatch: {
1361+
key: { $nin: ['b', 'a', 'c'] },
1362+
},
1363+
},
1364+
});
1365+
expect(result[1]).toEqual({
1366+
abacAttributes: {
1367+
$elemMatch: {
1368+
key: 'b',
1369+
values: { $elemMatch: { $nin: ['2', '1'] } },
1370+
},
1371+
},
1372+
});
1373+
expect(result[2]).toEqual({
1374+
abacAttributes: {
1375+
$elemMatch: {
1376+
key: 'a',
1377+
values: { $elemMatch: { $nin: ['x'] } },
1378+
},
1379+
},
1380+
});
1381+
expect(result[3]).toEqual({
1382+
abacAttributes: {
1383+
$elemMatch: {
1384+
key: 'c',
1385+
values: { $elemMatch: { $nin: ['z', 'y'] } },
1386+
},
1387+
},
1388+
});
1389+
});
1390+
1391+
it('does not mutate the input definitions array or their internal values', () => {
1392+
const defs: IAbacAttributeDefinition[] = [{ key: 'dept', values: ['eng', 'sales'] }];
1393+
const copy = JSON.parse(JSON.stringify(defs));
1394+
invoke(defs);
1395+
expect(defs).toEqual(copy);
1396+
});
1397+
});
12551398
});

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

Lines changed: 89 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,16 +52,16 @@ export class AbacService extends ServiceClass implements IAbacService {
5252

5353
if (!finalAttributes.length) {
5454
if (Array.isArray(user.abacAttributes) && user.abacAttributes.length) {
55-
await Users.updateOne({ _id: user._id }, { $unset: { abacAttributes: 1 } });
56-
await this.onSubjectAttributesChanged(user, []);
55+
const finalUser = await Users.unsetAbacAttributesById(user._id);
56+
await this.onSubjectAttributesChanged(finalUser!, []);
5757
}
5858
return;
5959
}
6060

61-
await Users.updateOne({ _id: user._id }, { $set: { abacAttributes: finalAttributes } });
61+
const finalUser = await Users.setAbacAttributesById(user._id, finalAttributes);
6262

6363
if (this.didSubjectLoseAttributes(user?.abacAttributes || [], finalAttributes)) {
64-
await this.onSubjectAttributesChanged(user, finalAttributes);
64+
await this.onSubjectAttributesChanged(finalUser!, finalAttributes);
6565
}
6666

6767
this.logger.debug({
@@ -748,8 +748,91 @@ export class AbacService extends ServiceClass implements IAbacService {
748748
return false;
749749
}
750750

751-
protected async onSubjectAttributesChanged(_user: IUser, _next: IAbacAttributeDefinition[]): Promise<void> {
752-
// no-op (hook point for when a user loses an ABAC attribute or value)
751+
protected async onSubjectAttributesChanged(user: IUser, _next: IAbacAttributeDefinition[]): Promise<void> {
752+
if (!user?._id || !Array.isArray(user.__rooms) || !user.__rooms.length) {
753+
return;
754+
}
755+
756+
const roomIds: string[] = user.__rooms;
757+
758+
try {
759+
// No attributes: no rooms :(
760+
if (!_next.length) {
761+
const cursor = Rooms.find(
762+
{
763+
_id: { $in: roomIds },
764+
abacAttributes: { $exists: true, $ne: [] },
765+
},
766+
{ projection: { _id: 1 } },
767+
);
768+
769+
const removalPromises: Promise<void>[] = [];
770+
for await (const room of cursor) {
771+
removalPromises.push(
772+
limit(() =>
773+
Room.removeUserFromRoom(room._id, user, {
774+
skipAppPreEvents: true,
775+
customSystemMessage: 'abac-removed-user-from-room' as const,
776+
}),
777+
),
778+
);
779+
}
780+
781+
await Promise.all(removalPromises);
782+
return;
783+
}
784+
785+
const query = {
786+
_id: { $in: roomIds },
787+
$or: this.buildRoomNonCompliantConditionsFromSubject(_next),
788+
};
789+
790+
const cursor = Rooms.find(query, { projection: { _id: 1 } });
791+
792+
const removalPromises: Promise<unknown>[] = [];
793+
for await (const room of cursor) {
794+
removalPromises.push(
795+
limit(() =>
796+
Room.removeUserFromRoom(room._id, user, {
797+
skipAppPreEvents: true,
798+
customSystemMessage: 'abac-removed-user-from-room' as const,
799+
}),
800+
),
801+
);
802+
}
803+
804+
await Promise.all(removalPromises);
805+
} catch (err) {
806+
this.logger.error({
807+
msg: 'Failed to query and remove user from non-compliant ABAC rooms',
808+
err,
809+
});
810+
}
811+
}
812+
813+
private buildRoomNonCompliantConditionsFromSubject(subjectAttributes: IAbacAttributeDefinition[]) {
814+
const map = new Map(subjectAttributes.map((a) => [a.key, new Set(a.values)]));
815+
const userKeys = Array.from(map.keys());
816+
const conditions = [];
817+
conditions.push({
818+
abacAttributes: {
819+
$elemMatch: {
820+
key: { $nin: userKeys },
821+
},
822+
},
823+
});
824+
for (const [key, valuesSet] of map.entries()) {
825+
const valuesArr = Array.from(valuesSet);
826+
conditions.push({
827+
abacAttributes: {
828+
$elemMatch: {
829+
key,
830+
values: { $elemMatch: { $nin: valuesArr } },
831+
},
832+
},
833+
});
834+
}
835+
return conditions;
753836
}
754837
}
755838

0 commit comments

Comments
 (0)