pal-e-docs

Review: Add Keycloak auth with admin/parent/player roles

review-1605-2026-06-24-v3 Review

review ready
Project

Verdict: READY

Third review (v3). Previous reviews: review-1605-2026-06-24 (spike, NEEDS_REFINEMENT), review-1605-2026-06-24-v2 (feature rewrite, NEEDS_REFINEMENT with 6 recommendations). Issue has been fully rewritten to address all prior feedback. This review evaluates the final version.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Related to #56 (DNS/proxy) and #57 (Keycloak redirect URIs)
  • [x] Repo -- ldraney/westside-basketball
  • [x] User Story -- As any user, I want to log in securely and see only what my role allows
  • [x] Context -- thorough, explains no auth exists today, three roles needed, Keycloak server config explicitly out of scope
  • [x] File Targets -- 2 create, 3 modify, 3 do-not-touch
  • [x] Feature Flag -- "none" with justification (foundational infrastructure). No docs/feature-flags.md in repo, correct to skip.
  • [x] Acceptance Criteria -- 9 criteria
  • [x] Test Expectations -- 4 tests with run command
  • [x] Constraints -- 4 constraints, references landscaping-assistant pattern
  • [x] Checklist -- standard 3-item
  • [x] Related -- references landscaping-assistant, #56, #57, #55
All required sections present. Template is complete.

Traceability

  • [x] story:WS-AUTH label -- custom auth story label, appropriate for foundational auth work that spans multiple roles
  • [ ] story note MISSING -- WS-AUTH is not listed in the project-westside-basketball user-stories section. The standard stories use WS-S* numbering. [SCOPE] Create user story entry on project-westside-basketball user-stories section for WS-AUTH (e.g. "As any user, I want to log in securely and see only what my role allows so that admin pages are protected and each role gets an appropriate experience").
  • [x] arch:keycloak-oidc label -- references the Keycloak OIDC integration component
  • [ ] arch note MISSING -- no arch-keycloak-oidc note exists in pal-e-docs. [SCOPE] Create architecture note arch-keycloak-oidc for the Keycloak OIDC integration component. (Carryover from v1 and v2 reviews.)
  • [x] Forgejo issue -- ldraney/westside-basketball#54, state: open

File Targets

  • [x] app/controllers/sessions_controller.rb -- verified: does NOT exist (correct, to be created)
  • [x] app/controllers/concerns/authentication.rb -- verified: does NOT exist (correct, to be created). Concerns directory exists with .keep file.
  • [x] Gemfile -- verified: exists, no omniauth/keycloak gems present
  • [x] config/routes.rb -- verified: exists, has namespace :admin, no auth routes. Public routes and webhook route present.
  • [x] app/controllers/application_controller.rb -- verified: exists, no auth concerns included, inherits ActionController::Base
  • [x] app/controllers/webhooks_controller.rb -- verified: exists, inherits from ActionController::Base (NOT ApplicationController), already isolated from any before_action on ApplicationController. Correctly listed as do-not-touch.
  • [x] app/controllers/public_controller.rb -- verified: exists, inherits from ApplicationController. Will need skip_before_action for auth. Correctly listed as do-not-touch (auth exemption, not content modification). AC #5 explicitly covers this.
  • [x] db/schema.rb -- verified: exists, has coachrole enum. Correctly listed as do-not-touch (session-based auth, no migrations).
Note: config/initializers/omniauth.rb is not listed but will be implicitly created by the agent as standard Rails OmniAuth setup. This is acceptable -- the agent will follow the landscaping-assistant pattern referenced in Constraints.

Repo Placement

OK. Issue filed on ldraney/westside-basketball, all file targets are in that repo. Cross-repo Keycloak server config (client creation, redirect URIs) has been correctly split into separate tickets #56 and #57. This was a key finding from v2 review and is now properly addressed.

Dependencies

  • Downstream: Board item #1606 (westside-basketball#55 "Admin player list page") has depends:54 label -- correctly documented.
  • Downstream: Board item #1609 (westside-basketball#58 "Admin player detail page") has depends:55, which transitively depends on #54.
  • Infrastructure prerequisites: #56 (DNS/proxy via Hetzner) and #57 (Keycloak redirect URIs) are documented in Lineage. These are needed for production but not for development/testing -- the agent can develop and test against a dev Keycloak instance.
  • No blockers: Nothing in in_progress or next_up blocks this ticket. All three auth-story items (#1605, #1607, #1608) are in backlog.

Acceptance Criteria

9 acceptance criteria. All are testable by an agent via controller tests:
  • AC 1-5: Standard OmniAuth setup verification -- gem presence, helper availability, default protection, exemptions. All verifiable via code inspection and controller tests.
  • AC 6: Admin namespace restriction -- verifiable via controller test (role-based access).
  • AC 7: Login redirects to root path -- simplified from v2's problematic /admin/players, /dashboard, /profile targets. Now correctly points to existing root_path. Downstream tickets handle role-specific dashboards.
  • AC 8-9: Logout and unauthenticated user behavior -- standard auth flow, verifiable via controller tests.
All criteria are well-scoped to this repo and testable. No cross-repo AC remain.

Blast Radius

Controlled. before_action :authenticate_user! in ApplicationController affects all inheriting controllers:
  • WelcomeController, PlayersController, EmailsController -- will require auth (correct, these are admin-facing)
  • PublicController -- explicitly exempted in AC #5 and do-not-touch list
  • WebhooksController -- inherits from ActionController::Base, NOT ApplicationController. Already isolated. AC #4 covers this.
  • Admin controllers (payments, roster_report, delinquency_report) -- will require auth AND admin role (AC #6)
  • Health check (/up) -- uses Rails::HealthController, does not inherit ApplicationController. No impact.
Blast radius is well-documented and all edge cases are covered.

Decomposition Assessment

9 AC in 1 repo, 5 file targets (2 create + 3 modify). Estimated agent work: 3-5 minutes. This is a single coherent unit -- OmniAuth gem + auth concern + sessions controller + route config + tests. No decomposition needed.
V2 review flagged 12 AC across 2 repos. The rewrite reduced to 9 AC in 1 repo by splitting Keycloak server config into #56/#57. Well within the 5-minute rule.

V2 Findings Resolution

V2 Finding Status
[LABEL] story:WS-S7 mismatch RESOLVED -- relabeled to story:WS-AUTH
[SCOPE] arch-keycloak-oidc missing OPEN -- still needs creation (carryover)
[BODY] AC #2/#3 cross-repo Keycloak config RESOLVED -- split to #56/#57
[BODY] AC #8 redirect targets don't exist RESOLVED -- simplified to root_path
[BODY] public_controller.rb exemption RESOLVED -- added to do-not-touch + AC #5
[DECOMPOSE] 12 AC across 2 repos RESOLVED -- 9 AC in 1 repo

Recommendation

All [BODY], [LABEL], and [DECOMPOSE] issues from v2 have been resolved. Two [SCOPE] items remain as non-blocking documentation tasks:
  • [SCOPE] Create user story entry WS-AUTH on project-westside-basketball user-stories section.
  • [SCOPE] Create architecture note arch-keycloak-oidc in pal-e-docs. (Carryover from v1 and v2.)
These are backing-note creation tasks that do not affect the ticket's scope, file targets, or agent executability. The ticket itself is well-scoped, all file targets verified, AC are testable, blast radius documented, and fits within a single agent pass. Verdict: READY.