fix(rbac): make updateRole atomic via dataSource.transaction (#1050)

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<Repository<Role>>;
   let permissionRepository: jest.Mocked<Repository<Permission>>;
   let auditLogService: jest.Mocked<Pick<AuditLogService, 'log'>>;
+  let dataSource: jest.Mocked<Pick<DataSource, 'transaction'>>;
 
   const baseRole: Role = {
     id: 'role-1',
@@ -28,6 +77,12 @@
   };
 
   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 @@
 
     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<any>) => {
+        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 @@
     );
   };
 
+  // ── 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 @@
       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<any>) => cb(manager),
+    );
 
     await service.updateRole('role-1', 'admin', undefined, ['p-new']);
 
@@ -195,4 +269,95 @@
       }),
     );
   });
+
+  // ── 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<any>) => {
+          // 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<any>) => 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<any>) => {
+          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']);
+    });
+  });
 });

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<Permission>,
     private readonly auditLogService: AuditLogService,
+    private readonly dataSource: DataSource,
   ) {}
 
   async createRole(
@@ -74,42 +75,75 @@ export class RolesService {
     permissionIds?: string[],
     context: RbacAuditContext = {},
   ): Promise<Role> {
-    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,15 +162,15 @@ export class RolesService {
         if (!newSet.has(permId)) {
           await this.writeAudit({
             action: AuditAction.RBAC_PERMISSION_REVOKED,
-            role: updated,
+            role: updated!,
             context,
             metadata: { permissionId: permId },
           });
         }
       }
     }
 
-    return updated;
+    return updated!;
   }
 
   async deleteRole(id: string, context: RbacAuditContext = {}): Promise<void> {