diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index 5b5f6869d2..1c6c4dc375 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -67,6 +67,8 @@ function createDeps(overrides: Partial = {}): 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).toHaveBeenNthCalledWith(1, 'editor', 'new-name'); - expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor'); + expect(deps.updateUsersByRole).toHaveBeenCalledTimes(1); + expect(deps.updateUsersByRole).toHaveBeenCalledWith('editor', 'new-name'); + 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).toHaveBeenNthCalledWith(1, 'editor', 'new-name'); - expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor'); + expect(deps.updateUsersByRole).toHaveBeenCalledTimes(1); + expect(deps.updateUsersByRole).toHaveBeenCalledWith('editor', 'new-name'); + 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 - .fn() - .mockResolvedValueOnce(undefined) - .mockRejectedValueOnce(new Error('rollback failed')), + findUserIdsByRole: jest.fn().mockResolvedValue(['uid-1']), + updateUsersRoleByIds: jest.fn().mockRejectedValue(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 () => { diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index f26b910d90..6920e4d583 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -85,6 +85,8 @@ export interface AdminRolesDeps { ) => Promise; updateUser: (userId: string, data: Partial) => Promise; updateUsersByRole: (oldRole: string, newRole: string) => Promise; + findUserIdsByRole: (roleName: string) => Promise; + updateUsersRoleByIds: (userIds: string[], newRole: string) => Promise; 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 { + 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, ): Promise { + const migratedIds = await findUserIdsByRole(currentName); await updateUsersByRole(currentName, newName); try { const updates: Partial = { name: newName, ...extraUpdates }; const role = await updateRoleByName(currentName, updates); if (!role) { - try { - await updateUsersByRole(newName, currentName); - } catch (rollbackError) { - logger.error( - `[adminRoles] CRITICAL: rename rollback failed — users have dangling role "${newName}":`, - rollbackError, - ); - } + await rollbackMigratedUsers(migratedIds, currentName, newName); } return role; } catch (error) { - try { - await updateUsersByRole(newName, currentName); - } catch (rollbackError) { - logger.error( - `[adminRoles] CRITICAL: rename rollback failed — users have dangling role "${newName}":`, - rollbackError, - ); - } + await rollbackMigratedUsers(migratedIds, currentName, newName); throw error; } } diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index 08fb92606e..f7558392a1 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/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 { + const User = mongoose.models.User as Model; + 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 { + if (userIds.length === 0) { + return; + } + const User = mongoose.models.User as Model; + 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, };