pal-e-docs

Review: S2 — pal-e-services: Keycloak client for westside-streamlit (#1070)

review-1070-2026-04-22 Review

review approved
Project

Verdict: APPROVED

Board item: #1070 — S2 — pal-e-services: Keycloak client for westside-streamlit (supersedes #10)
Forgejo issue: forgejo_admin/pal-e-services#61 (open)
Reviewer: Dottie
Date: 2026-04-22
Context: Third review pass. Two prior informal rounds completed; second round was APPROVED after locking public_client = true + PKCE S256. This formal review confirms the locked scope is sound for backlog→todo advancement.

Template Completeness (Feature)

  • [x] Type — Feature
  • [x] Lineage — supersedes incorrectly-closed forgejo_admin/westside-streamlit#10; sibling of pal-e-services#60
  • [x] Repo — forgejo_admin/pal-e-services
  • [x] User Story — Marcus loading dashboard in Safari, public client (PKCE), admin role claim flow
  • [x] Context — accurately describes app.py:13-18 as browser-side streamlit-keycloak wrapper requiring public client
  • [x] File Targets — single file (terraform/k3s.tfvars) with explicit "files NOT to touch" list
  • [x] Acceptance Criteria — 9 testable items, each grep/tofu-verifiable
  • [x] Test Expectations — concrete tofu fmt -check, tofu validate, tofu plan -lock=false commands
  • [x] Constraints — explicit "copy westside-spa shape verbatim", -lock=false per feedback_tofu_lock_false, no unrelated client edits
  • [x] Checklist — present
  • [x] Related — links to project page, story, three feedback memories, superseded issue

Traceability

  • [x] story:reachable label — "Dashboard Reachable from Phone"
  • [x] story note verified — story-westside-streamlit-reachable (id 1401) exists; entry on project-westside-streamlit user-stories table at row "reachable"
  • [x] arch:services-tf label — services terraform layer (Keycloak clients, services map)
  • [~] arch note for services-tf — no dedicated arch-services-tf note exists in pal-e-docs. The project's arch-deployment-westside-streamlit covers the consuming side but not the platform-level terraform module shape. Same gap exists on sibling #1069. This is a board-wide convention gap, not a #1070 blocker.
  • [x] Forgejo issue — #61 open, body well-formed, no blocking comments
  • [x] Repo placement — fix is in pal-e-services and the issue is filed there. Consistent.

File Targets

  • [x] terraform/k3s.tfvars — verified file exists. westside-spa entry occupies lines 92-111 (issue says "around line 92" — accurate). Shape includes realm_key, client_id, name, public_client = true, pkce_code_challenge_method = "S256", backchannel_logout_session_required = false, valid_redirect_uris, web_origins. Copy-verbatim instruction is achievable.
  • [x] terraform/keycloak.tf (NOT to touch) — verified the resource iterates var.keycloak_clients, wires access_type = each.value.public_client ? "PUBLIC" : "CONFIDENTIAL" at line 142, pkce_code_challenge_method at line 150, valid_redirect_uris/web_origins at 152-153, and gates the realm-roles mapper on include_realm_roles_mapper at line 169. No schema changes needed — confirmed.
  • [x] terraform/variables.tf — schema for keycloak_clients map (lines 137-163) accepts every attribute the issue requires: public_client, pkce_code_challenge_method, valid_redirect_uris, web_origins, include_realm_roles_mapper. All optional with safe defaults.
  • [x] Existing westside-landing, westside-spa, pal-e-docs-landing entries — verified untouched scope per "files NOT to touch."
  • [x] grep -rn streamlit terraform/ — confirmed zero existing references; supersession lineage from #10 is accurate (no orphan terraform left behind).

Repo Placement

OK. Single repo, single file. No multi-repo coordination required. The Keycloak resource is in pal-e-services/terraform; the consuming app is in westside-streamlit; the issue correctly targets the producing repo.

Dependencies

  • Sibling, not blocker: #1069 (S1 — pal-e-services#60, add westside-streamlit to var.services). Both touch the same repo (pal-e-services) but different terraform files (k3s.tfvars Keycloak block vs k3s.tfvars services block). Migration-slot-style collision is possible if dispatched in parallel — recommend serializing dispatch (S1 first → tofu apply → S2) per feedback_migration_slot_coordination. Flag for Ava when promoting to todo.
  • Downstream: O1 onboarding (#1074) and the existing in-progress streamlit-keycloak QA items (#960, #961, #962) all depend on this client landing in Keycloak.
  • Supersedes: closed-without-code westside-streamlit#10 (board #960 in qa). Lineage explicitly documented in issue body.

Acceptance Criteria Assessment

Strong. All 9 ACs are programmatically verifiable:
  • tofu plan -lock=false output is the canonical evidence — provides create-plan diff for keycloak_openid_client.westside-streamlit
  • client_id, public_client, pkce_code_challenge_method, redirect URIs, web origins, and include_realm_roles_mapper are all observable in plan output
  • tofu validate + tofu fmt commands are concrete and CI-runnable
  • "No changes to westside-landing or westside-spa" is verifiable by grepping the plan diff for those keys

Blast Radius

  • No write side-effects on existing clients — the Keycloak Terraform provider creates new clients additively when keys are added to the map. Existing clients are untouched provided the dev agent does not modify their map entries.
  • Realm-roles mapper: include_realm_roles_mapper = true emits a separate keycloak_openid_user_realm_role_protocol_mapper resource (or equivalent in the terraform-keycloak provider). Adds realm_access.roles claim to JWT — required by app.py:26-31 admin gate.
  • Funnel auth gate (feedback_funnel_requires_auth): This Keycloak client is the auth verification mechanism for the eventual funnel exposure of westside-streamlit.tail5b443a.ts.net. Per the memory, any funnel ingress requires verified auth documented in the PR. This ticket lands the auth side; the corresponding funnel ingress lands separately (board #937 / westside-streamlit#6, currently backlog).
  • Ordering with funnel: Confirm Keycloak client merges and applies BEFORE the funnel ingress is exposed publicly — otherwise the canonical hostname returns the dashboard without auth for the gap window.

Decomposition Assessment

Single file, single map entry, 9 well-bounded ACs, copy-verbatim instruction. Estimated agent work: 5-15 minutes (write entry, run tofu fmt/validate/plan -lock=false, capture plan output for PR body, open PR). No decomposition needed.

Recommendation

Verdict: APPROVED. Ticket is ready to advance from backlog to todo. The two prior informal review rounds did the heavy lifting (locking public_client = true PKCE S256). Live verification confirms file targets, line references, schema support, and absence of existing streamlit references in terraform.
  • [SCOPE] Convention gap: arch-services-tf note does not exist. Same gap on sibling #1069. Recommend Ava create a single platform-level architecture note for the pal-e-services terraform module shape (Keycloak clients map, services map, realms) to close the traceability triangle for all arch:services-tf labels. Do NOT block this ticket on it.
  • [SCOPE] Dispatch ordering: serialize #1069 (S1) before #1070 (S2) to avoid simultaneous edits to terraform/k3s.tfvars. Same-file collision risk per feedback_migration_slot_coordination.
  • [SCOPE] Funnel sequencing: ensure board #937 (Tailscale funnel exposure) lands AFTER #1070 has merged and applied, so the public hostname is gated from first byte. Per feedback_funnel_requires_auth.
  • No [BODY] or [LABEL] changes required. Issue body and labels are clean.