Ft/user central rbac#185
Open
AnuMonachan wants to merge 11 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates authorization from single-role-per-user to scoped grants. It adds a user_roles table, removes legacy role/organization columns and project_users, backfills grants, and rewires policies, middleware, repositories, routes, scripts, seeds, and tests to use grant-based, org+project-scoped permission checks. ChangesGrant-scoped authorization migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/domains/usfm/usfm.route.ts (1)
171-175:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAuthorization stops at the filename.
Line 175 only authenticates
/downloads/{filename}. UnlikeGET /jobs/{jobId}, the download handler never re-checks project access or ownership, so any authenticated user who learns a valid export filename can fetch another project's artifact. Persist the originatingprojectUnitIdor requester with the stored export and authorize before serving the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/usfm/usfm.route.ts` around lines 171 - 175, The download route (downloadExportRoute) authenticates only the path but does not re-authorize ownership of the artifact; update the download handler for downloadExportRoute to load the stored export metadata (persisting projectUnitId and/or requesterId when exports are created) and verify the current user has access to that project/unit before returning the file; if the metadata shows a different project or a different requester, return a 403; ensure export creation logic is updated to save projectUnitId/requesterId alongside the filename so the download handler can perform this authorization check.src/domains/translated-verses/translated-verse-auth.middleware.ts (1)
87-100:⚠️ Potential issue | 🟡 MinorUpdate authorization-source handling: EDIT is wired to BODY only
The divergence concern doesn’t apply to the
EDITflow:src/domains/translated-verses/translated-verses.route.tsregistersTRANSLATED_VERSE_ACTIONS.EDITwithPROJECT_UNIT_ID_SOURCES.BODY, so the middleware’s earlierprojectUnitIdand the laterbody.projectUnitIdboth come from the same request body and won’t desync acrossBODYvsQUERY/VERSE_PARAM.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/translated-verses/translated-verse-auth.middleware.ts` around lines 87 - 100, The middleware mixes projectUnitId sources (uses projectUnitId variable earlier but calls chapterAssignmentService.getAssignmentForVerse and projectService.getProjectIdByUnitId with body.projectUnitId), but for EDIT routes PROJECT_UNIT_ID_SOURCES.BODY is always used; fix by using the same BODY-derived value consistently — replace body.projectUnitId with the projectUnitId variable (or vice versa) in the calls to chapterAssignmentService.getAssignmentForVerse and projectService.getProjectIdByUnitId so getAssignmentForVerse, projectService.getProjectIdByUnitId and resolveIsProjectMember all use the identical projectUnitId source (see symbols projectUnitId, body.projectUnitId, chapterAssignmentService.getAssignmentForVerse, projectService.getProjectIdByUnitId, resolveIsProjectMember, TRANSLATED_VERSE_ACTIONS.EDIT).src/domains/chapter-assignments/chapter-assignments.route.ts (2)
173-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
submitis pre-filtered by the wrong permission.
ChapterAssignmentPolicy.submit()delegates toedit()and explicitly allows scopedCONTENT_ASSIGNusers to act fromCOMMUNITY_REVIEWonward, but Lines 173-177 requireCONTENT_UPDATEbefore that policy runs. Managers who only carry assignment grants will be blocked from advancing later workflow stages.Suggested fix
middleware: [ authenticateUser, - requirePermission(PERMISSIONS.CONTENT_UPDATE), requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.SUBMIT), ] as const,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/chapter-assignments/chapter-assignments.route.ts` around lines 173 - 177, The submit route is enforcing PERMISSIONS.CONTENT_UPDATE but ChapterAssignmentPolicy.submit() allows scoped CONTENT_ASSIGN users via its delegation to edit(); update the middleware on the route that uses CHAPTER_ASSIGNMENT_ACTIONS.SUBMIT to requirePermission(PERMISSIONS.CONTENT_ASSIGN) instead of PERMISSIONS.CONTENT_UPDATE so managers with assignment grants can advance workflow stages (leave authenticateUser and requireChapterAssignmentAccess(...) unchanged).
229-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
GET /chapter-assignments/{chapterAssignmentId}still enforces the old read gate.
ChapterAssignmentPolicy.view()now allows either scopedCONTENT_VIEWor project membership, but Lines 229-233 require globalPROJECT_VIEWfirst. That turns valid member/content-view access into a hard 403 before the record-level policy can run.Suggested fix
middleware: [ authenticateUser, - requirePermission(PERMISSIONS.PROJECT_VIEW), requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.READ), ] as const,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/chapter-assignments/chapter-assignments.route.ts` around lines 229 - 233, The route currently enforces a global requirePermission(PERMISSIONS.PROJECT_VIEW) before the record-level policy, which blocks users who pass ChapterAssignmentPolicy.view via scoped CONTENT_VIEW or project membership; remove the requirePermission(PERMISSIONS.PROJECT_VIEW) entry from the middleware array so authenticateUser runs first and then requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.READ) can invoke ChapterAssignmentPolicy.view to allow scoped or member access.src/domains/chapter-assignments/chapter-assignment-auth.middleware.ts (1)
67-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
IS_PARTICIPANTno longer proves scoped access.Lines 67-68 authorize solely on
assignedUserId/peerCheckerId. With the presence routes still using an unscopedrequirePermission(PERMISSIONS.CONTENT_UPDATE), a user who lost access to this project but still hasCONTENT_UPDATEelsewhere can keep hitting the presence endpoints as long as the assignment row still points at them.Suggested fix
case CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT: - allowed = ChapterAssignmentPolicy.isParticipant(policyUser, policyAssignment); + allowed = + ChapterAssignmentPolicy.isParticipant(policyUser, policyAssignment) && + ChapterAssignmentPolicy.edit(policyUser, policyAssignment, ctx.isProjectMember); break;If presence must stay decoupled from edit-status rules, add a dedicated scoped participant policy instead of using
isParticipant()as the auth decision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/chapter-assignments/chapter-assignment-auth.middleware.ts` around lines 67 - 68, The IS_PARTICIPANT branch currently calls ChapterAssignmentPolicy.isParticipant(policyUser, policyAssignment) which only checks assignedUserId/peerCheckerId and does not enforce project scoping; replace this with (or add and call) a scoped check such as ChapterAssignmentPolicy.isScopedParticipant(policyUser, policyAssignment, projectId) that also verifies the user's access to the assignment's project/context, and update the CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT case to use that new method (or, if keeping presence decoupled, create a dedicated ChapterAssignmentPolicy.isPresenceParticipant(...) that performs scoped verification) instead of relying on requirePermission(PERMISSIONS.CONTENT_UPDATE) so presence endpoints cannot be accessed by users who lost project access.src/db/scripts/create-user.ts (1)
87-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop deriving the grant scope from
roleId === 1.This script now lives in a role model with multiple named roles, but it still assumes id
1means "Manager" and everything else means "Translator". Serial IDs are data-dependent, so this branch can attach the wrong role and even writeorgId: nullfor whichever role happens to own id1, turning it into a global grant. Resolve the selected role fromroles.name/ROLESand derive scope from the actual role definition instead of this sentinel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/scripts/create-user.ts` around lines 87 - 99, The code incorrectly treats roleId === 1 as the "Manager"/global grant; instead look up the role object by roleId (e.g., from roles array or ROLES map used elsewhere) and use its name/scope to decide orgId and the log message; in the tx.insert into schema.user_roles set orgId to null only when the resolved role's scope indicates a global grant (e.g., role.scope === 'global' or role.isGlobal), otherwise use organizationId, and log the resolved role.name rather than deriving label from roleId === 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/migrations/0012_added_user_role_grants.sql`:
- Around line 12-18: The migration currently drops project_users, role and
organization constraints and idx_role_permissions_role before performing the
RBAC backfill, which will destroy the only source data for the copy; modify the
migration so the backfill/copy runs before any destructive statements: move or
add the RBAC copy step (the same operation run by package.json RBAC copy) to
execute prior to ALTER TABLE "project_users" DROP/ALTER and DROP INDEX
"idx_role_permissions_role", or split this migration into
create/backfill/verify/drop phases (create new tables/copy data, verify
backfill, then DROP "project_users" and DROP CONSTRAINT
"users_role_roles_id_fk"/"users_organization_organizations_id_fk" and DROP INDEX
"idx_role_permissions_role"); ensure the backfill completes and is verified in
this migration before performing any DROP/ALTER.
- Line 24: The current unique index uq_user_role_grant on table user_roles over
(user_id, org_id, project_id, role_id) doesn’t prevent duplicate NULL-scoped
grants because PostgreSQL treats NULLs as distinct; replace it with scope-aware
constraints: drop uq_user_role_grant and add partial unique indexes for each
scope (global: WHERE org_id IS NULL AND project_id IS NULL on (user_id,
role_id); org-scoped: WHERE org_id IS NOT NULL AND project_id IS NULL on
(user_id, org_id, role_id); project-scoped: WHERE project_id IS NOT NULL on
(user_id, org_id, project_id, role_id)), or if your Postgres target supports it
use a single index with NULLS NOT DISTINCT on (user_id, org_id, project_id,
role_id); ensure the migration updates the index creation to one of these
options so one-grant-per-scope is enforced.
In `@src/db/schema.ts`:
- Around line 473-478: The composite unique index uq_user_role_grant allows
duplicates because orgId/projectId are nullable; replace it with
normalized/partial unique indexes: create a unique index for project-scoped
grants that applies when projectId IS NOT NULL (unique on table.userId,
table.orgId, table.projectId), a unique index for org-scoped grants that applies
when projectId IS NULL AND orgId IS NOT NULL (unique on table.userId,
table.orgId), and a unique index for global grants that applies when orgId IS
NULL (unique on table.userId); update the schema entries that currently use
uniqueIndex('uq_user_role_grant').on(...) to these three partial/expression
unique indexes so NULL scope values are normalized and duplicates cannot be
inserted.
In `@src/db/scripts/migrate-to-user-central-rbac.ts`:
- Around line 26-32: The migration in migrate-to-user-central-rbac.ts reads
legacy columns (users.role, users.organization) and project_users while writing
to user_roles, which no longer coexist; split this change into a safe three-step
rollout or snapshot legacy data: 1) create the new user_roles table
(createUserRoles / corresponding DDL) in one migration, 2) add a separate
backfill migration that reads the legacy sources (users.role,
users.organization, project_users) and populates user_roles, ensuring it runs
before any drops, or alternatively add a pre-migration step that selects and
stores the legacy data into a temporary snapshot table, and 3) in a final
migration drop the legacy columns/tables; update migrate-to-user-central-rbac.ts
to only perform the backfill against the snapshot or ensure it is the middle
migration that assumes both sources and target exist.
In `@src/domains/projects/projects.repository.ts`:
- Around line 108-118: The getByUserId() query in projects.repository.ts
currently innerJoins user_roles (user_roles and projects) which yields duplicate
projects when a user has multiple role rows; change the SQL boundary to
deduplicate by replacing the innerJoin with an EXISTS subquery (or a distinct
project-id subquery) that checks for any matching user_roles row for the given
userId and project/org scope, or alternatively apply a DISTINCT on projects.id
before mapping; update the query around the user_roles join (references:
getByUserId, user_roles, projects) to use WHERE EXISTS(...) or SELECT DISTINCT
projects.id to ensure each project is returned once.
In `@src/domains/projects/projects.route.ts`:
- Around line 114-121: The project creation currently commits via
projectService.createProject() and then separately calls grantRole(), risking a
partially-created project if the grant fails; modify the flow so the project
creation and creator grant occur in the same DB transaction: either update
projectService.createProject to accept an initialGrant (or options/transaction
callback) and perform both INSERTs inside the same transaction, or obtain a
transaction object from your DB layer (pass it into projectService.createProject
and grantRole) so both operations use the same transaction and are
committed/rolled back together; references: projectService.createProject,
grantRole, getRoleId, ROLES.PROJECT_MANAGER, currentUser,
projectData.organization.
In `@src/domains/projects/projects.types.ts`:
- Around line 48-50: The projects.types.ts schema currently requires the client
to supply organization via the organization: z.number().int() field which
conflicts with your comment that the route overwrites it from auth; change that
field to be optional or remove it from the public Project input (e.g.
organization: z.number().int().optional()) so older clients don't fail
validation, and then enforce and validate a required organization only in
CreateProjectServiceInput (or wherever server-side auth context populates it) so
the handler can overwrite/require it safely.
In `@src/domains/projects/users/project-users.repository.ts`:
- Around line 45-50: The current dedupe in getProjectUsers() treats numeric
roleID as privilege order; change it to use an explicit priority mapping or stop
collapsing grants. Update the uniqueUsers logic (the Map over rows and the
comparison using r.roleID) to consult a ROLE_PRIORITY map (or
roleName->priority) and compare ROLE_PRIORITY[existing.roleID] vs
ROLE_PRIORITY[r.roleID] when deciding which grant to keep, or alternatively
return all matching grants instead of deduplicating; ensure you reference the
role priority mapping where the comparison is done so future DB ordering won't
affect privilege selection.
- Around line 54-59: getProjectUsers() currently maps org-scoped grants
(projectId === null) to the current projectId which hides inherited scope and
prevents removeProjectUser() from working; update getProjectUsers() to preserve
the original scope by either (A) returning projectId exactly as stored (keep
null for org grants) or (B) adding an explicit flag like inheritedFromOrg or
isInherited to each item so callers can distinguish inherited grants, and then
update/removeProjectUser() logic to reject or prevent project-level deletion
when the item is an inherited org grant (use the symbols getProjectUsers and
removeProjectUser to locate the code paths to change).
In `@src/domains/user-roles/user-roles.repository.ts`:
- Line 99: The local variable map is declared as Map<number, any[]> which
weakens type safety; change its declaration to use the explicit return element
type from the function signature by declaring map as Map<number, Array<{
roleName: string; orgId: number | null; projectId: number | null }>> (update the
variable named map in user-roles.repository.ts accordingly so all references use
this stronger type).
- Line 33: The code unconditionally casts row.permission as Permission when
calling entry.permissions.add, which can blow up if the DB contains invalid
strings; modify the repository (around entry.permissions.add and references to
row.permission) to validate the value against the Permission enum before
casting—e.g., implement an isValidPermission helper that checks
Object.values(Permission).includes(row.permission) and then only call
entry.permissions.add(row.permission as Permission) when valid, otherwise handle
the case (skip, log a warning/error or throw) so invalid DB values are not
blindly cast.
In `@src/domains/users/user.policy.ts`:
- Around line 30-32: The delete method in user.policy.ts currently checks
org-scoped grants (authorize(user, PERMISSIONS.USER_DELETE, { orgId })) despite
the comment claiming account deletion is SuperAdmin-only; change the logic to
enforce a global-only check by verifying a global/superadmin grant (e.g., call
authorize(user, PERMISSIONS.USER_DELETE) without an orgId or call an existing
requireSuperAdmin(user) helper) and return false for org-scoped grants, or
alternatively require the route handler to use requireSuperAdmin; update the
delete(user: AppPolicyUser, target: PolicyTargetUser) implementation to only
allow deletion when the user has the global/superadmin permission.
- Around line 20-27: The view and update policy methods (view and update) only
check target.orgIds so targets with no org grants block global-permission
callers; modify both functions to, after the existing orgIds.some(...) check,
also check a global fallback by calling authorize(user, PERMISSIONS.USER_VIEW)
for view (and authorize(user, PERMISSIONS.USER_UPDATE) for update) when
target.orgIds is empty (or when the orgIds.some(...) returns false), so global
admins can pass the policy; keep existing self-check (user.id === target.id)
first and use the same authorize(...) helper and PERMISSIONS constants.
In `@src/domains/users/users.route.ts`:
- Around line 137-155: The grantRole call returns a Result<void> and is not
being checked, so failures there still return 201 and the deleteUser rollback is
never reached; update the code around grantRole (the import/use of grantRole and
getRoleId and the surrounding try/catch) to await grantRole and inspect its
Result (e.g., check success/failed or .isOk/.isError or .success flag), and if
the grant failed call userService.deleteUser(result.data.id) and return an
INTERNAL_SERVER_ERROR response containing the grant error message; keep using
currentUser.id, requestData.roleName/ROLES.PROJECT_TRANSLATOR and
requestData.orgId/projectId to locate the correct data when constructing the
grant and the error response.
In `@src/domains/users/users.service.ts`:
- Around line 41-78: The current mapping in getUsersForUser mutates the object
returned by toUserResponse by assigning response.orgGrants, which risks
type/shape drift; instead construct the full UserResponse in one step by merging
the value returned from toUserResponse(u) with an explicit orgGrants property
(set from grantsMap.get(u.id) ?? []), and ensure the UserResponse type/interface
includes orgGrants so the returned object matches the expected shape; update the
map inside getUsersForUser to return the new merged object rather than mutating
the original.
In `@src/domains/users/users.types.ts`:
- Around line 48-50: Change the Zod validators for the grant ID fields so they
require positive integers: update orgId in users.types.ts to use an integer
positive constraint (e.g., .int().positive()) and do the same for projectId
while preserving its optional().nullable() status (e.g.,
.int().positive().optional().nullable()); this ensures requestData.orgId and
requestData.projectId cannot be 0 or negative and prevents the conditional in
users.route.ts that checks requestData.orgId from skipping grant creation due to
falsy IDs.
In `@src/lib/services/auth/auth.service.ts`:
- Around line 21-24: The InviteUserInput interface currently allows
project-scoped roles to be passed with projectId=null which is treated as
org-wide access; fix by validating roleName and projectId where invites are
created/consumed: in the invite creation/validation logic that accepts
InviteUserInput (look for uses of InviteUserInput, roleName and projectId and
the invite handler function around lines ~74-80), enforce that any roleName
starting with "PROJECT_" must include a non-null projectId and reject
(throw/return validation error) if missing; alternatively if you prefer silent
fallback, map unspecified projectId + a project-scoped roleName to the
corresponding org-scoped role (e.g., PROJECT_TRANSLATOR -> ORG_TRANSLATOR)
before downstream grant resolution — implement one of these two behaviors
consistently and add unit-tests for both InviteUserInput handling and the invite
creation path.
In `@src/lib/services/permissions/authorize.test.ts`:
- Around line 15-100: Add unit tests in the existing authorize.test.ts suite
that directly exercise canAssignRole (the function in authorize.ts) covering
every target role: create tests that assert allow and deny for org-scoped and
project-scoped assignment boundaries, include cases where projectId is omitted
(missing projectId path) and where grants are project-pinned vs org-wide and in
different orgs; reuse the existing grant(...) helper and user/grants patterns to
construct users with org-wide grants, project-specific grants, and grants in
OTHER_ORG, and assert canAssignRole returns true/false for each role scenario to
mirror the authorize test patterns.
In `@src/lib/services/permissions/authorize.ts`:
- Around line 82-89: The project-level role branch (checking targetRoleName
against ROLES.PROJECT_MANAGER / PROJECT_TRANSLATOR / PROJECT_OBSERVER and
calling authorize(caller, PERMISSIONS.ROLE_ASSIGN_PROJECT, scope)) must reject
when no project is provided; add a guard that if scope.projectId (or the
projectId variable used in this function) is null/undefined, return a denying
result (e.g., false) instead of calling authorize so we never evaluate
project-only role assignment at org scope. Ensure this check runs before the
authorize(...) call in the block that matches the project roles.
In `@src/middlewares/authenticate.ts`:
- Around line 133-137: The grants lookup currently swallows failures and sets
grants to an empty array; update the middleware around the findGrantsByUserId
call so that when grantsResult.ok is false you log a warning/error with the
failure details (include grantsResult.error or similar) and context
(userResult.data.id), and then either (a) fail the request by throwing an
authentication/authorization error (so authentication stops) or (b) if empty
grants must be tolerated, at minimum log the failure and attach an explicit flag
on c.set('user', ...) indicating grantsFetchFailed; locate the logic around
findGrantsByUserId, userResult, and the c.set('user', ...) assignment in
authenticate.ts and implement the logging/throwing behavior there.
In `@src/middlewares/role-auth.ts`:
- Around line 108-123: The requireSuperAdmin middleware currently treats any
grant with {orgId: null, projectId: null} as SuperAdmin; change it to require an
explicit super-admin indicator on grants instead: update the isSuperAdmin check
in requireSuperAdmin to verify a dedicated property or role (e.g. g.role ===
'super_admin' or g.isSuperAdmin === true) in addition to the global scope, or
only check that explicit super-admin role and ignore scope alone; use the
existing user.grants array and the isSuperAdmin variable to implement this
stricter check so only true super-admin grants pass.
---
Outside diff comments:
In `@src/db/scripts/create-user.ts`:
- Around line 87-99: The code incorrectly treats roleId === 1 as the
"Manager"/global grant; instead look up the role object by roleId (e.g., from
roles array or ROLES map used elsewhere) and use its name/scope to decide orgId
and the log message; in the tx.insert into schema.user_roles set orgId to null
only when the resolved role's scope indicates a global grant (e.g., role.scope
=== 'global' or role.isGlobal), otherwise use organizationId, and log the
resolved role.name rather than deriving label from roleId === 1.
In `@src/domains/chapter-assignments/chapter-assignment-auth.middleware.ts`:
- Around line 67-68: The IS_PARTICIPANT branch currently calls
ChapterAssignmentPolicy.isParticipant(policyUser, policyAssignment) which only
checks assignedUserId/peerCheckerId and does not enforce project scoping;
replace this with (or add and call) a scoped check such as
ChapterAssignmentPolicy.isScopedParticipant(policyUser, policyAssignment,
projectId) that also verifies the user's access to the assignment's
project/context, and update the CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT case
to use that new method (or, if keeping presence decoupled, create a dedicated
ChapterAssignmentPolicy.isPresenceParticipant(...) that performs scoped
verification) instead of relying on
requirePermission(PERMISSIONS.CONTENT_UPDATE) so presence endpoints cannot be
accessed by users who lost project access.
In `@src/domains/chapter-assignments/chapter-assignments.route.ts`:
- Around line 173-177: The submit route is enforcing PERMISSIONS.CONTENT_UPDATE
but ChapterAssignmentPolicy.submit() allows scoped CONTENT_ASSIGN users via its
delegation to edit(); update the middleware on the route that uses
CHAPTER_ASSIGNMENT_ACTIONS.SUBMIT to
requirePermission(PERMISSIONS.CONTENT_ASSIGN) instead of
PERMISSIONS.CONTENT_UPDATE so managers with assignment grants can advance
workflow stages (leave authenticateUser and requireChapterAssignmentAccess(...)
unchanged).
- Around line 229-233: The route currently enforces a global
requirePermission(PERMISSIONS.PROJECT_VIEW) before the record-level policy,
which blocks users who pass ChapterAssignmentPolicy.view via scoped CONTENT_VIEW
or project membership; remove the requirePermission(PERMISSIONS.PROJECT_VIEW)
entry from the middleware array so authenticateUser runs first and then
requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.READ) can invoke
ChapterAssignmentPolicy.view to allow scoped or member access.
In `@src/domains/translated-verses/translated-verse-auth.middleware.ts`:
- Around line 87-100: The middleware mixes projectUnitId sources (uses
projectUnitId variable earlier but calls
chapterAssignmentService.getAssignmentForVerse and
projectService.getProjectIdByUnitId with body.projectUnitId), but for EDIT
routes PROJECT_UNIT_ID_SOURCES.BODY is always used; fix by using the same
BODY-derived value consistently — replace body.projectUnitId with the
projectUnitId variable (or vice versa) in the calls to
chapterAssignmentService.getAssignmentForVerse and
projectService.getProjectIdByUnitId so getAssignmentForVerse,
projectService.getProjectIdByUnitId and resolveIsProjectMember all use the
identical projectUnitId source (see symbols projectUnitId, body.projectUnitId,
chapterAssignmentService.getAssignmentForVerse,
projectService.getProjectIdByUnitId, resolveIsProjectMember,
TRANSLATED_VERSE_ACTIONS.EDIT).
In `@src/domains/usfm/usfm.route.ts`:
- Around line 171-175: The download route (downloadExportRoute) authenticates
only the path but does not re-authorize ownership of the artifact; update the
download handler for downloadExportRoute to load the stored export metadata
(persisting projectUnitId and/or requesterId when exports are created) and
verify the current user has access to that project/unit before returning the
file; if the metadata shows a different project or a different requester, return
a 403; ensure export creation logic is updated to save projectUnitId/requesterId
alongside the filename so the download handler can perform this authorization
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 523f7bfa-fd91-4c4a-aa45-38318898f39c
📒 Files selected for processing (49)
package.jsonsrc/db/migrations/0012_added_user_role_grants.sqlsrc/db/migrations/meta/0012_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/db/scripts/create-user.tssrc/db/scripts/migrate-to-user-central-rbac.tssrc/db/seeds/dev-users.tssrc/db/seeds/rbac.tssrc/domains/ai-tools/ai-tools.route.test.tssrc/domains/chapter-assignments/chapter-assignment-auth.middleware.tssrc/domains/chapter-assignments/chapter-assignments.policy.tssrc/domains/chapter-assignments/chapter-assignments.repository.tssrc/domains/chapter-assignments/chapter-assignments.route.tssrc/domains/chapter-assignments/chapter-assignments.service.tssrc/domains/chapter-assignments/presence/chapter-assignments-presence.route.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.route.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.service.tssrc/domains/projects/project-auth.middleware.tssrc/domains/projects/project.policy.tssrc/domains/projects/projects.repository.tssrc/domains/projects/projects.route.tssrc/domains/projects/projects.service.tssrc/domains/projects/projects.types.tssrc/domains/projects/users/project-users.repository.tssrc/domains/projects/users/project-users.service.tssrc/domains/translated-verses/translated-verse-auth.middleware.tssrc/domains/user-roles/user-roles.repository.test.tssrc/domains/user-roles/user-roles.repository.tssrc/domains/user-roles/user-roles.service.tssrc/domains/users/user-auth.middleware.tssrc/domains/users/user.policy.tssrc/domains/users/users.repository.tssrc/domains/users/users.route.tssrc/domains/users/users.service.test.tssrc/domains/users/users.service.tssrc/domains/users/users.types.tssrc/domains/usfm/usfm.route.tssrc/lib/permissions.tssrc/lib/roles.tssrc/lib/services/auth/auth.service.tssrc/lib/services/permissions/authorize.test.tssrc/lib/services/permissions/authorize.tssrc/lib/services/permissions/permissions.service.tssrc/lib/types.tssrc/middlewares/authenticate.test.tssrc/middlewares/authenticate.tssrc/middlewares/role-auth.tssrc/test/utils/test-helpers.ts
| ALTER TABLE "user_roles" ADD CONSTRAINT "user_roles_project_id_projects_id_fk" FOREIGN KEY ("project_id") REFERENCES "public"."projects"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "user_roles" ADD CONSTRAINT "user_roles_role_id_roles_id_fk" FOREIGN KEY ("role_id") REFERENCES "public"."roles"("id") ON DELETE no action ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "user_roles" ADD CONSTRAINT "user_roles_created_by_users_id_fk" FOREIGN KEY ("created_by") REFERENCES "public"."users"("id") ON DELETE no action ON UPDATE no action;--> statement-breakpoint | ||
| CREATE UNIQUE INDEX "uq_user_role_grant" ON "user_roles" USING btree ("user_id","org_id","project_id","role_id");--> statement-breakpoint |
There was a problem hiding this comment.
This unique index does not actually de-duplicate NULL-scoped grants.
In PostgreSQL, NULL values are distinct for ordinary unique indexes, so duplicate global/org grants like (user_id, org_id = NULL, project_id = NULL, role_id) or (user_id, org_id = 7, project_id = NULL, role_id) can still be inserted multiple times. That breaks the one-grant-per-scope contract this table is meant to enforce.
Use partial unique indexes per scope, or NULLS NOT DISTINCT if that matches the database target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/migrations/0012_added_user_role_grants.sql` at line 24, The current
unique index uq_user_role_grant on table user_roles over (user_id, org_id,
project_id, role_id) doesn’t prevent duplicate NULL-scoped grants because
PostgreSQL treats NULLs as distinct; replace it with scope-aware constraints:
drop uq_user_role_grant and add partial unique indexes for each scope (global:
WHERE org_id IS NULL AND project_id IS NULL on (user_id, role_id); org-scoped:
WHERE org_id IS NOT NULL AND project_id IS NULL on (user_id, org_id, role_id);
project-scoped: WHERE project_id IS NOT NULL on (user_id, org_id, project_id,
role_id)), or if your Postgres target supports it use a single index with NULLS
NOT DISTINCT on (user_id, org_id, project_id, role_id); ensure the migration
updates the index creation to one of these options so one-grant-per-scope is
enforced.
Comment on lines
+26
to
+32
| const allUsers = await tx | ||
| .select({ | ||
| id: users.id, | ||
| organization: sql<number>`organization`, | ||
| role: sql<number>`role`, | ||
| }) | ||
| .from(users); |
There was a problem hiding this comment.
This backfill is wired against a schema state that no longer exists in the same migration set.
The script writes to user_roles, but all of its source reads still depend on legacy users.role, users.organization, and project_users. In this PR, the schema contract has already moved away from those columns/tables, so there is no safe rollout point where both the new target table and the legacy source data are guaranteed to exist. As written, the migration either runs before user_roles exists or after its source data has been dropped. Split this into add new table → backfill → drop legacy structures, or snapshot the legacy data before the drop.
Also applies to: 49-52, 82-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/scripts/migrate-to-user-central-rbac.ts` around lines 26 - 32, The
migration in migrate-to-user-central-rbac.ts reads legacy columns (users.role,
users.organization) and project_users while writing to user_roles, which no
longer coexist; split this change into a safe three-step rollout or snapshot
legacy data: 1) create the new user_roles table (createUserRoles / corresponding
DDL) in one migration, 2) add a separate backfill migration that reads the
legacy sources (users.role, users.organization, project_users) and populates
user_roles, ensuring it runs before any drops, or alternatively add a
pre-migration step that selects and stores the legacy data into a temporary
snapshot table, and 3) in a final migration drop the legacy columns/tables;
update migrate-to-user-central-rbac.ts to only perform the backfill against the
snapshot or ensure it is the middle migration that assumes both sources and
target exist.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/db/migrations/0012_create_user_role_grants.sql (1)
17-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnique grant de-dup is still broken for NULL-scoped entries.
Line 17 still uses a nullable-column unique index, so duplicate grants with
NULLscope can bypass uniqueness.Suggested SQL change
-CREATE UNIQUE INDEX "uq_user_role_grant" ON "user_roles" USING btree ("user_id","org_id","project_id","role_id"); +-- Global scope (org_id/project_id both NULL) +CREATE UNIQUE INDEX "uq_user_role_grant_global" + ON "user_roles" USING btree ("user_id","role_id") + WHERE "org_id" IS NULL AND "project_id" IS NULL; + +-- Org scope (org_id set, project_id NULL) +CREATE UNIQUE INDEX "uq_user_role_grant_org" + ON "user_roles" USING btree ("user_id","org_id","role_id") + WHERE "org_id" IS NOT NULL AND "project_id" IS NULL; + +-- Project scope (project_id set) +CREATE UNIQUE INDEX "uq_user_role_grant_project" + ON "user_roles" USING btree ("user_id","project_id","role_id") + WHERE "project_id" IS NOT NULL;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations/0012_create_user_role_grants.sql` at line 17, The unique index "uq_user_role_grant" on table "user_roles" currently allows duplicate NULL-scoped grants; replace it with an index that normalizes NULLs via expressions so NULLs are treated as equal, e.g. drop the existing CREATE UNIQUE INDEX "uq_user_role_grant" and recreate it using COALESCE on the nullable scope columns (COALESCE(org_id, -1), COALESCE(project_id, -1)) along with user_id and role_id so NULL org/project values are deduplicated (ensure the sentinel -1 is compatible with the column types).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/migrations/0013_drop_legacy_rbac_columns.sql`:
- Line 2: The DROP TABLE "project_users" CASCADE; and the other DROP statements
at lines 8-9 remove legacy RBAC data with no safety gate; add a
precondition/backfill guard at the top of this migration that aborts unless a
safe condition is met (for example: require an environment-controlled migration
flag or confirm legacy tables are empty). Implement this by querying the tables
(e.g., SELECT count(*) FROM "project_users") and RAISE EXCEPTION (or use
pg_notify/RAISE NOTICE and exit) if counts > 0, or by checking a feature_flag
table/pg_settings key before executing the DROP, and include a clear message
instructing operators how to enable the migration after performing
backfill/verification.
---
Duplicate comments:
In `@src/db/migrations/0012_create_user_role_grants.sql`:
- Line 17: The unique index "uq_user_role_grant" on table "user_roles" currently
allows duplicate NULL-scoped grants; replace it with an index that normalizes
NULLs via expressions so NULLs are treated as equal, e.g. drop the existing
CREATE UNIQUE INDEX "uq_user_role_grant" and recreate it using COALESCE on the
nullable scope columns (COALESCE(org_id, -1), COALESCE(project_id, -1)) along
with user_id and role_id so NULL org/project values are deduplicated (ensure the
sentinel -1 is compatible with the column types).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cf0b70c8-d69a-4e9a-b433-cb47f720a0a2
📒 Files selected for processing (6)
src/db/migrations/0012_create_user_role_grants.sqlsrc/db/migrations/0013_drop_legacy_rbac_columns.sqlsrc/db/migrations/meta/0012_snapshot.jsonsrc/db/migrations/meta/0013_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.ts
💤 Files with no reviewable changes (1)
- src/db/schema.ts
| @@ -0,0 +1,9 @@ | |||
| ALTER TABLE "project_users" DISABLE ROW LEVEL SECURITY;--> statement-breakpoint | |||
| DROP TABLE "project_users" CASCADE;--> statement-breakpoint | |||
There was a problem hiding this comment.
Destructive RBAC drops run without a safety gate.
Lines 2, 8, and 9 remove legacy authorization data with no in-migration precondition/backfill guard, so running this migration early can permanently lose role assignments.
Suggested safety guard before DROP statements
+DO $$
+BEGIN
+ IF EXISTS (
+ SELECT 1
+ FROM "users"
+ WHERE "role" IS NOT NULL OR "organization" IS NOT NULL
+ )
+ AND NOT EXISTS (SELECT 1 FROM "user_roles")
+ THEN
+ RAISE EXCEPTION 'Refusing to drop legacy RBAC columns: user_roles backfill not detected';
+ END IF;
+END $$;
+
ALTER TABLE "project_users" DISABLE ROW LEVEL SECURITY;--> statement-breakpoint
DROP TABLE "project_users" CASCADE;--> statement-breakpoint
...Also applies to: 8-9
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/migrations/0013_drop_legacy_rbac_columns.sql` at line 2, The DROP
TABLE "project_users" CASCADE; and the other DROP statements at lines 8-9 remove
legacy RBAC data with no safety gate; add a precondition/backfill guard at the
top of this migration that aborts unless a safe condition is met (for example:
require an environment-controlled migration flag or confirm legacy tables are
empty). Implement this by querying the tables (e.g., SELECT count(*) FROM
"project_users") and RAISE EXCEPTION (or use pg_notify/RAISE NOTICE and exit) if
counts > 0, or by checking a feature_flag table/pg_settings key before executing
the DROP, and include a clear message instructing operators how to enable the
migration after performing backfill/verification.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/lib/services/auth/auth.service.ts (1)
74-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
SUPER_ADMINscope before callinggrantRole.This path always forwards
normalizedInput.orgIdintograntRole, butsrc/db/scripts/create-user.tsalready treatsROLES.SUPER_ADMINas a global grant with{ orgId: null, projectId: null }. If an invite assignsSUPER_ADMIN, this code persists a differently-scoped grant than the rest of the PR expects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/services/auth/auth.service.ts` around lines 74 - 98, The code forwards normalizedInput.orgId/ projectId into grantRole without normalizing the SUPER_ADMIN scope; ensure that when roleName === ROLES.SUPER_ADMIN you pass orgId: null and projectId: null to grantRole (i.e., compute scopedOrgId/scopedProjectId based on roleName after calling getRoleId and before calling grantRole) so SUPER_ADMIN grants are created as global grants consistent with create-user.ts; update the grantRole call to use these normalized values instead of normalizedInput.orgId/normalizedInput.projectId.src/db/scripts/migrate-to-user-central-rbac.ts (1)
79-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the expected migrated grant, not just “any grant”.
grantedIdsis built from the entireuser_rolestable, sograntedIds.has(u.id)passes even if this script failed to create the required PM/PT grant and the user merely had some unrelated grant already. That makes the backfill report success with incomplete migrated permissions. The post-check should verify the expected role/scope produced from the legacy data, not mere existence of anyuser_rolesrow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/scripts/migrate-to-user-central-rbac.ts` around lines 79 - 99, The check currently uses grantedIds built from any row in user_roles, which can hide missing specific PM/PT grants; instead, for each user in allUsers compute the expected target grant(s) from legacy data (use pmId/ptId and, for PT, the project membership from project_users via tx) and verify that tx.select from user_roles contains a matching row with the expected role/scope (e.g., matching userId and roleId and, when applicable, matching project/resource id) rather than just checking grantedIds.has(u.id); update the loop around pmId/ptId and the final validation to query user_roles for the specific expected grant(s) and push u.id into trueOrphans only if those exact grants are missing.src/domains/chapter-assignments/editor-state/user-chapter-assignment-editor-state.route.ts (1)
35-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDrop the stale 403 responses from these editor-state route specs.
Lines 35-38 and 82-85 now authorize entirely through
requireChapterAssignmentAccess(...), which fails with404, not403. Keeping the403entries here leaves the OpenAPI contract out of sync with the actual middleware behavior.Suggested contract cleanup
responses: { [HttpStatusCodes.OK]: jsonContent( editorStateResponseSchema, 'The editor state for the current user' ), [HttpStatusCodes.UNAUTHORIZED]: jsonContent( createMessageObjectSchema('Unauthorized'), 'Authentication required' ), - [HttpStatusCodes.FORBIDDEN]: jsonContent( - createMessageObjectSchema('Forbidden'), - 'Access denied' - ), [HttpStatusCodes.NOT_FOUND]: jsonContent( createMessageObjectSchema(HttpStatusPhrases.NOT_FOUND), 'Chapter assignment not found' ), [HttpStatusCodes.INTERNAL_SERVER_ERROR]: jsonContent( @@ responses: { [HttpStatusCodes.OK]: jsonContent(editorStateResponseSchema, 'The saved editor state'), [HttpStatusCodes.BAD_REQUEST]: jsonContent( createMessageObjectSchema('Bad Request'), 'Invalid request data' ), [HttpStatusCodes.UNAUTHORIZED]: jsonContent( createMessageObjectSchema('Unauthorized'), 'Authentication required' ), - [HttpStatusCodes.FORBIDDEN]: jsonContent( - createMessageObjectSchema('Forbidden'), - 'Access denied' - ), [HttpStatusCodes.NOT_FOUND]: jsonContent( createMessageObjectSchema(HttpStatusPhrases.NOT_FOUND), 'Chapter assignment not found' ), [HttpStatusCodes.INTERNAL_SERVER_ERROR]: jsonContent(Also applies to: 49-56, 82-85, 105-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/chapter-assignments/editor-state/user-chapter-assignment-editor-state.route.ts` around lines 35 - 38, The OpenAPI route specs for the editor-state endpoints are still declaring stale 403 responses even though authorization is handled by requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT) which returns 404; update the route contract to remove the 403 entries (or replace them with 404 where appropriate) so the responses match the middleware behavior—look for the middleware array using authenticateUser and requireChapterAssignmentAccess in user-chapter-assignment-editor-state.route.ts and edit the route responses for all editor-state specs (the other similar blocks referencing the same middleware) to drop 403s and ensure 404 is declared instead.src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts (1)
32-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare the 404 that the new authorization middleware can return.
Lines 32-35 and 57-60 now gate both endpoints with
requireChapterAssignmentAccess(...), but neither response block documents the404it emits on denial. That leaves both OpenAPI definitions incomplete for the current runtime behavior.Suggested OpenAPI fix
responses: { [HttpStatusCodes.OK]: jsonContent(presenceResponseSchema, 'Presence registered successfully'), [HttpStatusCodes.UNAUTHORIZED]: jsonContent( createMessageObjectSchema('Unauthorized'), 'Authentication required' ), + [HttpStatusCodes.NOT_FOUND]: jsonContent( + createMessageObjectSchema(HttpStatusPhrases.NOT_FOUND), + 'Chapter assignment not found' + ), [HttpStatusCodes.INTERNAL_SERVER_ERROR]: jsonContent( createMessageObjectSchema(HttpStatusPhrases.INTERNAL_SERVER_ERROR), 'Internal server error' ) }, @@ responses: { [HttpStatusCodes.NO_CONTENT]: { description: 'Presence removed successfully' }, [HttpStatusCodes.UNAUTHORIZED]: jsonContent( createMessageObjectSchema('Unauthorized'), 'Authentication required' ), + [HttpStatusCodes.NOT_FOUND]: jsonContent( + createMessageObjectSchema(HttpStatusPhrases.NOT_FOUND), + 'Chapter assignment not found' + ), [HttpStatusCodes.INTERNAL_SERVER_ERROR]: jsonContent( createMessageObjectSchema(HttpStatusPhrases.INTERNAL_SERVER_ERROR), 'Internal server error' ) },Also applies to: 37-47, 57-60, 62-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts` around lines 32 - 35, The OpenAPI operation responses are missing the 404 that requireChapterAssignmentAccess(...) can return; update the response documentation for both endpoints that use middleware: [authenticateUser, requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT)] (the route handlers in chapter-assignments-presence.route) to include a 404 response entry (with a short description and the existing error schema) alongside the existing 200/401 entries so the spec matches runtime behavior.src/domains/users/users.route.ts (3)
141-160:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe rollback is still best-effort, so a failed grant can leave an orphaned user.
Both failure paths call
await userService.deleteUser(result.data.id)and ignore itsResult. If that delete fails, the handler still returns500, but the user row remains created without its initial grant. This create+grant flow needs to be atomic in the service layer, or at minimum the rollback result needs to be checked and surfaced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/users/users.route.ts` around lines 141 - 160, The current handler performs create then grant using grantRole and calls userService.deleteUser(result.data.id) on grant failure but ignores the delete Result; either move the create+grant into an atomic service method (e.g., usersService.createUserWithInitialGrant) that encapsulates transaction/rollback, or at minimum capture and check the Result from userService.deleteUser(...) in both the grantResult failure branch and the catch block and include its failure details in the HTTP response (combine grant error and delete error messages) so any rollback failure is surfaced instead of silently leaving an orphaned user.
421-431:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
rolescrub is checking the wrong authorization rule.The comment says this should drop role changes when the caller lacks membership/grant-management permission, but
UserPolicy.update(...)only checks genericUSER_UPDATE/self access. Any caller who can edit the target user keepsupdates.role, so this branch does not actually protect role/grant changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/users/users.route.ts` around lines 421 - 431, The role-scrubbing branch currently uses UserPolicy.update(...) which only verifies generic user-update rights; replace that with an explicit check for the membership/grant-management permission (e.g. check that currentUser.grants includes the MEMBERSHIP_REVOKE or call the appropriate UserPolicy method for membership revocation) against the target user's org IDs (use the already-computed targetOrgIds from findOrgIdsForUser(targetUser.id)); if the caller lacks that membership/grant-management permission, delete (updates as Record<string, unknown>).role. Ensure you reference currentUser, targetUser, findOrgIdsForUser, targetOrgIds and the specific MEMBERSHIP_REVOKE permission or UserPolicy.membershipRevoke method in the change.
238-238:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSelf-service view/update is blocked by the preflight permission middleware.
UserPolicy.view()andUserPolicy.update()explicitly allow self access, but these routes requireUSER_VIEW/USER_UPDATEbefore that logic runs. That makes the “themselves only” paths unreachable unless those roles are given broad user permissions, which would also widen access elsewhere such asGET /users.Also applies to: 300-304, 343-347
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/users/users.route.ts` at line 238, The routes currently use authenticateUser plus requirePermission(PERMISSIONS.USER_VIEW / PERMISSIONS.USER_UPDATE), which prevents UserPolicy.view() and UserPolicy.update() from allowing “self” access; remove the top-level requirePermission middleware from the routes that should allow self-service (the handlers that invoke UserPolicy.view/update) so that authenticateUser runs but the finer-grained UserPolicy logic can permit access to the current user, while keeping requirePermission on global list/read endpoints (e.g. GET /users) that truly need the broad permission; specifically update the middleware arrays referencing authenticateUser and requirePermission(PERMISSIONS.USER_VIEW) / requirePermission(PERMISSIONS.USER_UPDATE) in users.route.ts so those routes rely on UserPolicy.view/update rather than the preflight requirePermission check.
♻️ Duplicate comments (3)
src/domains/projects/projects.route.ts (1)
115-127:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProject creation still has a partial-write window.
Line 119 awaits
getRoleId()inside the grant payload, and that helper throws when the role row is missing, so the handler can exit before it ever reaches the cleanup branch. Even when cleanup does run, Line 123 ignoresdeleteProject()'sResultdespite this same file treating that call as fallible in the DELETE handler below. Either failure leaves a persisted project without the creator's grant. This still needs one DB transaction around project creation and the initialPROJECT_MANAGERgrant instead of best-effort rollback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/projects/projects.route.ts` around lines 115 - 127, The handler risks partial writes because getRoleId(ROLES.PROJECT_MANAGER) is awaited inside the grant payload (and can throw) and deleteProject() result is ignored; change the flow in projects.route.ts to start a DB transaction that performs createProject and then grantRole within the same transaction (use the transaction-bound service/DB client), prefetch roleId before creating/granting and explicitly handle a missing roleId (return error without creating), and if you must rollback manually ensure you await and check projectService.deleteProject(result.data.id) result; reference grantRole, getRoleId, projectService.deleteProject, and ROLES.PROJECT_MANAGER when making these changes.src/middlewares/authenticate.ts (1)
133-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGrant-load failures should stop authentication, not downgrade the user to
grants: [].Logging helps, but this still turns a repository/DB failure into downstream
FORBIDDENresponses on every protected route, which hides the real incident and breaks authorized users during transient outages. Since grant data is now required for permission checks in this RBAC model, fail the request here instead of authenticating a user with an empty grant list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/middlewares/authenticate.ts` around lines 133 - 143, The code currently continues authentication when findGrantsByUserId(userResult.data.id) fails; instead, when grantsResult.ok is false you must stop authentication and surface the error (do not fall back to grants: []). Update the authenticate middleware so that after calling findGrantsByUserId you log the failure and then reject the request—e.g., throw or return an HTTP 500/503 error (or call the framework’s error helper) with the grantsResult.error—rather than proceeding to c.set('user', ...). Ensure the check references grantsResult and the c.set('user') call so the user is not set when grant loading fails.src/lib/services/permissions/authorize.test.ts (1)
103-161: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCover the remaining
canAssignRoleboundary denies.This suite still misses the cases that were previously called out: assigning a project role with no
projectId, and a project-pinned assign grant trying to assign into a sibling project or another org. Those are the branches most likely to regress into over-broad role-assignment authorization without being noticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/services/permissions/authorize.test.ts` around lines 103 - 161, Add tests for the boundary deny cases in the canAssignRole suite: (1) verify assigning a project-scoped role without providing a projectId is denied (call canAssignRole(superAdmin or orgPM, ROLES.PROJECT_MANAGER, ORG, null) and expect false); (2) verify a project-pinned assign grant cannot assign into a different project or different org — create a user with grant(ORG, PROJ, [PERMISSIONS.ROLE_ASSIGN_PROJECT]) and assert canAssignRole(thatUser, ROLES.PROJECT_MANAGER, ORG, PROJ2) === false and canAssignRole(thatUser, ROLES.PROJECT_MANAGER, ORG2, PROJ) === false to cover sibling-project and cross-org denial branches for canAssignRole.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/scripts/create-user.ts`:
- Around line 23-24: The script currently hard-codes organizationId = 1 and
projectId = 1 when assigning non-super-admin or project-scoped roles; change it
to require explicit scope CLI args instead of pinning IDs: parse and validate
organizationId and/or projectId from args (replace the organizationId/projectId
constants derived from args.slice or args[?] rather than using literal 1), and
when roleNameStr (derived from args.slice(3)) resolves to a scoped role
(anything other than ROLES.SUPER_ADMIN or global roles), enforce that the
corresponding org/project argument is present and exists in the DB before
inserting into user_roles; update any code paths that create user roles (the
blocks that reference organizationId and projectId and the role assignment
logic) to throw a clear error and exit if the required scope args are missing or
invalid.
In `@src/domains/user-roles/user-roles.service.ts`:
- Around line 21-41: The pre-insert existence check around
db.insert(user_roles)...onConflictDoNothing() is racy and must be backed by a
DB-level uniqueness guarantee: add a migration that creates a unique index on
the logical grant scope (e.g. UNIQUE(user_id, role_id, COALESCE(org_id, -1),
COALESCE(project_id, -1)) or equivalent expression index that treats NULLs as a
canonical value) for the user_roles table, keep or remove the app-level
pre-check as desired, and update grantRole (or the code calling
db.insert(user_roles)) to handle unique-constraint errors gracefully (or rely on
onConflictDoNothing after the index exists). Ensure the index uses the actual
column names from user_roles and that any sentinel value used in COALESCE does
not conflict with real ids.
---
Outside diff comments:
In `@src/db/scripts/migrate-to-user-central-rbac.ts`:
- Around line 79-99: The check currently uses grantedIds built from any row in
user_roles, which can hide missing specific PM/PT grants; instead, for each user
in allUsers compute the expected target grant(s) from legacy data (use pmId/ptId
and, for PT, the project membership from project_users via tx) and verify that
tx.select from user_roles contains a matching row with the expected role/scope
(e.g., matching userId and roleId and, when applicable, matching
project/resource id) rather than just checking grantedIds.has(u.id); update the
loop around pmId/ptId and the final validation to query user_roles for the
specific expected grant(s) and push u.id into trueOrphans only if those exact
grants are missing.
In
`@src/domains/chapter-assignments/editor-state/user-chapter-assignment-editor-state.route.ts`:
- Around line 35-38: The OpenAPI route specs for the editor-state endpoints are
still declaring stale 403 responses even though authorization is handled by
requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT) which
returns 404; update the route contract to remove the 403 entries (or replace
them with 404 where appropriate) so the responses match the middleware
behavior—look for the middleware array using authenticateUser and
requireChapterAssignmentAccess in user-chapter-assignment-editor-state.route.ts
and edit the route responses for all editor-state specs (the other similar
blocks referencing the same middleware) to drop 403s and ensure 404 is declared
instead.
In
`@src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts`:
- Around line 32-35: The OpenAPI operation responses are missing the 404 that
requireChapterAssignmentAccess(...) can return; update the response
documentation for both endpoints that use middleware: [authenticateUser,
requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT)] (the
route handlers in chapter-assignments-presence.route) to include a 404 response
entry (with a short description and the existing error schema) alongside the
existing 200/401 entries so the spec matches runtime behavior.
In `@src/domains/users/users.route.ts`:
- Around line 141-160: The current handler performs create then grant using
grantRole and calls userService.deleteUser(result.data.id) on grant failure but
ignores the delete Result; either move the create+grant into an atomic service
method (e.g., usersService.createUserWithInitialGrant) that encapsulates
transaction/rollback, or at minimum capture and check the Result from
userService.deleteUser(...) in both the grantResult failure branch and the catch
block and include its failure details in the HTTP response (combine grant error
and delete error messages) so any rollback failure is surfaced instead of
silently leaving an orphaned user.
- Around line 421-431: The role-scrubbing branch currently uses
UserPolicy.update(...) which only verifies generic user-update rights; replace
that with an explicit check for the membership/grant-management permission (e.g.
check that currentUser.grants includes the MEMBERSHIP_REVOKE or call the
appropriate UserPolicy method for membership revocation) against the target
user's org IDs (use the already-computed targetOrgIds from
findOrgIdsForUser(targetUser.id)); if the caller lacks that
membership/grant-management permission, delete (updates as Record<string,
unknown>).role. Ensure you reference currentUser, targetUser, findOrgIdsForUser,
targetOrgIds and the specific MEMBERSHIP_REVOKE permission or
UserPolicy.membershipRevoke method in the change.
- Line 238: The routes currently use authenticateUser plus
requirePermission(PERMISSIONS.USER_VIEW / PERMISSIONS.USER_UPDATE), which
prevents UserPolicy.view() and UserPolicy.update() from allowing “self” access;
remove the top-level requirePermission middleware from the routes that should
allow self-service (the handlers that invoke UserPolicy.view/update) so that
authenticateUser runs but the finer-grained UserPolicy logic can permit access
to the current user, while keeping requirePermission on global list/read
endpoints (e.g. GET /users) that truly need the broad permission; specifically
update the middleware arrays referencing authenticateUser and
requirePermission(PERMISSIONS.USER_VIEW) /
requirePermission(PERMISSIONS.USER_UPDATE) in users.route.ts so those routes
rely on UserPolicy.view/update rather than the preflight requirePermission
check.
In `@src/lib/services/auth/auth.service.ts`:
- Around line 74-98: The code forwards normalizedInput.orgId/ projectId into
grantRole without normalizing the SUPER_ADMIN scope; ensure that when roleName
=== ROLES.SUPER_ADMIN you pass orgId: null and projectId: null to grantRole
(i.e., compute scopedOrgId/scopedProjectId based on roleName after calling
getRoleId and before calling grantRole) so SUPER_ADMIN grants are created as
global grants consistent with create-user.ts; update the grantRole call to use
these normalized values instead of
normalizedInput.orgId/normalizedInput.projectId.
---
Duplicate comments:
In `@src/domains/projects/projects.route.ts`:
- Around line 115-127: The handler risks partial writes because
getRoleId(ROLES.PROJECT_MANAGER) is awaited inside the grant payload (and can
throw) and deleteProject() result is ignored; change the flow in
projects.route.ts to start a DB transaction that performs createProject and then
grantRole within the same transaction (use the transaction-bound service/DB
client), prefetch roleId before creating/granting and explicitly handle a
missing roleId (return error without creating), and if you must rollback
manually ensure you await and check projectService.deleteProject(result.data.id)
result; reference grantRole, getRoleId, projectService.deleteProject, and
ROLES.PROJECT_MANAGER when making these changes.
In `@src/lib/services/permissions/authorize.test.ts`:
- Around line 103-161: Add tests for the boundary deny cases in the
canAssignRole suite: (1) verify assigning a project-scoped role without
providing a projectId is denied (call canAssignRole(superAdmin or orgPM,
ROLES.PROJECT_MANAGER, ORG, null) and expect false); (2) verify a project-pinned
assign grant cannot assign into a different project or different org — create a
user with grant(ORG, PROJ, [PERMISSIONS.ROLE_ASSIGN_PROJECT]) and assert
canAssignRole(thatUser, ROLES.PROJECT_MANAGER, ORG, PROJ2) === false and
canAssignRole(thatUser, ROLES.PROJECT_MANAGER, ORG2, PROJ) === false to cover
sibling-project and cross-org denial branches for canAssignRole.
In `@src/middlewares/authenticate.ts`:
- Around line 133-143: The code currently continues authentication when
findGrantsByUserId(userResult.data.id) fails; instead, when grantsResult.ok is
false you must stop authentication and surface the error (do not fall back to
grants: []). Update the authenticate middleware so that after calling
findGrantsByUserId you log the failure and then reject the request—e.g., throw
or return an HTTP 500/503 error (or call the framework’s error helper) with the
grantsResult.error—rather than proceeding to c.set('user', ...). Ensure the
check references grantsResult and the c.set('user') call so the user is not set
when grant loading fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 08ebe360-f084-4dae-af14-32434de3a10e
📒 Files selected for processing (16)
src/db/scripts/create-user.tssrc/db/scripts/migrate-to-user-central-rbac.tssrc/domains/chapter-assignments/chapter-assignment-auth.middleware.tssrc/domains/chapter-assignments/chapter-assignments.policy.tssrc/domains/chapter-assignments/chapter-assignments.route.tssrc/domains/chapter-assignments/editor-state/user-chapter-assignment-editor-state.route.tssrc/domains/chapter-assignments/presence/chapter-assignments-presence.route.tssrc/domains/projects/projects.repository.tssrc/domains/projects/projects.route.tssrc/domains/user-roles/user-roles.repository.tssrc/domains/user-roles/user-roles.service.tssrc/domains/users/users.route.tssrc/lib/services/auth/auth.service.tssrc/lib/services/permissions/authorize.test.tssrc/middlewares/authenticate.tssrc/middlewares/role-auth.ts
💤 Files with no reviewable changes (1)
- src/domains/chapter-assignments/chapter-assignments.route.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/lib/services/permissions/authorize.ts (1)
24-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict org-scoped checks to org-wide grants.
isGrantApplicable()currently treats any grant in the org as applicable whenscope.projectIdis absent. That lets a project-pinned grant satisfy org-scoped permissions, which collapses project scope into org scope and is already being locked in bysrc/lib/services/permissions/authorize.test.tsLines 44-47. Org-scoped authorization should only consider grants wheregrant.projectId === null.Suggested fix
if (orgId !== null) { - return grant.orgId === orgId; + return grant.orgId === orgId && grant.projectId === null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/services/permissions/authorize.ts` around lines 24 - 25, The org-scoped check in isGrantApplicable incorrectly treats any grant from the same org as applicable when scope.projectId is null; change the logic so that when orgId (scope.orgId) is being checked you only accept grants that are org-wide by also verifying grant.projectId === null. Update the branch that returns grant.orgId === orgId to require both grant.orgId === orgId && grant.projectId === null (refer to isGrantApplicable, grant, scope.projectId, and scope.orgId to locate the code).src/db/scripts/migrate-to-user-central-rbac.ts (1)
36-42: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffRaw SQL for legacy structures is fragile but unavoidable in this migration context.
The script uses
sql<number>backticks to read from legacyusers.organization,users.role, and theproject_userstable (lines 39-40, 61, 100-101). This bypasses Drizzle's type checking because these structures no longer exist inschema.ts. If the column or table names are incorrect, the script will fail at runtime with a generic database error rather than a compile-time type error.This approach is acceptable given that the script must bridge the old and new schema states, but operators should be aware that any typos or schema drift will only surface when the script runs.
Also applies to: 59-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/scripts/migrate-to-user-central-rbac.ts` around lines 36 - 42, The migration uses raw sql<number> backticks for legacy columns (e.g., in the tx.select that sets allUsers with users.id, sql<number>`organization`, sql<number>`role`) and similar raw references to project_users; wrap each raw SQL usage in a small runtime validation and error wrapper: before executing the main migration query call a helper (e.g., validateColumnsExist) that queries information_schema for the referenced table/column names, and if validation fails throw a clear error mentioning the exact table/column string; also catch errors around the tx.select/transaction and rethrow or log with contextual details (identify the failing raw fragment like "users.organization" or "project_users.member_role") so any typo or schema drift surfaces with a descriptive runtime message rather than a generic DB error.src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts (1)
32-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing 400 response to both presence routes.
requireChapterAssignmentAccess()can returnBAD_REQUESTbefore the handler whenchapterAssignmentIdis missing or not numeric, but both route specs only document 401/404/500. The OpenAPI contract should include that runtime response.Suggested fix
responses: { [HttpStatusCodes.OK]: jsonContent(presenceResponseSchema, 'Presence registered successfully'), + [HttpStatusCodes.BAD_REQUEST]: jsonContent( + createMessageObjectSchema(HttpStatusPhrases.BAD_REQUEST), + 'Invalid chapter assignment ID' + ), [HttpStatusCodes.UNAUTHORIZED]: jsonContent( createMessageObjectSchema('Unauthorized'), 'Authentication required' @@ responses: { [HttpStatusCodes.NO_CONTENT]: { description: 'Presence removed successfully' }, + [HttpStatusCodes.BAD_REQUEST]: jsonContent( + createMessageObjectSchema(HttpStatusPhrases.BAD_REQUEST), + 'Invalid chapter assignment ID' + ), [HttpStatusCodes.UNAUTHORIZED]: jsonContent( createMessageObjectSchema('Unauthorized'), 'Authentication required'Also applies to: 61-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts` around lines 32 - 45, Add a documented 400 response to both presence route specs to reflect that requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT) may short-circuit with BAD_REQUEST when chapterAssignmentId is missing or non-numeric; update the responses object (alongside existing OK/UNAUTHORIZED/NOT_FOUND) to include [HttpStatusCodes.BAD_REQUEST]: jsonContent(createMessageObjectSchema(HttpStatusPhrases.BAD_REQUEST), 'Bad request') so the OpenAPI contract for the routes using authenticateUser, requireChapterAssignmentAccess, chapterAssignmentIdParam and presenceResponseSchema accurately reflects runtime behavior.src/domains/users/users.route.ts (3)
293-297:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGlobal-scope users become unreachable through these routes.
These handlers derive the target from
findOrgIdsForUser(...), but that repository dropsnullorg scopes while the new grant model storesSUPER_ADMINwithorgId: null. For those accountstargetOrgIdsbecomes[], soUserPolicy.view/updateandrequireUserAccess()deny every non-self request. Other admins will not be able to look up or update super-admin users by email or id unless you carry global-scope target information into policy evaluation instead of reducing the target toorgIdsonly.Also applies to: 309-310, 348-349
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/users/users.route.ts` around lines 293 - 297, The policy check reduces the target to orgIds from findOrgIdsForUser which drops global-scope entries (SUPER_ADMIN with orgId:null) causing UserPolicy.view/update and requireUserAccess to deny non-self requests; change the code that builds the policy target so it includes global-scope information (e.g., preserve a flag or include null in targetOrgIds when the user has a SUPER_ADMIN grant) before calling UserPolicy.view/update and requireUserAccess so global users are evaluated correctly (update usage sites referencing targetOrgIds in users.route.ts — the calls to findOrgIdsForUser, UserPolicy.view/update, requireUserAccess — to pass the augmented target that represents orgId:null/global scope).
140-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
POST /usersstill over-grants project roles whenprojectIdis absent.This path falls back to
ROLES.PROJECT_TRANSLATORand then inserts the grant withprojectId: requestData.projectId ?? null. That recreates the same org-wide access bug the invite flow now guards against: a request meant for one project translator can become access across every project in the org. Reject project-scoped roles without a concreteprojectId, or map them to an org-scoped role before callinggrantRole.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/users/users.route.ts` around lines 140 - 145, The POST /users handler currently defaults roleName to ROLES.PROJECT_TRANSLATOR and calls getRoleId/grantRole with projectId: requestData.projectId ?? null, which creates org-wide grants when projectId is missing; update the logic around roleName/roleId before calling getRoleId and grantRole: detect project-scoped roles (e.g., ROLES.PROJECT_TRANSLATOR or whatever project-scoped constants your ROLES set contains) and if requestData.projectId is absent either return a 400/validation error or remap to the appropriate org-scoped role (e.g., ROLES.ORG_TRANSLATOR) before calling getRoleId and grantRole; adjust any validation/error path in the POST /users handler where roleName, getRoleId, and grantRole are used so no project-scoped role is granted with a null projectId.
259-273:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe documented error responses no longer match runtime behavior.
After switching these routes to inline
UserPolicy.view(...)/requireUserAccess(...), authorization denials now surface as404, not403. Keeping403in the OpenAPI responses advertises statuses these handlers no longer emit, which will drift generated clients and API docs away from the actual contract.Also applies to: 318-331, 358-379
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/users/users.route.ts` around lines 259 - 273, The OpenAPI responses still list HttpStatusCodes.FORBIDDEN (403) for routes that now use inline UserPolicy.view(...) / requireUserAccess(...) and therefore return 404 on authorization failures; update the response maps in users.route.ts (the blocks currently listing HttpStatusCodes.FORBIDDEN) to remove or replace the 403 entry with HttpStatusCodes.NOT_FOUND and adjust the message to reflect "User not found" (or consolidate with the existing NOT_FOUND entry) for each affected response block (the ones around the current UserPolicy.view/requireUserAccess usages), so the documented responses match runtime behavior.
♻️ Duplicate comments (1)
src/db/scripts/create-user.ts (1)
104-123: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winValidate that
orgIdandprojectIdexist in the database.The script validates that scope arguments are provided and numeric, but does not verify they reference existing rows in
organizationsorprojects. If an invalid ID is supplied, the insert at line 125 will fail with a foreign-key constraint error rather than a clear validation message.Consider adding existence checks before the transaction, similar to the role-name resolution at lines 56-65.
♻️ Suggested validation queries
if (!role) { console.error(`Role '${roleNameStr}' not found in database.`); process.exit(1); } + + // Validate scope IDs if provided + if (orgIdArg !== undefined) { + const [org] = await db + .select({ id: schema.organizations.id }) + .from(schema.organizations) + .where(eq(schema.organizations.id, orgIdArg)) + .limit(1); + if (!org) { + console.error(`Organization with id ${orgIdArg} not found.`); + process.exit(1); + } + } + + if (projectIdArg !== undefined) { + const [project] = await db + .select({ id: schema.projects.id }) + .from(schema.projects) + .where(eq(schema.projects.id, projectIdArg)) + .limit(1); + if (!project) { + console.error(`Project with id ${projectIdArg} not found.`); + process.exit(1); + } + } const authUserId = crypto.randomUUID();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/scripts/create-user.ts` around lines 104 - 123, Add explicit existence checks for orgIdArg and projectIdArg before assigning grantOrgId/grantProjectId: when role.name !== ROLES.SUPER_ADMIN, query the organizations table to ensure orgIdArg exists (throw a clear Error if not) and when role requires project scope (PROJECT_TRANSLATOR/PROJECT_OBSERVER/PROJECT_MANAGER) query the projects table to ensure projectIdArg exists (throw a clear Error if not). Use the same DB client used for role-name resolution (the code around role lookup) and perform these validations prior to the transaction that inserts grants so you return a descriptive validation error instead of a foreign-key DB error; then set grantOrgId/grantProjectId from the validated orgIdArg/projectIdArg.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/schema.ts`:
- Around line 475-480: The COALESCE(..., -1) uniqueness sentinel in
uniqueIndex('uq_user_role_grant') risks collision if a row with org_id or
project_id = -1 is ever inserted; either convert this to partial unique indexes
(e.g., unique index on (user_id, role_id) WHERE org_id IS NULL and similarly for
project_id) if your SQL/Drizzle setup supports WHERE clauses, or keep the
COALESCE approach but add a DB CHECK constraint preventing negative IDs (org_id
>= 0 AND project_id >= 0) and document the sentinel choice in the schema comment
so callers never insert -1; update the code around
uniqueIndex('uq_user_role_grant') and the table definitions (table.orgId,
table.projectId, table.userId, table.roleId) accordingly.
In `@src/domains/projects/projects.route.ts`:
- Around line 134-140: The response always claims "Rolled back" even when
projectService.deleteProject returned an error; update the message construction
in the route that handles project creation (the block using result.data.id,
deleteResult and projectService.deleteProject) to conditionally state the
rollback outcome: if deleteResult.ok include "Rolled back." otherwise state that
rollback failed and the project may still exist (including
deleteResult.error.message), so the client is not misled about the persisted
state.
In `@src/lib/services/auth/auth.service.ts`:
- Around line 91-103: The code calls grantRole(...) but only catches thrown
exceptions; update the flow in the block that computes
roleId/scopedOrgId/scopedProjectId and calls grantRole to handle Result-style
failures: after awaiting grantRole({ userId: dbResult.data.id, orgId:
scopedOrgId, projectId: scopedProjectId, roleId, createdBy:
dbResult.data.createdBy ?? null }), check the returned object for ok === true
and, if not, abort (throw or return an error) before proceeding to send the
magic link—this ensures failures from grantRole() (which returns Result) are
handled the same way as exceptions and prevents creating a user without
permissions.
---
Outside diff comments:
In `@src/db/scripts/migrate-to-user-central-rbac.ts`:
- Around line 36-42: The migration uses raw sql<number> backticks for legacy
columns (e.g., in the tx.select that sets allUsers with users.id,
sql<number>`organization`, sql<number>`role`) and similar raw references to
project_users; wrap each raw SQL usage in a small runtime validation and error
wrapper: before executing the main migration query call a helper (e.g.,
validateColumnsExist) that queries information_schema for the referenced
table/column names, and if validation fails throw a clear error mentioning the
exact table/column string; also catch errors around the tx.select/transaction
and rethrow or log with contextual details (identify the failing raw fragment
like "users.organization" or "project_users.member_role") so any typo or schema
drift surfaces with a descriptive runtime message rather than a generic DB
error.
In
`@src/domains/chapter-assignments/presence/chapter-assignments-presence.route.ts`:
- Around line 32-45: Add a documented 400 response to both presence route specs
to reflect that
requireChapterAssignmentAccess(CHAPTER_ASSIGNMENT_ACTIONS.IS_PARTICIPANT) may
short-circuit with BAD_REQUEST when chapterAssignmentId is missing or
non-numeric; update the responses object (alongside existing
OK/UNAUTHORIZED/NOT_FOUND) to include [HttpStatusCodes.BAD_REQUEST]:
jsonContent(createMessageObjectSchema(HttpStatusPhrases.BAD_REQUEST), 'Bad
request') so the OpenAPI contract for the routes using authenticateUser,
requireChapterAssignmentAccess, chapterAssignmentIdParam and
presenceResponseSchema accurately reflects runtime behavior.
In `@src/domains/users/users.route.ts`:
- Around line 293-297: The policy check reduces the target to orgIds from
findOrgIdsForUser which drops global-scope entries (SUPER_ADMIN with orgId:null)
causing UserPolicy.view/update and requireUserAccess to deny non-self requests;
change the code that builds the policy target so it includes global-scope
information (e.g., preserve a flag or include null in targetOrgIds when the user
has a SUPER_ADMIN grant) before calling UserPolicy.view/update and
requireUserAccess so global users are evaluated correctly (update usage sites
referencing targetOrgIds in users.route.ts — the calls to findOrgIdsForUser,
UserPolicy.view/update, requireUserAccess — to pass the augmented target that
represents orgId:null/global scope).
- Around line 140-145: The POST /users handler currently defaults roleName to
ROLES.PROJECT_TRANSLATOR and calls getRoleId/grantRole with projectId:
requestData.projectId ?? null, which creates org-wide grants when projectId is
missing; update the logic around roleName/roleId before calling getRoleId and
grantRole: detect project-scoped roles (e.g., ROLES.PROJECT_TRANSLATOR or
whatever project-scoped constants your ROLES set contains) and if
requestData.projectId is absent either return a 400/validation error or remap to
the appropriate org-scoped role (e.g., ROLES.ORG_TRANSLATOR) before calling
getRoleId and grantRole; adjust any validation/error path in the POST /users
handler where roleName, getRoleId, and grantRole are used so no project-scoped
role is granted with a null projectId.
- Around line 259-273: The OpenAPI responses still list
HttpStatusCodes.FORBIDDEN (403) for routes that now use inline
UserPolicy.view(...) / requireUserAccess(...) and therefore return 404 on
authorization failures; update the response maps in users.route.ts (the blocks
currently listing HttpStatusCodes.FORBIDDEN) to remove or replace the 403 entry
with HttpStatusCodes.NOT_FOUND and adjust the message to reflect "User not
found" (or consolidate with the existing NOT_FOUND entry) for each affected
response block (the ones around the current UserPolicy.view/requireUserAccess
usages), so the documented responses match runtime behavior.
In `@src/lib/services/permissions/authorize.ts`:
- Around line 24-25: The org-scoped check in isGrantApplicable incorrectly
treats any grant from the same org as applicable when scope.projectId is null;
change the logic so that when orgId (scope.orgId) is being checked you only
accept grants that are org-wide by also verifying grant.projectId === null.
Update the branch that returns grant.orgId === orgId to require both grant.orgId
=== orgId && grant.projectId === null (refer to isGrantApplicable, grant,
scope.projectId, and scope.orgId to locate the code).
---
Duplicate comments:
In `@src/db/scripts/create-user.ts`:
- Around line 104-123: Add explicit existence checks for orgIdArg and
projectIdArg before assigning grantOrgId/grantProjectId: when role.name !==
ROLES.SUPER_ADMIN, query the organizations table to ensure orgIdArg exists
(throw a clear Error if not) and when role requires project scope
(PROJECT_TRANSLATOR/PROJECT_OBSERVER/PROJECT_MANAGER) query the projects table
to ensure projectIdArg exists (throw a clear Error if not). Use the same DB
client used for role-name resolution (the code around role lookup) and perform
these validations prior to the transaction that inserts grants so you return a
descriptive validation error instead of a foreign-key DB error; then set
grantOrgId/grantProjectId from the validated orgIdArg/projectIdArg.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e69fc41c-725d-4578-8840-9a391fbb6f2a
📒 Files selected for processing (15)
src/db/migrations/0014_add_user_roles_unique_index.sqlsrc/db/migrations/meta/0014_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/db/scripts/create-user.tssrc/db/scripts/migrate-to-user-central-rbac.tssrc/domains/chapter-assignments/editor-state/user-chapter-assignment-editor-state.route.tssrc/domains/chapter-assignments/presence/chapter-assignments-presence.route.tssrc/domains/projects/projects.route.tssrc/domains/user-roles/user-roles.service.tssrc/domains/users/users.route.tssrc/lib/services/auth/auth.service.tssrc/lib/services/permissions/authorize.test.tssrc/lib/services/permissions/authorize.tssrc/middlewares/authenticate.ts
💤 Files with no reviewable changes (1)
- src/domains/chapter-assignments/editor-state/user-chapter-assignment-editor-state.route.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/services/permissions/authorize.ts (1)
8-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the docblock to match the tightened org-scope behavior.
Lines 12-13 still say an org-scoped check accepts grants pinned to any project in the org, but Line 25 now requires
grant.projectId === null. Leaving the old rule here is risky in auth code because it documents the opposite behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/services/permissions/authorize.ts` around lines 8 - 14, Update the docblock in authorize.ts to reflect the tightened org-scope behavior: change the org-scoped rule to state that an org-scoped request only matches grants that are org-wide (i.e., grant.projectId === null) and not grants pinned to projects within the org; keep the other bullets as-is so the comment matches the implemented check (referencing grant.projectId === null).
♻️ Duplicate comments (2)
src/lib/services/auth/auth.service.ts (1)
76-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
PROJECT_MANAGERinvites without aprojectId.This guard only covers translator and observer.
PROJECT_MANAGERstill falls through withprojectId: null, which makes the grant org-scoped instead of project-scoped. The non-invite create path already treatsPROJECT_MANAGERas a project role, so the two user-creation flows now diverge.Suggested fix
if ( !normalizedInput.projectId && - [ROLES.PROJECT_TRANSLATOR, ROLES.PROJECT_OBSERVER].includes(roleName as any) + [ROLES.PROJECT_MANAGER, ROLES.PROJECT_TRANSLATOR, ROLES.PROJECT_OBSERVER].includes( + roleName as any + ) ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/services/auth/auth.service.ts` around lines 76 - 79, The invite-creation guard currently rejects only PROJECT_TRANSLATOR and PROJECT_OBSERVER when normalizedInput.projectId is missing; add PROJECT_MANAGER to that check so invites for PROJECT_MANAGER without a projectId are also rejected (update the condition that references normalizedInput.projectId and the role array including ROLES.PROJECT_TRANSLATOR and ROLES.PROJECT_OBSERVER to include ROLES.PROJECT_MANAGER) to ensure PROJECT_MANAGER invites remain project-scoped like the non-invite creation path.src/domains/users/user.policy.ts (1)
32-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
delete()still doesn't enforce the stated global-only contract.The comment says full account deletion is SuperAdmin-only, but this branch still authorizes through
target.orgIds.some(...). That means any future org-scopeduser:deletegrant would silently widen a hard-delete route beyond the intended scope. Require the global grant explicitly here instead of checking org scopes.Suggested fix
delete(user: AppPolicyUser, target: PolicyTargetUser): boolean { // Full account deletion is SuperAdmin-only (no role is seeded user:delete). Deferred otherwise. - return target.orgIds.some((orgId) => authorize(user, PERMISSIONS.USER_DELETE, { orgId })); + return authorize(user, PERMISSIONS.USER_DELETE, { orgId: null }); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/users/user.policy.ts` around lines 32 - 34, The delete method currently authorizes via org scopes (target.orgIds.some(...)) which contradicts the "SuperAdmin-only/global-only" comment; change the logic in delete(user: AppPolicyUser, target: PolicyTargetUser) to require the global grant explicitly instead of iterating orgIds — call authorize(user, PERMISSIONS.USER_DELETE, { global: true }) (or otherwise check the global-only grant) and return that result so only a global/user:delete grant allows a full account deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domains/users/users.route.ts`:
- Around line 147-152: The role-to-scope validation is happening after
userService.createUser succeeds (you can see isProjectRole and
requestData.projectId used and the subsequent
userService.deleteUser(result.data.id) rollback), which risks leaving a created
account if deleteUser fails; move the projectId validation before calling the
user creation logic (i.e., validate isProjectRole && requestData.projectId prior
to userService.createUser), or if you must keep post-create rollback, change the
flow so a failed rollback surfaces as an internal error (return 500 and include
the rollback failure) instead of returning 400 while ignoring deleteUser errors;
update the code paths around createUser, deleteUser(result.data.id), and the
response handling to enforce this fail-closed behavior.
---
Outside diff comments:
In `@src/lib/services/permissions/authorize.ts`:
- Around line 8-14: Update the docblock in authorize.ts to reflect the tightened
org-scope behavior: change the org-scoped rule to state that an org-scoped
request only matches grants that are org-wide (i.e., grant.projectId === null)
and not grants pinned to projects within the org; keep the other bullets as-is
so the comment matches the implemented check (referencing grant.projectId ===
null).
---
Duplicate comments:
In `@src/domains/users/user.policy.ts`:
- Around line 32-34: The delete method currently authorizes via org scopes
(target.orgIds.some(...)) which contradicts the "SuperAdmin-only/global-only"
comment; change the logic in delete(user: AppPolicyUser, target:
PolicyTargetUser) to require the global grant explicitly instead of iterating
orgIds — call authorize(user, PERMISSIONS.USER_DELETE, { global: true }) (or
otherwise check the global-only grant) and return that result so only a
global/user:delete grant allows a full account deletion.
In `@src/lib/services/auth/auth.service.ts`:
- Around line 76-79: The invite-creation guard currently rejects only
PROJECT_TRANSLATOR and PROJECT_OBSERVER when normalizedInput.projectId is
missing; add PROJECT_MANAGER to that check so invites for PROJECT_MANAGER
without a projectId are also rejected (update the condition that references
normalizedInput.projectId and the role array including ROLES.PROJECT_TRANSLATOR
and ROLES.PROJECT_OBSERVER to include ROLES.PROJECT_MANAGER) to ensure
PROJECT_MANAGER invites remain project-scoped like the non-invite creation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2827af39-a200-4a43-b965-a9dde16073c0
📒 Files selected for processing (6)
src/domains/chapter-assignments/presence/chapter-assignments-presence.route.tssrc/domains/users/user.policy.tssrc/domains/users/users.route.tssrc/lib/services/auth/auth.service.tssrc/lib/services/permissions/authorize.test.tssrc/lib/services/permissions/authorize.ts
kaseywright
reviewed
Jun 14, 2026
kaseywright
left a comment
kaseywright
left a comment
Contributor
There was a problem hiding this comment.
This looks good. We should wait to merge until we have the UI changes as well. This way, we can do a deploy with the current changes in QA and not have to wait until the UI changes land and pass QA.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#176
Summary by CodeRabbit
New Features
Chores
Tests