diff --git a/apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts b/apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts index e6b94aea3d259..6ca88c1fdabc5 100644 --- a/apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts +++ b/apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts @@ -1,5 +1,5 @@ import { Team } from '@rocket.chat/core-services'; -import type { IRoom, IRoomWithRetentionPolicy, IUser, MessageTypesValues } from '@rocket.chat/core-typings'; +import type { IRoom, IRoomWithRetentionPolicy, IUser, MessageTypesValues, ITeam } from '@rocket.chat/core-typings'; import { TEAM_TYPE } from '@rocket.chat/core-typings'; import type { ServerMethods } from '@rocket.chat/ddp-client'; import { Rooms, Users } from '@rocket.chat/models'; @@ -62,6 +62,19 @@ type RoomSettingsValidators = { const hasRetentionPolicy = (room: IRoom & { retention?: any }): room is IRoomWithRetentionPolicy => 'retention' in room && room.retention !== undefined; +const isAbacManagedRoom = (room: IRoom): boolean => { + return room.t === 'p' && settings.get('ABAC_Enabled') && Array.isArray(room?.abacAttributes) && room.abacAttributes.length > 0; +}; + +const isAbacManagedTeam = (team: Partial | null, teamRoom: IRoom): boolean => { + return ( + team?.type === TEAM_TYPE.PRIVATE && + settings.get('ABAC_Enabled') && + Array.isArray(teamRoom?.abacAttributes) && + teamRoom.abacAttributes.length > 0 + ); +}; + const validators: RoomSettingsValidators = { async default({ userId, room, value }) { if (!(await hasPermissionAsync(userId, 'view-room-administration'))) { @@ -70,7 +83,7 @@ const validators: RoomSettingsValidators = { action: 'Viewing_room_administration', }); } - if (settings.get('ABAC_Enabled') && value && room?.abacAttributes?.length) { + if (isAbacManagedRoom(room) && value) { throw new Meteor.Error('error-action-not-allowed', 'Setting an ABAC managed room as default is not allowed', { method: 'saveRoomSettings', action: 'Viewing_room_administration', @@ -105,7 +118,7 @@ const validators: RoomSettingsValidators = { }); } - if (settings.get('ABAC_Enabled') && room.t === 'p' && value !== 'p' && room?.abacAttributes?.length) { + if (isAbacManagedRoom(room) && value !== 'p') { throw new Meteor.Error('error-action-not-allowed', 'Changing an ABAC managed private room to public is not allowed', { method: 'saveRoomSettings', action: 'Change_Room_Type', @@ -131,7 +144,7 @@ const validators: RoomSettingsValidators = { }); } - if (settings.get('ABAC_Enabled') && team?.type === TEAM_TYPE.PRIVATE && value !== 'p' && room?.abacAttributes?.length) { + if (isAbacManagedTeam(team, room) && value !== 'p') { throw new Meteor.Error('error-action-not-allowed', 'Changing an ABAC managed private team room to public is not allowed', { method: 'saveRoomSettings', action: 'Change_Room_Type', diff --git a/ee/packages/abac/src/can-access-object.spec.ts b/ee/packages/abac/src/can-access-object.spec.ts index bea1f0ff804b4..94a7bec0ecc4d 100644 --- a/ee/packages/abac/src/can-access-object.spec.ts +++ b/ee/packages/abac/src/can-access-object.spec.ts @@ -7,7 +7,7 @@ const mockSubscriptionsFindOneByRoomIdAndUserId = jest.fn(); const mockSubscriptionsSetAbacLastTimeCheckedByUserIdAndRoomId = jest.fn(); const mockUsersFindOne = jest.fn(); const mockUsersFindOneById = jest.fn(); -const mockRoomRemoveUserFromRoom = jest.fn(); +const mockRoomRemoveUserFromRoom = jest.fn().mockResolvedValue(undefined); jest.mock('@rocket.chat/models', () => ({ Settings: { diff --git a/ee/packages/abac/src/helper.spec.ts b/ee/packages/abac/src/helper.spec.ts index 75b53018f52cb..c8421ca27a8fa 100644 --- a/ee/packages/abac/src/helper.spec.ts +++ b/ee/packages/abac/src/helper.spec.ts @@ -1,5 +1,3 @@ -import { expect } from 'chai'; - import { validateAndNormalizeAttributes, MAX_ABAC_ATTRIBUTE_KEYS, @@ -7,8 +5,20 @@ import { diffAttributeSets, buildRoomNonCompliantConditionsFromSubject, extractAttribute, + ensureAttributeDefinitionsExist, } from './helper'; +const attributesFindMock = jest.fn(); +jest.mock('@rocket.chat/models', () => { + return { + AbacAttributes: { + find: jest.fn().mockReturnValue({ + toArray: () => attributesFindMock(), + }), + }, + }; +}); + describe('validateAndNormalizeAttributes', () => { it('normalizes keys and merges duplicate values from multiple entries', () => { const result = validateAndNormalizeAttributes({ @@ -17,7 +27,7 @@ describe('validateAndNormalizeAttributes', () => { 'dept': ['engineering'], }); - expect(result).to.deep.equal([ + expect(result).toEqual([ { key: 'dept', values: ['sales', 'marketing', 'engineering'] }, { key: 'role', values: ['admin', 'user'] }, ]); @@ -28,7 +38,7 @@ describe('validateAndNormalizeAttributes', () => { validateAndNormalizeAttributes({ role: [' ', '\n', '\t'], }), - ).to.throw('error-invalid-attribute-values'); + ).toThrow('error-invalid-attribute-values'); }); it('throws when a key exceeds the maximum number of unique values', () => { @@ -37,12 +47,71 @@ describe('validateAndNormalizeAttributes', () => { validateAndNormalizeAttributes({ role: values, }), - ).to.throw('error-invalid-attribute-values'); + ).toThrow('error-invalid-attribute-values'); + }); + + it('throws when attempting to add a new key beyond MAX_ABAC_ATTRIBUTE_KEYS', () => { + const baseEntries = Object.fromEntries(Array.from({ length: MAX_ABAC_ATTRIBUTE_KEYS }, (_, i) => [`key-${i}`, ['value']])); + + expect(() => + validateAndNormalizeAttributes({ + ...baseEntries, + 'extra-key': ['value'], + }), + ).toThrow('error-invalid-attribute-values'); }); - it('throws when total unique attribute keys exceeds limit', () => { + it('throws when total unique attribute keys exceeds limit (post-aggregation check)', () => { const attributes = Object.fromEntries(Array.from({ length: MAX_ABAC_ATTRIBUTE_KEYS + 1 }, (_, i) => [`key-${i}`, ['value']])); - expect(() => validateAndNormalizeAttributes(attributes)).to.throw('error-invalid-attribute-values'); + + expect(() => validateAndNormalizeAttributes(attributes)).toThrow('error-invalid-attribute-values'); + }); + + it('throws when a key exceeds the maximum number of unique values (bucket size limit)', () => { + const values = Array.from({ length: MAX_ABAC_ATTRIBUTE_VALUES + 1 }, (_, i) => `value-${i}`); + + expect(() => + validateAndNormalizeAttributes({ + role: values, + }), + ).toThrow('error-invalid-attribute-values'); + }); + + it('throws when a key has only invalid/empty values after sanitization', () => { + expect(() => + validateAndNormalizeAttributes({ + role: [' ', '\n', '\t'], + }), + ).toThrow('error-invalid-attribute-values'); + }); +}); + +describe('ensureAttributeDefinitionsExist', () => { + afterEach(() => { + attributesFindMock.mockReset(); + }); + + it('does nothing when normalized is empty', async () => { + await ensureAttributeDefinitionsExist([]); + }); + + it('throws when some attribute definitions are missing (size mismatch)', async () => { + const normalized = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; + + attributesFindMock.mockResolvedValue([{ key: 'dept', values: ['eng', 'sales'] }]); + + await expect(ensureAttributeDefinitionsExist(normalized)).rejects.toThrow('error-attribute-definition-not-found'); + }); + + it('throws when a normalized value is not in the allowed definition values', async () => { + const normalized = [{ key: 'dept', values: ['eng', 'support'] }]; + + attributesFindMock.mockResolvedValue([{ key: 'dept', values: ['eng', 'sales'] }]); + + await expect(ensureAttributeDefinitionsExist(normalized)).rejects.toThrow('error-invalid-attribute-values'); }); }); @@ -50,7 +119,7 @@ describe('diffAttributeSets', () => { it('returns false/false when both sets are empty', () => { const result = diffAttributeSets([], []); - expect(result).to.deep.equal({ added: false, removed: false }); + expect(result).toEqual({ added: false, removed: false }); }); it('detects only additions when moving from empty to non-empty', () => { @@ -62,7 +131,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: true, removed: false }); + expect(result).toEqual({ added: true, removed: false }); }); it('detects only removals when moving from non-empty to empty', () => { @@ -74,7 +143,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: false, removed: true }); + expect(result).toEqual({ added: false, removed: true }); }); it('returns false/false when sets are identical (same keys and values)', () => { @@ -89,7 +158,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: false, removed: false }); + expect(result).toEqual({ added: false, removed: false }); }); it('ignores value ordering and still treats sets as identical', () => { @@ -104,7 +173,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: false, removed: false }); + expect(result).toEqual({ added: false, removed: false }); }); it('detects added values for an existing key', () => { @@ -113,7 +182,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: true, removed: false }); + expect(result).toEqual({ added: true, removed: false }); }); it('detects removed values for an existing key', () => { @@ -122,7 +191,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: false, removed: true }); + expect(result).toEqual({ added: false, removed: true }); }); it('detects added and removed values on the same key', () => { @@ -131,7 +200,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: true, removed: true }); + expect(result).toEqual({ added: true, removed: true }); }); it('detects newly added keys as additions', () => { @@ -143,7 +212,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: true, removed: false }); + expect(result).toEqual({ added: true, removed: false }); }); it('detects removed keys as removals', () => { @@ -155,7 +224,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: false, removed: true }); + expect(result).toEqual({ added: false, removed: true }); }); it('detects both added and removed keys across sets', () => { @@ -170,7 +239,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: true, removed: true }); + expect(result).toEqual({ added: true, removed: true }); }); it('handles mixed changes: new key, removed key, added and removed values', () => { @@ -185,7 +254,7 @@ describe('diffAttributeSets', () => { const result = diffAttributeSets(current, next); - expect(result).to.deep.equal({ added: true, removed: true }); + expect(result).toEqual({ added: true, removed: true }); }); }); @@ -211,14 +280,14 @@ describe('buildNonCompliantConditions', () => { it('returns empty array for empty attributes list', () => { const result = buildNonCompliantConditions([]); - expect(result).to.deep.equal([]); + expect(result).toEqual([]); }); it('maps single attribute to $not $elemMatch query', () => { const attrs: AttributeDef[] = [{ key: 'dept', values: ['eng', 'sales'] }]; const result = buildNonCompliantConditions(attrs); - expect(result).to.deep.equal([ + expect(result).toEqual([ { abacAttributes: { $not: { @@ -240,7 +309,7 @@ describe('buildNonCompliantConditions', () => { const result = buildNonCompliantConditions(attrs); - expect(result).to.deep.equal([ + expect(result).toEqual([ { abacAttributes: { $not: { @@ -271,7 +340,7 @@ describe('buildRoomNonCompliantConditionsFromSubject', () => { const result = buildRoomNonCompliantConditionsFromSubject(subject); - expect(result).to.deep.equal([ + expect(result).toEqual([ { abacAttributes: { $elemMatch: { @@ -298,7 +367,7 @@ describe('buildRoomNonCompliantConditionsFromSubject', () => { const result = buildRoomNonCompliantConditionsFromSubject(subject); - expect(result[0]).to.deep.equal({ + expect(result[0]).toEqual({ abacAttributes: { $elemMatch: { key: { $nin: ['dept', 'role'] }, @@ -306,7 +375,7 @@ describe('buildRoomNonCompliantConditionsFromSubject', () => { }, }); - expect(result[1]).to.deep.equal({ + expect(result[1]).toEqual({ abacAttributes: { $elemMatch: { key: 'dept', @@ -315,7 +384,7 @@ describe('buildRoomNonCompliantConditionsFromSubject', () => { }, }); - expect(result[2]).to.deep.equal({ + expect(result[2]).toEqual({ abacAttributes: { $elemMatch: { key: 'role', @@ -333,7 +402,7 @@ describe('buildRoomNonCompliantConditionsFromSubject', () => { const result = buildRoomNonCompliantConditionsFromSubject(subject); - expect(result[0]).to.deep.equal({ + expect(result[0]).toEqual({ abacAttributes: { $elemMatch: { key: { $nin: ['dept', 'role'] }, @@ -341,7 +410,7 @@ describe('buildRoomNonCompliantConditionsFromSubject', () => { }, }); - expect(result[1]).to.deep.equal({ + expect(result[1]).toEqual({ abacAttributes: { $elemMatch: { key: 'dept', @@ -350,7 +419,7 @@ describe('buildRoomNonCompliantConditionsFromSubject', () => { }, }); - expect(result[2]).to.deep.equal({ + expect(result[2]).toEqual({ abacAttributes: { $elemMatch: { key: 'role', @@ -365,14 +434,14 @@ describe('extractAttribute', () => { it('returns undefined when ldapKey or abacKey is missing', () => { const ldapUser = { memberOf: ['CN=Eng,OU=Groups'] } as any; - expect(extractAttribute(ldapUser, '', 'dept')).to.be.undefined; - expect(extractAttribute(ldapUser, 'memberOf', '')).to.be.undefined; + expect(extractAttribute(ldapUser, '', 'dept')).toBeUndefined(); + expect(extractAttribute(ldapUser, 'memberOf', '')).toBeUndefined(); }); it('returns undefined when ldapUser does not have the provided key', () => { const ldapUser = { other: ['value'] } as any; - expect(extractAttribute(ldapUser, 'memberOf', 'dept')).to.be.undefined; + expect(extractAttribute(ldapUser, 'memberOf', 'dept')).toBeUndefined(); }); it('extracts and normalizes a single string value', () => { @@ -380,7 +449,7 @@ describe('extractAttribute', () => { const result = extractAttribute(ldapUser, 'department', 'dept'); - expect(result).to.deep.equal({ + expect(result).toEqual({ key: 'dept', values: ['Engineering'], }); @@ -394,7 +463,7 @@ describe('extractAttribute', () => { const result = extractAttribute(ldapUser, 'memberOf', 'dept'); // Order is preserved by insertion into the Set in implementation: ['Eng', 'Sales'] - expect(result).to.deep.equal({ + expect(result).toEqual({ key: 'dept', values: ['Eng', 'Sales'], }); @@ -407,7 +476,7 @@ describe('extractAttribute', () => { const result = extractAttribute(ldapUser, 'memberOf', 'dept'); - expect(result).to.deep.equal({ + expect(result).toEqual({ key: 'dept', values: ['Eng'], }); @@ -420,6 +489,6 @@ describe('extractAttribute', () => { const result = extractAttribute(ldapUser, 'memberOf', 'dept'); - expect(result).to.be.undefined; + expect(result).toBeUndefined(); }); }); diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index 1b6e4bcbd3e3f..b8e92ad07bfbf 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -1,11 +1,20 @@ import { MeteorError, Room, ServiceClass } from '@rocket.chat/core-services'; import type { AbacActor, IAbacService } from '@rocket.chat/core-services'; import { AbacAccessOperation, AbacObjectType } from '@rocket.chat/core-typings'; -import type { IAbacAttribute, IAbacAttributeDefinition, IRoom, AtLeast, IUser, ILDAPEntry, ISubscription } from '@rocket.chat/core-typings'; +import type { + IAbacAttribute, + IAbacAttributeDefinition, + IRoom, + AtLeast, + IUser, + ILDAPEntry, + ISubscription, + AbacAuditReason, +} from '@rocket.chat/core-typings'; import { Logger } from '@rocket.chat/logger'; import { Rooms, AbacAttributes, Users, Subscriptions, Settings } from '@rocket.chat/models'; import { escapeRegExp } from '@rocket.chat/string-helpers'; -import type { Document, UpdateFilter } from 'mongodb'; +import type { Document, FindCursor, UpdateFilter } from 'mongodb'; import pLimit from 'p-limit'; import { Audit } from './audit'; @@ -550,12 +559,8 @@ export class AbacService extends ServiceClass implements IAbacService { } // When a user is not compliant, remove them from the room automatically - await Room.removeUserFromRoom(room._id, fullUser, { - skipAppPreEvents: true, - customSystemMessage: 'abac-removed-user-from-room' as const, - }); + await this.removeUserFromRoom(room, fullUser, 'realtime-policy-eval'); - void Audit.actionPerformed({ _id: user._id, username: fullUser.username }, { _id: room._id }, 'realtime-policy-eval'); return false; } @@ -595,22 +600,7 @@ export class AbacService extends ServiceClass implements IAbacService { const userRemovalPromises = []; for await (const doc of cursor) { usersToRemove.push(doc._id); - userRemovalPromises.push( - limit(() => - Room.removeUserFromRoom(rid, doc, { - skipAppPreEvents: true, - customSystemMessage: 'abac-removed-user-from-room' as const, - }) - .then(() => void Audit.actionPerformed({ _id: doc._id, username: doc.username }, { _id: rid }, 'room-attributes-change')) - .catch((err) => { - this.logger.error({ - msg: 'Failed to remove user from ABAC room after room attributes changed', - rid, - err, - }); - }), - ), - ); + userRemovalPromises.push(limit(() => this.removeUserFromRoom(room, doc, 'room-attributes-change'))); } if (!usersToRemove.length) { @@ -627,12 +617,36 @@ export class AbacService extends ServiceClass implements IAbacService { } } + private async removeUserFromRoom(room: AtLeast, user: IUser, reason: AbacAuditReason): Promise { + return Room.removeUserFromRoom(room._id, user, { + skipAppPreEvents: true, + customSystemMessage: 'abac-removed-user-from-room' as const, + }) + .then(() => void Audit.actionPerformed({ _id: user._id, username: user.username }, { _id: room._id }, reason)) + .catch((err) => { + this.logger.error({ + msg: 'Failed to remove user from ABAC room', + rid: room._id, + err, + reason, + }); + }); + } + + private async removeUserFromRoomList(roomList: FindCursor, user: IUser, reason: AbacAuditReason): Promise { + const removalPromises: Promise[] = []; + for await (const room of roomList) { + removalPromises.push(limit(() => this.removeUserFromRoom(room, user, reason))); + } + + await Promise.all(removalPromises); + } + protected async onSubjectAttributesChanged(user: IUser, _next: IAbacAttributeDefinition[]): Promise { if (!user?._id || !Array.isArray(user.__rooms) || !user.__rooms.length) { return; } - - const roomIds: string[] = user.__rooms; + const roomIds = user.__rooms; try { // No attributes: no rooms :( @@ -645,28 +659,7 @@ export class AbacService extends ServiceClass implements IAbacService { { 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, - }) - .then(() => void Audit.actionPerformed({ _id: user._id, username: user.username }, { _id: room._id }, 'ldap-sync')) - .catch((err) => { - this.logger.error({ - msg: 'Failed to remove user from ABAC room after room attributes changed', - rid: room._id, - err, - }); - }), - ), - ); - } - - await Promise.all(removalPromises); - return; + return await this.removeUserFromRoomList(cursor, user, 'ldap-sync'); } const query = { @@ -676,27 +669,7 @@ export class AbacService extends ServiceClass implements IAbacService { const cursor = Rooms.find(query, { 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, - }) - .then(() => void Audit.actionPerformed({ _id: user._id, username: user.username }, { _id: room._id }, 'ldap-sync')) - .catch((err) => { - this.logger.error({ - msg: 'Failed to remove user from ABAC room after room attributes changed', - rid: room._id, - err, - }); - }), - ), - ); - } - - await Promise.all(removalPromises); + return await this.removeUserFromRoomList(cursor, user, 'ldap-sync'); } catch (err) { this.logger.error({ msg: 'Failed to query and remove user from non-compliant ABAC rooms', diff --git a/ee/packages/abac/src/service.spec.ts b/ee/packages/abac/src/service.spec.ts index 9b67209b5349f..42be127aeafb7 100644 --- a/ee/packages/abac/src/service.spec.ts +++ b/ee/packages/abac/src/service.spec.ts @@ -144,6 +144,7 @@ describe('AbacService (unit)', () => { await service.addSubjectAttributes(user, ldapUser, map); + // This call is noop cause user doesnt have a __rooms property expect(mockUsersUnsetAbacAttributesById).toHaveBeenCalledTimes(1); }); @@ -210,6 +211,7 @@ describe('AbacService (unit)', () => { await service.addSubjectAttributes(user, ldapUser, map); + // This call is noop cause user doesnt have a __rooms property expect(spy).toHaveBeenCalledTimes(1); expect(spy.mock.calls[0][1]).toEqual([{ key: 'dept', values: ['eng'] }]); });