fix: scope rename rollback to only migrated users, prevent cross-role corruption

Capture user IDs before forward migration so the rollback path only
reverts users this request actually moved. Previously the rollback called
updateUsersByRole(newName, currentName) which would sweep all users with
the new role — including any independently assigned by a concurrent admin
request — causing silent cross-role data corruption.

Adds findUserIdsByRole and updateUsersRoleByIds to the data layer.
Extracts rollbackMigratedUsers helper to deduplicate rollback sites.

packages/api/src/admin/roles.spec.ts

@@ -67,6 +67,8 @@ function createDeps(overrides: Partial<AdminRolesDeps> = {}): AdminRolesDeps {
     findUser: jest.fn().mockResolvedValue(null),
     updateUser: jest.fn().mockResolvedValue(mockUser()),
     updateUsersByRole: jest.fn().mockResolvedValue(undefined),
+    findUserIdsByRole: jest.fn().mockResolvedValue(['uid-1', 'uid-2']),
+    updateUsersRoleByIds: jest.fn().mockResolvedValue(undefined),
     listUsersByRole: jest.fn().mockResolvedValue([]),
     countUsersByRole: jest.fn().mockResolvedValue(0),
     ...overrides,
@@ -455,6 +457,10 @@ describe('createAdminRolesHandlers', () => {
       const callOrder: string[] = [];
       const deps = createDeps({
         getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null),
+        findUserIdsByRole: jest.fn().mockImplementation(() => {
+          callOrder.push('findUserIdsByRole');
+          return Promise.resolve(['uid-1']);
+        }),
         updateUsersByRole: jest.fn().mockImplementation(() => {
           callOrder.push('updateUsersByRole');
           return Promise.resolve();
@@ -473,8 +479,9 @@ describe('createAdminRolesHandlers', () => {
       await handlers.updateRole(req, res);
 
       expect(status).toHaveBeenCalledWith(200);
+      expect(deps.findUserIdsByRole).toHaveBeenCalledWith('editor');
       expect(deps.updateUsersByRole).toHaveBeenCalledWith('editor', 'new-name');
-      expect(callOrder).toEqual(['updateUsersByRole', 'updateRoleByName']);
+      expect(callOrder).toEqual(['findUserIdsByRole', 'updateUsersByRole', 'updateRoleByName']);
     });
 
     it('does not rename role when user migration fails', async () => {
@@ -655,8 +662,10 @@ describe('createAdminRolesHandlers', () => {
     });
 
     it('rolls back user migration when rename fails', async () => {
+      const ids = ['uid-1', 'uid-2'];
       const deps = createDeps({
         getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null),
+        findUserIdsByRole: jest.fn().mockResolvedValue(ids),
         updateRoleByName: jest.fn().mockResolvedValue(null),
       });
       const handlers = createAdminRolesHandlers(deps);
@@ -669,14 +678,16 @@ describe('createAdminRolesHandlers', () => {
 
       expect(status).toHaveBeenCalledWith(404);
       expect(json).toHaveBeenCalledWith({ error: 'Role not found' });
-      expect(deps.updateUsersByRole).toHaveBeenCalledTimes(2);
+      expect(deps.updateUsersByRole).toHaveBeenCalledTimes(1);
-      expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(1, 'editor', 'new-name');
+      expect(deps.updateUsersByRole).toHaveBeenCalledWith('editor', 'new-name');
-      expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor');
+      expect(deps.updateUsersRoleByIds).toHaveBeenCalledWith(ids, 'editor');
     });
 
     it('rolls back user migration when rename throws', async () => {
+      const ids = ['uid-1', 'uid-2'];
       const deps = createDeps({
         getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null),
+        findUserIdsByRole: jest.fn().mockResolvedValue(ids),
         updateRoleByName: jest.fn().mockRejectedValue(new Error('db crash')),
       });
       const handlers = createAdminRolesHandlers(deps);
@@ -688,18 +699,16 @@ describe('createAdminRolesHandlers', () => {
       await handlers.updateRole(req, res);
 
       expect(status).toHaveBeenCalledWith(500);
-      expect(deps.updateUsersByRole).toHaveBeenCalledTimes(2);
+      expect(deps.updateUsersByRole).toHaveBeenCalledTimes(1);
-      expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(1, 'editor', 'new-name');
+      expect(deps.updateUsersByRole).toHaveBeenCalledWith('editor', 'new-name');
-      expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor');
+      expect(deps.updateUsersRoleByIds).toHaveBeenCalledWith(ids, 'editor');
     });
 
     it('logs rollback failure and still returns 500', async () => {
       const deps = createDeps({
         getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null),
-        updateUsersByRole: jest
+        findUserIdsByRole: jest.fn().mockResolvedValue(['uid-1']),
-          .fn()
+        updateUsersRoleByIds: jest.fn().mockRejectedValue(new Error('rollback failed')),
-          .mockResolvedValueOnce(undefined)
-          .mockRejectedValueOnce(new Error('rollback failed')),
         updateRoleByName: jest.fn().mockRejectedValue(new Error('rename failed')),
       });
       const handlers = createAdminRolesHandlers(deps);
@@ -711,7 +720,8 @@ describe('createAdminRolesHandlers', () => {
       await handlers.updateRole(req, res);
 
       expect(status).toHaveBeenCalledWith(500);
-      expect(deps.updateUsersByRole).toHaveBeenCalledTimes(2);
+      expect(deps.updateUsersByRole).toHaveBeenCalledTimes(1);
+      expect(deps.updateUsersRoleByIds).toHaveBeenCalledTimes(1);
     });
 
     it('returns 400 when description exceeds max length', async () => {

packages/api/src/admin/roles.ts

@@ -85,6 +85,8 @@ export interface AdminRolesDeps {
   ) => Promise<IUser | null>;
   updateUser: (userId: string, data: Partial<IUser>) => Promise<IUser | null>;
   updateUsersByRole: (oldRole: string, newRole: string) => Promise<void>;
+  findUserIdsByRole: (roleName: string) => Promise<string[]>;
+  updateUsersRoleByIds: (userIds: string[], newRole: string) => Promise<void>;
   listUsersByRole: (
     roleName: string,
     options?: { limit?: number; offset?: number },
@@ -104,6 +106,8 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
     findUser,
     updateUser,
     updateUsersByRole,
+    findUserIdsByRole,
+    updateUsersRoleByIds,
     listUsersByRole,
     countUsersByRole,
   } = deps;
@@ -176,35 +180,40 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
     }
   }
 
+  async function rollbackMigratedUsers(
+    migratedIds: string[],
+    currentName: string,
+    newName: string,
+  ): Promise<void> {
+    if (migratedIds.length === 0) {
+      return;
+    }
+    try {
+      await updateUsersRoleByIds(migratedIds, currentName);
+    } catch (rollbackError) {
+      logger.error(
+        `[adminRoles] CRITICAL: rename rollback failed — ${migratedIds.length} users have dangling role "${newName}":`,
+        rollbackError,
+      );
+    }
+  }
+
   async function renameRole(
     currentName: string,
     newName: string,
     extraUpdates?: Partial<IRole>,
   ): Promise<IRole | null> {
+    const migratedIds = await findUserIdsByRole(currentName);
     await updateUsersByRole(currentName, newName);
     try {
       const updates: Partial<IRole> = { name: newName, ...extraUpdates };
       const role = await updateRoleByName(currentName, updates);
       if (!role) {
-        try {
+        await rollbackMigratedUsers(migratedIds, currentName, newName);
-          await updateUsersByRole(newName, currentName);
-        } catch (rollbackError) {
-          logger.error(
-            `[adminRoles] CRITICAL: rename rollback failed — users have dangling role "${newName}":`,
-            rollbackError,
-          );
-        }
       }
       return role;
     } catch (error) {
-      try {
+      await rollbackMigratedUsers(migratedIds, currentName, newName);
-        await updateUsersByRole(newName, currentName);
-      } catch (rollbackError) {
-        logger.error(
-          `[adminRoles] CRITICAL: rename rollback failed — users have dangling role "${newName}":`,
-          rollbackError,
-        );
-      }
       throw error;
     }
   }

packages/data-schemas/src/methods/role.ts

@@ -452,6 +452,20 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
     await User.updateMany({ role: oldRole }, { $set: { role: newRole } });
   }
 
+  async function findUserIdsByRole(roleName: string): Promise<string[]> {
+    const User = mongoose.models.User as Model<IUser>;
+    const users = await User.find({ role: roleName }).select('_id').lean();
+    return users.map((u) => u._id.toString());
+  }
+
+  async function updateUsersRoleByIds(userIds: string[], newRole: string): Promise<void> {
+    if (userIds.length === 0) {
+      return;
+    }
+    const User = mongoose.models.User as Model<IUser>;
+    await User.updateMany({ _id: { $in: userIds } }, { $set: { role: newRole } });
+  }
+
   async function listUsersByRole(
     roleName: string,
     options?: { limit?: number; offset?: number },
@@ -483,6 +497,8 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
     createRoleByName,
     deleteRoleByName,
     updateUsersByRole,
+    findUserIdsByRole,
+    updateUsersRoleByIds,
     listUsersByRole,
     countUsersByRole,
   };