refactor(permissions): extract RBAC matrix + resolver to @shelf/permissions - #2757
refactor(permissions): extract RBAC matrix + resolver to @shelf/permissions#2757carlosvirreira wants to merge 5 commits into
Conversation
…ssions Companion and webapp previously had two sources of permission truth: the webapp's Role2PermissionMap (+ an ADMIN/OWNER allow-all short-circuit that lived only in hasPermission) and a hand-copied client mirror in the companion. Raised by CTO review on PR #2753. - new packages/permissions: PermissionAction/PermissionEntity enums, the role matrix, and roleHasPermission() — the pure resolver INCLUDING the ADMIN/OWNER short-circuit; dependency-free so Metro can bundle it - webapp permission.data.ts becomes a re-export shim (~250 import sites unchanged) with a compile-time Prisma<->package role parity guard: role drift now fails typecheck instead of surfacing at runtime - webapp validator delegates resolution to the shared resolver; keeps the UserOrganization lookup and ShelfError semantics; unknown entities now safe-deny instead of throwing a wrapped error - companion lib/permissions.ts drops its hand-copied matrix and delegates to the same resolver — web and mobile can no longer drift - vite ssr.noExternal + workspace deps wired; Metro export smoke-tested Tests: 7 new validator behavior tests (short-circuit incl. the qr:update case, matrix grants/denies, membership 403); webapp:validate green (3651 tests); companion tsc green; expo export produces a full Hermes bundle.
… CTO pass - qr.link-asset delegates to relinkAssetQrCode (the web asset-detail relink service): every QR-state guard, the asset's system note, and inline claiming of unclaimed codes now come from the shared service instead of ~60 lines of route-level re-implementation. Contract change (deliberate): the 400 reason "unclaimed" response no longer exists — an unclaimed code is claimed as part of the link. - companion: scanner's Link Existing action navigates directly (no pre-claim POST; an abandoned picker leaves the label unclaimed for anyone), the picker's claim-recovery retry is deleted as dead code, and the picker gains the scanner's workspace-switch guard (leave instead of listing the new org's assets against the old org's code). - claimQrCode: pass through our own ShelfErrors in the outer catch so user-facing guard messages (already-claimed, not-found) reach clients instead of the generic "Failed to claim qr code" (ShelfError copies status/title from a cause but not message). - rate limiting: new per-user "write" bucket (60/min) on claim and link-asset — irreversible single-row writes; a full label sheet fits, scripted hammering does not. - rules: align cross-app-mirrors description with its companion-only glob (CodeRabbit). Tests updated: link-asset suite now asserts delegation + service-error mapping; claim suites assert the real guard messages. 26/26 affected tests green; webapp + companion typecheck clean.
🩺 React Doctor — webapp✅ No new findings on the files changed by this PR. Run locally with |
🩺 React Doctor — companionFindings on the files changed by this PR:
|
WalkthroughThis PR adds the shared ChangesShared permissions package
QR claim and linking workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/companion/app/(tabs)/scanner.tsx (1)
528-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale JSDoc:
claimQrAndProceedno longer dispatches to a link destination.The docstring above still describes claiming "then continue into the chosen follow-up: create a new asset ... or pick an existing asset to link" and "Mirrors the web claim → new / claim → link flow," but the function signature dropped its follow-up parameter and now always navigates to
/(tabs)/assets/new(lines 612-615). The "link" path no longer goes through this function at all (see the secondary action at 744-750, which claims inline via the server instead). Update the JSDoc to reflect that this helper is now claim-then-create-only.📝 Suggested doc fix
/** - * Claim an unclaimed QR into the current workspace, then continue into the - * chosen follow-up: create a new asset (the create form links the QR on - * submit) or pick an existing asset to link. Mirrors the web - * claim → new / claim → link flow; mobile always claims into the ACTIVE - * workspace — the server refuses any body-supplied org. + * Claim an unclaimed QR into the current workspace, then navigate to the + * create-asset form (which links the QR on submit). Mobile always claims + * into the ACTIVE workspace — the server refuses any body-supplied org. + * The "link existing asset" path no longer calls this helper: it navigates + * straight to the link picker, which claims inline via the server. * * A failed claim re-resolves the code once before erroring: a * timed-out-but-landed claim (the request isn't retried — seeAlso applies to: 600-615
🤖 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 `@apps/companion/app/`(tabs)/scanner.tsx around lines 528 - 545, Update the JSDoc for claimQrAndProceed to describe only the claim-then-create flow: remove references to choosing a follow-up, linking existing assets, and mirroring the web claim/new-or-link flow. Keep the documentation aligned with the function’s current single-parameter behavior and navigation to the new-asset screen.
🤖 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 `@apps/companion/app/`(tabs)/assets/link-qr.tsx:
- Around line 102-121: Update the workspace guard around originOrgIdRef and its
useEffect so the first available currentOrg.id is stored when it becomes
available, while preserving it thereafter. Continue alerting and navigating back
only when a later currentOrg.id differs from the captured origin workspace.
In `@apps/webapp/app/routes/api`+/mobile+/qr.link-asset.test.ts:
- Around line 160-177: Update the already-linked error path in relinkAssetQrCode
to set status 403, matching the route’s documented guard behavior and sibling
cases. In the test “maps the service's already-linked guard to its error,”
assert that body.error?.status is 403 in addition to the existing message
assertion.
---
Nitpick comments:
In `@apps/companion/app/`(tabs)/scanner.tsx:
- Around line 528-545: Update the JSDoc for claimQrAndProceed to describe only
the claim-then-create flow: remove references to choosing a follow-up, linking
existing assets, and mirroring the web claim/new-or-link flow. Keep the
documentation aligned with the function’s current single-parameter behavior and
navigation to the new-asset screen.
🪄 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: CHILL
Plan: Pro Plus
Run ID: f5f50f9e-c6d9-4805-81cd-4ed48a2409ae
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.claude/rules/cross-app-mirrors-need-provenance.mdapps/companion/app/(tabs)/assets/link-qr.tsxapps/companion/app/(tabs)/scanner.tsxapps/companion/lib/permissions.tsapps/companion/package.jsonapps/webapp/app/modules/qr/service.server.test.tsapps/webapp/app/modules/qr/service.server.tsapps/webapp/app/routes/api+/mobile+/qr.claim.test.tsapps/webapp/app/routes/api+/mobile+/qr.claim.tsapps/webapp/app/routes/api+/mobile+/qr.link-asset.test.tsapps/webapp/app/routes/api+/mobile+/qr.link-asset.tsapps/webapp/app/utils/permissions/permission.data.tsapps/webapp/app/utils/permissions/permission.validator.server.test.tsapps/webapp/app/utils/permissions/permission.validator.server.tsapps/webapp/app/utils/rate-limit.server.tsapps/webapp/package.jsonapps/webapp/vite.config.tspackages/permissions/package.jsonpackages/permissions/src/index.tspackages/permissions/tsconfig.json
| /** | ||
| * Workspace-switch guard (mirrors the scanner's originOrgId pin): the | ||
| * scanned `qrId` belongs to the workspace that was active when the picker | ||
| * opened. If the user switches workspaces mid-flow, the list would show | ||
| * the NEW org's assets while the link targets the OLD org's code — the | ||
| * server would 403 on confirm, a confusing dead end. Leave instead. | ||
| */ | ||
| const originOrgIdRef = useRef(currentOrg?.id); | ||
| useEffect(() => { | ||
| if (!originOrgIdRef.current || !currentOrg?.id) return; | ||
| if (currentOrg.id !== originOrgIdRef.current) { | ||
| Alert.alert( | ||
| "Workspace Changed", | ||
| "The scanned QR code belongs to the workspace you started in. Scan it again from this workspace to link it here.", | ||
| [{ text: "OK", onPress: () => router.back() }], | ||
| { cancelable: false } | ||
| ); | ||
| } | ||
| }, [currentOrg?.id, router]); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate link-qr.tsx and scanner.tsx =="
fd -a 'link-qr.tsx|scanner.tsx$' . | sed 's#^\./##'
echo "== outline link-qr.tsx =="
if [ -f "apps/companion/app/(tabs)/assets/link-qr.tsx" ]; then
ast-grep outline "apps/companion/app/(tabs)/assets/link-qr.tsx" || true
echo "== relevant lines =="
sed -n '1,240p' "apps/companion/app/(tabs)/assets/link-qr.tsx" | nl -ba | sed -n '1,260p'
fi
echo "== locate scanner file =="
scanner_file="$(fd -a 'scanner.tsx$' . | grep -F 'apps/companion/app/(tabs)' | head -n1 || true)"
echo "$scanner_file"
if [ -n "$scanner_file" ] && [ -f "$scanner_file" ]; then
wc -l "$scanner_file"
ast-grep outline "$scanner_file" || true
echo "== scanner refs/effects/org guard =="
rg -n "activeOrgIdRef|originOrgId|useEffect|currentOrg" "$scanner_file" -C 4
fi
echo "== programmatic probe of one-shot useRef behavior from original effect =="
node - <<'JS'
const useRef = (initial) => ({ current: initial });
function run(original, currentOrgs = [undefined, { id: 'org-a' }, { id: 'org-b' }]) {
const originOrgIdRef = useRef(currentOrgs[0]?.id);
const alerts = [];
function go(n) {
if (!originOrgIdRef.current || !currentOrgs[n]?.id) return;
if (currentOrgs[n].id !== originOrgIdRef.current) alerts.push({ at: n, current: currentOrgs[n].id, origin: originOrgIdRef.current });
}
currentOrgs.forEach(go);
return { currentValue: originOrgIdRef.current, alerts };
}
console.log("original-first-undefined:", JSON.stringify(run(true)));
console.log("hardened-version:", (() => {
const originOrgIdRef = useRef(undefined);
const currentOrgs = [undefined, { id: 'org-a' }, { id: 'org-b' }];
const alerts = [];
currentOrgs.forEach((currentOrg, n) => {
if (!currentOrg?.id) return;
if (!originOrgIdRef.current) {
originOrgIdRef.current = currentOrg.id;
return;
}
if (currentOrg.id !== originOrgIdRef.current) alerts.push({ at: n, current: currentOrg.id, origin: originOrgIdRef.current });
});
return JSON.stringify({ currentValue: originOrgIdRef.current, alerts });
})());
JSRepository: Shelf-nu/shelf.nu
Length of output: 789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find useOrg definition/usages =="
rg -n "function useOrg|const useOrg|useOrg =|export .*useOrg|useOrg\\(" apps packages lib . --glob '!node_modules' --glob '!dist' --glob '!build' -C 3
echo "== companion assets route files =="
fd -a '.*' "apps/companion/app/(tabs)/assets" || trueRepository: Shelf-nu/shelf.nu
Length of output: 25422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/companion/lib/org-context.tsx relevant sections =="
sed -n '1,190p' apps/companion/lib/org-context.tsx | awk '{printf "%5d %s\n", NR, $0}'
echo "== apps/companion/app/(tabs)/assets/link-qr.tsx relevant sections =="
sed -n '55,140p' "apps/companion/app/(tabs)/assets/link-qr.tsx" | awk '{printf "%5d %s\n", NR+54, $0}'
sed -n '140,220p' "apps/companion/app/(tabs)/assets/link-qr.tsx" | awk '{printf "%5d %s\n", NR+139, $0}'
echo "== apps/companion/app/(tabs)/scanner.tsx origin/org guard sections =="
sed -n '160,225p' "apps/companion/app/(tabs)/scanner.tsx" | awk '{printf "%5d %s\n", NR+159, $0}'
rg -n "originOrgId|activeOrgIdRef|useEffect" "apps/companion/app/(tabs)/scanner.tsx" -C 5
echo "== programmatic probe of original guard with first render undefined =="
node - <<'JS'
function originalGuard(initialCurrentOrg, renders) {
const originOrgIdRef = { current: initialCurrentOrg?.id ?? undefined };
const alerts = [];
for (const [n, currentOrg] of renders.entries()) {
if (!originOrgIdRef.current || !currentOrg?.id) continue;
if (currentOrg.id !== originOrgIdRef.current) alerts.push({ at: n, current: currentOrg.id, origin: originOrgIdRef.current });
}
return { currentValue: originOrgIdRef.current, alerts };
}
function hardenedGuard(initialCurrentOrg, renders) {
const originOrgIdRef = { current: initialCurrentOrg?.id ?? undefined };
const alerts = [];
for (const [n, currentOrg] of renders.entries()) {
if (!currentOrg?.id) continue;
if (!originOrgIdRef.current) {
originOrgIdRef.current = currentOrg.id;
continue;
}
if (currentOrg.id !== originOrgIdRef.current) alerts.push({ at: n, current: currentOrg.id, origin: originOrgIdRef.current });
}
return { currentValue: originOrgIdRef.current, alerts };
}
const renders = [undefined, { id: 'org-a' }, { id: 'org-b' }];
console.log(JSON.stringify({
original: originalGuard(undefined, renders),
hardened: hardenedGuard(undefined, renders)
}, null, 2));
JSRepository: Shelf-nu/shelf.nu
Length of output: 20010
Initialize the workspace-switch guard when currentOrg becomes available.
originOrgIdRef is set once at mount to currentOrg?.id, and OrgProvider initializes currentOrg as null during loading. If this screen renders before the org is loaded, originOrgIdRef.current stays undefined, so the guard never fires for the first valid org. Store the first available org once, then alert on subsequent changes.
🛡️ Suggested hardening
const originOrgIdRef = useRef(currentOrg?.id);
useEffect(() => {
- if (!originOrgIdRef.current || !currentOrg?.id) return;
+ if (!currentOrg?.id) return;
+ if (!originOrgIdRef.current) {
+ originOrgIdRef.current = currentOrg.id;
+ return;
+ }
if (currentOrg.id !== originOrgIdRef.current) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Workspace-switch guard (mirrors the scanner's originOrgId pin): the | |
| * scanned `qrId` belongs to the workspace that was active when the picker | |
| * opened. If the user switches workspaces mid-flow, the list would show | |
| * the NEW org's assets while the link targets the OLD org's code — the | |
| * server would 403 on confirm, a confusing dead end. Leave instead. | |
| */ | |
| const originOrgIdRef = useRef(currentOrg?.id); | |
| useEffect(() => { | |
| if (!originOrgIdRef.current || !currentOrg?.id) return; | |
| if (currentOrg.id !== originOrgIdRef.current) { | |
| Alert.alert( | |
| "Workspace Changed", | |
| "The scanned QR code belongs to the workspace you started in. Scan it again from this workspace to link it here.", | |
| [{ text: "OK", onPress: () => router.back() }], | |
| { cancelable: false } | |
| ); | |
| } | |
| }, [currentOrg?.id, router]); | |
| /** | |
| * Workspace-switch guard (mirrors the scanner's originOrgId pin): the | |
| * scanned `qrId` belongs to the workspace that was active when the picker | |
| * opened. If the user switches workspaces mid-flow, the list would show | |
| * the NEW org's assets while the link targets the OLD org's code — the | |
| * server would 403 on confirm, a confusing dead end. Leave instead. | |
| */ | |
| const originOrgIdRef = useRef(currentOrg?.id); | |
| useEffect(() => { | |
| if (!currentOrg?.id) return; | |
| if (!originOrgIdRef.current) { | |
| originOrgIdRef.current = currentOrg.id; | |
| return; | |
| } | |
| if (currentOrg.id !== originOrgIdRef.current) { | |
| Alert.alert( | |
| "Workspace Changed", | |
| "The scanned QR code belongs to the workspace you started in. Scan it again from this workspace to link it here.", | |
| [{ text: "OK", onPress: () => router.back() }], | |
| { cancelable: false } | |
| ); | |
| } | |
| }, [currentOrg?.id, router]); |
🤖 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 `@apps/companion/app/`(tabs)/assets/link-qr.tsx around lines 102 - 121, Update
the workspace guard around originOrgIdRef and its useEffect so the first
available currentOrg.id is stored when it becomes available, while preserving it
thereafter. Continue alerting and navigating back only when a later
currentOrg.id differs from the captured origin workspace.
| it("maps the service's already-linked guard to its error", async () => { | ||
| vi.mocked(relinkAssetQrCode).mockRejectedValue( | ||
| new ShelfError({ | ||
| cause: null, | ||
| message: | ||
| "You cannot link to this code because its already linked to another asset. Delete the other asset to free up the code and try again.", | ||
| label: "QR", | ||
| status: 500, | ||
| }) | ||
| ); | ||
|
|
||
| const { body, status } = await callAction({ | ||
| qrId: "qr-1", | ||
| const { body } = await callAction({ | ||
| qrId: "qr-linked", | ||
| assetId: "asset-1", | ||
| }); | ||
|
|
||
| expect(status).toBe(403); | ||
| expect(body.error?.message).toBe( | ||
| "This QR code is already linked to an asset or a kit." | ||
| ); | ||
| expect(updateAssetQrCode).not.toHaveBeenCalled(); | ||
| expect(body.error?.message).toContain("already linked to another asset"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'qr\.link-asset\.test\.ts|service\.server\.ts|shelf\.config\.ts$|.*shelf.*\.ts$' apps/webapp/app | sed 's#^\./##' | head -100
echo
echo "== test slice =="
cat -n apps/webapp/app/routes/api+/mobile+/qr.link-asset.test.ts | sed -n '1,240p'
echo
echo "== search for ShelfError constructor/status defaults =="
rg -n "class ShelfError|constructor\\(|status || cause\\.status|default.*status|500" apps/webapp/app -g '*.{ts,tsx}' | head -200
echo
echo "== search relinkAssetQrCode and already linked errors =="
rg -n "relinkAssetQrCode|already linked to another asset|alreadyLinked|linked to this code" apps/webapp/app -g '*.{ts,tsx}' | head -200Repository: Shelf-nu/shelf.nu
Length of output: 30352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== utils error =="
cat -n apps/webapp/app/utils/error.ts | sed -n '1,220p'
echo
echo "== route action =="
cat -n apps/webapp/app/routes/api+/mobile+/qr.link-asset.ts | sed -n '1,190p'
echo
echo "== relinkAssetQrCode already linked branch =="
cat -n apps/webapp/app/modules/asset/service.server.ts | sed -n '6801,6875p'
echo
echo "== same error elsewhere: kit relink branch =="
cat -n apps/webapp/app/modules/kit/service.server.ts | sed -n '2945,2980p'
echo
echo "== programmatic JS simulation of ShelfError default status =="
node - <<'JS'
class ShelfError extends Error {
constructor({ cause, message, label, status } = {}) {
super(message);
this.label = label;
this.shouldBeCaptured = false;
this.status = status || (cause && cause.status) || 500;
}
}
for (const err of [
new ShelfError({ cause: null, message: "missing status", label: "QR" }),
new ShelfError({ cause: { status: 403 }, message: "cause status", label: "QR" }),
]
) {
console.log(`${err.message}: ${err.status}`);
}
JSRepository: Shelf-nu/shelf.nu
Length of output: 18478
Use the guarded 403 for already-linked QR failures.
relinkAssetQrCode’s already-linked branch throws without status, so this error falls back to 500 despite the route doc saying this condition should return 403. Align the service error with the documented/guarding status, and add a status assertion here so the guard error matches the sibling 403 cases.
🤖 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 `@apps/webapp/app/routes/api`+/mobile+/qr.link-asset.test.ts around lines 160 -
177, Update the already-linked error path in relinkAssetQrCode to set status
403, matching the route’s documented guard behavior and sibling cases. In the
test “maps the service's already-linked guard to its error,” assert that
body.error?.status is 403 in addition to the existing message assertion.
Need
PR #2753's review surfaced that the companion carried a hand-copied mirror of the webapp's permission matrix, and that the webapp's effective authorization was itself split across two places:
Role2PermissionMap(data) and an ADMIN/OWNER allow-all short-circuit living only insidehasPermission(). Two apps, three fragments, zero drift protection.Hypothesis
If the matrix AND the resolution logic live in one dependency-free workspace package consumed by both apps, then web and mobile literally cannot disagree about what a role may do — and role changes become compile errors instead of runtime drift.
What changed
packages/permissions(@shelf/permissions) —PermissionAction/PermissionEntity, the full role matrix (mechanically transformed from the webapp file, zero manual retyping), androleHasPermission(): the pure resolver including the ADMIN/OWNER short-circuit. No Prisma, no Node APIs — Metro-bundleable by design.permission.data.ts→ re-export shim. ~250 existing import sites untouched. Includes a compile-time parity guard: if Prisma'sOrganizationRolesand the package's role union ever diverge, the webapp stops typechecking.UserOrganizationlookup +ShelfErrorsemantics. One deliberate behavior refinement: an unknown entity now safe-denies (false→ 403) instead of throwing a wrapped 500-path error.lib/permissions.ts— the hand-copied matrix is deleted;userHasPermissionis now a thin adapter over the same resolver the server uses. Public API unchanged, all 5 call sites untouched.ssr.noExternal, lockfile.Test plan
qr:updateis in no role's matrix entry, yet ADMIN/OWNER get it via the short-circuit (asserted explicitly).pnpm webapp:validategreen: 3,651 tests / 277 files, lint 0 errors, typecheck clean (all ~250 sites through the shim).tsc --noEmitgreen.expo export --platform iosproduces a full 6.55 MB Hermes bundle with the workspace package inlined — the RN-consumption risk is tested, not assumed.Notes for review
mainonce feat(companion): native claim + link/create for unclaimed QR codes #2753 merges..claude/rules/cross-app-mirrors-need-provenance.md(added in feat(companion): native claim + link/create for unclaimed QR codes #2753) is the policy this PR completes: the companion mirror it documents now ceases to exist.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes