fix(ui): hide write/edit controls from READ_ALL (read-only) users #696
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
Users in a group without
WRITE_ALLcan read everything but cannot create, edit, upload, or delete. The backend enforces this (@RequirePermission(WRITE_ALL)on every mutating endpoint + server-side route redirects), so this is not a privilege-escalation hole — but the UI still shows them controls they cannot use, which is confusing friction on the main read journey (a reader clicks "Hochladen" and lands on a form that redirects them away).The frontend already derives three gating booleans in
frontend/src/routes/+layout.server.tsand drills them down as page data:This issue audits the whole UI for write/edit controls and closes the one confirmed leak. (The complex transcription read-only case is split into #697.)
Audit result — current gating state
✅ Already correctly gated (no change needed)
routes/documents/+page.svelte:332canWriteroutes/documents/+page.svelte:316canWritelib/document/BulkSelectionBar.sveltecanWriteroutes/+page.svelte:92canWritelib/document/DocumentTopBarActions.svelte:73canWritelib/document/DocumentTopBarActions.svelte:25canWrite(becomes readable in #697)routes/persons/+page.svelte:105canWriteroutes/persons/+page.svelte:95canWriteroutes/persons/[id]/PersonCard.svelte:120canWriteroutes/persons/[id]/edit/NameHistoryEditCard.svelte:75,103canWriteroutes/enrich/+page.svelte:73,106canWriteroutes/stammbaum/+page.svelte:158,166canWriteroutes/briefwechsel/ConversationTimeline.svelte:92canWriteroutes/documents/[id]/+page.svelte:348(canComment={canWrite})routes/geschichten/+page.svelte:57,[id]/+page.svelte:125canBlogWriteroutes/+page.svelte:43canBlogWriteroutes/+layout.svelte:42(AppNav)ADMIN✅ Protected by server-side route redirect (not rendered for non-writers)
documents/[id]/edit,persons/[id]/edit,persons/new,persons/review,documents/bulk-edit,enrich/[id], all ofadmin/*. Their+page.server.tsredirects a non-writer before the page renders. The transcription edit controls are also unreachable today because the only entry point — the "Transkribieren" button — iscanWrite-gated.❌ The leak — fix in this issue
Header "Hochladen" (Upload) button —
frontend/src/routes/+layout.svelte:78-102The gate is
{#if data?.user}(any authenticated user) instead of also requiringdata.canWrite. A non-writer sees the button, clicks it, hits/documents/new, and is bounced by the server-side redirect indocuments/new/+page.server.ts:18(if (!hasWriteAll(locals)) throw redirect(303, '/')). This is the only confirmed write control visible to a non-writer in the global chrome.Change
frontend/src/routes/+layout.sveltecanWriteis already ondata— no new plumbing. Nothing else in the global layout needs changing (bell, theme toggle, language switcher, user menu are read-only and correctly shown to all authenticated users).Defensive note —
DropZone.svelteroutes/DropZone.sveltehas no internal permission guard; it relies on its mount site. Today its only mount (routes/+page.svelte:92) is already{#if data.canWrite}. Do not add an internal guard; any future mount must wrap it incanWrite.Tests
frontend/src/routes/layout.svelte.spec.ts(exists — extend): withdata.canWrite = false→ no element witharia-label = upload_action(); withcanWrite = true→ present withhref="/documents/new". Use amakeData()factory.ANNOTATE_ALL-only user (canWrite = false) → no upload button. Documents that the gate is "lacks WRITE_ALL", not "is READ_ALL".POSTto the document-create and/api/documents/quick-uploadendpoints returns 403 for aREAD_ALL-only principal (and 401 unauthenticated). Hiding the button is not the control; the endpoint authz is.Acceptance criteria
upload_actionalready exists).Out of scope / Related
ANNOTATE_ALLusers cannot comment/annotate: comment & annotation controls are gated oncanWrite(routes/documents/[id]/+page.svelte:348), but the intent ofANNOTATE_ALLis "read + annotate/comment".canAnnotateis already derived in+layout.server.tsbut unused. Re-wiring those gates fromcanWritetocanAnnotategrants more, not less — a separate follow-up, not part of this issue.marcel referenced this issue2026-05-31 10:53:07 +02:00
Implemented on
feat/issue-696-hide-write-controlsThe confirmed leak is closed and the boundary is documented. Three atomic, TDD commits:
c3652f5b+layout.svelteupload-button gate changed from{#if data?.user}→{#if data?.user && data.canWrite}. Drove it red-first with acanWrite: false→ no-upload-link test.97274bebANNOTATE_ALL-only user (canWrite: false, canAnnotate: true) still sees no upload link, proving the gate is "lacks WRITE_ALL", not "is READ_ALL".5edefdd0@WithMockUser(authorities = "READ_ALL")403 boundary tests forPOST /api/documentsandPOST /api/documents/quick-upload— the control that actually matters.Acceptance criteria
canWrite=false, incl.ANNOTATE_ALL-only) — header upload button not rendered. No other write control in the global chrome.WRITE_ALLuser — upload button still rendered, links to/documents/new(existing tests stay green).READ_ALL-only principal with 403 (and 401 unauthenticated — already covered).Verification
layout.svelte.spec.ts(client project): 11 passed — newcanWrite=falsetest confirmed red before the gate, green after.DocumentControllerTest: 104 passed (102 + 2 new READ_ALL boundary tests).svelte-check: no errors in the touched files (+layout.svelte,layout.svelte.spec.ts). The 32 repo-wide errors are pre-existing onmain(users/[id],admin/users) and untouched here.Scope honored
upload_actionalready exists), no doc updates (no new route/permission/ErrorCode).DropZone.svelteleft without an internal guard (per the defensive note).canAnnotaterewiring and #697 (read-only transcription) left out of scope."Writer sees it" note
I did not add a third "writer sees the button" test — that contract is already locked by the two existing upload-link tests (
has aria-label,navigates to /documents/new), both of which run withcanWrite: true. Adding a duplicate would violate DRY.Branch pushed. Next: open a PR / run
/review-pr.🏛️ Markus Keller — Application Architect
Observations
@RequirePermission(WRITE_ALL)+ server-side route redirects) is the real control and the UI button is friction reduction only. That framing keeps the security boundary at the lowest trustworthy layer and treats the UI gate as cosmetic — correct.+layout.server.ts(server-side, fromlocals.user.groups) and drilled down as page data. Single source of truth, computed where the session is authoritative. No client-side permission inference. Good.canUploadderived (+layout.svelte:46, extracted in944370dc) pulls the boolean out of the template. That's the kind of small, named boundary I want — the template reads a flag, the script owns the rule.Recommendations
routes/+page.svelte:92) and the component top:<!-- No internal permission guard: caller MUST wrap in {#if canWrite}. Server enforces upload authz. -->. The issue text evaporates; a code comment survives.Permission/ErrorCode, and the actual auth/upload flow is unchanged (backend untouched).seq-document-upload.pumldescribes the server flow, which does not change. Confirm: nothing to update. Don't over-document a{#if}.Open Decisions
👨💻 Felix Brandt — Senior Fullstack Developer
Observations
{#if canUpload}backed byconst canUpload = $derived(Boolean(data?.user && data.canWrite))(+layout.svelte:46,80). Business logic out of the template, intent-revealing name, single$derived— this is the rule I'd flag a violation for, applied correctly.canWrite: false → no upload linktest was driven red-first before the gate change (c3652f5b), and themakeData()factory was reused with a one-key override (makeData({ canWrite: false })). Exactly the factory pattern I want — no 10-line builder copied per test.has aria-labelandnavigates to /documents/newboth already run undercanWrite: true(themakeData()default). A duplicate positive test would be DRY noise.Recommendations
Boolean(...)wrapping the&&— it normalizesundefined(no user) to a realfalseso the{#if}and any future prop pass-through stay boolean-clean. Don't drop it.Open Decisions
🛡️ Nora "NullX" Steiner — Application Security Engineer
Observations
<a>stops zero attackers; acurl -X POSTignores the DOM entirely.DocumentControllerTesthas@WithMockUser(authorities = "READ_ALL")→403for bothPOST /api/documentsandPOST /api/documents/quick-upload, plus the unauthenticated401path. That's the correct pair — 401 (not authenticated) and 403 (authenticated but unauthorized) are different failures and both are covered.POST,PUT /{id},PUT /{id}/file,DELETE /{id},PATCH /bulk, training-labels) carries@RequirePermission(WRITE_ALL). No write path leaks to a reader.Recommendations
POST /api/documents/batch-metadata(DocumentController.java:330) is gated@RequirePermission(Permission.READ_ALL), notWRITE_ALL. CLAUDE.md states WRITE_ALL is required on everyPOST. Ifbatch-metadatais a read-shaped query (fetch metadata for many IDs) theREAD_ALLgate is intentional and fine — but aPOSTwith onlyREAD_ALLis exactly the kind of thing that silently becomes a write leak after a future refactor. Confirm it is non-mutating, and if so add a one-line comment explaining why aPOSTisREAD_ALL-gated. Not a blocker for #696; do not expand this issue's scope.READ_ALL → 403upload tests as permanent regression fixtures. If someone ever widens a gate toREAD_ALL, these tests must scream. They will.Open Decisions
🧪 Sara Holt — QA Engineer & Test Strategist
Observations
layout.svelte.spec.ts, vitest-browser-svelte against real DOM); the authorization boundary → controller-slice test (DocumentControllerTest). No reaching for E2E to verify a{#if}. That's the right cost/coverage trade.ANNOTATE_ALL-only user (canWrite:false, canAnnotate:true) → no upload link. It documents that the gate means "lacks WRITE_ALL", not "is READ_ALL" — which prevents a future dev from re-deriving the gate aspermissions.includes('READ_ALL')and accidentally showing the button to annotators..not.toBeInTheDocument()(DOM removal), not "not visible" — correct for a{#if}, and it also confirms the control is gone from the a11y tree, not just hidden.Recommendations
WRITE_ALL+ANNOTATE_ALL(→canWrite:true) sees the button — already covered by themakeData()default in the two positive tests. No new test needed.Open Decisions
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Observations
/violates Nielsen #1 (visibility of system status) and #5 (error prevention) — the interface offered an affordance it couldn't honor. Removing the false affordance is the correct fix for the read journey.{#if canUpload}removes the link from the accessibility tree entirely, not just visually — so screen-reader and keyboard users on the read path never land on a dead control. That's the accessible way to gate, and it's what the implementation does.aria-label={m.upload_action()}and thexl:-responsive label are preserved for writers — i18n and a11y on the button itself are intact.Recommendations
flex items-center gap-3; removing the first child just collapses one gap, so no orphaned spacing — but confirm visually at 320px that the bell + theme + avatar cluster still sits balanced and nothing reflows oddly. This is the senior-on-a-small-phone reader, our hardest constraint, and they're exactly the population that no longer sees the button.Open Decisions
⚙️ Tobias Wendt — DevOps & Platform Engineer
Observations
{#if}gate plus tests — nothing for me to size or operate.READ_ALLseed user is created, and it explicitly notes that if one is later added, creds go via Gitea secrets (${{ secrets.* }}), never hardcoded in workflow YAML. That's the right call and the right pattern.Recommendations
npx vitest run src/routes/layout.svelte.spec.tslocally; let the full sweep + the Chromium browser project run in CI. No pipeline change required.Open Decisions
📋 Elicit — Requirements Engineer & Business Analyst
Observations
canAnnotaterewiring named as a separate follow-up), and the audit table makes the as-is state traceable. This is the spec-dense style this project relies on.Recommendations
ANNOTATE_ALLusers-cannot-comment note in "Out of scope" is correctly parked — but it's a real requirement gap (the intent ofANNOTATE_ALLis "read + annotate/comment", yet comment controls are gated oncanWrite, andcanAnnotateis derived-but-unused). Make sure a follow-up issue actually exists so this doesn't live only as a paragraph in a closed issue. A deferred requirement with no ticket is a lost requirement.Open Decisions
🗳️ Decision Queue — Action Required
1 decision needs your input — and it is a follow-up, not a blocker for #696. The upload-button fix can ship as-is.
Product / UX
(Raised by: Leonie, Elicit)
Everything else from the personas was a concrete recommendation, not a decision — captured inline above:
POST /api/documents/batch-metadata(READ_ALL-gated) is non-mutating → separate ticket if needed.ANNOTATE_ALL-can't-comment (canAnnotaterewiring).