diff --git a/src/rbac/roles/roles.service.spec.ts b/src/rbac/roles/roles.service.spec.ts index 6a3dd1ae..66844a12 100644 --- a/src/rbac/roles/roles.service.spec.ts +++ b/src/rbac/roles/roles.service.spec.ts @@ -13,6 +13,7 @@ describe('RolesService', () => { let roleRepository: jest.Mocked>; let permissionRepository: jest.Mocked>; let auditLogService: jest.Mocked>; + let dataSource: jest.Mocked>; const baseRole: Role = { id: 'role-1', @@ -27,6 +28,12 @@ describe('RolesService', () => { } as Role; 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 +56,21 @@ describe('RolesService', () => { 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(); @@ -276,4 +292,95 @@ describe('RolesService', () => { }), ); }); + + // ── 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 7ec04c35..f789f355 100644 --- a/src/rbac/roles/roles.service.ts +++ b/src/rbac/roles/roles.service.ts @@ -35,6 +35,7 @@ export class RolesService { @InjectRepository(Permission) private readonly permissionRepository: Repository, private readonly auditLogService: AuditLogService, + private readonly dataSource: DataSource, ) {} /** @@ -176,13 +177,13 @@ export class RolesService { 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), }, }); @@ -194,7 +195,7 @@ export class RolesService { if (!oldSet.has(permId)) { await this.writeAudit({ action: AuditAction.RBAC_PERMISSION_GRANTED, - role: updated, + role: updated!, context, metadata: { permissionId: permId }, }); @@ -205,7 +206,7 @@ export class RolesService { if (!newSet.has(permId)) { await this.writeAudit({ action: AuditAction.RBAC_PERMISSION_REVOKED, - role: updated, + role: updated!, context, metadata: { permissionId: permId }, }); @@ -213,7 +214,7 @@ export class RolesService { } } - return updated; + return updated!; } async deleteRole(id: string, context: RbacAuditContext = {}): Promise {