From 16678c0eced831bfc1f2bc8cb8e33b7684384783 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Thu, 26 Mar 2026 16:38:11 -0700 Subject: [PATCH] fix: guard spurious rollback, harden createRole error path, validate before DB calls - Add migrationRan flag to prevent rollback of user migration that never ran - Return generic message on 500 in createRoleHandler, specific only for 409 - Move description validation before DB queries in updateRoleHandler - Return existing role early when update body has no changes - Wrap cache.set in createRoleByName with try/catch to prevent masking DB success - Add JSDoc on 11000 catch explaining compound unique index - Add tests: spurious rollback guard, empty update body, description validation ordering, listUsersByRole pagination --- packages/api/src/admin/roles.spec.ts | 54 ++++++++++++++++++- packages/api/src/admin/roles.ts | 29 +++++----- .../src/methods/role.methods.spec.ts | 21 ++++++++ packages/data-schemas/src/methods/role.ts | 16 ++++-- 4 files changed, 104 insertions(+), 16 deletions(-) diff --git a/packages/api/src/admin/roles.spec.ts b/packages/api/src/admin/roles.spec.ts index 6ca7d53e1a..519c1c528e 100644 --- a/packages/api/src/admin/roles.spec.ts +++ b/packages/api/src/admin/roles.spec.ts @@ -247,7 +247,7 @@ describe('createAdminRolesHandlers', () => { await handlers.createRole(req, res); expect(status).toHaveBeenCalledWith(500); - expect(json).toHaveBeenCalledWith({ error: 'db crash' }); + expect(json).toHaveBeenCalledWith({ error: 'Failed to create role' }); }); it('does not classify unrelated errors as 409', async () => { @@ -557,6 +557,58 @@ describe('createAdminRolesHandlers', () => { expect(status).toHaveBeenCalledWith(500); expect(json).toHaveBeenCalledWith({ error: 'Failed to update role' }); }); + + it('does not roll back when error occurs before user migration', async () => { + const deps = createDeps({ + getRoleByName: jest + .fn() + .mockResolvedValueOnce(mockRole()) + .mockRejectedValueOnce(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).not.toHaveBeenCalled(); + }); + + it('returns existing role early when update body has no changes', async () => { + const role = mockRole(); + const deps = createDeps({ + getRoleByName: jest.fn().mockResolvedValue(role), + }); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: {}, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(200); + expect(json).toHaveBeenCalledWith({ role }); + expect(deps.updateRoleByName).not.toHaveBeenCalled(); + }); + + it('rejects invalid description before making DB calls', async () => { + const deps = createDeps(); + const handlers = createAdminRolesHandlers(deps); + const { req, res, status, json } = createReqRes({ + params: { name: 'editor' }, + body: { description: 123 }, + }); + + await handlers.updateRole(req, res); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ error: 'description must be a string' }); + expect(deps.getRoleByName).not.toHaveBeenCalled(); + }); }); describe('updateRolePermissions', () => { diff --git a/packages/api/src/admin/roles.ts b/packages/api/src/admin/roles.ts index 53c4abc6ee..834b948fc8 100644 --- a/packages/api/src/admin/roles.ts +++ b/packages/api/src/admin/roles.ts @@ -109,12 +109,12 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { }); return res.status(201).json({ role }); } catch (error) { - const message = error instanceof Error ? error.message : 'Failed to create role'; logger.error('[adminRoles] createRole error:', error); const is409 = error instanceof Error && (error.message.startsWith('Role "') || error.message.startsWith('Cannot create role with reserved')); + const message = is409 && error instanceof Error ? error.message : 'Failed to create role'; return res.status(is409 ? 409 : 500).json({ error: message }); } } @@ -124,6 +124,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { const body = req.body as Partial; let isRename = false; let trimmedName = ''; + let migrationRan = false; try { if ( body.name !== undefined && @@ -136,6 +137,14 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { .status(400) .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` }); } + 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` }); + } trimmedName = body.name?.trim() ?? ''; isRename = trimmedName !== '' && trimmedName !== name; @@ -159,15 +168,6 @@ 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 (isRename) { updates.name = trimmedName; @@ -176,13 +176,18 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { updates.description = body.description; } + if (Object.keys(updates).length === 0) { + return res.status(200).json({ role: existing }); + } + if (isRename) { await updateUsersByRole(name, trimmedName); + migrationRan = true; } const role = await updateRoleByName(name, updates); if (!role) { - if (isRename) { + if (migrationRan) { await updateUsersByRole(trimmedName, name); } return res.status(404).json({ error: 'Role not found' }); @@ -190,7 +195,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) { return res.status(200).json({ role }); } catch (error) { - if (isRename) { + if (migrationRan) { try { await updateUsersByRole(trimmedName, name); } catch (rollbackError) { diff --git a/packages/data-schemas/src/methods/role.methods.spec.ts b/packages/data-schemas/src/methods/role.methods.spec.ts index 605c25abc9..1c19165f6c 100644 --- a/packages/data-schemas/src/methods/role.methods.spec.ts +++ b/packages/data-schemas/src/methods/role.methods.spec.ts @@ -655,6 +655,27 @@ describe('listUsersByRole', () => { expect(users).toEqual([]); }); + it('respects limit and offset for pagination', async () => { + await User.create([ + { name: 'Alice', email: 'a@test.com', role: 'editor', username: 'a' }, + { name: 'Bob', email: 'b@test.com', role: 'editor', username: 'b' }, + { name: 'Carol', email: 'c@test.com', role: 'editor', username: 'c' }, + { name: 'Dave', email: 'd@test.com', role: 'editor', username: 'd' }, + { name: 'Eve', email: 'e@test.com', role: 'editor', username: 'e' }, + ]); + + const page1 = await listUsersByRole('editor', { limit: 2, offset: 0 }); + const page2 = await listUsersByRole('editor', { limit: 2, offset: 2 }); + const page3 = await listUsersByRole('editor', { limit: 2, offset: 4 }); + + expect(page1).toHaveLength(2); + expect(page2).toHaveLength(2); + expect(page3).toHaveLength(1); + + const allIds = [...page1, ...page2, ...page3].map((u) => u._id!.toString()); + expect(new Set(allIds).size).toBe(5); + }); + it('selects only expected fields', async () => { await User.create({ name: 'Alice', diff --git a/packages/data-schemas/src/methods/role.ts b/packages/data-schemas/src/methods/role.ts index 1d5b285fb9..2ca7e745ae 100644 --- a/packages/data-schemas/src/methods/role.ts +++ b/packages/data-schemas/src/methods/role.ts @@ -364,14 +364,24 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol name: name.trim(), }).save(); } catch (err) { + /** + * The compound unique index `{ name: 1, tenantId: 1 }` on the role schema + * (roleSchema.index in schema/role.ts) triggers error 11000 when a concurrent + * request races past the findOne check above. This catch converts it into + * the same user-facing message as the application-level duplicate check. + */ if (err && typeof err === 'object' && 'code' in err && err.code === 11000) { throw new Error(`Role "${name.trim()}" already exists`); } throw err; } - const cache = deps.getCache?.(CacheKeys.ROLES); - if (cache) { - await cache.set(role.name, role.toObject()); + try { + const cache = deps.getCache?.(CacheKeys.ROLES); + if (cache) { + await cache.set(role.name, role.toObject()); + } + } catch (cacheError) { + logger.error(`[createRoleByName] cache set failed for "${role.name}":`, cacheError); } return role.toObject() as IRole; }