From 38a170266f40303a0a0ab0ad20b2447c12f9cd70 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Sat, 14 Mar 2026 06:14:19 +0000 Subject: [PATCH 1/2] feat(api): add users tRPC router with CRUD operations and admin access control --- src/api/router.ts | 2 + src/api/routers/users.ts | 126 ++++++++++ tests/unit/api/routers/users.test.ts | 340 +++++++++++++++++++++++++++ 3 files changed, 468 insertions(+) create mode 100644 src/api/routers/users.ts create mode 100644 tests/unit/api/routers/users.test.ts diff --git a/src/api/router.ts b/src/api/router.ts index 58f022d3..c35d1467 100644 --- a/src/api/router.ts +++ b/src/api/router.ts @@ -10,6 +10,7 @@ import { projectsRouter } from './routers/projects.js'; import { promptsRouter } from './routers/prompts.js'; import { prsRouter } from './routers/prs.js'; import { runsRouter } from './routers/runs.js'; +import { usersRouter } from './routers/users.js'; import { webhookLogsRouter } from './routers/webhookLogs.js'; import { webhooksRouter } from './routers/webhooks.js'; import { workItemsRouter } from './routers/workItems.js'; @@ -31,6 +32,7 @@ export const appRouter = router({ integrationsDiscovery: integrationsDiscoveryRouter, prs: prsRouter, workItems: workItemsRouter, + users: usersRouter, }); export type AppRouter = typeof appRouter; diff --git a/src/api/routers/users.ts b/src/api/routers/users.ts new file mode 100644 index 00000000..a4a3cd84 --- /dev/null +++ b/src/api/routers/users.ts @@ -0,0 +1,126 @@ +import { TRPCError } from '@trpc/server'; +import bcrypt from 'bcrypt'; +import { z } from 'zod'; +import { + createUser, + deleteUser, + getUserById, + listOrgUsers, + updateUser, +} from '../../db/repositories/usersRepository.js'; +import { adminProcedure, router } from '../trpc.js'; + +export const usersRouter = router({ + list: adminProcedure.query(async ({ ctx }) => { + return listOrgUsers(ctx.effectiveOrgId); + }), + + create: adminProcedure + .input( + z.object({ + email: z.string().email(), + name: z.string().min(1), + password: z.string().min(1), + role: z.enum(['member', 'admin', 'superadmin']).optional(), + }), + ) + .mutation(async ({ ctx, input }) => { + const role = input.role ?? 'member'; + + // Only superadmins can create users with superadmin role + if (role === 'superadmin' && ctx.user.role !== 'superadmin') { + throw new TRPCError({ + code: 'FORBIDDEN', + message: 'Only superadmins can create superadmin users', + }); + } + + const passwordHash = await bcrypt.hash(input.password, 10); + + return createUser({ + orgId: ctx.effectiveOrgId, + email: input.email, + name: input.name, + passwordHash, + role, + }); + }), + + update: adminProcedure + .input( + z.object({ + id: z.string(), + name: z.string().min(1).optional(), + email: z.string().email().optional(), + role: z.enum(['member', 'admin', 'superadmin']).optional(), + password: z.string().min(1).optional(), + }), + ) + .mutation(async ({ ctx, input }) => { + // Verify target user belongs to same org + const targetUser = await getUserById(input.id); + + if (!targetUser) { + throw new TRPCError({ code: 'NOT_FOUND' }); + } + + if (targetUser.orgId !== ctx.effectiveOrgId && ctx.user.role !== 'superadmin') { + throw new TRPCError({ code: 'NOT_FOUND' }); + } + + // Prevent self-demotion (can't change own role) + if (input.role !== undefined && ctx.user.id === input.id) { + throw new TRPCError({ + code: 'FORBIDDEN', + message: 'Cannot change your own role', + }); + } + + // Only superadmins can set role to superadmin + if (input.role === 'superadmin' && ctx.user.role !== 'superadmin') { + throw new TRPCError({ + code: 'FORBIDDEN', + message: 'Only superadmins can assign superadmin role', + }); + } + + const updates: { + name?: string; + email?: string; + role?: string; + passwordHash?: string; + } = {}; + + if (input.name !== undefined) updates.name = input.name; + if (input.email !== undefined) updates.email = input.email; + if (input.role !== undefined) updates.role = input.role; + if (input.password !== undefined) { + updates.passwordHash = await bcrypt.hash(input.password, 10); + } + + await updateUser(input.id, updates); + }), + + delete: adminProcedure.input(z.object({ id: z.string() })).mutation(async ({ ctx, input }) => { + // Prevent self-deletion + if (ctx.user.id === input.id) { + throw new TRPCError({ + code: 'FORBIDDEN', + message: 'Cannot delete your own account', + }); + } + + // Verify org ownership + const targetUser = await getUserById(input.id); + + if (!targetUser) { + throw new TRPCError({ code: 'NOT_FOUND' }); + } + + if (targetUser.orgId !== ctx.effectiveOrgId && ctx.user.role !== 'superadmin') { + throw new TRPCError({ code: 'NOT_FOUND' }); + } + + await deleteUser(input.id); + }), +}); diff --git a/tests/unit/api/routers/users.test.ts b/tests/unit/api/routers/users.test.ts new file mode 100644 index 00000000..191cc7ad --- /dev/null +++ b/tests/unit/api/routers/users.test.ts @@ -0,0 +1,340 @@ +import { TRPCError } from '@trpc/server'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { TRPCContext } from '../../../../src/api/trpc.js'; +import { createMockSuperAdmin, createMockUser } from '../../../helpers/factories.js'; + +const mockListOrgUsers = vi.fn(); +const mockCreateUser = vi.fn(); +const mockUpdateUser = vi.fn(); +const mockDeleteUser = vi.fn(); +const mockGetUserById = vi.fn(); + +vi.mock('../../../../src/db/repositories/usersRepository.js', () => ({ + listOrgUsers: (...args: unknown[]) => mockListOrgUsers(...args), + createUser: (...args: unknown[]) => mockCreateUser(...args), + updateUser: (...args: unknown[]) => mockUpdateUser(...args), + deleteUser: (...args: unknown[]) => mockDeleteUser(...args), + getUserById: (...args: unknown[]) => mockGetUserById(...args), +})); + +const mockBcryptHash = vi.fn(); + +vi.mock('bcrypt', () => ({ + default: { + hash: (...args: unknown[]) => mockBcryptHash(...args), + }, +})); + +import { usersRouter } from '../../../../src/api/routers/users.js'; + +function createCaller(ctx: TRPCContext) { + return usersRouter.createCaller(ctx); +} + +const mockAdminUser = createMockUser({ role: 'admin' }); +const mockSuperAdmin = createMockSuperAdmin(); +const mockMember = createMockUser({ id: 'member-1', role: 'member' }); + +describe('usersRouter', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockBcryptHash.mockResolvedValue('hashed-password'); + }); + + describe('list', () => { + it('returns org-scoped user list without passwordHash', async () => { + const orgUsers = [ + { + id: 'user-1', + orgId: 'org-1', + email: 'alice@example.com', + name: 'Alice', + role: 'admin', + createdAt: null, + updatedAt: null, + }, + { + id: 'user-2', + orgId: 'org-1', + email: 'bob@example.com', + name: 'Bob', + role: 'member', + createdAt: null, + updatedAt: null, + }, + ]; + mockListOrgUsers.mockResolvedValue(orgUsers); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + const result = await caller.list(); + + expect(mockListOrgUsers).toHaveBeenCalledWith('org-1'); + expect(result).toEqual(orgUsers); + // Ensure no passwordHash in result + for (const user of result) { + expect(user).not.toHaveProperty('passwordHash'); + } + }); + + it('returns empty array when no users', async () => { + mockListOrgUsers.mockResolvedValue([]); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + const result = await caller.list(); + expect(result).toEqual([]); + }); + + it('throws UNAUTHORIZED when not authenticated', async () => { + const caller = createCaller({ user: null, effectiveOrgId: null }); + await expect(caller.list()).rejects.toMatchObject({ code: 'UNAUTHORIZED' }); + }); + + it('throws FORBIDDEN when user is a member', async () => { + const caller = createCaller({ user: mockMember, effectiveOrgId: mockMember.orgId }); + await expect(caller.list()).rejects.toMatchObject({ code: 'FORBIDDEN' }); + }); + }); + + describe('create', () => { + it('creates user with hashed password', async () => { + mockCreateUser.mockResolvedValue({ id: 'new-user-1' }); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + const result = await caller.create({ + email: 'newuser@example.com', + name: 'New User', + password: 'secret123', + }); + + expect(mockBcryptHash).toHaveBeenCalledWith('secret123', 10); + expect(mockCreateUser).toHaveBeenCalledWith({ + orgId: 'org-1', + email: 'newuser@example.com', + name: 'New User', + passwordHash: 'hashed-password', + role: 'member', + }); + expect(result).toEqual({ id: 'new-user-1' }); + }); + + it('creates admin user when role is specified', async () => { + mockCreateUser.mockResolvedValue({ id: 'new-admin-1' }); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await caller.create({ + email: 'newadmin@example.com', + name: 'New Admin', + password: 'secret123', + role: 'admin', + }); + + expect(mockCreateUser).toHaveBeenCalledWith(expect.objectContaining({ role: 'admin' })); + }); + + it('rejects superadmin role assignment when caller is not superadmin (FORBIDDEN)', async () => { + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect( + caller.create({ + email: 'superuser@example.com', + name: 'Super User', + password: 'secret123', + role: 'superadmin', + }), + ).rejects.toMatchObject({ code: 'FORBIDDEN' }); + + expect(mockCreateUser).not.toHaveBeenCalled(); + }); + + it('allows superadmin to create superadmin users', async () => { + mockCreateUser.mockResolvedValue({ id: 'new-super-1' }); + const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); + + await caller.create({ + email: 'super2@example.com', + name: 'Super 2', + password: 'secret123', + role: 'superadmin', + }); + + expect(mockCreateUser).toHaveBeenCalledWith(expect.objectContaining({ role: 'superadmin' })); + }); + + it('throws UNAUTHORIZED when not authenticated', async () => { + const caller = createCaller({ user: null, effectiveOrgId: null }); + await expect( + caller.create({ email: 'x@x.com', name: 'X', password: 'x' }), + ).rejects.toMatchObject({ code: 'UNAUTHORIZED' }); + }); + + it('throws FORBIDDEN when user is a member', async () => { + const caller = createCaller({ user: mockMember, effectiveOrgId: mockMember.orgId }); + await expect( + caller.create({ email: 'x@x.com', name: 'X', password: 'x' }), + ).rejects.toMatchObject({ code: 'FORBIDDEN' }); + }); + }); + + describe('update', () => { + it('allows sparse update for name', async () => { + mockGetUserById.mockResolvedValue({ + id: 'user-2', + orgId: 'org-1', + role: 'member', + }); + mockUpdateUser.mockResolvedValue(undefined); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await caller.update({ id: 'user-2', name: 'Updated Name' }); + + expect(mockUpdateUser).toHaveBeenCalledWith('user-2', { name: 'Updated Name' }); + }); + + it('allows sparse update for email', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-2', orgId: 'org-1', role: 'member' }); + mockUpdateUser.mockResolvedValue(undefined); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await caller.update({ id: 'user-2', email: 'updated@example.com' }); + + expect(mockUpdateUser).toHaveBeenCalledWith('user-2', { email: 'updated@example.com' }); + }); + + it('hashes password when provided', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-2', orgId: 'org-1', role: 'member' }); + mockUpdateUser.mockResolvedValue(undefined); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await caller.update({ id: 'user-2', password: 'newpassword' }); + + expect(mockBcryptHash).toHaveBeenCalledWith('newpassword', 10); + expect(mockUpdateUser).toHaveBeenCalledWith('user-2', { passwordHash: 'hashed-password' }); + }); + + it('prevents self-demotion (cannot change own role)', async () => { + mockGetUserById.mockResolvedValue({ + id: 'user-1', + orgId: 'org-1', + role: 'admin', + }); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.update({ id: 'user-1', role: 'member' })).rejects.toMatchObject({ + code: 'FORBIDDEN', + }); + + expect(mockUpdateUser).not.toHaveBeenCalled(); + }); + + it('throws NOT_FOUND when user does not exist', async () => { + mockGetUserById.mockResolvedValue(null); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.update({ id: 'nonexistent', name: 'X' })).rejects.toMatchObject({ + code: 'NOT_FOUND', + }); + }); + + it('throws NOT_FOUND when user belongs to different org', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-other', orgId: 'other-org', role: 'member' }); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.update({ id: 'user-other', name: 'X' })).rejects.toMatchObject({ + code: 'NOT_FOUND', + }); + + expect(mockUpdateUser).not.toHaveBeenCalled(); + }); + + it('prevents non-superadmin from assigning superadmin role', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-2', orgId: 'org-1', role: 'member' }); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.update({ id: 'user-2', role: 'superadmin' })).rejects.toMatchObject({ + code: 'FORBIDDEN', + }); + + expect(mockUpdateUser).not.toHaveBeenCalled(); + }); + + it('allows superadmin to assign superadmin role', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-2', orgId: 'org-1', role: 'member' }); + mockUpdateUser.mockResolvedValue(undefined); + const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); + + await caller.update({ id: 'user-2', role: 'superadmin' }); + + expect(mockUpdateUser).toHaveBeenCalledWith('user-2', { role: 'superadmin' }); + }); + + it('throws UNAUTHORIZED when not authenticated', async () => { + const caller = createCaller({ user: null, effectiveOrgId: null }); + await expect(caller.update({ id: 'user-2', name: 'X' })).rejects.toMatchObject({ + code: 'UNAUTHORIZED', + }); + }); + + it('throws FORBIDDEN when user is a member', async () => { + const caller = createCaller({ user: mockMember, effectiveOrgId: mockMember.orgId }); + await expect(caller.update({ id: 'user-2', name: 'X' })).rejects.toMatchObject({ + code: 'FORBIDDEN', + }); + }); + }); + + describe('delete', () => { + it('deletes user after verifying org ownership', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-2', orgId: 'org-1', role: 'member' }); + mockDeleteUser.mockResolvedValue(undefined); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await caller.delete({ id: 'user-2' }); + + expect(mockDeleteUser).toHaveBeenCalledWith('user-2'); + }); + + it('prevents self-deletion', async () => { + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.delete({ id: 'user-1' })).rejects.toMatchObject({ + code: 'FORBIDDEN', + }); + + expect(mockDeleteUser).not.toHaveBeenCalled(); + }); + + it('throws NOT_FOUND when user does not exist', async () => { + mockGetUserById.mockResolvedValue(null); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.delete({ id: 'nonexistent' })).rejects.toMatchObject({ + code: 'NOT_FOUND', + }); + }); + + it('throws NOT_FOUND when user belongs to different org', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-other', orgId: 'other-org', role: 'member' }); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.delete({ id: 'user-other' })).rejects.toMatchObject({ + code: 'NOT_FOUND', + }); + + expect(mockDeleteUser).not.toHaveBeenCalled(); + }); + + it('throws UNAUTHORIZED when not authenticated', async () => { + const caller = createCaller({ user: null, effectiveOrgId: null }); + await expect(caller.delete({ id: 'user-2' })).rejects.toMatchObject({ + code: 'UNAUTHORIZED', + }); + }); + + it('throws FORBIDDEN when user is a member', async () => { + const caller = createCaller({ user: mockMember, effectiveOrgId: mockMember.orgId }); + await expect(caller.delete({ id: 'user-2' })).rejects.toMatchObject({ + code: 'FORBIDDEN', + }); + }); + }); +}); From a5777b3f3b81a047c047c9fb587a747d54e3fd0b Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Sat, 14 Mar 2026 06:26:38 +0000 Subject: [PATCH 2/2] fix(users): enforce superadmin privilege hierarchy for update and delete - Prevent regular admins from revoking superadmin role (update procedure) - Prevent regular admins from deleting superadmin users (delete procedure) - Add tests covering both new security guards - Replace misleading passwordHash assertion with explanatory comment Co-Authored-By: Claude Opus 4.6 --- src/api/routers/users.ts | 20 ++++++++++++ tests/unit/api/routers/users.test.ts | 49 +++++++++++++++++++++++++--- 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/api/routers/users.ts b/src/api/routers/users.ts index a4a3cd84..a1ba311e 100644 --- a/src/api/routers/users.ts +++ b/src/api/routers/users.ts @@ -84,6 +84,18 @@ export const usersRouter = router({ }); } + // Only superadmins can change a superadmin's role + if ( + targetUser.role === 'superadmin' && + input.role !== 'superadmin' && + ctx.user.role !== 'superadmin' + ) { + throw new TRPCError({ + code: 'FORBIDDEN', + message: 'Only superadmins can change a superadmin user role', + }); + } + const updates: { name?: string; email?: string; @@ -121,6 +133,14 @@ export const usersRouter = router({ throw new TRPCError({ code: 'NOT_FOUND' }); } + // Only superadmins can delete superadmin users + if (targetUser.role === 'superadmin' && ctx.user.role !== 'superadmin') { + throw new TRPCError({ + code: 'FORBIDDEN', + message: 'Only superadmins can delete superadmin users', + }); + } + await deleteUser(input.id); }), }); diff --git a/tests/unit/api/routers/users.test.ts b/tests/unit/api/routers/users.test.ts index 191cc7ad..034e141c 100644 --- a/tests/unit/api/routers/users.test.ts +++ b/tests/unit/api/routers/users.test.ts @@ -70,10 +70,9 @@ describe('usersRouter', () => { expect(mockListOrgUsers).toHaveBeenCalledWith('org-1'); expect(result).toEqual(orgUsers); - // Ensure no passwordHash in result - for (const user of result) { - expect(user).not.toHaveProperty('passwordHash'); - } + // Note: passwordHash exclusion is enforced at the repository layer (listOrgUsers selects + // specific columns). The mock already returns data without passwordHash, reflecting + // the contract that the repository never returns this field. }); it('returns empty array when no users', async () => { @@ -267,6 +266,27 @@ describe('usersRouter', () => { expect(mockUpdateUser).toHaveBeenCalledWith('user-2', { role: 'superadmin' }); }); + it('prevents non-superadmin from revoking superadmin role', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-2', orgId: 'org-1', role: 'superadmin' }); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.update({ id: 'user-2', role: 'admin' })).rejects.toMatchObject({ + code: 'FORBIDDEN', + }); + + expect(mockUpdateUser).not.toHaveBeenCalled(); + }); + + it('allows superadmin to revoke superadmin role', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-2', orgId: 'org-1', role: 'superadmin' }); + mockUpdateUser.mockResolvedValue(undefined); + const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); + + await caller.update({ id: 'user-2', role: 'admin' }); + + expect(mockUpdateUser).toHaveBeenCalledWith('user-2', { role: 'admin' }); + }); + it('throws UNAUTHORIZED when not authenticated', async () => { const caller = createCaller({ user: null, effectiveOrgId: null }); await expect(caller.update({ id: 'user-2', name: 'X' })).rejects.toMatchObject({ @@ -323,6 +343,27 @@ describe('usersRouter', () => { expect(mockDeleteUser).not.toHaveBeenCalled(); }); + it('prevents non-superadmin from deleting superadmin user', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-super', orgId: 'org-1', role: 'superadmin' }); + const caller = createCaller({ user: mockAdminUser, effectiveOrgId: mockAdminUser.orgId }); + + await expect(caller.delete({ id: 'user-super' })).rejects.toMatchObject({ + code: 'FORBIDDEN', + }); + + expect(mockDeleteUser).not.toHaveBeenCalled(); + }); + + it('allows superadmin to delete another superadmin user', async () => { + mockGetUserById.mockResolvedValue({ id: 'user-super2', orgId: 'org-1', role: 'superadmin' }); + mockDeleteUser.mockResolvedValue(undefined); + const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); + + await caller.delete({ id: 'user-super2' }); + + expect(mockDeleteUser).toHaveBeenCalledWith('user-super2'); + }); + it('throws UNAUTHORIZED when not authenticated', async () => { const caller = createCaller({ user: null, effectiveOrgId: null }); await expect(caller.delete({ id: 'user-2' })).rejects.toMatchObject({