From f2c2e8b614ea3d6bf5d24c3da2fa245c8e6d3055 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 2 Dec 2025 12:43:17 -0600 Subject: [PATCH 01/10] catch promise & log error --- ee/packages/abac/src/index.ts | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index b220dbf19e47d..e868c6e064360 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -641,7 +641,15 @@ export class AbacService extends ServiceClass implements IAbacService { 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')), + }) + .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, + }); + }), ), ); } @@ -685,7 +693,15 @@ export class AbacService extends ServiceClass implements IAbacService { 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')), + }) + .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, + }); + }), ), ); } @@ -708,7 +724,15 @@ export class AbacService extends ServiceClass implements IAbacService { 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')), + }) + .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, + }); + }), ), ); } From fd93cfd55c2a5e35afd2db85778d108efb24ac1b Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 2 Dec 2025 12:47:01 -0600 Subject: [PATCH 02/10] unique attributes --- ee/packages/abac/src/helper.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/packages/abac/src/helper.ts b/ee/packages/abac/src/helper.ts index 733eeecd4b8b4..4bceff33a8d64 100644 --- a/ee/packages/abac/src/helper.ts +++ b/ee/packages/abac/src/helper.ts @@ -127,11 +127,11 @@ export async function ensureAttributeDefinitionsExist(normalized: IAbacAttribute return; } - const keys = normalized.map((a) => a.key); - const attributeDefinitions = await AbacAttributes.find({ key: { $in: keys } }, { projection: { key: 1, values: 1 } }).toArray(); + const uniqueKeys = [...new Set(normalized.map((a) => a.key))]; + const attributeDefinitions = await AbacAttributes.find({ key: { $in: uniqueKeys } }, { projection: { key: 1, values: 1 } }).toArray(); const definitionValuesMap = new Map>(attributeDefinitions.map((def: any) => [def.key, new Set(def.values)])); - if (definitionValuesMap.size !== keys.length) { + if (definitionValuesMap.size !== uniqueKeys.length) { throw new Error('error-attribute-definition-not-found'); } From 6890e54dd40a8dbeef4a899be9c88b1361a38b68 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 2 Dec 2025 13:08:52 -0600 Subject: [PATCH 03/10] dedupe attributes before saving --- ee/packages/abac/src/helper.spec.ts | 39 +++++++++++++++++++++++- ee/packages/abac/src/helper.ts | 45 +++++++++++++++++++++++----- ee/packages/abac/src/service.spec.ts | 15 ++++------ 3 files changed, 81 insertions(+), 18 deletions(-) diff --git a/ee/packages/abac/src/helper.spec.ts b/ee/packages/abac/src/helper.spec.ts index e81b9e409b1b0..96d138f5a026e 100644 --- a/ee/packages/abac/src/helper.spec.ts +++ b/ee/packages/abac/src/helper.spec.ts @@ -1,6 +1,6 @@ import { expect } from 'chai'; -import { didSubjectLoseAttributes } from './helper'; +import { didSubjectLoseAttributes, validateAndNormalizeAttributes } from './helper'; describe('didSubjectLoseAttributes', () => { const call = (prev: { key: string; values: string[] }[], next: { key: string; values: string[] }[]) => @@ -348,6 +348,43 @@ describe('didSubjectLoseAttributes', () => { }); }); +describe('validateAndNormalizeAttributes', () => { + it('normalizes keys and merges duplicate values from multiple entries', () => { + const result = validateAndNormalizeAttributes({ + ' dept ': [' sales ', 'marketing', 'sales', '', 'marketing '], + 'role': [' admin ', 'user', 'user '], + 'dept': ['engineering'], + }); + + expect(result).to.deep.equal([ + { key: 'dept', values: ['sales', 'marketing', 'engineering'] }, + { key: 'role', values: ['admin', 'user'] }, + ]); + }); + + it('throws when a key has no remaining sanitized values', () => { + expect(() => + validateAndNormalizeAttributes({ + role: [' ', '\n', '\t'], + }), + ).to.throw('error-invalid-attribute-values'); + }); + + it('throws when a key exceeds the maximum number of unique values', () => { + const values = Array.from({ length: 11 }, (_, i) => `value-${i}`); + expect(() => + validateAndNormalizeAttributes({ + role: values, + }), + ).to.throw('error-invalid-attribute-values'); + }); + + it('throws when total unique attribute keys exceeds limit', () => { + const attributes = Object.fromEntries(Array.from({ length: 11 }, (_, i) => [`key-${i}`, ['value']])); + expect(() => validateAndNormalizeAttributes(attributes)).to.throw('error-invalid-attribute-values'); + }); +}); + describe('buildNonCompliantConditions', () => { type AttributeDef = { key: string; values: string[] }; diff --git a/ee/packages/abac/src/helper.ts b/ee/packages/abac/src/helper.ts index 4bceff33a8d64..826745f4fb312 100644 --- a/ee/packages/abac/src/helper.ts +++ b/ee/packages/abac/src/helper.ts @@ -105,18 +105,49 @@ export function validateAndNormalizeAttributes(attributes: Record 10) { - throw new Error('error-invalid-attribute-values'); - } + const entries = Object.entries(attributes); + + const aggregated = new Map>(); - for (const [key, values] of Object.entries(attributes)) { - if (!keyPattern.test(key)) { + for (const [rawKey, values] of entries) { + const key = rawKey.trim(); + + if (!key.length || !keyPattern.test(key)) { throw new Error('error-invalid-attribute-key'); } - if (values.length > 10) { + + const bucket = aggregated.get(key) ?? new Set(); + if (!aggregated.has(key)) { + if (aggregated.size >= 10) { + throw new Error('error-invalid-attribute-values'); + } + aggregated.set(key, bucket); + } + + for (const value of values) { + if (typeof value !== 'string') { + continue; + } + const trimmed = value.trim(); + if (!trimmed.length) { + continue; + } + if (bucket.size >= 10 && !bucket.has(trimmed)) { + throw new Error('error-invalid-attribute-values'); + } + bucket.add(trimmed); + } + } + + if (aggregated.size > 10) { + throw new Error('error-invalid-attribute-values'); + } + + for (const [key, valueSet] of aggregated.entries()) { + if (!valueSet.size) { throw new Error('error-invalid-attribute-values'); } - normalized.push({ key, values }); + normalized.push({ key, values: Array.from(valueSet) }); } return normalized; diff --git a/ee/packages/abac/src/service.spec.ts b/ee/packages/abac/src/service.spec.ts index 111f44a78a488..ee98dc8759c00 100644 --- a/ee/packages/abac/src/service.spec.ts +++ b/ee/packages/abac/src/service.spec.ts @@ -603,11 +603,9 @@ describe('AbacService (unit)', () => { expect(mockSetAbacAttributesById).not.toHaveBeenCalled(); }); - it('throws error-attribute-definition-not-found for empty value array', async () => { + it('throws error-invalid-attribute-values for empty value array', async () => { mockFindOneByIdAndType.mockResolvedValueOnce({ _id: 'r1', abacAttributes: [] }); - await expect(service.setRoomAbacAttributes('r1', { dept: [] as any }, fakeActor)).rejects.toThrow( - 'error-attribute-definition-not-found', - ); + await expect(service.setRoomAbacAttributes('r1', { dept: [] as any }, fakeActor)).rejects.toThrow('error-invalid-attribute-values'); expect(mockSetAbacAttributesById).not.toHaveBeenCalled(); }); @@ -632,18 +630,15 @@ describe('AbacService (unit)', () => { expect(mockSetAbacAttributesById).not.toHaveBeenCalled(); }); - it('sets attributes for new key (with duplicate values) and calls hook', async () => { - mockFindOneByIdAndType.mockResolvedValueOnce({ _id: 'r1', abacAttributes: [] }); + it('does not call onroomattributechanged when the change is a duplicated attribute value', async () => { + mockFindOneByIdAndType.mockResolvedValueOnce({ _id: 'r1', abacAttributes: [{ key: 'dept', values: ['eng', 'sales'] }] }); mockAbacFind.mockReturnValueOnce({ toArray: async () => [{ key: 'dept', values: ['eng', 'sales'] }], }); await service.setRoomAbacAttributes('r1', { dept: ['eng', 'eng', 'sales'] }, fakeActor); - expect(mockSetAbacAttributesById).toHaveBeenCalledWith('r1', [{ key: 'dept', values: ['eng', 'eng', 'sales'] }]); - expect((service as any).onRoomAttributesChanged).toHaveBeenCalledWith(expect.objectContaining({ _id: 'r1' }), [ - { key: 'dept', values: ['eng', 'eng', 'sales'] }, - ]); + expect((service as any).onRoomAttributesChanged).not.toHaveBeenCalled(); }); it('does not call onRoomAttributesChanged when an existing value is removed', async () => { From 25cdd5680b52b7952318af58e6747d3f66064563 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 2 Dec 2025 13:37:14 -0600 Subject: [PATCH 04/10] centralize limits --- ee/packages/abac/src/helper.spec.ts | 6 +++--- ee/packages/abac/src/helper.ts | 9 ++++++--- ee/packages/abac/src/index.ts | 7 ++++--- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/ee/packages/abac/src/helper.spec.ts b/ee/packages/abac/src/helper.spec.ts index 96d138f5a026e..d9261d3e9934d 100644 --- a/ee/packages/abac/src/helper.spec.ts +++ b/ee/packages/abac/src/helper.spec.ts @@ -1,6 +1,6 @@ import { expect } from 'chai'; -import { didSubjectLoseAttributes, validateAndNormalizeAttributes } from './helper'; +import { didSubjectLoseAttributes, validateAndNormalizeAttributes, MAX_ABAC_ATTRIBUTE_KEYS, MAX_ABAC_ATTRIBUTE_VALUES } from './helper'; describe('didSubjectLoseAttributes', () => { const call = (prev: { key: string; values: string[] }[], next: { key: string; values: string[] }[]) => @@ -371,7 +371,7 @@ describe('validateAndNormalizeAttributes', () => { }); it('throws when a key exceeds the maximum number of unique values', () => { - const values = Array.from({ length: 11 }, (_, i) => `value-${i}`); + const values = Array.from({ length: MAX_ABAC_ATTRIBUTE_VALUES + 1 }, (_, i) => `value-${i}`); expect(() => validateAndNormalizeAttributes({ role: values, @@ -380,7 +380,7 @@ describe('validateAndNormalizeAttributes', () => { }); it('throws when total unique attribute keys exceeds limit', () => { - const attributes = Object.fromEntries(Array.from({ length: 11 }, (_, i) => [`key-${i}`, ['value']])); + 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'); }); }); diff --git a/ee/packages/abac/src/helper.ts b/ee/packages/abac/src/helper.ts index 826745f4fb312..d4d24d160b465 100644 --- a/ee/packages/abac/src/helper.ts +++ b/ee/packages/abac/src/helper.ts @@ -1,6 +1,9 @@ import type { ILDAPEntry, IAbacAttributeDefinition } from '@rocket.chat/core-typings'; import { AbacAttributes } from '@rocket.chat/models'; +export const MAX_ABAC_ATTRIBUTE_KEYS = 10; +export const MAX_ABAC_ATTRIBUTE_VALUES = 10; + export const extractAttribute = (ldapUser: ILDAPEntry, ldapKey: string, abacKey: string): IAbacAttributeDefinition | undefined => { if (!ldapKey || !abacKey) { return; @@ -118,7 +121,7 @@ export function validateAndNormalizeAttributes(attributes: Record(); if (!aggregated.has(key)) { - if (aggregated.size >= 10) { + if (aggregated.size >= MAX_ABAC_ATTRIBUTE_KEYS) { throw new Error('error-invalid-attribute-values'); } aggregated.set(key, bucket); @@ -132,14 +135,14 @@ export function validateAndNormalizeAttributes(attributes: Record= 10 && !bucket.has(trimmed)) { + if (bucket.size >= MAX_ABAC_ATTRIBUTE_VALUES && !bucket.has(trimmed)) { throw new Error('error-invalid-attribute-values'); } bucket.add(trimmed); } } - if (aggregated.size > 10) { + if (aggregated.size > MAX_ABAC_ATTRIBUTE_KEYS) { throw new Error('error-invalid-attribute-values'); } diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index e868c6e064360..45652c23f7eae 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -20,6 +20,7 @@ import { validateAndNormalizeAttributes, ensureAttributeDefinitionsExist, buildRoomNonCompliantConditionsFromSubject, + MAX_ABAC_ATTRIBUTE_KEYS, } from './helper'; // Limit concurrent user removals to avoid overloading the server with too many operations at once @@ -343,7 +344,7 @@ export class AbacService extends ServiceClass implements IAbacService { const existingIndex = previous.findIndex((a) => a.key === key); const isNewKey = existingIndex === -1; - if (isNewKey && previous.length >= 10) { + if (isNewKey && previous.length >= MAX_ABAC_ATTRIBUTE_KEYS) { throw new Error('error-invalid-attribute-values'); } @@ -442,7 +443,7 @@ export class AbacService extends ServiceClass implements IAbacService { throw new Error('error-duplicate-attribute-key'); } - if (previous.length >= 10) { + if (previous.length >= MAX_ABAC_ATTRIBUTE_KEYS) { throw new Error('error-invalid-attribute-values'); } @@ -490,7 +491,7 @@ export class AbacService extends ServiceClass implements IAbacService { return; } - if (room?.abacAttributes?.length === 10) { + if (room?.abacAttributes?.length === MAX_ABAC_ATTRIBUTE_KEYS) { throw new Error('error-invalid-attribute-values'); } From 9895cd84a151f5b28040fa264f021dd5bebb53fc Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 2 Dec 2025 13:51:03 -0600 Subject: [PATCH 05/10] make errors pretty --- ee/packages/abac/src/errors.ts | 93 ++++++++++++++++++++++++++++++++++ ee/packages/abac/src/helper.ts | 18 ++++--- ee/packages/abac/src/index.ts | 60 +++++++++++++--------- 3 files changed, 138 insertions(+), 33 deletions(-) create mode 100644 ee/packages/abac/src/errors.ts diff --git a/ee/packages/abac/src/errors.ts b/ee/packages/abac/src/errors.ts new file mode 100644 index 0000000000000..9ce3b7c278b5a --- /dev/null +++ b/ee/packages/abac/src/errors.ts @@ -0,0 +1,93 @@ +export enum AbacErrorCode { + InvalidAttributeValues = 'error-invalid-attribute-values', + InvalidAttributeKey = 'error-invalid-attribute-key', + AttributeNotFound = 'error-attribute-not-found', + AttributeInUse = 'error-attribute-in-use', + DuplicateAttributeKey = 'error-duplicate-attribute-key', + AttributeDefinitionNotFound = 'error-attribute-definition-not-found', + RoomNotFound = 'error-room-not-found', + CannotConvertDefaultRoomToAbac = 'error-cannot-convert-default-room-to-abac', + UsernamesNotMatchingAbacAttributes = 'error-usernames-not-matching-abac-attributes', + AbacUnsupportedObjectType = 'error-abac-unsupported-object-type', + AbacUnsupportedOperation = 'error-abac-unsupported-operation', +} + +export class AbacError extends Error { + public readonly code: AbacErrorCode; + + public readonly details?: unknown; + + constructor(code: AbacErrorCode, details?: unknown) { + super(code); + this.code = code; + this.details = details; + + Object.setPrototypeOf(this, new.target.prototype); + } +} + +export class AbacInvalidAttributeValuesError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.InvalidAttributeValues, details); + } +} + +export class AbacInvalidAttributeKeyError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.InvalidAttributeKey, details); + } +} + +export class AbacAttributeNotFoundError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.AttributeNotFound, details); + } +} + +export class AbacAttributeInUseError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.AttributeInUse, details); + } +} + +export class AbacDuplicateAttributeKeyError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.DuplicateAttributeKey, details); + } +} + +export class AbacAttributeDefinitionNotFoundError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.AttributeDefinitionNotFound, details); + } +} + +export class AbacRoomNotFoundError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.RoomNotFound, details); + } +} + +export class AbacCannotConvertDefaultRoomToAbacError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.CannotConvertDefaultRoomToAbac, details); + } +} + +export class AbacUsernamesNotMatchingAttributesError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.UsernamesNotMatchingAbacAttributes, details); + } +} + +export class AbacUnsupportedObjectTypeError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.AbacUnsupportedObjectType, details); + } +} + +export class AbacUnsupportedOperationError extends AbacError { + constructor(details?: unknown) { + super(AbacErrorCode.AbacUnsupportedOperation, details); + } +} diff --git a/ee/packages/abac/src/helper.ts b/ee/packages/abac/src/helper.ts index d4d24d160b465..31c7aa509287a 100644 --- a/ee/packages/abac/src/helper.ts +++ b/ee/packages/abac/src/helper.ts @@ -1,6 +1,8 @@ import type { ILDAPEntry, IAbacAttributeDefinition } from '@rocket.chat/core-typings'; import { AbacAttributes } from '@rocket.chat/models'; +import { AbacAttributeDefinitionNotFoundError, AbacInvalidAttributeKeyError, AbacInvalidAttributeValuesError } from './errors'; + export const MAX_ABAC_ATTRIBUTE_KEYS = 10; export const MAX_ABAC_ATTRIBUTE_VALUES = 10; @@ -116,13 +118,13 @@ export function validateAndNormalizeAttributes(attributes: Record(); if (!aggregated.has(key)) { if (aggregated.size >= MAX_ABAC_ATTRIBUTE_KEYS) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } aggregated.set(key, bucket); } @@ -136,19 +138,19 @@ export function validateAndNormalizeAttributes(attributes: Record= MAX_ABAC_ATTRIBUTE_VALUES && !bucket.has(trimmed)) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } bucket.add(trimmed); } } if (aggregated.size > MAX_ABAC_ATTRIBUTE_KEYS) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } for (const [key, valueSet] of aggregated.entries()) { if (!valueSet.size) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } normalized.push({ key, values: Array.from(valueSet) }); } @@ -166,17 +168,17 @@ export async function ensureAttributeDefinitionsExist(normalized: IAbacAttribute const definitionValuesMap = new Map>(attributeDefinitions.map((def: any) => [def.key, new Set(def.values)])); if (definitionValuesMap.size !== uniqueKeys.length) { - throw new Error('error-attribute-definition-not-found'); + throw new AbacAttributeDefinitionNotFoundError(); } for (const a of normalized) { const allowed = definitionValuesMap.get(a.key); if (!allowed) { - throw new Error('error-attribute-definition-not-found'); + throw new AbacAttributeDefinitionNotFoundError(); } for (const v of a.values) { if (!allowed.has(v)) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } } } diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index 45652c23f7eae..f66c685689bc0 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -9,6 +9,16 @@ import type { Document, UpdateFilter } from 'mongodb'; import pLimit from 'p-limit'; import { Audit } from './audit'; +import { + AbacAttributeInUseError, + AbacAttributeNotFoundError, + AbacCannotConvertDefaultRoomToAbacError, + AbacDuplicateAttributeKeyError, + AbacInvalidAttributeValuesError, + AbacRoomNotFoundError, + AbacUnsupportedObjectTypeError, + AbacUnsupportedOperationError, +} from './errors'; import { diffAttributes, extractAttribute, @@ -85,7 +95,7 @@ export class AbacService extends ServiceClass implements IAbacService { async addAbacAttribute(attribute: IAbacAttributeDefinition, actor: AbacActor): Promise { if (!attribute.values.length) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } try { @@ -93,7 +103,7 @@ export class AbacService extends ServiceClass implements IAbacService { void Audit.attributeCreated(attribute, actor); } catch (e) { if (e instanceof Error && e.message.includes('E11000')) { - throw new Error('error-duplicate-attribute-key'); + throw new AbacDuplicateAttributeKeyError(); } throw e; } @@ -205,11 +215,11 @@ export class AbacService extends ServiceClass implements IAbacService { const existing = await AbacAttributes.findOneById(_id, { projection: { key: 1, values: 1 } }); if (!existing) { - throw new Error('error-attribute-not-found'); + throw new AbacAttributeNotFoundError(); } if (update.values && !update.values.length) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } const newKey = update.key ?? existing.key; @@ -223,7 +233,7 @@ export class AbacService extends ServiceClass implements IAbacService { if (keyChanged || valuesToCheck.length) { const inUse = await Rooms.isAbacAttributeInUse(existing.key, valuesToCheck.length ? valuesToCheck : existing.values); if (inUse) { - throw new Error('error-attribute-in-use'); + throw new AbacAttributeInUseError(); } } @@ -244,7 +254,7 @@ export class AbacService extends ServiceClass implements IAbacService { void Audit.attributeUpdated(existing, modifier as IAbacAttributeDefinition, actor); } catch (e) { if (e instanceof Error && e.message.includes('E11000')) { - throw new Error('error-duplicate-attribute-key'); + throw new AbacDuplicateAttributeKeyError(); } throw e; } @@ -253,12 +263,12 @@ export class AbacService extends ServiceClass implements IAbacService { async deleteAbacAttributeById(_id: string, actor: AbacActor): Promise { const existing = await AbacAttributes.findOneById(_id, { projection: { key: 1, values: 1 } }); if (!existing) { - throw new Error('error-attribute-not-found'); + throw new AbacAttributeNotFoundError(); } const inUse = await Rooms.isAbacAttributeInUse(existing.key, existing.values); if (inUse) { - throw new Error('error-attribute-in-use'); + throw new AbacAttributeInUseError(); } await AbacAttributes.removeById(_id); @@ -268,7 +278,7 @@ export class AbacService extends ServiceClass implements IAbacService { async getAbacAttributeById(_id: string): Promise<{ key: string; values: string[]; usage: Record }> { const attribute = await AbacAttributes.findOneById(_id, { projection: { key: 1, values: 1 } }); if (!attribute) { - throw new Error('error-attribute-not-found'); + throw new AbacAttributeNotFoundError(); } const usageEntries = await Promise.all( @@ -301,10 +311,10 @@ export class AbacService extends ServiceClass implements IAbacService { projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, }); if (!room) { - throw new Error('error-room-not-found'); + throw new AbacRoomNotFoundError(); } if (room.default || room.teamDefault) { - throw new Error('error-cannot-convert-default-room-to-abac'); + throw new AbacCannotConvertDefaultRoomToAbacError(); } if (!Object.keys(attributes).length && room.abacAttributes?.length) { @@ -333,11 +343,11 @@ export class AbacService extends ServiceClass implements IAbacService { projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, }); if (!room) { - throw new Error('error-room-not-found'); + throw new AbacRoomNotFoundError(); } if (room.default || room.teamDefault) { - throw new Error('error-cannot-convert-default-room-to-abac'); + throw new AbacCannotConvertDefaultRoomToAbacError(); } const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; @@ -345,7 +355,7 @@ export class AbacService extends ServiceClass implements IAbacService { const existingIndex = previous.findIndex((a) => a.key === key); const isNewKey = existingIndex === -1; if (isNewKey && previous.length >= MAX_ABAC_ATTRIBUTE_KEYS) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } await ensureAttributeDefinitionsExist([{ key, values }]); @@ -391,11 +401,11 @@ export class AbacService extends ServiceClass implements IAbacService { projection: { abacAttributes: 1, default: 1, teamDefault: 1, name: 1 }, }); if (!room) { - throw new Error('error-room-not-found'); + throw new AbacRoomNotFoundError(); } if (room.default || room.teamDefault) { - throw new Error('error-cannot-convert-default-room-to-abac'); + throw new AbacCannotConvertDefaultRoomToAbacError(); } const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; @@ -431,20 +441,20 @@ export class AbacService extends ServiceClass implements IAbacService { projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, }); if (!room) { - throw new Error('error-room-not-found'); + throw new AbacRoomNotFoundError(); } if (room.default || room.teamDefault) { - throw new Error('error-cannot-convert-default-room-to-abac'); + throw new AbacCannotConvertDefaultRoomToAbacError(); } const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; if (previous.some((a) => a.key === key)) { - throw new Error('error-duplicate-attribute-key'); + throw new AbacDuplicateAttributeKeyError(); } if (previous.length >= MAX_ABAC_ATTRIBUTE_KEYS) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } const updated = await Rooms.insertAbacAttributeIfNotExistsById(rid, key, values); @@ -464,11 +474,11 @@ export class AbacService extends ServiceClass implements IAbacService { projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, }); if (!room) { - throw new Error('error-room-not-found'); + throw new AbacRoomNotFoundError(); } if (room.default || room.teamDefault) { - throw new Error('error-cannot-convert-default-room-to-abac'); + throw new AbacCannotConvertDefaultRoomToAbacError(); } const exists = room?.abacAttributes?.some((a) => a.key === key); @@ -492,7 +502,7 @@ export class AbacService extends ServiceClass implements IAbacService { } if (room?.abacAttributes?.length === MAX_ABAC_ATTRIBUTE_KEYS) { - throw new Error('error-invalid-attribute-values'); + throw new AbacInvalidAttributeValuesError(); } const updated = await Rooms.insertAbacAttributeIfNotExistsById(rid, key, values); @@ -546,11 +556,11 @@ export class AbacService extends ServiceClass implements IAbacService { ) { // We may need this flex for phase 2, but for now only ROOM/READ is supported if (objectType !== AbacObjectType.ROOM) { - throw new Error('error-abac-unsupported-object-type'); + throw new AbacUnsupportedObjectTypeError(); } if (action !== AbacAccessOperation.READ) { - throw new Error('error-abac-unsupported-operation'); + throw new AbacUnsupportedOperationError(); } if (!user?._id || !room?.abacAttributes?.length) { From 8459ffd9f6103b606148f904d579ed437e01ab18 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 2 Dec 2025 14:01:04 -0600 Subject: [PATCH 06/10] extract helper for room --- ee/packages/abac/src/errors.ts | 6 --- ee/packages/abac/src/helper.ts | 32 ++++++++++-- ee/packages/abac/src/index.ts | 89 ++++++++-------------------------- 3 files changed, 49 insertions(+), 78 deletions(-) diff --git a/ee/packages/abac/src/errors.ts b/ee/packages/abac/src/errors.ts index 9ce3b7c278b5a..490ace6c9b7f1 100644 --- a/ee/packages/abac/src/errors.ts +++ b/ee/packages/abac/src/errors.ts @@ -74,12 +74,6 @@ export class AbacCannotConvertDefaultRoomToAbacError extends AbacError { } } -export class AbacUsernamesNotMatchingAttributesError extends AbacError { - constructor(details?: unknown) { - super(AbacErrorCode.UsernamesNotMatchingAbacAttributes, details); - } -} - export class AbacUnsupportedObjectTypeError extends AbacError { constructor(details?: unknown) { super(AbacErrorCode.AbacUnsupportedObjectType, details); diff --git a/ee/packages/abac/src/helper.ts b/ee/packages/abac/src/helper.ts index 31c7aa509287a..1932acd63c244 100644 --- a/ee/packages/abac/src/helper.ts +++ b/ee/packages/abac/src/helper.ts @@ -1,7 +1,13 @@ -import type { ILDAPEntry, IAbacAttributeDefinition } from '@rocket.chat/core-typings'; -import { AbacAttributes } from '@rocket.chat/models'; +import type { ILDAPEntry, IAbacAttributeDefinition, IRoom } from '@rocket.chat/core-typings'; +import { AbacAttributes, Rooms } from '@rocket.chat/models'; -import { AbacAttributeDefinitionNotFoundError, AbacInvalidAttributeKeyError, AbacInvalidAttributeValuesError } from './errors'; +import { + AbacAttributeDefinitionNotFoundError, + AbacCannotConvertDefaultRoomToAbacError, + AbacInvalidAttributeKeyError, + AbacInvalidAttributeValuesError, + AbacRoomNotFoundError, +} from './errors'; export const MAX_ABAC_ATTRIBUTE_KEYS = 10; export const MAX_ABAC_ATTRIBUTE_VALUES = 10; @@ -166,7 +172,7 @@ export async function ensureAttributeDefinitionsExist(normalized: IAbacAttribute const uniqueKeys = [...new Set(normalized.map((a) => a.key))]; const attributeDefinitions = await AbacAttributes.find({ key: { $in: uniqueKeys } }, { projection: { key: 1, values: 1 } }).toArray(); - const definitionValuesMap = new Map>(attributeDefinitions.map((def: any) => [def.key, new Set(def.values)])); + const definitionValuesMap = new Map>(attributeDefinitions.map((def) => [def.key, new Set(def.values)])); if (definitionValuesMap.size !== uniqueKeys.length) { throw new AbacAttributeDefinitionNotFoundError(); } @@ -256,3 +262,21 @@ export function buildRoomNonCompliantConditionsFromSubject(subjectAttributes: IA } return conditions; } + +export async function getAbacRoom( + rid: string, +): Promise> { + const room = await Rooms.findOneByIdAndType< + Pick + >(rid, 'p', { + projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, + }); + if (!room) { + throw new AbacRoomNotFoundError(); + } + if (room.default || room.teamDefault) { + throw new AbacCannotConvertDefaultRoomToAbacError(); + } + + return room; +} diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index f66c685689bc0..63b15094c4756 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -1,7 +1,7 @@ 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 } from '@rocket.chat/core-typings'; +import type { IAbacAttribute, IAbacAttributeDefinition, IRoom, AtLeast, IUser, ILDAPEntry, ISubscription } 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'; @@ -31,6 +31,7 @@ import { ensureAttributeDefinitionsExist, buildRoomNonCompliantConditionsFromSubject, MAX_ABAC_ATTRIBUTE_KEYS, + getAbacRoom, } from './helper'; // Limit concurrent user removals to avoid overloading the server with too many operations at once @@ -305,17 +306,7 @@ export class AbacService extends ServiceClass implements IAbacService { } async setRoomAbacAttributes(rid: string, attributes: Record, actor: AbacActor): Promise { - const room = await Rooms.findOneByIdAndType< - Pick - >(rid, 'p', { - projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, - }); - if (!room) { - throw new AbacRoomNotFoundError(); - } - if (room.default || room.teamDefault) { - throw new AbacCannotConvertDefaultRoomToAbacError(); - } + const room = await getAbacRoom(rid); if (!Object.keys(attributes).length && room.abacAttributes?.length) { await Rooms.unsetAbacAttributesById(rid); @@ -337,18 +328,7 @@ export class AbacService extends ServiceClass implements IAbacService { } async updateRoomAbacAttributeValues(rid: string, key: string, values: string[], actor: AbacActor): Promise { - const room = await Rooms.findOneByIdAndType< - Pick - >(rid, 'p', { - projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, - }); - if (!room) { - throw new AbacRoomNotFoundError(); - } - - if (room.default || room.teamDefault) { - throw new AbacCannotConvertDefaultRoomToAbacError(); - } + const room = await getAbacRoom(rid); const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; @@ -397,16 +377,7 @@ export class AbacService extends ServiceClass implements IAbacService { } async removeRoomAbacAttribute(rid: string, key: string, actor: AbacActor): Promise { - const room = await Rooms.findOneByIdAndType>(rid, 'p', { - projection: { abacAttributes: 1, default: 1, teamDefault: 1, name: 1 }, - }); - if (!room) { - throw new AbacRoomNotFoundError(); - } - - if (room.default || room.teamDefault) { - throw new AbacCannotConvertDefaultRoomToAbacError(); - } + const room = await getAbacRoom(rid); const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; const exists = previous.some((a) => a.key === key); @@ -435,18 +406,7 @@ export class AbacService extends ServiceClass implements IAbacService { async addRoomAbacAttributeByKey(rid: string, key: string, values: string[], actor: AbacActor): Promise { await ensureAttributeDefinitionsExist([{ key, values }]); - const room = await Rooms.findOneByIdAndType< - Pick - >(rid, 'p', { - projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, - }); - if (!room) { - throw new AbacRoomNotFoundError(); - } - - if (room.default || room.teamDefault) { - throw new AbacCannotConvertDefaultRoomToAbacError(); - } + const room = await getAbacRoom(rid); const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; if (previous.some((a) => a.key === key)) { @@ -468,18 +428,7 @@ export class AbacService extends ServiceClass implements IAbacService { async replaceRoomAbacAttributeByKey(rid: string, key: string, values: string[], actor: AbacActor): Promise { await ensureAttributeDefinitionsExist([{ key, values }]); - const room = await Rooms.findOneByIdAndType< - Pick - >(rid, 'p', { - projection: { abacAttributes: 1, t: 1, teamMain: 1, teamDefault: 1, default: 1, name: 1 }, - }); - if (!room) { - throw new AbacRoomNotFoundError(); - } - - if (room.default || room.teamDefault) { - throw new AbacCannotConvertDefaultRoomToAbacError(); - } + const room = await getAbacRoom(rid); const exists = room?.abacAttributes?.some((a) => a.key === key); @@ -548,6 +497,19 @@ export class AbacService extends ServiceClass implements IAbacService { }); } + private shouldUseCache(decisionCacheTimeout: number, userSub: ISubscription) { + // Cases: + // 1) Never checked before -> check now + // 2) Checked before, but cache expired -> check now + // 3) Checked before, and cache valid -> use cached decision (subsciprtion exists) + // 4) Cache disabled (0) -> always check + return ( + decisionCacheTimeout > 0 && + userSub.abacLastTimeChecked && + Date.now() - userSub.abacLastTimeChecked.getTime() < decisionCacheTimeout * 1000 + ); + } + async canAccessObject( room: Pick, user: Pick, @@ -573,16 +535,7 @@ export class AbacService extends ServiceClass implements IAbacService { return false; } - // Cases: - // 1) Never checked before -> check now - // 2) Checked before, but cache expired -> check now - // 3) Checked before, and cache valid -> use cached decision (subsciprtion exists) - // 4) Cache disabled (0) -> always check - if ( - decisionCacheTimeout > 0 && - userSub.abacLastTimeChecked && - Date.now() - userSub.abacLastTimeChecked.getTime() < decisionCacheTimeout * 1000 - ) { + if (this.shouldUseCache(decisionCacheTimeout, userSub)) { this.logger.debug({ msg: 'Using cached ABAC decision', userId: user._id, roomId: room._id }); return !!userSub; } From c48d13a12d23eb013471769c514233b92493309f Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Tue, 2 Dec 2025 14:23:26 -0600 Subject: [PATCH 07/10] fix ts --- ee/packages/abac/src/index.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index 63b15094c4756..cfa7991268b19 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -12,10 +12,8 @@ import { Audit } from './audit'; import { AbacAttributeInUseError, AbacAttributeNotFoundError, - AbacCannotConvertDefaultRoomToAbacError, AbacDuplicateAttributeKeyError, AbacInvalidAttributeValuesError, - AbacRoomNotFoundError, AbacUnsupportedObjectTypeError, AbacUnsupportedOperationError, } from './errors'; From 081033bd5687d6ea7d39b13eec5dc37974166f33 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Wed, 3 Dec 2025 10:13:48 -0600 Subject: [PATCH 08/10] mem --- ee/packages/abac/package.json | 1 + ee/packages/abac/src/helper.ts | 12 ++++++++++-- ee/packages/abac/src/index.ts | 4 ++-- yarn.lock | 1 + 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/ee/packages/abac/package.json b/ee/packages/abac/package.json index a9629ff00b994..9c6ea60fab5d9 100644 --- a/ee/packages/abac/package.json +++ b/ee/packages/abac/package.json @@ -25,6 +25,7 @@ "@rocket.chat/core-typings": "workspace:^", "@rocket.chat/logger": "workspace:^", "@rocket.chat/models": "workspace:^", + "mem": "^8.1.1", "mongodb": "6.10.0", "p-limit": "3.1.0" }, diff --git a/ee/packages/abac/src/helper.ts b/ee/packages/abac/src/helper.ts index 1932acd63c244..6658e21405555 100644 --- a/ee/packages/abac/src/helper.ts +++ b/ee/packages/abac/src/helper.ts @@ -1,5 +1,6 @@ import type { ILDAPEntry, IAbacAttributeDefinition, IRoom } from '@rocket.chat/core-typings'; import { AbacAttributes, Rooms } from '@rocket.chat/models'; +import mem from 'mem'; import { AbacAttributeDefinitionNotFoundError, @@ -89,7 +90,7 @@ export function diffAttributes(a: IAbacAttributeDefinition[] = [], b: IAbacAttri return diff; } -export function didAttributesChange(current: IAbacAttributeDefinition[], next: IAbacAttributeDefinition[]) { +export function wereAttributesAdded(current: IAbacAttributeDefinition[], next: IAbacAttributeDefinition[]) { let added = false; const prevMap = new Map(current.map((a) => [a.key, new Set(a.values)])); for (const { key, values } of next) { @@ -164,13 +165,20 @@ export function validateAndNormalizeAttributes(attributes: Record + AbacAttributes.find({ key: { $in: keys } }, { projection: { key: 1, values: 1 } }).toArray(); + +const getAttributeDefinitionsCached = mem(getAttributeDefinitionsFromDb, { + maxAge: 30_000, +}); + export async function ensureAttributeDefinitionsExist(normalized: IAbacAttributeDefinition[]): Promise { if (!normalized.length) { return; } const uniqueKeys = [...new Set(normalized.map((a) => a.key))]; - const attributeDefinitions = await AbacAttributes.find({ key: { $in: uniqueKeys } }, { projection: { key: 1, values: 1 } }).toArray(); + const attributeDefinitions = await getAttributeDefinitionsCached(uniqueKeys); const definitionValuesMap = new Map>(attributeDefinitions.map((def) => [def.key, new Set(def.values)])); if (definitionValuesMap.size !== uniqueKeys.length) { diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index cfa7991268b19..abf529b11af68 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -20,7 +20,7 @@ import { import { diffAttributes, extractAttribute, - didAttributesChange, + wereAttributesAdded, didSubjectLoseAttributes, wereAttributeValuesAdded, buildCompliantConditions, @@ -320,7 +320,7 @@ export class AbacService extends ServiceClass implements IAbacService { void Audit.objectAttributeChanged({ _id: room._id, name: room.name }, room.abacAttributes || [], normalized, 'updated', actor); const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; - if (didAttributesChange(previous, normalized)) { + if (wereAttributesAdded(previous, normalized)) { await this.onRoomAttributesChanged(room, (updated?.abacAttributes as IAbacAttributeDefinition[] | undefined) ?? normalized); } } diff --git a/yarn.lock b/yarn.lock index 1ebeea37ba9d1..a26dcb758b62d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8287,6 +8287,7 @@ __metadata: "@types/node": "npm:~22.16.1" eslint: "npm:~8.45.0" jest: "npm:~30.0.5" + mem: "npm:^8.1.1" mongodb: "npm:6.10.0" mongodb-memory-server: "npm:^10.1.4" p-limit: "npm:3.1.0" From c6d3f604de9dae1e702c251685131a143919fd12 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Wed, 3 Dec 2025 10:41:52 -0600 Subject: [PATCH 09/10] unify diff functions --- ee/packages/abac/src/helper.spec.ts | 491 ++++++++-------------------- ee/packages/abac/src/helper.ts | 127 ++++--- ee/packages/abac/src/index.ts | 17 +- 3 files changed, 231 insertions(+), 404 deletions(-) diff --git a/ee/packages/abac/src/helper.spec.ts b/ee/packages/abac/src/helper.spec.ts index d9261d3e9934d..d443f4d989420 100644 --- a/ee/packages/abac/src/helper.spec.ts +++ b/ee/packages/abac/src/helper.spec.ts @@ -1,352 +1,6 @@ import { expect } from 'chai'; -import { didSubjectLoseAttributes, validateAndNormalizeAttributes, MAX_ABAC_ATTRIBUTE_KEYS, MAX_ABAC_ATTRIBUTE_VALUES } from './helper'; - -describe('didSubjectLoseAttributes', () => { - const call = (prev: { key: string; values: string[] }[], next: { key: string; values: string[] }[]) => - didSubjectLoseAttributes(prev, next); - - it('returns false if previous is empty (no attributes to lose)', () => { - const prev: { key: string; values: string[] }[] = []; - const next: { key: string; values: string[] }[] = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - expect(call(prev, next)).to.be.false; - }); - - it('returns false if all previous attributes and values are preserved', () => { - const prev = [ - { - key: 'role', - values: ['admin', 'user'], - }, - { - key: 'dept', - values: ['sales'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin', 'user'], - }, - { - key: 'dept', - values: ['sales'], - }, - ]; - - expect(call(prev, next)).to.be.false; - }); - - it('returns false if previous values are a subset of next values (nothing lost)', () => { - const prev = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin', 'user'], - }, - ]; - - expect(call(prev, next)).to.be.false; - }); - - it('returns true if an entire previous attribute key is missing in next', () => { - const prev = [ - { - key: 'role', - values: ['admin'], - }, - { - key: 'dept', - values: ['sales'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - expect(call(prev, next)).to.be.true; - }); - - it('returns true if any previous value is missing from corresponding next attribute', () => { - const prev = [ - { - key: 'role', - values: ['admin', 'user'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - expect(call(prev, next)).to.be.true; - }); - - it('returns true if multiple attributes exist and one loses a value', () => { - const prev = [ - { - key: 'role', - values: ['admin', 'user'], - }, - { - key: 'dept', - values: ['sales'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - { - key: 'dept', - values: ['sales'], - }, - ]; - - expect(call(prev, next)).to.be.true; - }); - - it('returns true when next is empty but previous had attributes (all lost)', () => { - const prev = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - const next: { key: string; values: string[] }[] = []; - - expect(call(prev, next)).to.be.true; - }); - - it('returns true if one attribute key remains but all its values are lost', () => { - const prev = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - const next = [ - { - key: 'role', - values: [], - }, - ]; - - expect(call(prev, next)).to.be.true; - }); - - it('returns true on first detected loss even if multiple losses exist', () => { - const prev = [ - { - key: 'role', - values: ['admin', 'user'], - }, - { - key: 'dept', - values: ['sales', 'marketing'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - { - key: 'dept', - values: ['sales'], - }, - ]; - - expect(call(prev, next)).to.be.true; - }); - - it('does not mutate input arrays (pure function)', () => { - const prev = [ - { - key: 'role', - values: ['admin', 'user'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - const prevClone = JSON.parse(JSON.stringify(prev)); - const nextClone = JSON.parse(JSON.stringify(next)); - - call(prev, next); - - expect(prev).to.deep.equal(prevClone); - expect(next).to.deep.equal(nextClone); - }); - - it('returns false if ordering of values changes but values remain (order-insensitive)', () => { - const prev = [ - { - key: 'role', - values: ['admin', 'user'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['user', 'admin'], - }, - ]; - - expect(call(prev, next)).to.be.false; - }); - - it('returns true if previous attribute key replaced with a different key only', () => { - const prev = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - const next = [ - { - key: 'dept', - values: ['admin'], - }, - ]; - - expect(call(prev, next)).to.be.true; - }); - - it('returns false when next adds a new attribute without removing previous ones', () => { - const prev = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - { - key: 'dept', - values: ['sales'], - }, - ]; - - expect(call(prev, next)).to.be.false; - }); - - it('returns false when attribute keys are reordered but unchanged', () => { - const prev = [ - { - key: 'role', - values: ['admin'], - }, - { - key: 'dept', - values: ['sales'], - }, - ]; - - const next = [ - { - key: 'dept', - values: ['sales'], - }, - { - key: 'role', - values: ['admin'], - }, - ]; - - expect(call(prev, next)).to.be.false; - }); - - it('returns false when duplicate values are present but no unique value is lost', () => { - const prev = [ - { - key: 'role', - values: ['admin', 'admin'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - expect(call(prev, next)).to.be.false; - }); - - it('returns false when previous attribute has an empty values array (nothing to lose)', () => { - const prev = [ - { - key: 'role', - values: [], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - expect(call(prev, next)).to.be.false; - }); - - it('returns true when duplicate previous values include one that is lost', () => { - const prev = [ - { - key: 'role', - values: ['admin', 'admin', 'user'], - }, - ]; - - const next = [ - { - key: 'role', - values: ['admin'], - }, - ]; - - expect(call(prev, next)).to.be.true; - }); -}); +import { validateAndNormalizeAttributes, MAX_ABAC_ATTRIBUTE_KEYS, MAX_ABAC_ATTRIBUTE_VALUES, diffAttributeSets } from './helper'; describe('validateAndNormalizeAttributes', () => { it('normalizes keys and merges duplicate values from multiple entries', () => { @@ -410,6 +64,149 @@ describe('buildNonCompliantConditions', () => { expect(result).to.deep.equal([]); }); + describe('diffAttributeSets', () => { + it('returns false/false when both sets are empty', () => { + const result = diffAttributeSets([], []); + + expect(result).to.deep.equal({ added: false, removed: false }); + }); + + it('detects only additions when moving from empty to non-empty', () => { + const current: { key: string; values: string[] }[] = []; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: true, removed: false }); + }); + + it('detects only removals when moving from non-empty to empty', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; + const next: { key: string; values: string[] }[] = []; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: false, removed: true }); + }); + + it('returns false/false when sets are identical (same keys and values)', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'role', values: ['admin'] }, + ]; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'role', values: ['admin'] }, + ]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: false, removed: false }); + }); + + it('ignores value ordering and still treats sets as identical', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'role', values: ['admin', 'user'] }, + ]; + const next: { key: string; values: string[] }[] = [ + { key: 'role', values: ['user', 'admin'] }, + { key: 'dept', values: ['sales', 'eng'] }, + ]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: false, removed: false }); + }); + + it('detects added values for an existing key', () => { + const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; + const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: true, removed: false }); + }); + + it('detects removed values for an existing key', () => { + const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; + const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: false, removed: true }); + }); + + it('detects added and removed values on the same key', () => { + const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; + const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['support'] }]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: true, removed: true }); + }); + + it('detects newly added keys as additions', () => { + const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: true, removed: false }); + }); + + it('detects removed keys as removals', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; + const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: false, removed: true }); + }); + + it('detects both added and removed keys across sets', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'region', values: ['us'] }, + ]; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: true, removed: true }); + }); + + it('handles mixed changes: new key, removed key, added and removed values', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'region', values: ['us', 'eu'] }, + ]; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'support'] }, // sales removed, support added + { key: 'role', values: ['admin'] }, // new key + ]; + + const result = diffAttributeSets(current, next); + + expect(result).to.deep.equal({ added: true, removed: true }); + }); + }); + it('maps single attribute to $not $elemMatch query', () => { const attrs: AttributeDef[] = [{ key: 'dept', values: ['eng', 'sales'] }]; const result = buildNonCompliantConditions(attrs); diff --git a/ee/packages/abac/src/helper.ts b/ee/packages/abac/src/helper.ts index 6658e21405555..8c5313fecb5c4 100644 --- a/ee/packages/abac/src/helper.ts +++ b/ee/packages/abac/src/helper.ts @@ -90,29 +90,6 @@ export function diffAttributes(a: IAbacAttributeDefinition[] = [], b: IAbacAttri return diff; } -export function wereAttributesAdded(current: IAbacAttributeDefinition[], next: IAbacAttributeDefinition[]) { - let added = false; - const prevMap = new Map(current.map((a) => [a.key, new Set(a.values)])); - for (const { key, values } of next) { - const prevValues = prevMap.get(key); - if (!prevValues) { - added = true; - break; - } - for (const v of values) { - if (!prevValues.has(v)) { - added = true; - break; - } - } - if (added) { - break; - } - } - - return added; -} - export function validateAndNormalizeAttributes(attributes: Record): IAbacAttributeDefinition[] { const keyPattern = /^[A-Za-z0-9_-]+$/; const normalized: IAbacAttributeDefinition[] = []; @@ -198,11 +175,6 @@ export async function ensureAttributeDefinitionsExist(normalized: IAbacAttribute } } -export function wereAttributeValuesAdded(prevValues: string[], newValues: string[]) { - const prevSet = new Set(prevValues); - return newValues.some((v) => !prevSet.has(v)); -} - export function buildNonCompliantConditions(newAttributes: IAbacAttributeDefinition[]) { return newAttributes.map(({ key, values }) => ({ abacAttributes: { @@ -227,25 +199,6 @@ export function buildCompliantConditions(attributes: IAbacAttributeDefinition[]) })); } -export function didSubjectLoseAttributes(previous: IAbacAttributeDefinition[], next: IAbacAttributeDefinition[]): boolean { - if (!previous.length) { - return false; - } - const nextMap = new Map(next.map((a) => [a.key, new Set(a.values)])); - for (const prevAttr of previous) { - const nextValues = nextMap.get(prevAttr.key); - if (!nextValues) { - return true; - } - for (const v of prevAttr.values) { - if (!nextValues.has(v)) { - return true; - } - } - } - return false; -} - export function buildRoomNonCompliantConditionsFromSubject(subjectAttributes: IAbacAttributeDefinition[]) { const map = new Map(subjectAttributes.map((a) => [a.key, new Set(a.values)])); const userKeys = Array.from(map.keys()); @@ -288,3 +241,83 @@ export async function getAbacRoom( return room; } + +export function diffAttributeSets( + current: IAbacAttributeDefinition[] = [], + next: IAbacAttributeDefinition[] = [], +): { added: boolean; removed: boolean } { + const currentMap = new Map>(current.map((attr) => [attr.key, new Set(attr.values)])); + const nextMap = new Map>(next.map((attr) => [attr.key, new Set(attr.values)])); + + let added = false; + let removed = false; + + // Check removals and per-key value changes + for (const [key, currentValues] of currentMap) { + const nextValues = nextMap.get(key); + + if (!nextValues) { + // Key was completely removed + removed = true; + } else { + // Check removed values + for (const v of currentValues) { + if (!nextValues.has(v)) { + removed = true; + break; + } + } + } + + if (removed) { + break; + } + } + + // Check additions (new keys or new values on existing keys) + if (!removed) { + for (const [key, nextValues] of nextMap) { + const currentValues = currentMap.get(key); + + if (!currentValues) { + // New key added + added = true; + break; + } + + for (const v of nextValues) { + if (!currentValues.has(v)) { + added = true; + break; + } + } + + if (added) { + break; + } + } + } else { + // Even if we've already seen removals, we might still want to know if additions happened too + for (const [key, nextValues] of nextMap) { + const currentValues = currentMap.get(key); + + if (!currentValues) { + added = true; + break; + } + + for (const v of nextValues) { + if (!currentValues.has(v)) { + added = true; + break; + } + } + + if (added) { + break; + } + } + } + + return { added, removed }; +} diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index abf529b11af68..1b6e4bcbd3e3f 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -18,18 +18,16 @@ import { AbacUnsupportedOperationError, } from './errors'; import { + getAbacRoom, diffAttributes, extractAttribute, - wereAttributesAdded, - didSubjectLoseAttributes, - wereAttributeValuesAdded, + diffAttributeSets, buildCompliantConditions, buildNonCompliantConditions, validateAndNormalizeAttributes, ensureAttributeDefinitionsExist, buildRoomNonCompliantConditionsFromSubject, MAX_ABAC_ATTRIBUTE_KEYS, - getAbacRoom, } from './helper'; // Limit concurrent user removals to avoid overloading the server with too many operations at once @@ -82,7 +80,7 @@ export class AbacService extends ServiceClass implements IAbacService { const finalUser = await Users.setAbacAttributesById(user._id, finalAttributes); - if (didSubjectLoseAttributes(user?.abacAttributes || [], finalAttributes)) { + if (diffAttributeSets(user?.abacAttributes || [], finalAttributes).removed) { await this.onSubjectAttributesChanged(finalUser!, finalAttributes); } @@ -320,7 +318,7 @@ export class AbacService extends ServiceClass implements IAbacService { void Audit.objectAttributeChanged({ _id: room._id, name: room.name }, room.abacAttributes || [], normalized, 'updated', actor); const previous: IAbacAttributeDefinition[] = room.abacAttributes || []; - if (wereAttributesAdded(previous, normalized)) { + if (diffAttributeSets(previous, normalized).added) { await this.onRoomAttributesChanged(room, (updated?.abacAttributes as IAbacAttributeDefinition[] | undefined) ?? normalized); } } @@ -368,7 +366,7 @@ export class AbacService extends ServiceClass implements IAbacService { actor, ); - if (wereAttributeValuesAdded(prevValues, values)) { + if (diffAttributeSets([previous[existingIndex]], [{ key, values }]).added) { const next = previous.map((a, i) => (i === existingIndex ? { key, values } : a)); await this.onRoomAttributesChanged(room, next); } @@ -428,11 +426,10 @@ export class AbacService extends ServiceClass implements IAbacService { const room = await getAbacRoom(rid); - const exists = room?.abacAttributes?.some((a) => a.key === key); + const exists = room?.abacAttributes?.find((a) => a.key === key); if (exists) { const updated = await Rooms.updateAbacAttributeValuesArrayFilteredById(rid, key, values); - const prevValues = room.abacAttributes?.find((a) => a.key === key)?.values ?? []; void Audit.objectAttributeChanged( { _id: room._id, name: room.name }, @@ -441,7 +438,7 @@ export class AbacService extends ServiceClass implements IAbacService { 'key-updated', actor, ); - if (wereAttributeValuesAdded(prevValues, values)) { + if (diffAttributeSets([exists], [{ key, values }]).added) { await this.onRoomAttributesChanged(room, updated?.abacAttributes || []); } From 85036c9ba486b67814c352639b7073a563f96a29 Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Wed, 3 Dec 2025 13:56:29 -0600 Subject: [PATCH 10/10] more tests --- ee/packages/abac/src/helper.spec.ts | 424 +++++++++++++++++++-------- ee/packages/abac/src/helper.ts | 1 + ee/packages/abac/src/service.spec.ts | 4 + 3 files changed, 300 insertions(+), 129 deletions(-) diff --git a/ee/packages/abac/src/helper.spec.ts b/ee/packages/abac/src/helper.spec.ts index d443f4d989420..75b53018f52cb 100644 --- a/ee/packages/abac/src/helper.spec.ts +++ b/ee/packages/abac/src/helper.spec.ts @@ -1,6 +1,13 @@ import { expect } from 'chai'; -import { validateAndNormalizeAttributes, MAX_ABAC_ATTRIBUTE_KEYS, MAX_ABAC_ATTRIBUTE_VALUES, diffAttributeSets } from './helper'; +import { + validateAndNormalizeAttributes, + MAX_ABAC_ATTRIBUTE_KEYS, + MAX_ABAC_ATTRIBUTE_VALUES, + diffAttributeSets, + buildRoomNonCompliantConditionsFromSubject, + extractAttribute, +} from './helper'; describe('validateAndNormalizeAttributes', () => { it('normalizes keys and merges duplicate values from multiple entries', () => { @@ -39,172 +46,172 @@ describe('validateAndNormalizeAttributes', () => { }); }); -describe('buildNonCompliantConditions', () => { - type AttributeDef = { key: string; values: string[] }; +describe('diffAttributeSets', () => { + it('returns false/false when both sets are empty', () => { + const result = diffAttributeSets([], []); - const buildNonCompliantConditions = (attrs: AttributeDef[]): any[] => { - if (!attrs.length) { - return []; - } + expect(result).to.deep.equal({ added: false, removed: false }); + }); - return attrs.map((attr) => ({ - abacAttributes: { - $not: { - $elemMatch: { - key: attr.key, - values: { $all: attr.values }, - }, - }, - }, - })); - }; + it('detects only additions when moving from empty to non-empty', () => { + const current: { key: string; values: string[] }[] = []; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; - it('returns empty array for empty attributes list', () => { - const result = buildNonCompliantConditions([]); - expect(result).to.deep.equal([]); - }); + const result = diffAttributeSets(current, next); - describe('diffAttributeSets', () => { - it('returns false/false when both sets are empty', () => { - const result = diffAttributeSets([], []); + expect(result).to.deep.equal({ added: true, removed: false }); + }); - expect(result).to.deep.equal({ added: false, removed: false }); - }); + it('detects only removals when moving from non-empty to empty', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; + const next: { key: string; values: string[] }[] = []; - it('detects only additions when moving from empty to non-empty', () => { - const current: { key: string; values: string[] }[] = []; - const next: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng'] }, - { key: 'role', values: ['admin'] }, - ]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: false, removed: true }); + }); - expect(result).to.deep.equal({ added: true, removed: false }); - }); + it('returns false/false when sets are identical (same keys and values)', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'role', values: ['admin'] }, + ]; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'role', values: ['admin'] }, + ]; - it('detects only removals when moving from non-empty to empty', () => { - const current: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng'] }, - { key: 'role', values: ['admin'] }, - ]; - const next: { key: string; values: string[] }[] = []; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: false, removed: false }); + }); - expect(result).to.deep.equal({ added: false, removed: true }); - }); + it('ignores value ordering and still treats sets as identical', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'role', values: ['admin', 'user'] }, + ]; + const next: { key: string; values: string[] }[] = [ + { key: 'role', values: ['user', 'admin'] }, + { key: 'dept', values: ['sales', 'eng'] }, + ]; - it('returns false/false when sets are identical (same keys and values)', () => { - const current: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng', 'sales'] }, - { key: 'role', values: ['admin'] }, - ]; - const next: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng', 'sales'] }, - { key: 'role', values: ['admin'] }, - ]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: false, removed: false }); + }); - expect(result).to.deep.equal({ added: false, removed: false }); - }); + it('detects added values for an existing key', () => { + const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; + const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; - it('ignores value ordering and still treats sets as identical', () => { - const current: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng', 'sales'] }, - { key: 'role', values: ['admin', 'user'] }, - ]; - const next: { key: string; values: string[] }[] = [ - { key: 'role', values: ['user', 'admin'] }, - { key: 'dept', values: ['sales', 'eng'] }, - ]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: true, removed: false }); + }); - expect(result).to.deep.equal({ added: false, removed: false }); - }); + it('detects removed values for an existing key', () => { + const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; + const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; - it('detects added values for an existing key', () => { - const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; - const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: false, removed: true }); + }); - expect(result).to.deep.equal({ added: true, removed: false }); - }); + it('detects added and removed values on the same key', () => { + const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; + const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['support'] }]; - it('detects removed values for an existing key', () => { - const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; - const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: true, removed: true }); + }); - expect(result).to.deep.equal({ added: false, removed: true }); - }); + it('detects newly added keys as additions', () => { + const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; - it('detects added and removed values on the same key', () => { - const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng', 'sales'] }]; - const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['support'] }]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: true, removed: false }); + }); - expect(result).to.deep.equal({ added: true, removed: true }); - }); + it('detects removed keys as removals', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; + const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; - it('detects newly added keys as additions', () => { - const current: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; - const next: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng'] }, - { key: 'role', values: ['admin'] }, - ]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: false, removed: true }); + }); - expect(result).to.deep.equal({ added: true, removed: false }); - }); + it('detects both added and removed keys across sets', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'region', values: ['us'] }, + ]; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng'] }, + { key: 'role', values: ['admin'] }, + ]; - it('detects removed keys as removals', () => { - const current: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng'] }, - { key: 'role', values: ['admin'] }, - ]; - const next: { key: string; values: string[] }[] = [{ key: 'dept', values: ['eng'] }]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: true, removed: true }); + }); - expect(result).to.deep.equal({ added: false, removed: true }); - }); + it('handles mixed changes: new key, removed key, added and removed values', () => { + const current: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'region', values: ['us', 'eu'] }, + ]; + const next: { key: string; values: string[] }[] = [ + { key: 'dept', values: ['eng', 'support'] }, // sales removed, support added + { key: 'role', values: ['admin'] }, // new key + ]; - it('detects both added and removed keys across sets', () => { - const current: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng'] }, - { key: 'region', values: ['us'] }, - ]; - const next: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng'] }, - { key: 'role', values: ['admin'] }, - ]; + const result = diffAttributeSets(current, next); - const result = diffAttributeSets(current, next); + expect(result).to.deep.equal({ added: true, removed: true }); + }); +}); - expect(result).to.deep.equal({ added: true, removed: true }); - }); +describe('buildNonCompliantConditions', () => { + type AttributeDef = { key: string; values: string[] }; - it('handles mixed changes: new key, removed key, added and removed values', () => { - const current: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng', 'sales'] }, - { key: 'region', values: ['us', 'eu'] }, - ]; - const next: { key: string; values: string[] }[] = [ - { key: 'dept', values: ['eng', 'support'] }, // sales removed, support added - { key: 'role', values: ['admin'] }, // new key - ]; + const buildNonCompliantConditions = (attrs: AttributeDef[]): any[] => { + if (!attrs.length) { + return []; + } - const result = diffAttributeSets(current, next); + return attrs.map((attr) => ({ + abacAttributes: { + $not: { + $elemMatch: { + key: attr.key, + values: { $all: attr.values }, + }, + }, + }, + })); + }; - expect(result).to.deep.equal({ added: true, removed: true }); - }); + it('returns empty array for empty attributes list', () => { + const result = buildNonCompliantConditions([]); + expect(result).to.deep.equal([]); }); it('maps single attribute to $not $elemMatch query', () => { @@ -257,3 +264,162 @@ describe('buildNonCompliantConditions', () => { ]); }); }); + +describe('buildRoomNonCompliantConditionsFromSubject', () => { + it('returns a single condition matching rooms with keys not in the subject set when subject has one key', () => { + const subject = [{ key: 'dept', values: ['eng'] }]; + + const result = buildRoomNonCompliantConditionsFromSubject(subject); + + expect(result).to.deep.equal([ + { + abacAttributes: { + $elemMatch: { + key: { $nin: ['dept'] }, + }, + }, + }, + { + abacAttributes: { + $elemMatch: { + key: 'dept', + values: { $elemMatch: { $nin: ['eng'] } }, + }, + }, + }, + ]); + }); + + it('builds key-$nin condition using all subject keys and per-key $nin for each attribute', () => { + const subject = [ + { key: 'dept', values: ['eng', 'sales'] }, + { key: 'role', values: ['admin'] }, + ]; + + const result = buildRoomNonCompliantConditionsFromSubject(subject); + + expect(result[0]).to.deep.equal({ + abacAttributes: { + $elemMatch: { + key: { $nin: ['dept', 'role'] }, + }, + }, + }); + + expect(result[1]).to.deep.equal({ + abacAttributes: { + $elemMatch: { + key: 'dept', + values: { $elemMatch: { $nin: ['eng', 'sales'] } }, + }, + }, + }); + + expect(result[2]).to.deep.equal({ + abacAttributes: { + $elemMatch: { + key: 'role', + values: { $elemMatch: { $nin: ['admin'] } }, + }, + }, + }); + }); + + it('returns only the key-$nin condition when subject has no values for keys (empty arrays)', () => { + const subject = [ + { key: 'dept', values: [] }, + { key: 'role', values: [] }, + ]; + + const result = buildRoomNonCompliantConditionsFromSubject(subject); + + expect(result[0]).to.deep.equal({ + abacAttributes: { + $elemMatch: { + key: { $nin: ['dept', 'role'] }, + }, + }, + }); + + expect(result[1]).to.deep.equal({ + abacAttributes: { + $elemMatch: { + key: 'dept', + values: { $elemMatch: { $nin: [] } }, + }, + }, + }); + + expect(result[2]).to.deep.equal({ + abacAttributes: { + $elemMatch: { + key: 'role', + values: { $elemMatch: { $nin: [] } }, + }, + }, + }); + }); +}); + +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; + }); + + 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; + }); + + it('extracts and normalizes a single string value', () => { + const ldapUser = { department: ' Engineering ' } as any; + + const result = extractAttribute(ldapUser, 'department', 'dept'); + + expect(result).to.deep.equal({ + key: 'dept', + values: ['Engineering'], + }); + }); + + it('extracts from an array, trimming and deduplicating values', () => { + const ldapUser = { + memberOf: [' Eng ', 'Sales', 'Eng', ' ', '\tSales\t'], + } as any; + + const result = extractAttribute(ldapUser, 'memberOf', 'dept'); + + // Order is preserved by insertion into the Set in implementation: ['Eng', 'Sales'] + expect(result).to.deep.equal({ + key: 'dept', + values: ['Eng', 'Sales'], + }); + }); + + it('ignores non-string values and empty/whitespace-only strings', () => { + const ldapUser = { + memberOf: [' ', 123, null, ' Eng ', undefined, {}, '\n'], + } as any; + + const result = extractAttribute(ldapUser, 'memberOf', 'dept'); + + expect(result).to.deep.equal({ + key: 'dept', + values: ['Eng'], + }); + }); + + it('returns undefined when all values are filtered out', () => { + const ldapUser = { + memberOf: [' ', '\n', '\t'], + } as any; + + const result = extractAttribute(ldapUser, 'memberOf', 'dept'); + + expect(result).to.be.undefined; + }); +}); diff --git a/ee/packages/abac/src/helper.ts b/ee/packages/abac/src/helper.ts index 8c5313fecb5c4..5522b09b99662 100644 --- a/ee/packages/abac/src/helper.ts +++ b/ee/packages/abac/src/helper.ts @@ -147,6 +147,7 @@ const getAttributeDefinitionsFromDb = async (keys: string[]) => const getAttributeDefinitionsCached = mem(getAttributeDefinitionsFromDb, { maxAge: 30_000, + cacheKey: JSON.stringify, }); export async function ensureAttributeDefinitionsExist(normalized: IAbacAttributeDefinition[]): Promise { diff --git a/ee/packages/abac/src/service.spec.ts b/ee/packages/abac/src/service.spec.ts index ee98dc8759c00..9b67209b5349f 100644 --- a/ee/packages/abac/src/service.spec.ts +++ b/ee/packages/abac/src/service.spec.ts @@ -69,6 +69,10 @@ jest.mock('@rocket.chat/core-services', () => { }; }); +jest.mock('mem', () => { + return jest.fn((fn: any) => fn); +}); + describe('AbacService (unit)', () => { let service: AbacService;