pal-e-docs

Review: Keycloak cookie SSR auth + admin role gate (westside-admin#2)

review-1132-2026-05-03 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Board item: #1132 on board-pal-e-platform (mirror of board-westside-admin item #1090).
Forgejo issue: forgejo_admin/westside-admin#2 (open).
Type: Feature.
One-line: Scope is technically sound and the issue body is high quality, but the ticket is over the 5-minute rule by its own admission, two label/arch notes need fixing, and the "prior art" hint is misleading.

Template Completeness

  • [x] Type
  • [x] Lineage (cites #6 scaffolding + pal-e-platform#301 Keycloak client)
  • [x] Repo
  • [x] User Story (story-westside-admin-admin-row-crud, ties to Keycloak admin gate safety constraint)
  • [x] Context (funnel auth rationale, env vars, prior art note, dataflow reference)
  • [x] File Targets (6 create, 1 update, 1 explicit do-not-modify)
  • [x] Acceptance Criteria (10 testable items)
  • [x] Test Expectations (unit + integration + manual)
  • [x] Constraints (cookie attrs, cookie name, AES-GCM, no-log rule, redirect URI, OIDC state)
  • [x] Decomposition note (self-aware: ticket is at upper edge of 5-min rule, proposes 4-way split fallback)
  • [x] Checklist
  • [x] Related

Traceability

  • [!] story label MISMATCH — board item carries story:superuser-onboard-service, but project-westside-admin Safety Constraints section explicitly states "Every ticket on the board carries story:admin-row-crud." The issue body correctly references story-westside-admin-admin-row-crud. Mirrored items on board-westside-admin (item #1090) already use story:admin-row-crud. [LABEL] Replace story:superuser-onboard-service with story:admin-row-crud on board item #1132.
  • [x] story note verified — story-westside-admin-admin-row-crud (note 1616) exists; entry present in project-westside-admin user-stories table.
  • [~] arch:westside-admin label — no single "arch-westside-admin" note exists, but the project page architecture section links three concrete arch notes (arch-domain-westside-admin, arch-dataflow-westside-admin, arch-deployment-westside-admin) and the issue body cites arch-dataflow-westside-admin Flow 1 specifically. Treat the label as an umbrella satisfied by those three. Acceptable.
  • [!] arch:keycloak has NO backing note — list_notes(tags="arch", project="westside-admin") returns no arch-keycloak, and global search_notes("arch-keycloak") is empty. Same gap exists on board item 1096 (#301) which already lives in todo. [SCOPE] Create arch-keycloak architecture note covering realm westside-basketball, JWKS endpoint, client registry, and SSO surface across westside-app + westside-admin. Could be deferred to a follow-on if Ava decides keycloak is documented well enough by the dataflow note.
  • [x] Forgejo issue valid and open. Comment count 2, last updated today.

File Targets

Repo at /home/ldraney/westside-admin verified. Current source tree: src/app.css, src/app.d.ts, src/app.html, src/lib/server/ (empty), src/routes/+layout.svelte, src/routes/+page.svelte, src/routes/health/+server.ts. Adapter is @sveltejs/adapter-node (confirmed in svelte.config.js) — correct for SSR cookie auth.
  • [x] src/hooks.server.ts — does not exist (correct, to create).
  • [x] src/routes/auth/callback/+server.ts — route does not exist (correct, to create).
  • [x] src/routes/auth/logout/+server.ts — route does not exist (correct, to create).
  • [x] src/routes/(unauthorized)/+page.svelte — route group does not exist (correct, to create). SvelteKit route-group naming is valid.
  • [x] src/lib/server/keycloak.ts — src/lib/server/ directory exists and is empty; correct location for server-only code (SvelteKit enforces no client import from $lib/server).
  • [x] src/app.d.ts — exists with empty App namespace stubs; App.Locals is commented out, ready to extend.
  • [x] package.json — exists. jose not currently a dependency (verified via head -50 of package.json); cookie also not present. Both need adding. Acceptable.
  • [ ] "Prior art" hint about westside-contracts SSR auth helpers — misleading. grep -rn -i "keycloak\|jose\|JWKS" /home/ldraney/westside-contracts/src returns zero hits. westside-contracts uses signed-token URLs (no Keycloak). westside-app uses browser-side keycloak-js (adapter-static). Neither is SSR cookie auth prior art. [BODY] Replace the "Prior art" sentence with: "No SSR cookie-auth prior art exists in this codebase — westside-contracts uses signed tokens, westside-app uses browser-side keycloak-js. Implement from scratch using jose per Constraints below." Minor; not a blocker if dev agent reads context carefully.

Repo Placement

OK — forgejo_admin/westside-admin. The work is purely SvelteKit code in this repo. Configuration changes (Keycloak client, k8s secret) are correctly separated into pal-e-platform#301 (closed-equivalent — client created in admin console today per prompt) and pal-e-deployments#147 (open — client_secret + 4 other env vars).

Dependencies

Three external dependencies, ordering matters:
  • westside-admin#6 (scaffolding) — CLOSED. Source tree confirms scaffold is in place.
  • pal-e-platform#301 (Keycloak westside-admin OIDC client creation + SOP) — per prompt context, the client was created today (2026-05-03) per sop-keycloak-client-creation. Client is confidential (validated by the existence of a KEYCLOAK_CLIENT_SECRET in the secret). Effectively done.
  • pal-e-deployments#147 (land real KEYCLOAK_CLIENT_SECRET; full env-var set) — OPEN, QA-approved, awaiting merge. The five env vars (KEYCLOAK_URL, KEYCLOAK_REALM, KEYCLOAK_CLIENT_ID, KEYCLOAK_CLIENT_SECRET, COOKIE_SIGNING_KEY) are confirmed in overlays/westside-admin/prod/westside-admin-secrets.enc.yaml on the keycloak-westside-admin-secret branch. Must merge + ArgoCD sync before manual / SSO acceptance criteria can be validated. Code can be written and unit-tested without it; integration / manual flow cannot.
Sibling tickets in board-westside-admin todo (Drizzle setup #1, scoped-db helper #3, players list #4, players row edit #5) all carry story:admin-row-crud and depend on this hooks.server.ts ticket landing first because all data routes need event.locals.user populated. This ticket is correctly first-in-line; recommend pulling it directly into next_up after refinement.

Acceptance Criteria

Strong. 10 criteria, every one verifiable by an agent:
  • Anonymous redirect path verifiable via Playwright follow-redirects=false.
  • state CSRF check verifiable via crafted callback request with mismatched state.
  • Cookie attribute set verifiable via curl -i + grep for HttpOnly; Secure; SameSite=Lax.
  • Role gate verifiable via two test users (one with admin, one without).
  • JWKS cache TTL verifiable via Keycloak rotation + cache-hit timing.
  • JWT validation legs (audience, exp, iss) verifiable via unit tests with crafted tokens (jose offers fixtures).
  • Token refresh verifiable via fast-forwarding system clock or short access-token lifetime.
  • No-log rule verifiable via grep over server logs after running auth flow.
  • SSO verification is the one manual-only AC; acceptable.
One implicit gap: no AC for the case where Keycloak is unreachable (JWKS fetch fails). Suggest adding: "If JWKS fetch fails on a cold cache, request returns 503; existing cached JWKS continues serving until TTL expires." [BODY] Add resilience AC. Optional — would tighten the spec.

Blast Radius

Self-contained: code lives entirely in westside-admin. Downstream board items (Drizzle, scoped-db, players list, players row edit) all consume event.locals.user — that contract must remain stable. The App.Locals.user shape ({ sub, email, name, tenantId }) becomes a load-bearing interface for every subsequent feature; freeze it carefully. tenantId hardcoded to 1 matches Safety Constraint #2 ("v1: hardcoded TENANT_ID = 1").
No similar bug to fix elsewhere — no other adapter-node + cookie SSR + Keycloak service exists in this org yet (westside-app is adapter-static, westside-contracts uses signed tokens). This ticket establishes the pattern; future cookie-SSR services will copy keycloak.ts.

Decomposition Assessment

NEEDS DECOMPOSITION (advisory).
  • File count: 6 new files + 1 updated file + new dependencies. Over the 3-file threshold.
  • AC count: 10. Over the 5-AC threshold.
  • Estimated time: issue body itself says "30-60 min agent run." Well over the 5-min rule.
  • Cryptographic correctness on critical path: CSRF state generation+validation, AES-GCM encrypt/decrypt, JWT signature+audience+expiry+issuer verification, token refresh window. Mistakes here bypass funnel auth and replay the 2026-04-10 PII leak.
The issue body proposes a 4-way split as fallback:
  • keycloak.ts lib — src/lib/server/keycloak.ts + unit tests. Pure functions: JWKS fetch+cache, JWT verify, AES-GCM encrypt/decrypt, state generate+validate. No SvelteKit coupling. Largest crypto surface; deserves isolated test pass.
  • hooks.server.ts — consumes keycloak.ts; reads cookie, validates JWT, populates event.locals.user, redirects anonymous to Keycloak with state. Also extends src/app.d.ts with App.Locals.user.
  • auth/callback + auth/logout endpoints — +server.ts handlers for OIDC code exchange, cookie set, logout + Keycloak SLO redirect. Depends on (1) and (2).
  • (unauthorized)/+page.svelte — small 403 page with logout button. Trivial; could fold into (3).
Tradeoff: the modules are tightly coupled (keycloak.ts is consumed by both hooks.server.ts and the callback endpoint), so a 4-way split costs ~3 PR coordination cycles vs ~1 oversized PR. The crypto correctness argument tips it toward decomposition: the keycloak.ts unit-test surface alone is large enough that bundling it with the integration code makes review hard.
Recommendation: Ava's call. If Ava trusts a single-shot dev agent on this, accept the issue body's self-flag and proceed. If Ava wants the safety net, route to skill-decompose-ticket with the 4-way split.

Recommendation

  • [LABEL] Fix on board item #1132: replace story:superuser-onboard-service with story:admin-row-crud to match project Safety Constraints contract.
  • [SCOPE] Decide whether to create arch-keycloak note now or defer (label currently lacks a backing note; same gap exists on item #1096 / pal-e-platform#301).
  • [BODY] Replace the misleading "Prior art: westside-contracts may have helpers" sentence; westside-contracts has no Keycloak code. Optional but would save the dev agent a wasted grep.
  • [BODY] Optionally add a JWKS-unreachable resilience AC (cold-cache failure → 503; warm-cache continues serving until TTL).
  • [DECOMPOSE] 6 files + 10 AC + crypto + OIDC state on funnel-auth critical path. Over 5-min rule. Issue body's proposed 4-way split (keycloak.ts / hooks.server.ts+app.d.ts / auth endpoints / 403 page) is sound. Route to skill-decompose-ticket if Ava wants the safety net; otherwise accept oversized with the dev agent on standby for escalation.
  • Sequencing note for whoever dispatches: do not move to in_progress until pal-e-deployments#147 is merged and ArgoCD has synced the secret — otherwise the manual / SSO ACs cannot be validated. Code+unit can begin earlier, but a dev agent that hits the "deploy and SSO-verify" AC will block.