feat(usfm): scope export endpoints to the caller's project/org#201
feat(usfm): scope export endpoints to the caller's project/org#201henrique221 wants to merge 1 commit into
Conversation
After #189 the USFM routes required authentication but no project/org check, so any logged-in user could export another organization's translated content by guessing a projectUnitId. - Add requireProjectUnitAccess middleware: resolves projectUnitId -> project -> org and applies ProjectPolicy.read, mirroring translated-verse-auth.middleware.ts. Returns 404 (not 403) on denial to avoid enumeration, and stashes the resolved project on the context. - Apply it to the three /project-units/{projectUnitId}/usfm* routes (books, sync export, async export). /jobs and /downloads are async-only; binding them to an owner is tracked in #196. - Document each route's 404 as { message } (createMessageObjectSchema), matching what the middleware/global handler returns. - Sync handler now reads the project name from the context instead of re-querying via getProjectName, removing a redundant query and its duplicate { error } 404. Closes #198.
📝 WalkthroughWalkthroughA new ChangesUSFM project-scoped authorization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
|
This PR is stacked on #189 — its base is Do not merge before #189 — doing so merges #198 into the #189 branch and bundles the two separately-reviewed changes. Since this repo squash-merges and #200 has already landed on
Marked as draft until #189 merges; the rebase + retarget will be handled then. The diff itself (the new |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/domains/usfm/usfm-auth.middleware.ts`:
- Around line 45-53: The resolveIsProjectMember call is being made
unconditionally, but the ProjectPolicy.read check only uses the isProjectMember
value for TRANSLATOR roles and ignores it for PROJECT_MANAGER roles. To optimize
the hot export path, add a conditional check on user.roleName before calling
resolveIsProjectMember. Only invoke resolveIsProjectMember when the user role is
TRANSLATOR, and for PROJECT_MANAGER roles, skip the database call by either
passing a default value or using a ternary operator to conditionally set
isProjectMember based on the role.
🪄 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: 7b92048d-6981-4ef1-93a8-0f94693d04d7
📒 Files selected for processing (2)
src/domains/usfm/usfm-auth.middleware.tssrc/domains/usfm/usfm.route.ts
| const isProjectMember = await resolveIsProjectMember( | ||
| unitResult.data.projectId, | ||
| user.id, | ||
| user.roleName | ||
| ); | ||
|
|
||
| if (!ProjectPolicy.read(policyUser, projectResult.data, isProjectMember)) { | ||
| return c.json({ message: 'Project not found' }, HttpStatusCodes.NOT_FOUND); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any consumers of projectAuthContext / isProjectMember downstream
rg -nP "projectAuthContext|isProjectMember" -C3 --type=ts -g '!**/*.test.ts'Repository: eten-tech-foundation/fluent-api
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Search without -P flag for simpler regex
rg "projectAuthContext" --type=ts -g '!**/*.test.ts' -nRepository: eten-tech-foundation/fluent-api
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Search for isProjectMember usage
rg "isProjectMember" --type=ts -g '!**/*.test.ts' -nRepository: eten-tech-foundation/fluent-api
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Check the actual middleware file mentioned in the review
cat -n src/domains/usfm/usfm-auth.middleware.tsRepository: eten-tech-foundation/fluent-api
Length of output: 2808
🏁 Script executed:
#!/bin/bash
# Find and inspect ProjectPolicy implementation
fd -t f "ProjectPolicy" -e ts | head -20Repository: eten-tech-foundation/fluent-api
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Search for projectAuthContext without glob exclusion
rg "projectAuthContext" --type=ts -n -A 2Repository: eten-tech-foundation/fluent-api
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Search for isProjectMember without glob exclusion
rg "isProjectMember" --type=ts -n -A 2Repository: eten-tech-foundation/fluent-api
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Find and inspect ProjectPolicy implementation
fd -t f -e ts | xargs grep -l "class ProjectPolicy\|export.*ProjectPolicy" | head -5Repository: eten-tech-foundation/fluent-api
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Look for the project policy file directly
fd "project.policy" -e tsRepository: eten-tech-foundation/fluent-api
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Read the ProjectPolicy file
cat -n src/domains/projects/project.policy.tsRepository: eten-tech-foundation/fluent-api
Length of output: 2997
🏁 Script executed:
#!/bin/bash
# Try grep instead of rg for finding downstream usage
grep -rn "projectAuthContext" src --include="*.ts" | head -20Repository: eten-tech-foundation/fluent-api
Length of output: 711
🏁 Script executed:
#!/bin/bash
# Search for handlers that might use the middleware
grep -rn "requireProjectUnitAccess\|projectAuthContext" src --include="*.ts" | head -30Repository: eten-tech-foundation/fluent-api
Length of output: 1367
🏁 Script executed:
#!/bin/bash
# Search for where handlers actually access/read projectAuthContext
grep -rn "\.get.*projectAuthContext\|get('projectAuthContext')" src --include="*.ts"Repository: eten-tech-foundation/fluent-api
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Check usfm.route.ts handlers to see if they consume projectAuthContext
cat -n src/domains/usfm/usfm.route.tsRepository: eten-tech-foundation/fluent-api
Length of output: 18351
🏁 Script executed:
#!/bin/bash
# Check the other middleware files that set projectAuthContext to see if they're used
grep -rn "\.get('projectAuthContext')\|\.get(\"projectAuthContext\")" src --include="*.ts" -B 2 -A 2Repository: eten-tech-foundation/fluent-api
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Check if there are any queue workers or async handlers that might use projectAuthContext
grep -rn "projectAuthContext" src/lib/queue src/jobs src/workers --include="*.ts" 2>/dev/null || echo "No matches in queue/jobs/workers"Repository: eten-tech-foundation/fluent-api
Length of output: 202
Short-circuit resolveIsProjectMember for project managers to avoid unnecessary database calls.
ProjectPolicy.read only examines organization for PROJECT_MANAGER and uses isAssignedToProject only for TRANSLATOR. Since managers are likely the primary exporters, this adds an unneeded database round-trip on the hot export path.
The value is stored in projectAuthContext but is never read by any downstream handler, making the optimization straightforward: check the user role before calling resolveIsProjectMember.
🤖 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-auth.middleware.ts` around lines 45 - 53, The
resolveIsProjectMember call is being made unconditionally, but the
ProjectPolicy.read check only uses the isProjectMember value for TRANSLATOR
roles and ignores it for PROJECT_MANAGER roles. To optimize the hot export path,
add a conditional check on user.roleName before calling resolveIsProjectMember.
Only invoke resolveIsProjectMember when the user role is TRANSLATOR, and for
PROJECT_MANAGER roles, skip the database call by either passing a default value
or using a ternary operator to conditionally set isProjectMember based on the
role.
1 participant
Closes #198.
Summary
After #189 the USFM routes require authentication but perform no project/org check — the export pulls per-project translated content (
usfm.service.tsgetProjectBooks+getBookVerses), so any logged-in user from another org could export a project they don't belong to by guessing aprojectUnitId.requireProjectUnitAccessmiddleware (usfm-auth.middleware.ts): resolvesprojectUnitId→ project → org and appliesProjectPolicy.read, mirroringtranslated-verse-auth.middleware.ts. Returns 404 (not 403) on denial to avoid project-unit enumeration, and stashes the resolved project on the context./project-units/{projectUnitId}/usfm*routes (books, sync export, async export)./jobsand/downloadsare async-only — owner-binding is tracked in Async USFM export pipeline hardening (pre-enablement) #196.{ message }(createMessageObjectSchema), matching what the middleware/global handler returns.getProjectName, removing a redundant query and a duplicate{ error }404.Behaviour
Testing note
Mirrors the existing record-level auth middlewares (translated-verse-auth, project-auth), which aren't unit-tested in this repo, so no new test file — the logic is identical to the proven translated-verses read path.
npm run typechecknpm run lintnpm run test— 80/80 passSummary by CodeRabbit