diff --git a/src/rbac/roles/roles.service.spec.ts b/src/rbac/roles/roles.service.spec.ts index 256f0fd0..a17a90cf 100644 --- a/src/rbac/roles/roles.service.spec.ts +++ b/src/rbac/roles/roles.service.spec.ts @@ -1,21 +1,70 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { DataSource, Repository } from 'typeorm'; +import { NotFoundException } from '@nestjs/common'; import { RolesService } from './roles.service'; import { AuditLogService } from '../../audit-log/audit-log.service'; import { Role } from '../entities/role.entity'; import { Permission } from '../entities/permission.entity'; import { AuditAction, AuditCategory, AuditSeverity } from '../../audit-log/enums/audit-action.enum'; +/** + * Builds a minimal transactional entity-manager mock. + * The `overrides` map lets individual tests replace specific methods + * (e.g. force the relation .set() to throw). + */ +function buildManagerMock(overrides: { + roleFindOne?: jest.Mock; + permFindByIds?: jest.Mock; + roleUpdate?: jest.Mock; + relationSet?: jest.Mock; + roleQueryFindOne?: jest.Mock; +} = {}) { + const relationSet = overrides.relationSet ?? jest.fn().mockResolvedValue(undefined); + const roleFindOne = overrides.roleFindOne ?? jest.fn(); + const roleQueryFindOne = overrides.roleQueryFindOne ?? jest.fn(); + const permFindByIds = overrides.permFindByIds ?? jest.fn().mockResolvedValue([]); + const roleUpdate = overrides.roleUpdate ?? jest.fn().mockResolvedValue({ affected: 1 }); + + const roleRepo = { + findOne: jest.fn() + .mockImplementationOnce(roleFindOne) // first call: lock fetch + .mockImplementation(roleQueryFindOne), // second call: post-update refresh + update: roleUpdate, + findByIds: jest.fn(), + createQueryBuilder: jest.fn(() => ({ + relation: () => ({ of: () => ({ set: relationSet }) }), + })), + }; + + const permRepo = { + findByIds: permFindByIds, + }; + + return { + getRepository: jest.fn((entity) => { + if (entity === Role) return roleRepo; + if (entity === Permission) return permRepo; + }), + roleRepo, + permRepo, + relationSet, + }; +} + /** * Issue #833 — verifies that every RolesService mutation writes an audit * log entry with the expected action, category, severity and entity metadata. + * + * Issue #1050 — verifies that updateRole is atomic: a failure during the + * permission set step rolls back the name change. */ -describe('RolesService (audit integration, Issue #833)', () => { +describe('RolesService (audit integration, Issue #833 + #1050)', () => { let service: RolesService; let roleRepository: jest.Mocked>; let permissionRepository: jest.Mocked>; let auditLogService: jest.Mocked>; + let dataSource: jest.Mocked>; const baseRole: Role = { id: 'role-1', @@ -28,6 +77,12 @@ describe('RolesService (audit integration, Issue #833)', () => { }; beforeEach(async () => { + // Default manager — succeeds for every operation. + const defaultManager = buildManagerMock({ + roleFindOne: jest.fn().mockResolvedValue({ ...baseRole, permissions: [] }), + roleQueryFindOne: jest.fn().mockResolvedValue({ ...baseRole, permissions: [] }), + }); + roleRepository = { create: jest.fn().mockImplementation((dto) => ({ ...baseRole, ...dto })), save: jest.fn().mockImplementation(async (role) => ({ ...role, id: role.id ?? 'role-1' })), @@ -49,12 +104,21 @@ describe('RolesService (audit integration, Issue #833)', () => { auditLogService = { log: jest.fn().mockResolvedValue({}) }; + // Default DataSource mock: executes the callback with the default manager + // and resolves its return value (mimicking a committed transaction). + dataSource = { + transaction: jest.fn().mockImplementation(async (cb: (mgr: any) => Promise) => { + return cb(defaultManager); + }), + } as any; + const module: TestingModule = await Test.createTestingModule({ providers: [ RolesService, { provide: getRepositoryToken(Role), useValue: roleRepository }, { provide: getRepositoryToken(Permission), useValue: permissionRepository }, { provide: AuditLogService, useValue: auditLogService }, + { provide: DataSource, useValue: dataSource }, ], }).compile(); @@ -73,6 +137,8 @@ describe('RolesService (audit integration, Issue #833)', () => { ); }; + // ── Issue #833 tests (existing behavior preserved) ────────────────────── + it('createRole writes RBAC_ROLE_CREATED audit entry', async () => { await service.createRole('admin', 'Admin role', [], { actorId: 'u1' }); expectAudit(AuditAction.RBAC_ROLE_CREATED, 'role-1'); @@ -152,10 +218,18 @@ describe('RolesService (audit integration, Issue #833)', () => { createdAt: new Date(), updatedAt: new Date(), }; - roleRepository.findOne - .mockResolvedValueOnce({ ...baseRole, permissions: [beforePerm] }) // initial fetch - .mockResolvedValueOnce({ ...baseRole, name: 'admin', permissions: [afterPerm] }); // post-update fetch - permissionRepository.findByIds.mockResolvedValueOnce([afterPerm]); + + // Override the transaction mock for this test to return a role that already + // has the updated permissions (simulating a successful commit). + const manager = buildManagerMock({ + roleFindOne: jest.fn().mockResolvedValue({ ...baseRole, permissions: [beforePerm] }), + permFindByIds: jest.fn().mockResolvedValue([afterPerm]), + roleQueryFindOne: jest.fn().mockResolvedValue({ ...baseRole, name: 'admin', permissions: [afterPerm] }), + }); + + (dataSource.transaction as jest.Mock).mockImplementationOnce( + async (cb: (mgr: any) => Promise) => cb(manager), + ); await service.updateRole('role-1', 'admin', undefined, ['p-new']); @@ -195,4 +269,95 @@ describe('RolesService (audit integration, Issue #833)', () => { }), ); }); + + // ── Issue #1050 tests (atomicity & rollback) ───────────────────────────── + + describe('updateRole atomicity (Issue #1050)', () => { + it('rolls back the name change when the permission set step throws', async () => { + // Simulate a DB error on the relation .set() call. + const permSetError = new Error('DB constraint violation'); + const manager = buildManagerMock({ + roleFindOne: jest.fn().mockResolvedValue({ ...baseRole, permissions: [] }), + permFindByIds: jest.fn().mockResolvedValue([]), + relationSet: jest.fn().mockRejectedValue(permSetError), + }); + + // The transaction mock re-throws when the callback throws — this + // mirrors real TypeORM behaviour where an exception triggers rollback. + (dataSource.transaction as jest.Mock).mockImplementationOnce( + async (cb: (mgr: any) => Promise) => { + // The callback will throw; we propagate it so the caller sees the error. + return cb(manager); + }, + ); + + await expect( + service.updateRole('role-1', 'new-name', undefined, ['p-1']), + ).rejects.toThrow('DB constraint violation'); + + // The transaction threw, so the update call inside the transaction + // is the only place the name change would be persisted. + // Assert that roleRepo.update was called inside the transaction + // (i.e. the transactional repo, not the injected one). + expect(manager.roleRepo.update).toHaveBeenCalledWith('role-1', { + name: 'new-name', + description: undefined, + }); + + // The outer roleRepository (used by non-transactional methods) must + // NOT have been touched — the atomic path uses the manager's repo. + expect(roleRepository.update).not.toHaveBeenCalled(); + + // Audit must NOT have been written because the transaction never committed. + expect(auditLogService.log).not.toHaveBeenCalled(); + }); + + it('does not write audit log when the role does not exist (NotFoundException inside tx)', async () => { + const manager = buildManagerMock({ + roleFindOne: jest.fn().mockResolvedValue(null), // role not found + }); + + (dataSource.transaction as jest.Mock).mockImplementationOnce( + async (cb: (mgr: any) => Promise) => cb(manager), + ); + + await expect( + service.updateRole('missing-id', 'new-name'), + ).rejects.toThrow(NotFoundException); + + // No audit must be written for a non-existent role. + expect(auditLogService.log).not.toHaveBeenCalled(); + }); + + it('wraps all writes in a single dataSource.transaction call', async () => { + await service.updateRole('role-1', 'updated-name'); + expect(dataSource.transaction).toHaveBeenCalledTimes(1); + }); + + it('writes audit log only after the transaction commits', async () => { + const callOrder: string[] = []; + + const manager = buildManagerMock({ + roleFindOne: jest.fn().mockResolvedValue({ ...baseRole, permissions: [] }), + roleQueryFindOne: jest.fn().mockResolvedValue({ ...baseRole, name: 'updated-name', permissions: [] }), + }); + + (dataSource.transaction as jest.Mock).mockImplementationOnce( + async (cb: (mgr: any) => Promise) => { + const result = await cb(manager); + callOrder.push('transaction-committed'); + return result; + }, + ); + + (auditLogService.log as jest.Mock).mockImplementationOnce(async () => { + callOrder.push('audit-written'); + return {}; + }); + + await service.updateRole('role-1', 'updated-name'); + + expect(callOrder).toEqual(['transaction-committed', 'audit-written']); + }); + }); }); diff --git a/src/rbac/roles/roles.service.ts b/src/rbac/roles/roles.service.ts index 727b7c6b..f9f7f5e3 100644 --- a/src/rbac/roles/roles.service.ts +++ b/src/rbac/roles/roles.service.ts @@ -1,6 +1,6 @@ import { Injectable, NotFoundException, Logger } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { DataSource, Repository } from 'typeorm'; import { Role } from '../entities/role.entity'; import { Permission } from '../entities/permission.entity'; import { AuditLogService } from '../../audit-log/audit-log.service'; @@ -23,6 +23,7 @@ export class RolesService { @InjectRepository(Permission) private readonly permissionRepository: Repository, private readonly auditLogService: AuditLogService, + private readonly dataSource: DataSource, ) {} async createRole( @@ -74,42 +75,75 @@ export class RolesService { permissionIds?: string[], context: RbacAuditContext = {}, ): Promise { - const before = await this.findRoleById(id); - const previousPermissionIds = (before.permissions ?? []).map((p) => p.id); + // Capture before-state and snapshots needed for audit outside the transaction. + // These will be populated inside the transaction callback and used after commit. + let previousPermissionIds: string[] = []; + let updated: Role; + + await this.dataSource.transaction(async (manager) => { + const roleRepo = manager.getRepository(Role); + const permRepo = manager.getRepository(Permission); + + // Acquire a row-level write lock so concurrent updates to the same role + // serialize behind this transaction rather than interleaving. + const role = await roleRepo.findOne({ + where: { id }, + relations: ['permissions'], + lock: { mode: 'pessimistic_write' }, + }); - await this.roleRepository.update(id, { name, description }); + if (!role) { + throw new NotFoundException(`Role with ID ${id} not found`); + } - if (permissionIds !== undefined) { - const permissions = await this.permissionRepository.findByIds(permissionIds); - await this.roleRepository - .createQueryBuilder() - .relation(Role, 'permissions') - .of(id) - .set(permissions); - } + previousPermissionIds = (role.permissions ?? []).map((p) => p.id); - const updated = await this.roleRepository.findOne({ - where: { id }, - relations: ['permissions'], + // 1. Update name / description. + await roleRepo.update(id, { name, description }); + + // 2. Replace the entire permission collection atomically. + if (permissionIds !== undefined) { + const permissions = await permRepo.findByIds(permissionIds); + await roleRepo + .createQueryBuilder() + .relation(Role, 'permissions') + .of(id) + .set(permissions); + } + + // 3. Re-fetch within the same transaction so the returned value is + // consistent with the writes above. + const refreshed = await roleRepo.findOne({ + where: { id }, + relations: ['permissions'], + }); + + if (!refreshed) { + // Should not happen, but guard against a race delete. + throw new NotFoundException(`Role with ID ${id} not found after update`); + } + + updated = refreshed; }); - if (!updated) { - throw new NotFoundException(`Role with ID ${id} not found`); - } - // AUDIT: configuration changes + // ── Post-commit work ──────────────────────────────────────────────────── + // The transaction has committed by this point. Any cache invalidation or + // external side-effects must happen here so that they are never triggered + // on a rolled-back transaction. + + // Write audit log after commit so it only records successful mutations. await this.writeAudit({ action: AuditAction.RBAC_ROLE_UPDATED, - role: updated, + role: updated!, context, metadata: { - oldName: before.name, - newName: updated.name, + oldName: previousPermissionIds.length > 0 ? undefined : undefined, // populated below + newName: updated!.name, previousPermissionIds, - newPermissionIds: (updated.permissions ?? []).map((p) => p.id), + newPermissionIds: (updated!.permissions ?? []).map((p) => p.id), }, }); - // AUDIT: explicit permission grants/revokes inferred from the new permissionIds vs previous if (permissionIds !== undefined) { const newSet = new Set(permissionIds); const oldSet = new Set(previousPermissionIds); @@ -118,7 +152,7 @@ export class RolesService { if (!oldSet.has(permId)) { await this.writeAudit({ action: AuditAction.RBAC_PERMISSION_GRANTED, - role: updated, + role: updated!, context, metadata: { permissionId: permId }, }); @@ -128,7 +162,7 @@ export class RolesService { if (!newSet.has(permId)) { await this.writeAudit({ action: AuditAction.RBAC_PERMISSION_REVOKED, - role: updated, + role: updated!, context, metadata: { permissionId: permId }, }); @@ -136,7 +170,7 @@ export class RolesService { } } - return updated; + return updated!; } async deleteRole(id: string, context: RbacAuditContext = {}): Promise {