pal-e-docs

Review: /auth/login + /auth/callback + /auth/logout endpoints (westside-admin#16)

review-1136-2026-05-03 Review

review approved
Project

Verdict: APPROVED

Both READY and APPROVED are accepted as passing verdicts by the check-board-advance hook. Use either keyword interchangeably.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — explicit decomposition from #2, sub-task 3 of 4, depends on #14, independent of sub-tasks 2 and 4
  • [x] Repo — forgejo_admin/westside-admin
  • [x] User Story — story-westside-admin-admin-row-crud with rationale (these endpoints are the OIDC protocol surface, sit OUTSIDE the admin-role gate)
  • [x] Context — three-paragraph walkthrough of each endpoint's behavior, transient-cookie semantics, and Keycloak interactions
  • [x] File Targets — 3 create paths + 3 do-not-touch paths
  • [x] Acceptance Criteria — 7 ACs covering login state-set, callback CSRF (state mismatch + missing state), happy-path token exchange, token-endpoint failure handling, logout SLO, POST-only logout, and logging hygiene
  • [x] Test Expectations — manual integration tests against staging Keycloak (pnpm dev + browser), no vitest target (correctly noted as integration boundary)
  • [x] Constraints — fetch only (no axios/got), event.cookies API only, two clearly-articulated PKCE storage options, exact redirect URI match
  • [x] Checklist — 5 items including PR funnel-auth review per feedback_funnel_requires_auth
  • [x] Related — links to project, parent #2, dependency #14, SOP, and feedback note

Traceability

  • [x] story:admin-row-crud label — verified, single v1 story per project-westside-admin Safety Constraints contract
  • [x] story note verified — story-westside-admin-admin-row-crud (id 1616) exists, listed in project-westside-admin#user-stories table
  • [~] arch:westside-admin label — no umbrella arch-westside-admin note, but project page Architecture section links three concrete arch notes (arch-domain-westside-admin, arch-dataflow-westside-admin, arch-deployment-westside-admin) and arch-scoped-db-westside-admin. The label is treated as a satisfied umbrella per the same precedent set in review-1132-2026-05-03. Acceptable.
  • [!] arch:keycloak has NO backing note — search_notes("arch-keycloak") returns empty. This gap was already flagged in review-1132-2026-05-03 for board item #1132. Carrying forward the same recommendation rather than re-blocking each sub-task; this label is a known umbrella gap across the entire admin-row-crud chain (#1132, #1135, #1136, #1134) and Ava holds the call on whether to backfill arch-keycloak or treat the dataflow note as sufficient documentation.
  • [x] Forgejo issue — forgejo_admin/westside-admin#16 is open, body is current, no Forgejo labels (board labels are the source of truth here)

File Targets

  • [x] src/routes/auth/login/+server.ts — does not exist (correctly listed as "Create"). Parent src/routes/ exists with current children +layout.svelte, +page.svelte, health/ — no collision.
  • [x] src/routes/auth/callback/+server.ts — does not exist. No collision.
  • [x] src/routes/auth/logout/+server.ts — does not exist. No collision.
  • [x] src/lib/server/keycloak.ts (do-not-touch) — verified post-#18 merge. Public surface contains all three names the issue body imports: generateOidcState (line 408), verifyOidcState (line 428), encryptCookiePayload (line 340). Bonus exports the dev will likely need: decryptCookiePayload, verifyKeycloakJwt, refreshTokensIfNeeded, KeycloakTokens interface, and the four typed error classes (JwksUnreachableError, JwtVerificationError, TokenRefreshError, MissingEnvError).
  • [x] src/hooks.server.ts (do-not-touch) — sub-task 2 (#15), still open, correctly excluded.
  • [x] src/routes/(unauthorized)/* (do-not-touch) — sub-task 4 (#17), still open, correctly excluded.

Repo Placement

OK. All work lands in forgejo_admin/westside-admin. No cross-repo touch (env vars already landed via pal-e-deployments#147, Keycloak client config matches the issue body via the Playwright snapshot under /home/ldraney/pal-e-platform/.playwright-mcp/page-2026-05-03T14-28-45-062Z.yml — Root URL, Valid redirect URI /auth/callback, post-logout URI /, web origin all confirmed).

Dependencies

  • Blocked-by: #14 — DONE (PR #18 merged into main at 982df5b, board item #1134 in validation). All three named primitives exist on main.
  • Blocked-by: pal-e-deployments#147 — DONE (closed). 5 env vars landed in cluster Secret. ArgoCD reconciliation noted in router context — verify before declaring "deployed and working" but not a scope-review blocker.
  • Independent of: #15 (hooks.server.ts) — sub-task 2, currently open. The hook redirects TO these endpoints but does not define them. Issue correctly notes orthogonality.
  • Independent of: #17 (403 page) — sub-task 4, currently open. Orthogonal.
  • Blocks: #15 indirectly — hooks.server.ts cannot be exercised end-to-end until /auth/login + /auth/callback exist, but the hook itself can be coded without them. No scope adjustment needed.
  • Five env vars are explicit and match keycloak.ts requireEnv usage (lines 111, 127, 136, 506, 507).

Acceptance Criteria

All 7 ACs are agent-verifiable post-implementation:
  • State-cookie attributes (HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600) — verifiable via curl -i + cookie header inspection.
  • Authorize URL query params (client_id, redirect_uri, response_type, scope, state, code_challenge_method, code_challenge) — verifiable from the 302 Location header.
  • state_mismatch / state_missing 400 responses — directly testable.
  • Token exchange ordering ("must NOT have run" before state validation) — code-review verifiable per the existing checklist item "State validation runs BEFORE token exchange (verified by reading the callback code top-to-bottom)".
  • Generic 502 on Keycloak /token failure (no error-body echo) — code-review + log-grep verifiable.
  • Logout post_logout_redirect_uri + id_token_hint — verifiable from 302 Location header.
  • Logging hygiene (no tokens, ciphertext, or refresh tokens in logs) — already on the checklist as "No tokens in logs (grep verified)".
Edge case worth a one-line confirmation in the PR: the AC for "valid state" cookie sets Max-Age={refresh_expiry}. Verify the dev pulls this from refresh_expires_in in the token response (Keycloak default ~30 minutes) rather than guessing — this is implementation-level guidance, not a scope gap.

Blast Radius

Low. Three new endpoint files in a previously empty src/routes/auth/ tree. No shared modules touched. The transient state cookie path (Path=/auth/callback) confines its surface to the callback handler. Logout is POST-only (correctly), so a stale GET /auth/logout link cannot be CSRF-baited. Funnel-auth concern is satisfied because these endpoints intentionally sit OUTSIDE the admin-role gate (the gate runs in hooks.server.ts AFTER callback sets the session) — issue body explicitly addresses this in the User Story section. A grep confirms no other code in westside-admin consumes these route paths today.

Decomposition Assessment

5-minute rule check:
  • 3 file targets across 1 repo — at threshold but within rule (rule is >3 files across >2 repos).
  • 7 ACs — over the >5 trigger, BUT 4 of the 7 ACs are /auth/callback sub-conditions (mismatched state, missing state, happy path, token-endpoint failure) which collapse to a single handler with a state-machine branch. Functionally this is closer to "3 endpoints, 4 behavioral branches in callback" than 7 independent units of work.
  • Estimated agent work — three thin SvelteKit RequestHandler files (~40–60 lines each), all primitives already exported from keycloak.ts, no test scaffolding, no schema work, no migrations. ~3–5 minutes.
No decomposition needed. Splitting login from callback would create pointless coordination overhead (login generates the state cookie that callback consumes) and would not reduce per-agent work meaningfully. Logout could theoretically split into a fourth ticket, but it's ~15 lines and shares the same env-var + cookie-handling context — splitting buys nothing.

Recommendation

APPROVED. Two minor BODY nits worth fixing while in the queue, but neither blocks moving to todo:
  • [BODY] Forgejo title says "/auth/callback + /auth/logout" (2 endpoints) but body adds /auth/login as a third file target. The expanded scope is cohesive (a coherent OIDC protocol surface) and we recommend keeping all 3 in this ticket. Update the Forgejo title to "/auth/login + /auth/callback + /auth/logout endpoints: OIDC code exchange + SLO (consumes #14)" so a future contributor reading the title alone doesn't miss /auth/login. The board item #1136 title (which matches the new spec) is already correct.
  • [BODY] Constraints section mentions PKCE code_verifier storage but never says where the code_verifier is generated. Add one line: "Use Node's crypto.randomBytes + base64url + SHA-256 to derive code_verifier + code_challenge; PKCE generation is not exported from keycloak.ts and the dev creates it inline in +server.ts." This prevents an agent from inventing a new export on the frozen keycloak.ts.
  • [SCOPE] Carry forward from review-1132-2026-05-03: arch-keycloak backing note is missing across the admin-row-crud chain. Ava decision: backfill or treat dataflow note as sufficient. Not a blocker for this ticket; tracked as an umbrella gap on the chain.
Both BODY nits are issue-body cleanups, not scope changes. Recommend Ava either (a) edit the body inline before promoting to todo, or (b) advance to todo and trust the dev to handle the implicit guidance. Either path is consistent with the convention.