From 05b6ac77d98c2ce21de693d6cd73118be24fed2f Mon Sep 17 00:00:00 2001 From: Kevin Aleman Date: Mon, 15 Dec 2025 12:52:20 -0600 Subject: [PATCH] fix auditions --- .../server/hooks/abac/beforeAddUserToRoom.ts | 2 +- apps/meteor/ee/server/lib/audit/methods.ts | 5 ++- apps/meteor/tests/end-to-end/api/abac.ts | 31 ++++++++++++- ee/packages/abac/src/audit.ts | 10 ++++- ee/packages/abac/src/index.ts | 7 ++- ee/packages/abac/src/service.spec.ts | 45 ++++++++++++++++--- .../core-services/src/types/IAbacService.ts | 2 +- .../src/ServerAudit/IAuditServerAbacAction.ts | 8 ++-- 8 files changed, 91 insertions(+), 19 deletions(-) diff --git a/apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts b/apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts index 5b298a0271553..4b202c1c5735c 100644 --- a/apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts +++ b/apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts @@ -18,5 +18,5 @@ beforeAddUserToRoom.patch(async (prev, users, room, actor) => { throw new Error('error-room-is-abac-managed'); } - await Abac.checkUsernamesMatchAttributes(validUsers as string[], room.abacAttributes); + await Abac.checkUsernamesMatchAttributes(validUsers as string[], room.abacAttributes, room._id); }); diff --git a/apps/meteor/ee/server/lib/audit/methods.ts b/apps/meteor/ee/server/lib/audit/methods.ts index 429309cb8edb9..3cb9a142452e2 100644 --- a/apps/meteor/ee/server/lib/audit/methods.ts +++ b/apps/meteor/ee/server/lib/audit/methods.ts @@ -37,7 +37,8 @@ const getRoomInfoByAuditParams = async ({ userId: string; }) => { if (rid) { - return getValue(await Rooms.findOne({ _id: rid })); + // When ABAC is enabled, only rooms without ABAC attributes are considered for auditing by room ID. + return getValue(await Rooms.findOne({ _id: rid, abacAttributes: { $exists: false } })); } if (type === 'd') { @@ -165,7 +166,7 @@ Meteor.methods({ } else { const roomInfo = await getRoomInfoByAuditParams({ type, roomId: rid, users: usernames, visitor, agent, userId: user._id }); if (!roomInfo) { - throw new Meteor.Error('Room doesn`t exist'); + throw new Meteor.Error(`Room doesn't exist`); } rids = roomInfo.rids; diff --git a/apps/meteor/tests/end-to-end/api/abac.ts b/apps/meteor/tests/end-to-end/api/abac.ts index c5c3e7cb2a7ad..c82a1984234b4 100644 --- a/apps/meteor/tests/end-to-end/api/abac.ts +++ b/apps/meteor/tests/end-to-end/api/abac.ts @@ -4,7 +4,7 @@ import { expect } from 'chai'; import { before, after, describe, it } from 'mocha'; import { MongoClient } from 'mongodb'; -import { getCredentials, request, credentials } from '../../data/api-data'; +import { getCredentials, request, credentials, methodCall } from '../../data/api-data'; import { sleep } from '../../data/livechat/utils'; import { updatePermission, updateSetting } from '../../data/permissions.helper'; import { createRoom, deleteRoom } from '../../data/rooms.helper'; @@ -387,6 +387,35 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I }); }); + it('should throw an error when trying to audit the messages of an abac managed room', async () => { + await request + .post(methodCall('auditGetMessages')) + .set(credentials) + .send({ + message: JSON.stringify({ + method: 'auditGetMessages', + params: [ + { + type: '', + msg: 'test1234', + startDate: { $date: new Date() }, + endDate: { $date: new Date() }, + rid: testRoom._id, + users: [], + }, + ], + id: '14', + msg: 'method', + }), + }) + .expect(200) + .expect((res) => { + const result = JSON.parse(res.body.message); + expect(result).to.have.property('error'); + expect(result.error).to.have.property('error', `Room doesn't exist`); + }); + }); + it('PUT room attribute should replace values and keep inUse=true', async () => { await request .put(`${v1}/abac/rooms/${testRoom._id}/attributes/${updatedKey}`) diff --git a/ee/packages/abac/src/audit.ts b/ee/packages/abac/src/audit.ts index e1b076f188607..83977a9af0d78 100644 --- a/ee/packages/abac/src/audit.ts +++ b/ee/packages/abac/src/audit.ts @@ -9,6 +9,7 @@ import type { AbacAuditReason, MinimalRoom, MinimalUser, + AbacActionPerformed, } from '@rocket.chat/core-typings'; import { ServerEvents } from '@rocket.chat/models'; @@ -116,11 +117,16 @@ export const Audit = { { type: 'user', _id: actor._id, username: actor.username!, ip: '0.0.0.0', useragent: '' }, ); }, - actionPerformed: async (subject: MinimalUser, object: MinimalRoom, reason: AbacAuditReason = 'room-attributes-change') => { + actionPerformed: async ( + subject: MinimalUser, + object: MinimalRoom, + reason: AbacAuditReason = 'room-attributes-change', + actionPerformed: AbacActionPerformed = 'revoked-object-access', + ) => { return audit( 'abac.action.performed', { - action: 'revoked-object-access', + action: actionPerformed, reason, subject, object, diff --git a/ee/packages/abac/src/index.ts b/ee/packages/abac/src/index.ts index 622d353dab3aa..216b003b29ca1 100644 --- a/ee/packages/abac/src/index.ts +++ b/ee/packages/abac/src/index.ts @@ -461,7 +461,7 @@ export class AbacService extends ServiceClass implements IAbacService { await this.onRoomAttributesChanged(room, updated?.abacAttributes || []); } - async checkUsernamesMatchAttributes(usernames: string[], attributes: IAbacAttributeDefinition[]): Promise { + async checkUsernamesMatchAttributes(usernames: string[], attributes: IAbacAttributeDefinition[], objectId: string): Promise { if (!usernames.length || !attributes.length) { return; } @@ -486,9 +486,8 @@ export class AbacService extends ServiceClass implements IAbacService { ); } - this.logger.debug({ - msg: 'User list complied with ABAC attributes for room', - usernames, + usernames.forEach((username) => { + void Audit.actionPerformed({ username }, { _id: objectId }, 'system', 'granted-object-access'); }); } diff --git a/ee/packages/abac/src/service.spec.ts b/ee/packages/abac/src/service.spec.ts index 8eacd697d69da..f80c00407bb37 100644 --- a/ee/packages/abac/src/service.spec.ts +++ b/ee/packages/abac/src/service.spec.ts @@ -21,6 +21,7 @@ const mockUsersUpdateOne = jest.fn(); const mockUsersSetAbacAttributesById = jest.fn(); const mockUsersUnsetAbacAttributesById = jest.fn(); const mockAbacFindOneAndUpdate = jest.fn(); +const mockCreateAuditServerEvent = jest.fn(); jest.mock('@rocket.chat/models', () => ({ Rooms: { @@ -53,7 +54,7 @@ jest.mock('@rocket.chat/models', () => ({ updateOne: (...args: any[]) => mockUsersUpdateOne(...args), }, ServerEvents: { - createAuditServerEvent: jest.fn(), + createAuditServerEvent: (...args: any[]) => mockCreateAuditServerEvent(...args), }, })); @@ -1030,17 +1031,18 @@ describe('AbacService (unit)', () => { describe('checkUsernamesMatchAttributes', () => { beforeEach(() => { mockUsersFind.mockReset(); + mockCreateAuditServerEvent.mockReset(); }); const attributes = [{ key: 'dept', values: ['eng'] }]; it('returns early (no query) when usernames array is empty', async () => { - await expect(service.checkUsernamesMatchAttributes([], attributes as any)).resolves.toBeUndefined(); + await expect(service.checkUsernamesMatchAttributes([], attributes as any, 'objectId')).resolves.toBeUndefined(); expect(mockUsersFind).not.toHaveBeenCalled(); }); it('returns early (no query) when attributes array is empty', async () => { - await expect(service.checkUsernamesMatchAttributes(['alice'], [])).resolves.toBeUndefined(); + await expect(service.checkUsernamesMatchAttributes(['alice'], [], 'objectId')).resolves.toBeUndefined(); expect(mockUsersFind).not.toHaveBeenCalled(); }); @@ -1052,7 +1054,7 @@ describe('AbacService (unit)', () => { }), })); - await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any)).resolves.toBeUndefined(); + await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any, 'objectId')).resolves.toBeUndefined(); expect(mockUsersFind).toHaveBeenCalledWith( { @@ -1083,11 +1085,44 @@ describe('AbacService (unit)', () => { }), })); - await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any)).rejects.toMatchObject({ + await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any, 'objectId')).rejects.toMatchObject({ error: 'error-usernames-not-matching-abac-attributes', message: expect.stringContaining('[error-usernames-not-matching-abac-attributes]'), details: expect.arrayContaining(['bob', 'charlie']), }); }); + + it('generates an audit log for every compliant username', async () => { + const usernames = ['alice', 'bob']; + + mockUsersFind.mockImplementationOnce(() => ({ + map: () => ({ + toArray: async () => [], + }), + })); + + await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any, 'objectId')).resolves.toBeUndefined(); + + expect(mockCreateAuditServerEvent).toHaveBeenCalledTimes(usernames.length); + const calledUsernames = mockCreateAuditServerEvent.mock.calls.map(([, payload]: any[]) => payload?.subject?.username).filter(Boolean); + expect(calledUsernames.sort()).toEqual(usernames.sort()); + }); + + it('does not generate audit logs when usernames do not match attributes', async () => { + const usernames = ['alice', 'bob', 'charlie']; + const nonCompliantDocs = [{ username: 'alice' }, { username: 'bob' }, { username: 'charlie' }]; + + mockUsersFind.mockImplementationOnce(() => ({ + map: (fn: (u: any) => string) => ({ + toArray: async () => nonCompliantDocs.map(fn), + }), + })); + + await expect(service.checkUsernamesMatchAttributes(usernames, attributes as any, 'objectId')).rejects.toMatchObject({ + error: 'error-usernames-not-matching-abac-attributes', + }); + + expect(mockCreateAuditServerEvent).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/core-services/src/types/IAbacService.ts b/packages/core-services/src/types/IAbacService.ts index 9d567052c4aa9..415dd26bfbebc 100644 --- a/packages/core-services/src/types/IAbacService.ts +++ b/packages/core-services/src/types/IAbacService.ts @@ -38,7 +38,7 @@ export interface IAbacService { removeRoomAbacAttribute(rid: string, key: string, actor: AbacActor | undefined): Promise; addRoomAbacAttributeByKey(rid: string, key: string, values: string[], actor: AbacActor | undefined): Promise; replaceRoomAbacAttributeByKey(rid: string, key: string, values: string[], actor: AbacActor | undefined): Promise; - checkUsernamesMatchAttributes(usernames: string[], attributes: IAbacAttributeDefinition[]): Promise; + checkUsernamesMatchAttributes(usernames: string[], attributes: IAbacAttributeDefinition[], objectId: string): Promise; canAccessObject( room: Pick, user: Pick, diff --git a/packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts b/packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts index e8f97f7606b22..f080168446c9f 100644 --- a/packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts +++ b/packages/core-typings/src/ServerAudit/IAuditServerAbacAction.ts @@ -1,10 +1,12 @@ -import type { IUser, IRoom, IAuditServerEventType, IAbacAttributeDefinition, IServerEvents } from '..'; +import type { IUser, IRoom, IAuditServerEventType, IAbacAttributeDefinition, IServerEvents, Optional } from '..'; -export type MinimalUser = Pick; +export type MinimalUser = Pick & Optional, '_id'>; export type MinimalRoom = Pick; export type AbacAuditReason = 'ldap-sync' | 'room-attributes-change' | 'system' | 'api' | 'realtime-policy-eval'; +export type AbacActionPerformed = 'revoked-object-access' | 'granted-object-access'; + export type AbacAttributeDefinitionChangeType = | 'created' | 'updated' @@ -54,7 +56,7 @@ interface IServerEventAbacAttributeChanged interface IServerEventAbacActionPerformed extends IAuditServerEventType< - | { key: 'action'; value: 'revoked-object-access' } + | { key: 'action'; value: AbacActionPerformed } | { key: 'reason'; value: AbacAuditReason } | { key: 'subject'; value: MinimalUser | undefined } | { key: 'object'; value: MinimalRoom | undefined }