feat(api): add users tRPC router with CRUD operations and admin access control#792
feat(api): add users tRPC router with CRUD operations and admin access control#792
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Solid implementation that follows existing codebase patterns well. However, there are two security gaps where the PR description claims "only superadmins can grant/revoke superadmin role" but the code doesn't fully enforce this.
Code Issues
Blocking
1. Missing superadmin revocation guard (src/api/routers/users.ts:80-84)
The update procedure checks if (input.role === 'superadmin' && ctx.user.role !== 'superadmin') — this prevents granting superadmin, but does NOT prevent a regular admin from revoking superadmin status by changing a superadmin's role to member or admin. The PR description claims "only superadmins can grant/revoke superadmin role" but only the grant side is enforced.
Fix: add a guard that checks the target user's current 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' });
}2. Admin can delete superadmin users (src/api/routers/users.ts:105-121)
The delete procedure has no check preventing a regular admin from deleting a user who has the superadmin role. This is a privilege hierarchy violation — a lower-privileged admin should not be able to remove a higher-privileged superadmin.
Fix: add a guard after fetching the target user:
if (targetUser.role === 'superadmin' && ctx.user.role !== 'superadmin') {
throw new TRPCError({ code: 'FORBIDDEN', message: 'Only superadmins can delete superadmin users' });
}Should Fix
3. Misleading test assertion (tests/unit/api/routers/users.test.ts:47-64)
The test "returns org-scoped user list without passwordHash" passes trivially because the mock data itself doesn't contain passwordHash. The not.toHaveProperty('passwordHash') assertion tests the mock, not the actual behavior. The real guarantee comes from listOrgUsers in the repository selecting specific columns. Consider removing the misleading assertion or noting it as a documentation comment.
- 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 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured users CRUD router that follows existing codebase patterns. Access control logic is comprehensive, tests are thorough, and CI is green. Two minor suggestions below but nothing blocking.
Should Fix
-
src/api/routers/users.ts:41 — The
createmutation doesn't handle duplicate email errors. Sinceusers.emailhas a unique constraint, inserting a duplicate will throw a raw Drizzle/pg error that surfaces as an opaqueINTERNAL_SERVER_ERROR. Other routers in this codebase handle uniqueness conflicts explicitly (e.g.,runs.tsusesCONFLICT). Consider catching the DB error and throwingTRPCError({ code: 'CONFLICT', message: 'Email already in use' }). -
src/api/routers/users.ts:24 —
password: z.string().min(1)allows single-character passwords. While thecreate-admin-user.tstool doesn't enforce a minimum either, this is a user-facing API endpoint where a reasonable minimum (e.g.,min(8)) would be appropriate. Same applies to theupdateschema on line 65.
Summary
src/api/routers/users.tswithlist,create,update,deleteproceduresadminProceduremiddleware (admin/superadmin only)create-admin-user.tspattern)usersRouterinappRouterasusersinsrc/api/router.tsTest plan
listreturns org-scoped user list without passwordHashlistthrows UNAUTHORIZED for unauthenticated requestslistthrows FORBIDDEN for memberscreatehashes password with bcrypt before savingcreaterejects superadmin role assignment by non-superadmins (FORBIDDEN)createallows superadmins to create superadmin usersupdateallows sparse updates (name, email, role, password)updateprevents self-demotion (FORBIDDEN when changing own role)updateprevents non-superadmins from assigning superadmin roleupdateverifies org ownership (NOT_FOUND for cross-org access)deleteprevents self-deletion (FORBIDDEN)deleteverifies org ownership before deletingTrello card: https://trello.com/c/69b4f7503a01d76fed8352b2
🤖 Generated with Claude Code