From ce526ed51a99f89b30e8163b3c7f6f85df390079 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 26 Mar 2026 16:25:14 -0700 Subject: [PATCH] fix: rollback on rename throw, description validation, delete/DRY cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Hoist isRename/trimmedName above try block so catch can roll back user migration when updateRoleByName throws (not just returns null) - Add description type + max-length (2000) validation in create and update, consistent with groups handler - Remove redundant getRoleByName existence check in deleteRoleHandler — use deleteRoleByName return value directly - Skip no-op name write when body.name equals current name (use isRename) - Extract getUserModel() accessor to DRY repeated Model casts - Use name.trim() consistently in createRoleByName error messages - Add tests: rename-throw rollback, description validation (create+update), update delete test mocks to match simplified handler --- packages/api/src/admin/roles.spec.ts | 59 +++++++++++++++++++++-- packages/api/src/admin/roles.ts | 44 +++++++++++++---- packages/data-schemas/src/methods/role.ts | 17 +++---- 3 files changed, 98 insertions(+), 22 deletions(-) diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index 20675f8ac6..6ca7d53e1a 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -191,6 +191,22 @@ describe('createAdminRolesHandlers', () => { expect(deps.createRoleByName).not.toHaveBeenCalled(); }); + it('returns 400 when description exceeds max length', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + body: { name: 'editor', description: 'a'.repeat(2001) }, + }); + + await handlers.createRole(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ + error: 'description must not exceed 2000 characters', + }); + expect(deps.createRoleByName).not.toHaveBeenCalled(); + }); + it('returns 409 when role already exists', async () => { const deps = createDeps({ createRoleByName: jest.fn().mockRejectedValue(new Error('Role "editor" already exists')), @@ -487,6 +503,44 @@ describe('createAdminRolesHandlers', () => { expect(deps.updateUsersByRole).toHaveBeenNthCalledWith(2, 'new-name', 'editor'); }); + it('rolls back user migration when rename throws', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValueOnce(mockRole()).mockResolvedValueOnce(null), + updateRoleByName: jest.fn().mockRejectedValue(new Error('db crash')), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status } = createReqRes({ + params: { name: 'editor' }, + body: { name: 'new-name' }, + }); + + 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'); + }); + + it('returns 400 when description exceeds max length', async () => { + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(mockRole()), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { description: 'a'.repeat(2001) }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ + error: 'description must not exceed 2000 characters', + }); + expect(deps.updateRoleByName).not.toHaveBeenCalled(); + }); + it('returns 500 on unexpected error', async () => { const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()), @@ -586,7 +640,7 @@ describe('createAdminRolesHandlers', () => { describe('deleteRole', () => { it('deletes role and returns 200', async () => { - const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(mockRole()) }); + const deps = createDeps(); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ params: { name: 'editor' } }); @@ -610,7 +664,7 @@ describe('createAdminRolesHandlers', () => { }); it('returns 404 when role not found', async () => { - const deps = createDeps({ getRoleByName: jest.fn().mockResolvedValue(null) }); + const deps = createDeps({ deleteRoleByName: jest.fn().mockResolvedValue(null) }); const handlers = createAdminRolesHandlers(deps); const { req, res, status, json } = createReqRes({ params: { name: 'nonexistent' } }); @@ -622,7 +676,6 @@ describe('createAdminRolesHandlers', () => { it('returns 500 on error', async () => { const deps = createDeps({ - getRoleByName: jest.fn().mockResolvedValue(mockRole()), deleteRoleByName: jest.fn().mockRejectedValue(new Error('db down')), }); const handlers = createAdminRolesHandlers(deps); diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index 7b22dde5e0..53c4abc6ee 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -6,6 +6,7 @@ import type { Response } from 'express'; import type { ServerRequest } from '~/types/http'; const MAX_NAME_LENGTH = 500; +const MAX_DESCRIPTION_LENGTH = 2000; interface RoleNameParams { name: string; @@ -93,6 +94,14 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { .status(400) .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` }); } + if (description !== undefined && typeof description !== 'string') { + return res.status(400).json({ error: 'description must be a string' }); + } + if (description && description.length > MAX_DESCRIPTION_LENGTH) { + return res + .status(400) + .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` }); + } const role = await createRoleByName({ name: name.trim(), description: description ?? '', @@ -111,10 +120,11 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { } async function updateRoleHandler(req: ServerRequest, res: Response) { + const { name } = req.params as RoleNameParams; + const body = req.body as Partial; + let isRename = false; + let trimmedName = ''; try { - const { name } = req.params as RoleNameParams; - const body = req.body as Partial; - if ( body.name !== undefined && (!body.name || typeof body.name !== 'string' || !body.name.trim()) @@ -127,8 +137,8 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` }); } - const trimmedName = body.name?.trim(); - const isRename = trimmedName !== undefined && trimmedName !== name; + trimmedName = body.name?.trim() ?? ''; + isRename = trimmedName !== '' && trimmedName !== name; if (isRename && SystemRoles[name as keyof typeof SystemRoles]) { return res.status(403).json({ error: 'Cannot rename system role' }); @@ -149,8 +159,17 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { } } + if (body.description !== undefined && typeof body.description !== 'string') { + return res.status(400).json({ error: 'description must be a string' }); + } + if (body.description && body.description.length > MAX_DESCRIPTION_LENGTH) { + return res + .status(400) + .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` }); + } + const updates: Partial = {}; - if (trimmedName !== undefined) { + if (isRename) { updates.name = trimmedName; } if (body.description !== undefined) { @@ -171,6 +190,13 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(200).json({ role }); } catch (error) { + if (isRename) { + try { + await updateUsersByRole(trimmedName, name); + } catch (rollbackError) { + logger.error('[adminRoles] rollback failed after updateRole error:', rollbackError); + } + } logger.error('[adminRoles] updateRole error:', error); return res.status(500).json({ error: 'Failed to update role' }); } @@ -208,12 +234,10 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(403).json({ error: 'Cannot delete system role' }); } - const existing = await getRoleByName(name); - if (!existing) { + const deleted = await deleteRoleByName(name); + if (!deleted) { return res.status(404).json({ error: 'Role not found' }); } - - await deleteRoleByName(name); return res.status(200).json({ success: true }); } catch (error) { logger.error('[adminRoles] deleteRole error:', error); diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index b1aed411da..1d5b285fb9 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -355,7 +355,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol const Role = mongoose.models.Role; const existing = await Role.findOne({ name: name.trim() }).lean(); if (existing) { - throw new Error(`Role "${name}" already exists`); + throw new Error(`Role "${name.trim()}" already exists`); } let role; try { @@ -376,6 +376,8 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol return role.toObject() as IRole; } + const getUserModel = () => mongoose.models.User as Model; + /** Guards against deleting system roles. Reassigns affected users back to USER. */ async function deleteRoleByName(roleName: string): Promise { if (SystemRoles[roleName as keyof typeof SystemRoles]) { @@ -386,8 +388,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol if (!exists) { return null; } - const User = mongoose.models.User as Model; - await User.updateMany({ role: roleName }, { $set: { role: SystemRoles.USER } }); + await getUserModel().updateMany({ role: roleName }, { $set: { role: SystemRoles.USER } }); const deleted = await Role.findOneAndDelete({ name: roleName }).lean(); const cache = deps.getCache?.(CacheKeys.ROLES); if (cache) { @@ -397,18 +398,17 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol } async function updateUsersByRole(oldRole: string, newRole: string): Promise { - const User = mongoose.models.User as Model; - await User.updateMany({ role: oldRole }, { $set: { role: newRole } }); + await getUserModel().updateMany({ role: oldRole }, { $set: { role: newRole } }); } async function listUsersByRole( roleName: string, options?: { limit?: number; offset?: number }, ): Promise { - const User = mongoose.models.User as Model; const limit = options?.limit ?? 50; const offset = options?.offset ?? 0; - return await User.find({ role: roleName }) + return await getUserModel() + .find({ role: roleName }) .select('_id name email avatar') .sort({ _id: 1 }) .skip(offset) @@ -417,8 +417,7 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol } async function countUsersByRole(roleName: string): Promise { - const User = mongoose.models.User as Model; - return User.countDocuments({ role: roleName }); + return getUserModel().countDocuments({ role: roleName }); } return {