pal-e-docs

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

review-1605-2026-06-24-r2 Review

review approved
Project

Verdict: APPROVED

Re-review after refinement. The three changes (roles corrected to admin/coach/player, dependency direction fixed to depends:57, context updated to reference #57 for client creation) are all correct and do not introduce new issues. Scope and file targets are unchanged from the previously approved version.

Template Completeness

  • [x] Type (Feature)
  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Feature Flag (none -- foundational, appropriate)
  • [x] Acceptance Criteria (9 criteria)
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:WS-AUTH label -- foundational auth infrastructure story
  • [ ] story note MISSING -- [SCOPE] WS-AUTH not found in project-westside-basketball user-stories section. The section uses numbered stories (WS-S1 through WS-S33). Create a WS-AUTH entry or map to an existing numbered story.
  • [x] arch:keycloak-oidc label -- Keycloak OIDC component
  • [ ] arch note MISSING -- [SCOPE] Create architecture note arch-keycloak-oidc for the Keycloak OIDC integration component.
  • [x] Forgejo issue -- ldraney/westside-basketball#54, state: open

File Targets

  • [x] app/controllers/sessions_controller.rb -- verified: does not exist (to be created). Landscaping-assistant has this file as reference.
  • [x] app/controllers/concerns/authentication.rb -- verified: concerns/ directory exists but is empty (to be created). Note: landscaping-assistant puts auth directly in application_controller.rb; concern approach is cleaner.
  • [x] Gemfile -- verified: exists, no OmniAuth/Keycloak gems present. Landscaping-assistant uses omniauth_openid_connect ~0.8 and omniauth-rails_csrf_protection ~1.0.
  • [x] config/routes.rb -- verified: exists, has admin namespace (lines 12-20) and public routes (lines 32-39), no auth routes yet.
  • [x] app/controllers/application_controller.rb -- verified: exists, currently bare (only allow_browser and stale_when_importmap_changes).
  • [x] app/controllers/webhooks_controller.rb -- verified: exists, inherits from ActionController::Base (NOT ApplicationController). Naturally bypasses any before_action on ApplicationController. Correctly listed as "should NOT touch."
  • [x] app/controllers/public_controller.rb -- verified: exists, inherits from ApplicationController. See recommendation below re: conflict with "should NOT touch" vs AC #5.

Repo Placement

Correct. Rails auth work is in westside-basketball. Keycloak client creation is in pal-e-services (#57). No cross-repo confusion. Issue filed on westside-basketball, all file targets in westside-basketball.

Dependencies

  • #57 (Add westside-basketball Keycloak client and seed admin user) -- board item #1608, column: backlog, labels: depends:56,sprint:4. This ticket depends on #57 (client must exist before Rails can authenticate against it). Direction CORRECT after refinement.
  • #56 (Set up westsidekingsandqueens.com via Hetzner edge proxy) -- board item #1607, column: next_up, sprint:3. #57 depends on #56 for public redirect URIs. Transitive dependency.
  • #55 (Admin player list page) -- board item #1606, column: backlog, labels: depends:54,sprint:5. Downstream consumer that depends on THIS ticket for auth/role checks.
  • Full dependency chain verified: #56 (DNS, sprint:3) -> #57 (Keycloak client, sprint:4) -> #54 (Rails auth, sprint:4) -> #55 (admin player list, sprint:5). All directions correct, sprint ordering consistent.

Acceptance Criteria

9 acceptance criteria, all agent-verifiable:
  • AC 1-3: OmniAuth config, current_user, before_action -- verifiable via code inspection and test run
  • AC 4: Webhooks exemption -- automatically satisfied (WebhooksController inherits from ActionController::Base, not ApplicationController)
  • AC 5: Public controller exemption -- requires skip_before_action in public_controller.rb (see recommendation)
  • AC 6: Admin namespace restricted to admin role -- verifiable via controller test
  • AC 7-9: Login redirect, logout, unauthenticated redirect -- verifiable via controller tests
4 test expectations with run command: rails test test/controllers/sessions_controller_test.rb. Test directory exists at test/controllers/ with existing test files for admin/, players, public, and webhooks controllers.

Blast Radius

  • Landscaping-assistant has a working Keycloak OIDC implementation (reference pattern confirmed: omniauth_openid_connect gem, sessions_controller.rb, auth helpers in application_controller.rb).
  • No other westside-basketball controllers will break: webhooks inherits from Base (safe), public inherits from ApplicationController (needs skip_before_action). Controllers that will correctly gain auth: emails_controller, players_controller, welcome_controller, admin/payments_controller, admin/roster_report_controller.
  • The coaches table role enum mentioned in context was not found in app/models/ -- likely referring to Keycloak roles, not database roles. No conflict.

Decomposition Assessment

2 new files + 3 modified files, all in one repo. 9 AC but tightly coupled (single auth integration). Estimated agent work: 3-4 minutes. No decomposition needed.

Refinement Changes Verified

  • Roles corrected to admin/coach/player: Matches Keycloak realm definition confirmed in #57 body ("roles: admin, coach, player"). CORRECT.
  • Dependency direction fixed to depends:57: #57 creates the Keycloak client in pal-e-services, #54 consumes it in Rails. Client must exist before Rails auth. CORRECT.
  • Context references #57: Issue body says "Keycloak client creation and user seeding is handled by #57 (pal-e-services)." Matches #57's scope exactly. CORRECT.

Recommendation

  • [BODY] Minor conflict: public_controller.rb is listed under "Files the agent should NOT touch" but AC #5 requires it to be exempted from auth. Since PublicController inherits from ApplicationController, it will need at minimum a skip_before_action :authenticate_user! line added. Either move public_controller.rb to the "should modify" list with a note about adding skip_before_action, or clarify that exemption is handled via routing/concern logic. Non-blocking.
  • [SCOPE] Create user story entry WS-AUTH on project-westside-basketball user-stories section (currently only has numbered WS-S* stories).
  • [SCOPE] Create architecture note arch-keycloak-oidc for the Keycloak OIDC integration component.