From c89b5db55c1151d19cc18f3cad9002b5c1a058eb Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Fri, 27 Mar 2026 08:50:16 -0700 Subject: [PATCH] fix: scope rename rollback to only migrated users, prevent cross-role corruption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 34 ++++++++++++------- packages/api/src/admin/roles.ts | 41 ++++++++++++++--------- packages/data-schemas/src/methods/role.ts | 16 +++++++++ 3 files changed, 63 insertions(+), 28 deletions(-) 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, };