Skip to content

feat: Remove users from rooms when subject attributes no longer match #37506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tassoevan merged 5 commits into from
Nov 19, 2025
Merged

feat: Remove users from rooms when subject attributes no longer match #37506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6cc53a7
on subject attribute changed
KevLehman Nov 13, 2025
1e513f0
findoneandupdate
KevLehman Nov 14, 2025
7346eb5
move function to model
KevLehman Nov 14, 2025
0c716c5
timeout
KevLehman Nov 14, 2025
f5f152d
double closing
KevLehman Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 151 additions & 8 deletions ee/packages/abac/src/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ const mockRemoveAbacAttributeByRoomIdAndKey = jest.fn();
const mockInsertAbacAttributeIfNotExistsById = jest.fn();
const mockUsersFind = jest.fn();
const mockUsersUpdateOne = jest.fn();
const mockUsersSetAbacAttributesById = jest.fn();
const mockUsersUnsetAbacAttributesById = jest.fn();

jest.mock('@rocket.chat/models', () => ({
Rooms: {
Expand All @@ -43,6 +45,9 @@ jest.mock('@rocket.chat/models', () => ({
},
Users: {
find: (...args: any[]) => mockUsersFind(...args),
setAbacAttributesById: (...args: any[]) => mockUsersSetAbacAttributesById(...args),
unsetAbacAttributesById: (...args: any[]) => mockUsersUnsetAbacAttributesById(...args),
findOneAndUpdate: (...args: any[]) => mockUsersUpdateOne(...args),
updateOne: (...args: any[]) => mockUsersUpdateOne(...args),
},
}));
Expand All @@ -69,8 +74,8 @@ describe('AbacService (unit)', () => {

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

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

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

expect(mockUsersUpdateOne).toHaveBeenCalledTimes(1);
expect(mockUsersSetAbacAttributesById).toHaveBeenCalledTimes(1);
const final = getUpdatedAttributesFromCall();
expect(final).toBeDefined();
expect(final).toHaveLength(1);
Expand Down Expand Up @@ -130,8 +135,7 @@ describe('AbacService (unit)', () => {

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

const unsetCall = mockUsersUpdateOne.mock.calls.find((c) => c[1]?.$unset?.abacAttributes);
expect(unsetCall).toBeDefined();
expect(mockUsersUnsetAbacAttributesById).toHaveBeenCalledTimes(1);
});

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

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

expect(mockUsersUpdateOne).not.toHaveBeenCalled();
expect(mockUsersSetAbacAttributesById).not.toHaveBeenCalled();
expect(mockUsersUnsetAbacAttributesById).not.toHaveBeenCalled();
});

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

const unsetCall = mockUsersUpdateOne.mock.calls.find((c) => c[1]?.$unset?.abacAttributes);
expect(unsetCall).toBeDefined();
expect(mockUsersUnsetAbacAttributesById).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledTimes(1);
expect(spy.mock.calls[0][1]).toEqual([]);
});
Expand Down Expand Up @@ -1252,4 +1256,143 @@ describe('AbacService (unit)', () => {
]);
});
});
describe('buildRoomNonCompliantConditionsFromSubject (private)', () => {
const invoke = (defs: IAbacAttributeDefinition[]) => (service as any).buildRoomNonCompliantConditionsFromSubject(defs) as any[];

it('returns a single $nin condition when given no subject attributes', () => {
const result = invoke([]);
expect(result).toHaveLength(1);
expect(result[0]).toEqual({
abacAttributes: {
$elemMatch: {
key: { $nin: [] },
},
},
});
});

it('builds conditions for a single attribute with multiple values', () => {
const result = invoke([{ key: 'dept', values: ['eng', 'sales'] }]);
expect(result).toHaveLength(2);
expect(result[0]).toEqual({
abacAttributes: {
$elemMatch: {
key: { $nin: ['dept'] },
},
},
});
expect(result[1]).toEqual({
abacAttributes: {
$elemMatch: {
key: 'dept',
values: { $elemMatch: { $nin: ['eng', 'sales'] } },
},
},
});
});

it('deduplicates attribute values and preserves key insertion order', () => {
const defs: IAbacAttributeDefinition[] = [
{ key: 'dept', values: ['eng', 'sales', 'eng'] },
{ key: 'region', values: ['emea', 'emea', 'apac'] },
];
const result = invoke(defs);
expect(result).toHaveLength(3);
expect(result[0]).toEqual({
abacAttributes: {
$elemMatch: {
key: { $nin: ['dept', 'region'] },
},
},
});
expect(result[1]).toEqual({
abacAttributes: {
$elemMatch: {
key: 'dept',
values: { $elemMatch: { $nin: ['eng', 'sales'] } },
},
},
});
expect(result[2]).toEqual({
abacAttributes: {
$elemMatch: {
key: 'region',
values: { $elemMatch: { $nin: ['emea', 'apac'] } },
},
},
});
});

it('overrides duplicated keys using the last occurrence only', () => {
const defs: IAbacAttributeDefinition[] = [
{ key: 'dept', values: ['eng', 'sales'] },
{ key: 'dept', values: ['support'] },
];
const result = invoke(defs);
expect(result).toHaveLength(2);
expect(result[0]).toEqual({
abacAttributes: {
$elemMatch: {
key: { $nin: ['dept'] },
},
},
});
expect(result[1]).toEqual({
abacAttributes: {
$elemMatch: {
key: 'dept',
values: { $elemMatch: { $nin: ['support'] } },
},
},
});
});

it('is resilient to mixed ordering of attributes', () => {
const defs: IAbacAttributeDefinition[] = [
{ key: 'b', values: ['2', '1'] },
{ key: 'a', values: ['x'] },
{ key: 'c', values: ['z', 'z', 'y'] },
];
const result = invoke(defs);
expect(result).toHaveLength(4);
expect(result[0]).toEqual({
abacAttributes: {
$elemMatch: {
key: { $nin: ['b', 'a', 'c'] },
},
},
});
expect(result[1]).toEqual({
abacAttributes: {
$elemMatch: {
key: 'b',
values: { $elemMatch: { $nin: ['2', '1'] } },
},
},
});
expect(result[2]).toEqual({
abacAttributes: {
$elemMatch: {
key: 'a',
values: { $elemMatch: { $nin: ['x'] } },
},
},
});
expect(result[3]).toEqual({
abacAttributes: {
$elemMatch: {
key: 'c',
values: { $elemMatch: { $nin: ['z', 'y'] } },
},
},
});
});

it('does not mutate the input definitions array or their internal values', () => {
const defs: IAbacAttributeDefinition[] = [{ key: 'dept', values: ['eng', 'sales'] }];
const copy = JSON.parse(JSON.stringify(defs));
invoke(defs);
expect(defs).toEqual(copy);
});
});
});
95 changes: 89 additions & 6 deletions ee/packages/abac/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,16 @@ export class AbacService extends ServiceClass implements IAbacService {

if (!finalAttributes.length) {
if (Array.isArray(user.abacAttributes) && user.abacAttributes.length) {
await Users.updateOne({ _id: user._id }, { $unset: { abacAttributes: 1 } });
await this.onSubjectAttributesChanged(user, []);
const finalUser = await Users.unsetAbacAttributesById(user._id);
await this.onSubjectAttributesChanged(finalUser!, []);
}
return;
}

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

if (this.didSubjectLoseAttributes(user?.abacAttributes || [], finalAttributes)) {
await this.onSubjectAttributesChanged(user, finalAttributes);
await this.onSubjectAttributesChanged(finalUser!, finalAttributes);
}

this.logger.debug({
Expand Down Expand Up @@ -748,8 +748,91 @@ export class AbacService extends ServiceClass implements IAbacService {
return false;
}

protected async onSubjectAttributesChanged(_user: IUser, _next: IAbacAttributeDefinition[]): Promise<void> {
// no-op (hook point for when a user loses an ABAC attribute or value)
protected async onSubjectAttributesChanged(user: IUser, _next: IAbacAttributeDefinition[]): Promise<void> {
if (!user?._id || !Array.isArray(user.__rooms) || !user.__rooms.length) {
return;
}

const roomIds: string[] = user.__rooms;

try {
// No attributes: no rooms :(
if (!_next.length) {
const cursor = Rooms.find(
{
_id: { $in: roomIds },
abacAttributes: { $exists: true, $ne: [] },
},
{ projection: { _id: 1 } },
);

const removalPromises: Promise<void>[] = [];
for await (const room of cursor) {
removalPromises.push(
limit(() =>
Room.removeUserFromRoom(room._id, user, {
skipAppPreEvents: true,
customSystemMessage: 'abac-removed-user-from-room' as const,
}),
),
);
}

await Promise.all(removalPromises);
return;
}

const query = {
_id: { $in: roomIds },
$or: this.buildRoomNonCompliantConditionsFromSubject(_next),
};

const cursor = Rooms.find(query, { projection: { _id: 1 } });

const removalPromises: Promise<unknown>[] = [];
for await (const room of cursor) {
removalPromises.push(
limit(() =>
Room.removeUserFromRoom(room._id, user, {
skipAppPreEvents: true,
customSystemMessage: 'abac-removed-user-from-room' as const,
}),
),
);
}

await Promise.all(removalPromises);
} catch (err) {
this.logger.error({
msg: 'Failed to query and remove user from non-compliant ABAC rooms',
err,
});
}
}

private buildRoomNonCompliantConditionsFromSubject(subjectAttributes: IAbacAttributeDefinition[]) {
const map = new Map(subjectAttributes.map((a) => [a.key, new Set(a.values)]));
const userKeys = Array.from(map.keys());
const conditions = [];
conditions.push({
abacAttributes: {
$elemMatch: {
key: { $nin: userKeys },
},
},
});
for (const [key, valuesSet] of map.entries()) {
const valuesArr = Array.from(valuesSet);
conditions.push({
abacAttributes: {
$elemMatch: {
key,
values: { $elemMatch: { $nin: valuesArr } },
},
},
});
}
return conditions;
}
}

Expand Down
Loading
Loading