From 6cc53a74ada15be22e437c74c0826b409e60a3d2 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Thu, 13 Nov 2025 15:07:47 -0600 Subject: [PATCH 1/5] on subject attribute changed --- ee/packages/abac/src/index.spec.ts | 139 +++++ ee/packages/abac/src/index.ts | 88 +++- .../subject-attributes-validations.spec.ts | 481 ++++++++++++++++++ 3 files changed, 706 insertions(+), 2 deletions(-) create mode 100644 ee/packages/abac/src/subject-attributes-validations.spec.ts diff --git a/ee/packages/abac/src/index.spec.ts b/ee/packages/abac/src/index.spec.ts index 2f7b0c4b6de19..cabaa73e976f6 100644 --- a/ee/packages/abac/src/index.spec.ts +++ b/ee/packages/abac/src/index.spec.ts @@ -1252,4 +1252,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); + }); + }); }); diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index ba03bea2048fa..4661ab137329f 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -748,8 +748,92 @@ export class AbacService extends ServiceClass implements IAbacService { return false; } - protected async onSubjectAttributesChanged(_user: IUser, _next: IAbacAttributeDefinition[]): Promise { - // no-op (hook point for when a user loses an ABAC attribute or value) + protected async onSubjectAttributesChanged(user: IUser, _next: IAbacAttributeDefinition[]): Promise { + 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[] = []; + 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[] = []; + for await (const room of cursor) { + console.log(room); + 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; } } diff --git a/ee/packages/abac/src/subject-attributes-validations.spec.ts b/ee/packages/abac/src/subject-attributes-validations.spec.ts new file mode 100644 index 0000000000000..8efa764f4ae9a --- /dev/null +++ b/ee/packages/abac/src/subject-attributes-validations.spec.ts @@ -0,0 +1,481 @@ +import type { ILDAPEntry, IUser, IAbacAttributeDefinition } from '@rocket.chat/core-typings'; +import { registerServiceModels, Users } from '@rocket.chat/models'; +import type { Collection, Db } from 'mongodb'; +import { MongoClient } from 'mongodb'; +import { MongoMemoryServer } from 'mongodb-memory-server'; + +import { AbacService } from './index'; + +let mongo: MongoMemoryServer; +let client: MongoClient; +let db: Db; + +beforeAll(async () => { + mongo = await MongoMemoryServer.create(); + client = await MongoClient.connect(mongo.getUri(), {}); + db = client.db('abac_global'); + registerServiceModels(db); +}); + +afterAll(async () => { + await client.close(); + await mongo.stop(); +}); + +jest.mock('@rocket.chat/core-services', () => ({ + ServiceClass: class {}, + Room: { + removeUserFromRoom: jest.fn(), + }, + MeteorError: class extends Error {}, +})); + +const makeUser = (overrides: Partial = {}): IUser => + ({ + _id: 'u1', + username: 'user1', + roles: [], + type: 'user', + active: true, + createdAt: new Date(), + _updatedAt: new Date(), + ...overrides, + }) as IUser; + +const makeLdap = (overrides: Partial = {}): ILDAPEntry => + ({ + ...overrides, + }) as ILDAPEntry; + +describe('AbacService.addSubjectAttributes (unit)', () => { + let service: AbacService; + + beforeEach(async () => { + await Users.deleteMany({}); + service = new AbacService(); + }); + + describe('early returns and no-ops', () => { + it('returns early when user has no _id', async () => { + const user = makeUser({ _id: undefined }); + await service.addSubjectAttributes(user, makeLdap(), { group: 'dept' }); + // Nothing inserted, ensure no user doc created + const found = await Users.findOne({ username: user.username }); + expect(found).toBeFalsy(); + }); + + it('does nothing (no update) when map produces no attributes and user had none', async () => { + const user = makeUser({ abacAttributes: undefined }); + await Users.insertOne(user); + const ldap = makeLdap({ group: '' }); + await service.addSubjectAttributes(user, ldap, { group: 'dept' }); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated).toBeTruthy(); + expect(updated?.abacAttributes ?? undefined).toBeUndefined(); + }); + }); + + describe('building and setting attributes', () => { + it('merges multiple LDAP keys mapping to the same ABAC key, deduplicating values', async () => { + const user = makeUser(); + await Users.insertOne(user); + const ldap = makeLdap({ + memberOf: ['eng', 'sales', 'eng'], + department: ['sales', 'support'], + }); + const map = { memberOf: 'dept', department: 'dept' }; + await service.addSubjectAttributes(user, ldap, map); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toEqual([{ key: 'dept', values: ['eng', 'sales', 'support'] }]); + }); + + it('creates distinct ABAC attributes for different mapped keys preserving insertion order', async () => { + const user = makeUser(); + await Users.insertOne(user); + const ldap = makeLdap({ + groups: ['alpha', 'beta'], + regionCodes: ['emea', 'apac'], + role: 'admin', + }); + const map = { groups: 'team', regionCodes: 'region', role: 'role' }; + await service.addSubjectAttributes(user, ldap, map); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toEqual([ + { key: 'team', values: ['alpha', 'beta'] }, + { key: 'region', values: ['emea', 'apac'] }, + { key: 'role', values: ['admin'] }, + ]); + }); + + it('merges array and string LDAP values into one attribute', async () => { + const user = makeUser(); + await Users.insertOne(user); + const ldap = makeLdap({ deptCode: 'eng', deptName: ['engineering', 'eng'] }); + const map = { deptCode: 'dept', deptName: 'dept' }; + await service.addSubjectAttributes(user, ldap, map); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toEqual([{ key: 'dept', values: ['eng', 'engineering'] }]); + }); + }); + + describe('unsetting attributes when none extracted', () => { + it('unsets abacAttributes when user previously had attributes but now extracts none', async () => { + const user = makeUser({ abacAttributes: [{ key: 'dept', values: ['eng', 'sales'] }] }); + await Users.insertOne(user); + const ldap = makeLdap({ other: ['x'] }); + const map = { missing: 'dept' }; + await service.addSubjectAttributes(user, ldap, map); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toBeUndefined(); + }); + + it('does not unset when user had no prior attributes and extraction yields none', async () => { + const user = makeUser({ abacAttributes: [] }); + await Users.insertOne(user); + const ldap = makeLdap({}); + const map = { missing: 'dept' }; + await service.addSubjectAttributes(user, ldap, map); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toEqual([]); + }); + }); + + describe('loss detection triggering hook (attribute changes)', () => { + it('updates attributes reducing values on loss', async () => { + const user = makeUser({ abacAttributes: [{ key: 'dept', values: ['eng', 'qa'] }] }); + await Users.insertOne(user); + const ldap = makeLdap({ memberOf: ['eng'] }); + await service.addSubjectAttributes(user, ldap, { memberOf: 'dept' }); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toEqual([{ key: 'dept', values: ['eng'] }]); + }); + + it('updates attributes removing an entire key', async () => { + const user = makeUser({ + abacAttributes: [ + { key: 'dept', values: ['eng'] }, + { key: 'region', values: ['emea'] }, + ], + }); + await Users.insertOne(user); + const ldap = makeLdap({ department: ['eng'] }); + await service.addSubjectAttributes(user, ldap, { department: 'dept' }); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toEqual([{ key: 'dept', values: ['eng'] }]); + }); + + it('gains new values without triggering loss logic', async () => { + const user = makeUser({ abacAttributes: [{ key: 'dept', values: ['eng'] }] }); + await Users.insertOne(user); + const ldap = makeLdap({ memberOf: ['eng', 'qa'] }); + await service.addSubjectAttributes(user, ldap, { memberOf: 'dept' }); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toEqual([{ key: 'dept', values: ['eng', 'qa'] }]); + }); + + it('keeps attributes unchanged when only ordering differs', async () => { + const user = makeUser({ abacAttributes: [{ key: 'dept', values: ['eng', 'qa'] }] }); + await Users.insertOne(user); + const ldap = makeLdap({ memberOf: ['qa', 'eng'] }); + await service.addSubjectAttributes(user, ldap, { memberOf: 'dept' }); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes?.[0].key).toBe('dept'); + expect(new Set(updated?.abacAttributes?.[0].values)).toEqual(new Set(['eng', 'qa'])); + }); + + it('merges duplicate LDAP mapping keys retaining union of values', async () => { + const user = makeUser({ abacAttributes: [{ key: 'dept', values: ['eng', 'sales'] }] }); + await Users.insertOne(user); + const ldap = makeLdap({ deptA: ['eng', 'sales'], deptB: ['eng'] }); + await service.addSubjectAttributes(user, ldap, { deptA: 'dept', deptB: 'dept' }); + const updated = await Users.findOneById(user._id, { projection: { abacAttributes: 1 } }); + expect(updated?.abacAttributes).toEqual([{ key: 'dept', values: ['eng', 'sales'] }]); + }); + }); + + describe('input immutability', () => { + it('does not mutate original user.abacAttributes array reference contents', async () => { + const original = [{ key: 'dept', values: ['eng', 'sales'] }] as IAbacAttributeDefinition[]; + const user = makeUser({ abacAttributes: original }); + await Users.insertOne(user); + const clone = JSON.parse(JSON.stringify(original)); + const ldap = makeLdap({ memberOf: ['eng', 'sales', 'support'] }); + await service.addSubjectAttributes(user, ldap, { memberOf: 'dept' }); + expect(original).toEqual(clone); + }); + }); +}); + +describe('AbacService.addSubjectAttributes (room removals)', () => { + let service: AbacService; + let roomsCol: Collection; + let usersCol: Collection; + + const originalCoreServices = jest.requireMock('@rocket.chat/core-services'); + originalCoreServices.Room.removeUserFromRoom = async (rid: string, user: IUser) => { + // @ts-expect-error - test + await usersCol.updateOne({ _id: user._id }, { $pull: { __rooms: rid } }); + }; + + const insertRoom = async (room: { _id: string; abacAttributes?: IAbacAttributeDefinition[] }) => + roomsCol.insertOne({ + _id: room._id, + name: room._id, + t: 'p', + abacAttributes: room.abacAttributes, + }); + + const insertUser = async (user: IUser & { __rooms?: string[] }) => + usersCol.insertOne({ + ...user, + __rooms: user.__rooms || [], + }); + + afterAll(async () => { + await client.close(); + await mongo.stop(); + }); + + beforeEach(async () => { + service = new AbacService(); + roomsCol = db.collection('rocketchat_room'); + usersCol = db.collection('users'); + await Promise.all([roomsCol.deleteMany({}), usersCol.deleteMany({})]); + }); + + it('removes user from rooms whose attributes become non-compliant after losing a value', async () => { + const user: IUser = { + _id: 'u-loss', + username: 'lossy', + roles: [], + type: 'user', + active: true, + createdAt: new Date(), + _updatedAt: new Date(), + abacAttributes: [{ key: 'dept', values: ['eng', 'qa'] }], + __rooms: ['rKeep', 'rRemove'], + }; + + // Rooms: + // rKeep requires only 'eng' (will remain compliant) + // rRemove requires both 'eng' and 'qa' (will become non-compliant after loss) + await Promise.all([ + insertRoom({ _id: 'rKeep', abacAttributes: [{ key: 'dept', values: ['eng'] }] }), + insertRoom({ _id: 'rRemove', abacAttributes: [{ key: 'dept', values: ['eng', 'qa'] }] }), + ]); + + await insertUser({ ...user, __rooms: ['rKeep', 'rRemove'] }); + + const ldap: ILDAPEntry = { + memberOf: ['eng'], + _raw: {}, + }; + + await service.addSubjectAttributes(user, ldap, { memberOf: 'dept' }); + + const updatedUser = await usersCol.findOne({ _id: user._id }, { projection: { abacAttributes: 1, __rooms: 1 } }); + expect(updatedUser?.abacAttributes).toEqual([{ key: 'dept', values: ['eng'] }]); + expect(updatedUser?.abacAttributes).not.toEqual(user.abacAttributes); + expect(updatedUser?.__rooms.sort()).toEqual(['rKeep']); + }); + + it('removes user from rooms containing attribute keys they no longer possess (key loss)', async () => { + const user: IUser = { + _id: 'u-key-loss', + username: 'keyloss', + roles: [], + type: 'user', + active: true, + createdAt: new Date(), + _updatedAt: new Date(), + abacAttributes: [ + { key: 'dept', values: ['eng'] }, + { key: 'region', values: ['emea'] }, + ], + __rooms: ['rDeptOnly', 'rRegionOnly', 'rBoth'], + }; + + // Rooms: + // rDeptOnly -> only dept (will stay) + // rRegionOnly -> region only (will be removed after region key loss) + // rBoth -> both dept & region (will be removed) + await Promise.all([ + insertRoom({ _id: 'rDeptOnly', abacAttributes: [{ key: 'dept', values: ['eng'] }] }), + insertRoom({ _id: 'rRegionOnly', abacAttributes: [{ key: 'region', values: ['emea'] }] }), + insertRoom({ + _id: 'rBoth', + abacAttributes: [ + { key: 'dept', values: ['eng'] }, + { key: 'region', values: ['emea'] }, + ], + }), + ]); + + await insertUser({ ...user, __rooms: ['rDeptOnly', 'rRegionOnly', 'rBoth'] }); + + const ldap: ILDAPEntry = { + department: ['eng'], + _raw: {}, + }; + + await service.addSubjectAttributes(user, ldap, { department: 'dept' }); + + const updatedUser = await usersCol.findOne({ _id: user._id }, { projection: { abacAttributes: 1, __rooms: 1 } }); + expect(updatedUser?.abacAttributes).toEqual([{ key: 'dept', values: ['eng'] }]); + expect(updatedUser?.abacAttributes).not.toEqual(user.abacAttributes); + expect(updatedUser?.__rooms).toEqual(['rDeptOnly']); + }); + + it('does not remove user from any room when attribute values only grow (gain without loss)', async () => { + const user: IUser = { + _id: 'u-growth', + username: 'growth', + roles: [], + type: 'user', + active: true, + createdAt: new Date(), + _updatedAt: new Date(), + abacAttributes: [{ key: 'dept', values: ['eng'] }], + __rooms: ['rGrowthA', 'rGrowthB'], + }; + + await Promise.all([ + insertRoom({ _id: 'rGrowthA', abacAttributes: [{ key: 'dept', values: ['eng'] }] }), + insertRoom({ _id: 'rGrowthB', abacAttributes: [{ key: 'dept', values: ['eng', 'qa'] }] }), // superset; still compliant after growth + ]); + + await insertUser({ ...user, __rooms: ['rGrowthA', 'rGrowthB'] }); + + const ldap: ILDAPEntry = { + memberOf: ['eng', 'qa'], + _raw: {}, + }; + + await service.addSubjectAttributes(user, ldap, { memberOf: 'dept' }); + + const updatedUser = await usersCol.findOne({ _id: user._id }, { projection: { abacAttributes: 1, __rooms: 1 } }); + expect(updatedUser?.abacAttributes).toEqual([{ key: 'dept', values: ['eng', 'qa'] }]); + expect(updatedUser?.__rooms.sort()).toEqual(['rGrowthA', 'rGrowthB']); + }); + + it('removes user from rooms having attribute keys not present in new attribute set (extra keys in room)', async () => { + const user: IUser = { + _id: 'u-extra-room-key', + username: 'extrakey', + roles: [], + type: 'user', + active: true, + createdAt: new Date(), + _updatedAt: new Date(), + abacAttributes: [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'otherKey', values: ['value'] }, + ], + __rooms: ['rExtraKeyRoom', 'rBaseline'], + }; + + await Promise.all([ + insertRoom({ + _id: 'rExtraKeyRoom', + abacAttributes: [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'project', values: ['X'] }, + ], + }), + insertRoom({ + _id: 'rBaseline', + abacAttributes: [{ key: 'dept', values: ['eng', 'sales'] }], + }), + ]); + + await insertUser({ ...user, __rooms: ['rExtraKeyRoom', 'rBaseline'] }); + + const ldap: ILDAPEntry = { + deptCodes: ['eng', 'sales'], + _raw: {}, + }; + + await service.addSubjectAttributes(user, ldap, { deptCodes: 'dept' }); + + const updatedUser = await usersCol.findOne({ _id: user._id }, { projection: { abacAttributes: 1, __rooms: 1 } }); + expect(updatedUser?.abacAttributes).toEqual([{ key: 'dept', values: ['eng', 'sales'] }]); + expect(updatedUser?.__rooms.sort()).toEqual(['rBaseline']); + }); + + it('unsets attributes and removes user from all ABAC rooms when no LDAP values extracted', async () => { + const user: IUser = { + _id: 'u-empty', + username: 'empty', + roles: [], + type: 'user', + active: true, + createdAt: new Date(), + _updatedAt: new Date(), + abacAttributes: [{ key: 'dept', values: ['eng'] }], + __rooms: ['rAny1', 'rAny2'], + }; + + await Promise.all([ + insertRoom({ _id: 'rAny1', abacAttributes: [{ key: 'dept', values: ['eng'] }] }), + insertRoom({ _id: 'rAny2', abacAttributes: [{ key: 'dept', values: ['eng', 'qa'] }] }), + ]); + + await insertUser({ ...user, __rooms: ['rAny1', 'rAny2'] }); + + const ldap: ILDAPEntry = { + unrelated: ['x'], + _raw: {}, + }; + + await service.addSubjectAttributes(user, ldap, { missing: 'dept' }); + + const updatedUser = await usersCol.findOne({ _id: user._id }, { projection: { abacAttributes: 1, __rooms: 1 } }); + expect(updatedUser?.abacAttributes).toBeUndefined(); + expect(updatedUser?.abacAttributes).not.toEqual(user.abacAttributes); + expect(updatedUser?.__rooms).toEqual([]); + }); + + it('does not remove user from room when losing attribute not used by room (hook runs but no change)', async () => { + const user: IUser = { + _id: 'u-lose-unrelated', + username: 'unrelated', + roles: [], + type: 'user', + active: true, + createdAt: new Date(), + _updatedAt: new Date(), + abacAttributes: [ + { key: 'dept', values: ['eng'] }, + { key: 'region', values: ['emea'] }, + { key: 'project', values: ['X'] }, + ], + __rooms: ['rDeptRegion'], + }; + + await insertRoom({ + _id: 'rDeptRegion', + abacAttributes: [ + { key: 'dept', values: ['eng'] }, + { key: 'region', values: ['emea'] }, + ], + }); + + await insertUser({ ...user, __rooms: ['rDeptRegion'] }); + + const ldap: ILDAPEntry = { + department: ['eng', 'ceo'], + regionCodes: ['emea', 'apac'], + _raw: {}, + }; + + await service.addSubjectAttributes(user, ldap, { department: 'dept', regionCodes: 'region', projectCodes: 'project' }); + + const updatedUser = await usersCol.findOne({ _id: user._id }, { projection: { abacAttributes: 1, __rooms: 1 } }); + expect(updatedUser?.abacAttributes).toEqual([ + { key: 'dept', values: ['eng', 'ceo'] }, + { key: 'region', values: ['emea', 'apac'] }, + ]); + expect(updatedUser?.abacAttributes).not.toEqual(user.abacAttributes); + expect(updatedUser?.__rooms).toEqual(['rDeptRegion']); + }); +}); From 1e513f07998427857de56236737eff5e8e43ddf9 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Fri, 14 Nov 2025 12:45:22 -0600 Subject: [PATCH 2/5] findoneandupdate --- ee/packages/abac/src/index.spec.ts | 1 + ee/packages/abac/src/index.ts | 13 ++++++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/ee/packages/abac/src/index.spec.ts b/ee/packages/abac/src/index.spec.ts index cabaa73e976f6..002c9cc26594a 100644 --- a/ee/packages/abac/src/index.spec.ts +++ b/ee/packages/abac/src/index.spec.ts @@ -43,6 +43,7 @@ jest.mock('@rocket.chat/models', () => ({ }, Users: { find: (...args: any[]) => mockUsersFind(...args), + findOneAndUpdate: (...args: any[]) => mockUsersUpdateOne(...args), updateOne: (...args: any[]) => mockUsersUpdateOne(...args), }, })); diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index 4661ab137329f..3c641c33171c2 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -52,16 +52,20 @@ 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.findOneAndUpdate({ _id: user._id }, { $unset: { abacAttributes: 1 } }, { returnDocument: 'after' }); + await this.onSubjectAttributesChanged(finalUser!, []); } return; } - await Users.updateOne({ _id: user._id }, { $set: { abacAttributes: finalAttributes } }); + const finalUser = await Users.findOneAndUpdate( + { _id: user._id }, + { $set: { abacAttributes: finalAttributes } }, + { returnDocument: 'after' }, + ); if (this.didSubjectLoseAttributes(user?.abacAttributes || [], finalAttributes)) { - await this.onSubjectAttributesChanged(user, finalAttributes); + await this.onSubjectAttributesChanged(finalUser!, finalAttributes); } this.logger.debug({ @@ -791,7 +795,6 @@ export class AbacService extends ServiceClass implements IAbacService { const removalPromises: Promise[] = []; for await (const room of cursor) { - console.log(room); removalPromises.push( limit(() => Room.removeUserFromRoom(room._id, user, { From 7346eb537be8c7f07bc817fe77d27be2e42849aa Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Fri, 14 Nov 2025 13:31:43 -0600 Subject: [PATCH 3/5] move function to model --- ee/packages/abac/src/index.spec.ts | 19 +++++++++++-------- ee/packages/abac/src/index.ts | 8 ++------ .../model-typings/src/models/IUsersModel.ts | 3 +++ packages/models/src/models/Users.ts | 8 ++++++++ 4 files changed, 24 insertions(+), 14 deletions(-) diff --git a/ee/packages/abac/src/index.spec.ts b/ee/packages/abac/src/index.spec.ts index 002c9cc26594a..6dbc49ca6ddfd 100644 --- a/ee/packages/abac/src/index.spec.ts +++ b/ee/packages/abac/src/index.spec.ts @@ -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: { @@ -43,6 +45,8 @@ 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), }, @@ -70,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 () => { @@ -88,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); @@ -131,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 () => { @@ -142,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 () => { @@ -227,8 +231,7 @@ describe('AbacService (unit)', () => { const spy = jest.spyOn(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([]); }); diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index 3c641c33171c2..fd7bf8dd02816 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -52,17 +52,13 @@ export class AbacService extends ServiceClass implements IAbacService { if (!finalAttributes.length) { if (Array.isArray(user.abacAttributes) && user.abacAttributes.length) { - const finalUser = await Users.findOneAndUpdate({ _id: user._id }, { $unset: { abacAttributes: 1 } }, { returnDocument: 'after' }); + const finalUser = await Users.unsetAbacAttributesById(user._id); await this.onSubjectAttributesChanged(finalUser!, []); } return; } - const finalUser = await Users.findOneAndUpdate( - { _id: user._id }, - { $set: { abacAttributes: finalAttributes } }, - { returnDocument: 'after' }, - ); + const finalUser = await Users.setAbacAttributesById(user._id, finalAttributes); if (this.didSubjectLoseAttributes(user?.abacAttributes || [], finalAttributes)) { await this.onSubjectAttributesChanged(finalUser!, finalAttributes); diff --git a/packages/model-typings/src/models/IUsersModel.ts b/packages/model-typings/src/models/IUsersModel.ts index 065a947ee34b3..80bec619c5278 100644 --- a/packages/model-typings/src/models/IUsersModel.ts +++ b/packages/model-typings/src/models/IUsersModel.ts @@ -161,6 +161,9 @@ export interface IUsersModel extends IBaseModel { getUserLanguages(): Promise<{ _id: string; total: number }[]>; + setAbacAttributesById(userId: IUser['_id'], attributes: NonNullable): Promise; + unsetAbacAttributesById(userId: IUser['_id']): Promise; + updateStatusText(_id: IUser['_id'], statusText: string, options?: UpdateOptions): Promise; updateStatusByAppId(appId: string, status: UserStatus): Promise; diff --git a/packages/models/src/models/Users.ts b/packages/models/src/models/Users.ts index 57ad050a78360..3a8b07ba4b849 100644 --- a/packages/models/src/models/Users.ts +++ b/packages/models/src/models/Users.ts @@ -161,6 +161,14 @@ export class UsersRaw extends BaseRaw> implements IU return this.find(query, options); } + setAbacAttributesById(_id: IUser['_id'], attributes: NonNullable) { + return this.findOneAndUpdate({ _id }, { $set: { abacAttributes: attributes } }, { returnDocument: 'after' }); + } + + unsetAbacAttributesById(_id: IUser['_id']) { + return this.findOneAndUpdate({ _id }, { $unset: { abacAttributes: 1 } }, { returnDocument: 'after' }); + } + /** * @param {string} uid * @param {IRole['_id'][]} roles list of role ids From 0c716c5e30ff9f78398ee65780c4a05d41ffa2a8 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Fri, 14 Nov 2025 13:54:20 -0600 Subject: [PATCH 4/5] timeout --- ee/packages/abac/src/subject-attributes-validations.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/packages/abac/src/subject-attributes-validations.spec.ts b/ee/packages/abac/src/subject-attributes-validations.spec.ts index 8efa764f4ae9a..9a9314d5c6c64 100644 --- a/ee/packages/abac/src/subject-attributes-validations.spec.ts +++ b/ee/packages/abac/src/subject-attributes-validations.spec.ts @@ -15,7 +15,7 @@ beforeAll(async () => { client = await MongoClient.connect(mongo.getUri(), {}); db = client.db('abac_global'); registerServiceModels(db); -}); +}, 30_000); afterAll(async () => { await client.close(); From f5f152dec8bdcd63d20565226fdec4ab391e4139 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 17 Nov 2025 13:23:44 -0600 Subject: [PATCH 5/5] double closing --- ee/packages/abac/src/subject-attributes-validations.spec.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/ee/packages/abac/src/subject-attributes-validations.spec.ts b/ee/packages/abac/src/subject-attributes-validations.spec.ts index 9a9314d5c6c64..1c9e7a4aad865 100644 --- a/ee/packages/abac/src/subject-attributes-validations.spec.ts +++ b/ee/packages/abac/src/subject-attributes-validations.spec.ts @@ -231,11 +231,6 @@ describe('AbacService.addSubjectAttributes (room removals)', () => { __rooms: user.__rooms || [], }); - afterAll(async () => { - await client.close(); - await mongo.stop(); - }); - beforeEach(async () => { service = new AbacService(); roomsCol = db.collection('rocketchat_room');