Add Keycloak OmniAuth login with Authorization Code + PKCE #134

Merged

ldraney merged 3 commits from 115-keycloak-omniauth-login into main 2026-06-07 00:57:21 +00:00

Conversation 4 Commits 3 Files changed 12 +408 -16

ldraney commented 2026-06-07 00:37:40 +00:00

Owner

Copy link

Summary

• Wire up OmniAuth OpenID Connect for Keycloak SSO using Authorization Code + PKCE flow
• Session-based auth stores username, email, and realm roles from the ID token
• Authentication is NOT enforced globally -- login/logout UI only appears when KEYCLOAK_URL is configured

Changes

• Gemfile: Added omniauth_openid_connect (> 0.8) and omniauth-rails_csrf_protection (> 1.0)
• config/initializers/omniauth.rb: OmniAuth OpenID Connect strategy configured from KEYCLOAK_* env vars; guarded so it skips in dev without Keycloak; loads in test mode for spec support
• app/controllers/sessions_controller.rb: OmniAuth callback extracts username/email/realm_access roles into session; logout clears session and redirects through Keycloak end-session endpoint; failure handler with flash
• app/controllers/application_controller.rb: Added current_user, logged_in?, authenticate_user!, current_user_has_role?, keycloak_configured? helpers exposed to views
• config/routes.rb: Auth callback routes (GET+POST /auth/keycloak/callback), failure route, DELETE /logout
• app/views/layouts/application.html.erb: Auth bar header with username + Logout when logged in, Login button when not, only when Keycloak configured
• app/assets/stylesheets/application.css: AuthBar component styles (plain CSS)
• spec/support/omniauth.rb: OmniAuth test mode setup with mock Keycloak auth helper
• spec/requests/sessions_spec.rb: 7 request specs covering callback, roles, logout, failure, unauthenticated access

Test Plan

• Tests pass locally -- all 101 specs pass (94 existing + 7 new)
• Manual verification: deploy to staging with KEYCLOAK_* env vars, verify Login redirects to Keycloak
• Authenticate as each of 5 test users, verify username and roles in session
• Verify Logout redirects through Keycloak end-session endpoint
• Verify app works without auth when KEYCLOAK_URL is unset (local dev)

Review Checklist

• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive
• No Tailwind -- plain CSS only

Related Notes

• Closes #115
• landscaping-assistant -- landscaping assistant project

## Summary - Wire up OmniAuth OpenID Connect for Keycloak SSO using Authorization Code + PKCE flow - Session-based auth stores username, email, and realm roles from the ID token - Authentication is NOT enforced globally -- login/logout UI only appears when KEYCLOAK_URL is configured ## Changes - Gemfile: Added `omniauth_openid_connect` (~> 0.8) and `omniauth-rails_csrf_protection` (~> 1.0) - config/initializers/omniauth.rb: OmniAuth OpenID Connect strategy configured from KEYCLOAK_* env vars; guarded so it skips in dev without Keycloak; loads in test mode for spec support - app/controllers/sessions_controller.rb: OmniAuth callback extracts username/email/realm_access roles into session; logout clears session and redirects through Keycloak end-session endpoint; failure handler with flash - app/controllers/application_controller.rb: Added `current_user`, `logged_in?`, `authenticate_user!`, `current_user_has_role?`, `keycloak_configured?` helpers exposed to views - config/routes.rb: Auth callback routes (GET+POST `/auth/keycloak/callback`), failure route, DELETE `/logout` - app/views/layouts/application.html.erb: Auth bar header with username + Logout when logged in, Login button when not, only when Keycloak configured - app/assets/stylesheets/application.css: AuthBar component styles (plain CSS) - spec/support/omniauth.rb: OmniAuth test mode setup with mock Keycloak auth helper - spec/requests/sessions_spec.rb: 7 request specs covering callback, roles, logout, failure, unauthenticated access ## Test Plan - [x] Tests pass locally -- all 101 specs pass (94 existing + 7 new) - [ ] Manual verification: deploy to staging with KEYCLOAK_* env vars, verify Login redirects to Keycloak - [ ] Authenticate as each of 5 test users, verify username and roles in session - [ ] Verify Logout redirects through Keycloak end-session endpoint - [ ] Verify app works without auth when KEYCLOAK_URL is unset (local dev) ## Review Checklist - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive - [x] No Tailwind -- plain CSS only ## Related Notes - Closes #115 - `landscaping-assistant` -- landscaping assistant project

ldraney added 1 commit 2026-06-07 00:37:40 +00:00

Add Keycloak OmniAuth login with Authorization Code + PKCE ... 52890798ed

Wire up OmniAuth OpenID Connect for Keycloak SSO without enforcing
authentication globally. Session-based auth stores username, email,
and realm roles from the ID token. Login/logout links appear in the
layout header only when KEYCLOAK_URL is configured, keeping local dev
and CI unaffected.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-07 00:38:25 +00:00

Author

Owner

Copy link

QA Review -- PR #134

Findings

1. Duplicate keycloak_configured? definitions (low severity)

keycloak_configured? is defined in both ApplicationController (line 37, checks only KEYCLOAK_URL) and SessionsController (line 48, checks KEYCLOAK_URL and KEYCLOAK_REALM). The two definitions have different logic -- the ApplicationController version is less strict. The SessionsController private method shadows the inherited one within that controller, which is intentional but could cause confusion if a future contributor expects consistent behavior. Consider removing the SessionsController override and using the inherited one, or aligning the checks.

2. mock_keycloak_auth defined as a global method (nit)

In spec/support/omniauth.rb, mock_keycloak_auth is defined as a top-level method rather than inside an RSpec helper module. This works but pollutes the global namespace. A minor style point -- not blocking.

What looks good

• OmniAuth initializer correctly guards on all 4 env vars for production and falls back to test-mode config
• PKCE enabled, confidential client with server-side secret -- matches the Keycloak client config
• Session stores only username/email/roles (no sensitive tokens) -- correct for this phase
• Logout redirects through Keycloak end-session endpoint with post_logout_redirect_uri and client_id
• Auth is not enforced globally -- authenticate_user! exists but is not called in a before_action
• button_to with method: :post for login link respects CSRF protection
• data: { turbo: false } on login button prevents Turbo from intercepting the OAuth redirect
• 7 request specs cover the key flows; all 101 specs pass
• Plain CSS, no Tailwind -- matches project conventions
• keycloak_configured? exposed as view helper so the auth bar only renders when relevant

VERDICT: APPROVE

Two minor style nits, no functional issues. The implementation correctly handles the OmniAuth OIDC flow, session management, and graceful degradation when Keycloak is not configured.

## QA Review -- PR #134 ### Findings **1. Duplicate `keycloak_configured?` definitions (low severity)** `keycloak_configured?` is defined in both `ApplicationController` (line 37, checks only `KEYCLOAK_URL`) and `SessionsController` (line 48, checks `KEYCLOAK_URL` and `KEYCLOAK_REALM`). The two definitions have different logic -- the ApplicationController version is less strict. The SessionsController private method shadows the inherited one within that controller, which is intentional but could cause confusion if a future contributor expects consistent behavior. Consider removing the SessionsController override and using the inherited one, or aligning the checks. **2. `mock_keycloak_auth` defined as a global method (nit)** In `spec/support/omniauth.rb`, `mock_keycloak_auth` is defined as a top-level method rather than inside an RSpec helper module. This works but pollutes the global namespace. A minor style point -- not blocking. ### What looks good - OmniAuth initializer correctly guards on all 4 env vars for production and falls back to test-mode config - PKCE enabled, confidential client with server-side secret -- matches the Keycloak client config - Session stores only username/email/roles (no sensitive tokens) -- correct for this phase - Logout redirects through Keycloak end-session endpoint with `post_logout_redirect_uri` and `client_id` - Auth is not enforced globally -- `authenticate_user!` exists but is not called in a `before_action` - `button_to` with `method: :post` for login link respects CSRF protection - `data: { turbo: false }` on login button prevents Turbo from intercepting the OAuth redirect - 7 request specs cover the key flows; all 101 specs pass - Plain CSS, no Tailwind -- matches project conventions - `keycloak_configured?` exposed as view helper so the auth bar only renders when relevant ### VERDICT: APPROVE Two minor style nits, no functional issues. The implementation correctly handles the OmniAuth OIDC flow, session management, and graceful degradation when Keycloak is not configured.

ldraney referenced this pull request 2026-06-07 00:44:11 +00:00

Phase 1: Keycloak login with Authorization Code + PKCE (five test accounts) #115

ldraney commented 2026-06-07 00:46:34 +00:00

Author

Owner

Copy link

PR #134 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8.1.3 / OmniAuth OpenID Connect / Keycloak / RSpec request specs / Plain CSS

This PR adds Keycloak SSO via OmniAuth with Authorization Code + PKCE flow. The implementation is well-structured: session-based auth, graceful degradation when Keycloak is unconfigured, proper CSRF protection on the login button (button_to POST), and clean separation between the OmniAuth initializer, SessionsController, and ApplicationController helpers.

OmniAuth initializer (config/initializers/omniauth.rb):

• Guard logic is correct: middleware only loads when all four KEYCLOAK_* env vars are present OR in test mode.
• PKCE enabled, Authorization Code flow, correct scope.
• Test mode fallback with dummy issuer is a good pattern.

SessionsController:

• Role extraction path extra.raw_info.realm_access.roles is correct for Keycloak ID tokens.
• Logout properly clears Rails session AND redirects through Keycloak's end-session endpoint.
• Failure handler surfaces the OmniAuth error message via flash.

ApplicationController helpers:

• current_user returns a with_indifferent_access hash from session -- correct pattern.
• authenticate_user! is defined but not enforced globally -- intentional and documented.
• current_user_has_role? casts to string for comparison -- safe.

Layout (application.html.erb):

• Auth bar conditionally rendered only when keycloak_configured? -- correct.
• Login uses button_to with method: :post and data: { turbo: false } -- CSRF-safe, Turbo-safe.
• Logout uses button_to with method: :delete -- correct.

CSS:

• Plain CSS only, no Tailwind. Uses design tokens (--color-*, --spacing-*, --radius). Consistent with existing component patterns.

BLOCKERS

1. DRY violation in auth path: keycloak_configured? defined twice with divergent logic

ApplicationController#keycloak_configured? checks only ENV["KEYCLOAK_URL"].present?.
SessionsController#keycloak_configured? checks ENV["KEYCLOAK_URL"].present? && ENV["KEYCLOAK_REALM"].present?.

The child version silently shadows the parent. This is a DRY violation in a security-adjacent path. The view uses the ApplicationController version (checks only URL), but the logout redirect depends on KEYCLOAK_REALM being present. If KEYCLOAK_URL is set but KEYCLOAK_REALM is not, the auth bar renders (login button visible) but the initializer won't load the middleware (it requires all four vars), so clicking Login would hit a routing error.

Fix: Remove the private keycloak_configured? from SessionsController. The parent's version is the one that should be canonical. If you want the stricter four-var check, move it to ApplicationController and use it everywhere.

2. OmniAuth.config.allowed_request_methods includes :get -- CSRF risk on request phase

Line 36 of omniauth.rb: OmniAuth.config.allowed_request_methods = %i[post get]

This is a global setting. While the callback legitimately receives GET from Keycloak, adding :get to allowed_request_methods also means the request phase (/auth/keycloak) can be initiated via GET without a CSRF token. The omniauth-rails_csrf_protection gem specifically exists to block GET-initiated request phases to prevent CSRF login attacks (an attacker can craft a link that logs the victim into the attacker's account).

The callback route is handled by your Rails router (get "/auth/keycloak/callback") and does not go through OmniAuth's request phase, so you do NOT need :get in allowed_request_methods.

Fix: Remove :get from allowed_request_methods and keep only %i[post]:

OmniAuth.config.allowed_request_methods = %i[post]

The GET callback works because your Rails routes explicitly map GET /auth/keycloak/callback to sessions#create -- OmniAuth's allowed_request_methods only governs the request phase (/auth/keycloak), not the callback.

NITS

1. Logout URL missing KEYCLOAK_CLIENT_ID presence check

SessionsController#destroy guards on keycloak_configured? (which checks URL + REALM in the controller's version), then builds the logout URL using ENV["KEYCLOAK_CLIENT_ID"]. If CLIENT_ID is somehow nil, the URL would contain client_id= with an empty value. This is unlikely in practice (the initializer requires all four vars), but the guard and the usage are checking different subsets of env vars. Consolidating to a single four-var check in ApplicationController would close this gap.

2. Missing test for authenticate_user! redirect

The authenticate_user! before_action is defined but no spec verifies it. When a future controller adds before_action :authenticate_user!, there should be a spec showing that unauthenticated users get redirected to /auth/keycloak. Consider adding a spec now even if no controller uses it yet.

3. Missing test for current_user_has_role?

No spec exercises the role-checking helper. A quick unit-level test (or request spec that uses a controller leveraging this method) would be valuable.

4. Missing test for nil nickname fallback

SessionsController#create line 15: auth.info.nickname || auth.info.name. No spec covers the case where nickname is nil and name is used instead. This is a real possibility depending on Keycloak user configuration.

5. Session roles frozen at login time

Roles stored in the session at login are not refreshed until the user logs out and back in. If a Keycloak admin changes a user's roles, the app won't reflect it until re-authentication. This is standard for session-based auth, but worth documenting (perhaps a comment in current_user_has_role?).

6. params[:message] in failure flash

SessionsController#failure interpolates params[:message] into a flash string. Rails ERB auto-escapes this, so it is not an XSS vector. However, an attacker could craft a URL like /auth/failure?message=anything+they+want and the user would see "Login failed: anything they want" in the flash. Consider allowlisting known OmniAuth failure messages or at least truncating to a reasonable length.

7. OmniAuth.config.silence_get_warning in test support

spec/support/omniauth.rb line 2 sets silence_get_warning = true. This is fine for test mode but worth confirming this does not mask the GET request phase concern mentioned in Blocker #2.

SOP COMPLIANCE

• Branch named after issue: 115-keycloak-omniauth-login follows {issue-number}-{kebab-case-purpose}
• PR body has Summary, Changes, Test Plan, Related sections
• Related references issue #115 (Closes #115)
• Related does not reference a plan slug from pal-e-docs (minor -- may not apply to this project)
• No secrets committed -- env vars read at runtime, .env* in .gitignore, no hardcoded credentials
• No unnecessary file changes -- all 10 files are directly related to the auth feature
• Commit messages are descriptive (single commit with clear title)
• No Tailwind -- plain CSS with design tokens, consistent with project convention
• Test coverage exists -- 7 request specs covering happy path, role extraction, logout, failure, unauthenticated access

PROCESS OBSERVATIONS

• Deployment risk: Low-medium. The auth middleware is only loaded when env vars are present, so existing deployments without Keycloak are unaffected. First deploy with Keycloak vars will need manual verification of the full login/logout flow.
• Change failure risk: The GET allowed_request_methods concern (Blocker #2) is a genuine security issue that should be fixed before merge. The DRY violation (Blocker #1) creates divergence risk as the codebase grows.
• Documentation: The PR body is thorough. The inline comments in the initializer and controller are clear. Consider adding a docs/keycloak-auth.md in a follow-up for operational runbook (env var requirements, Keycloak realm setup, role mapping).
• Test coverage quality: Good for a first pass. The 7 specs cover the critical paths. The missing edge case tests (nits 2-4) are non-blocking but should be addressed soon.

VERDICT: NOT APPROVED

Two blockers must be addressed:

1. Remove the duplicate keycloak_configured? from SessionsController -- use the inherited version from ApplicationController (and consider making the ApplicationController version check all four vars).
2. Remove :get from OmniAuth.config.allowed_request_methods -- keep only %i[post]. The GET callback works through Rails routing, not OmniAuth's request phase handler.

## PR #134 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 8.1.3 / OmniAuth OpenID Connect / Keycloak / RSpec request specs / Plain CSS This PR adds Keycloak SSO via OmniAuth with Authorization Code + PKCE flow. The implementation is well-structured: session-based auth, graceful degradation when Keycloak is unconfigured, proper CSRF protection on the login button (`button_to` POST), and clean separation between the OmniAuth initializer, SessionsController, and ApplicationController helpers. **OmniAuth initializer (`config/initializers/omniauth.rb`):** - Guard logic is correct: middleware only loads when all four `KEYCLOAK_*` env vars are present OR in test mode. - PKCE enabled, Authorization Code flow, correct scope. - Test mode fallback with dummy issuer is a good pattern. **SessionsController:** - Role extraction path `extra.raw_info.realm_access.roles` is correct for Keycloak ID tokens. - Logout properly clears Rails session AND redirects through Keycloak's end-session endpoint. - Failure handler surfaces the OmniAuth error message via flash. **ApplicationController helpers:** - `current_user` returns a `with_indifferent_access` hash from session -- correct pattern. - `authenticate_user!` is defined but not enforced globally -- intentional and documented. - `current_user_has_role?` casts to string for comparison -- safe. **Layout (`application.html.erb`):** - Auth bar conditionally rendered only when `keycloak_configured?` -- correct. - Login uses `button_to` with `method: :post` and `data: { turbo: false }` -- CSRF-safe, Turbo-safe. - Logout uses `button_to` with `method: :delete` -- correct. **CSS:** - Plain CSS only, no Tailwind. Uses design tokens (`--color-*`, `--spacing-*`, `--radius`). Consistent with existing component patterns. ### BLOCKERS **1. DRY violation in auth path: `keycloak_configured?` defined twice with divergent logic** `ApplicationController#keycloak_configured?` checks only `ENV["KEYCLOAK_URL"].present?`. `SessionsController#keycloak_configured?` checks `ENV["KEYCLOAK_URL"].present? && ENV["KEYCLOAK_REALM"].present?`. The child version silently shadows the parent. This is a DRY violation in a security-adjacent path. The view uses the ApplicationController version (checks only URL), but the logout redirect depends on `KEYCLOAK_REALM` being present. If `KEYCLOAK_URL` is set but `KEYCLOAK_REALM` is not, the auth bar renders (login button visible) but the initializer won't load the middleware (it requires all four vars), so clicking Login would hit a routing error. **Fix:** Remove the private `keycloak_configured?` from `SessionsController`. The parent's version is the one that should be canonical. If you want the stricter four-var check, move it to ApplicationController and use it everywhere. **2. `OmniAuth.config.allowed_request_methods` includes `:get` -- CSRF risk on request phase** Line 36 of `omniauth.rb`: `OmniAuth.config.allowed_request_methods = %i[post get]` This is a global setting. While the callback legitimately receives GET from Keycloak, adding `:get` to `allowed_request_methods` also means the *request phase* (`/auth/keycloak`) can be initiated via GET without a CSRF token. The `omniauth-rails_csrf_protection` gem specifically exists to block GET-initiated request phases to prevent CSRF login attacks (an attacker can craft a link that logs the victim into the attacker's account). The callback route is handled by your Rails router (`get "/auth/keycloak/callback"`) and does not go through OmniAuth's request phase, so you do NOT need `:get` in `allowed_request_methods`. **Fix:** Remove `:get` from `allowed_request_methods` and keep only `%i[post]`: ```ruby OmniAuth.config.allowed_request_methods = %i[post] ``` The GET callback works because your Rails routes explicitly map `GET /auth/keycloak/callback` to `sessions#create` -- OmniAuth's `allowed_request_methods` only governs the request phase (`/auth/keycloak`), not the callback. ### NITS **1. Logout URL missing `KEYCLOAK_CLIENT_ID` presence check** `SessionsController#destroy` guards on `keycloak_configured?` (which checks URL + REALM in the controller's version), then builds the logout URL using `ENV["KEYCLOAK_CLIENT_ID"]`. If `CLIENT_ID` is somehow nil, the URL would contain `client_id=` with an empty value. This is unlikely in practice (the initializer requires all four vars), but the guard and the usage are checking different subsets of env vars. Consolidating to a single four-var check in ApplicationController would close this gap. **2. Missing test for `authenticate_user!` redirect** The `authenticate_user!` before_action is defined but no spec verifies it. When a future controller adds `before_action :authenticate_user!`, there should be a spec showing that unauthenticated users get redirected to `/auth/keycloak`. Consider adding a spec now even if no controller uses it yet. **3. Missing test for `current_user_has_role?`** No spec exercises the role-checking helper. A quick unit-level test (or request spec that uses a controller leveraging this method) would be valuable. **4. Missing test for nil nickname fallback** `SessionsController#create` line 15: `auth.info.nickname || auth.info.name`. No spec covers the case where `nickname` is nil and `name` is used instead. This is a real possibility depending on Keycloak user configuration. **5. Session roles frozen at login time** Roles stored in the session at login are not refreshed until the user logs out and back in. If a Keycloak admin changes a user's roles, the app won't reflect it until re-authentication. This is standard for session-based auth, but worth documenting (perhaps a comment in `current_user_has_role?`). **6. `params[:message]` in failure flash** `SessionsController#failure` interpolates `params[:message]` into a flash string. Rails ERB auto-escapes this, so it is not an XSS vector. However, an attacker could craft a URL like `/auth/failure?message=anything+they+want` and the user would see "Login failed: anything they want" in the flash. Consider allowlisting known OmniAuth failure messages or at least truncating to a reasonable length. **7. `OmniAuth.config.silence_get_warning` in test support** `spec/support/omniauth.rb` line 2 sets `silence_get_warning = true`. This is fine for test mode but worth confirming this does not mask the GET request phase concern mentioned in Blocker #2. ### SOP COMPLIANCE - [x] Branch named after issue: `115-keycloak-omniauth-login` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references issue #115 (`Closes #115`) - [ ] Related does not reference a plan slug from pal-e-docs (minor -- may not apply to this project) - [x] No secrets committed -- env vars read at runtime, `.env*` in `.gitignore`, no hardcoded credentials - [x] No unnecessary file changes -- all 10 files are directly related to the auth feature - [x] Commit messages are descriptive (single commit with clear title) - [x] No Tailwind -- plain CSS with design tokens, consistent with project convention - [x] Test coverage exists -- 7 request specs covering happy path, role extraction, logout, failure, unauthenticated access ### PROCESS OBSERVATIONS - **Deployment risk:** Low-medium. The auth middleware is only loaded when env vars are present, so existing deployments without Keycloak are unaffected. First deploy with Keycloak vars will need manual verification of the full login/logout flow. - **Change failure risk:** The GET allowed_request_methods concern (Blocker #2) is a genuine security issue that should be fixed before merge. The DRY violation (Blocker #1) creates divergence risk as the codebase grows. - **Documentation:** The PR body is thorough. The inline comments in the initializer and controller are clear. Consider adding a `docs/keycloak-auth.md` in a follow-up for operational runbook (env var requirements, Keycloak realm setup, role mapping). - **Test coverage quality:** Good for a first pass. The 7 specs cover the critical paths. The missing edge case tests (nits 2-4) are non-blocking but should be addressed soon. ### VERDICT: NOT APPROVED Two blockers must be addressed: 1. Remove the duplicate `keycloak_configured?` from `SessionsController` -- use the inherited version from `ApplicationController` (and consider making the ApplicationController version check all four vars). 2. Remove `:get` from `OmniAuth.config.allowed_request_methods` -- keep only `%i[post]`. The GET callback works through Rails routing, not OmniAuth's request phase handler.

ldraney added 1 commit 2026-06-07 00:48:06 +00:00

Fix QA blockers: remove duplicate keycloak_configured? and CSRF-unsafe GET ... c4dae6d500

Remove shadowed keycloak_configured? from SessionsController — the parent
method in ApplicationController is sufficient. Remove :get from OmniAuth
allowed_request_methods to prevent CSRF bypass on the request phase;
the GET callback works via Rails routes, not OmniAuth's request handler.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-07 00:48:19 +00:00

Author

Owner

Copy link

QA Review Fixes — c4dae6d

Both blockers from QA review addressed:

1. Removed duplicate keycloak_configured? from SessionsController — the parent method in ApplicationController is sufficient. The child was silently shadowing with stricter logic (checking KEYCLOAK_REALM in addition to KEYCLOAK_URL), creating inconsistency between view rendering and logout routing.

2. Removed :get from OmniAuth.config.allowed_request_methods — having GET in the allowed methods meant the OmniAuth request phase (/auth/keycloak) could be triggered via GET without a CSRF token, defeating omniauth-rails_csrf_protection. The GET callback works because Rails routes explicitly map GET /auth/keycloak/callback to sessions#create, not because of this OmniAuth setting.

Ready for re-review.

### QA Review Fixes — c4dae6d Both blockers from QA review addressed: 1. **Removed duplicate `keycloak_configured?`** from `SessionsController` — the parent method in `ApplicationController` is sufficient. The child was silently shadowing with stricter logic (checking `KEYCLOAK_REALM` in addition to `KEYCLOAK_URL`), creating inconsistency between view rendering and logout routing. 2. **Removed `:get` from `OmniAuth.config.allowed_request_methods`** — having GET in the allowed methods meant the OmniAuth *request phase* (`/auth/keycloak`) could be triggered via GET without a CSRF token, defeating `omniauth-rails_csrf_protection`. The GET callback works because Rails routes explicitly map `GET /auth/keycloak/callback` to `sessions#create`, not because of this OmniAuth setting. Ready for re-review.

ldraney added 1 commit 2026-06-07 00:49:47 +00:00

Update keycloak-setup and user-stories-auth docs to reflect completed work ... ff782e4ee9

keycloak-setup.md: Add landscaping realm and client to tables (5 realms,
6 clients). Update cross-repo changes table with DONE statuses and PR
numbers. Strike through completed follow-up tickets.

user-stories-auth.md: Fix "four test accounts" to "five" in related
tickets section.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-07 00:51:55 +00:00

Author

Owner

Copy link

PR #134 Re-Review

Re-review after two blocker fixes pushed in commits c4dae6d and ff782e4.

PREVIOUS BLOCKERS -- STATUS

1. DRY violation: duplicate keycloak_configured? -- RESOLVED

SessionsController no longer defines its own keycloak_configured?. The method is defined once in ApplicationController (line 36 of the diff), and SessionsController#destroy correctly inherits it. No divergent logic.

2. CSRF risk: :get in allowed_request_methods -- RESOLVED

config/initializers/omniauth.rb now sets OmniAuth.config.allowed_request_methods = %i[post] (POST only). The view uses button_to with method: :post and data: { turbo: false }, which generates a proper form with a CSRF token. The omniauth-rails_csrf_protection gem provides additional CSRF verification on the request phase. Solid.

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, OmniAuth OpenID Connect, RSpec, plain CSS.

OmniAuth / Auth patterns:

• Authorization Code + PKCE flow correctly configured (response_type: :code, pkce: true)
• Confidential client with KEYCLOAK_CLIENT_SECRET -- appropriate for server-side Rails app
• Session stores username, email, and realm roles from ID token -- no access token stored in session (good)
• authenticate_user! is defined but NOT enforced globally via before_action -- intentional per PR description, correct for Phase 1
• Realm roles extracted via auth.dig("extra", "raw_info") with nil-safe fallback (|| {}, || [])
• Feature-flagged behind KEYCLOAK_URL presence -- app works without auth when env var is unset
• Logout redirects through Keycloak end-session endpoint with post_logout_redirect_uri and client_id -- correct OIDC RP-initiated logout

Initializer guard logic:

• keycloak_configured checks all four env vars (URL, REALM, CLIENT_ID, CLIENT_SECRET)
• Middleware only registered when configured OR in test mode -- clean separation
• discovery: keycloak_configured skips OIDC discovery in test mode (no real IdP) -- correct

CSS:

• Uses existing design tokens throughout (--color-surface, --color-border, --color-accent, --spacing-sm, --radius, etc.)
• No magic numbers, no Tailwind -- plain CSS with theme tokens. Consistent with codebase style.
• font-size: 0.85rem and 0.8rem are reasonable for a utility bar, not magic numbers in this context

Test coverage (7 specs):

• Happy path: callback creates session with correct fields
• Role extraction: verifies realm_access roles from mock auth hash
• Logout: session cleared, Keycloak redirect when configured
• Failure: flash alert with error message
• Unauthenticated access: root and properties accessible without login
• Mock helper (mock_keycloak_auth) is reusable with keyword args for different user profiles
• Missing: no test for logout when KEYCLOAK_URL is NOT set (the else branch in destroy). This is a gap but not a blocker since the logic is trivial (redirect with notice).

View (application.html.erb):

• button_to generates a form with CSRF token -- correct for POST-only OmniAuth
• data: { turbo: false } prevents Turbo from intercepting the redirect to Keycloak -- correct
• Conditional rendering gated on keycloak_configured? -- no auth UI leaks without Keycloak

Routes:

• Both GET and POST callbacks registered for /auth/keycloak/callback -- GET is needed for the OmniAuth redirect after the IdP callback, POST is the standard OmniAuth callback path. Correct.
• DELETE /logout follows RESTful convention
• /auth/failure captures OmniAuth errors

Docs (commit ff782e4):

• keycloak-setup.md cross-repo table updated from "New ticket" to specific PR numbers with "DONE" status -- good traceability
• Realm count updated (4 to 5), client count updated (5 to 6), landscaping realm/client rows added
• Follow-up tickets updated to reflect completed work
• user-stories-auth.md corrected from "four test accounts" to "five test accounts"

BLOCKERS

None. Both previous blockers are resolved. No new blockers found.

NITS

1. Missing test for logout without Keycloak -- The else branch in SessionsController#destroy (redirect to root_path with notice "Signed out") has no spec coverage. Consider adding a test where KEYCLOAK_URL is nil to cover both branches.

2. CGI.escape in logout URL -- CGI.escape(root_url) and CGI.escape(ENV["KEYCLOAK_CLIENT_ID"]) are correct, but consider extracting the logout URL construction into a private method for readability, especially if id_token_hint is added later for full RP-initiated logout compliance.

3. Blank line before end in SessionsController -- Line 45 in the new file has a blank line before the final end. Minor style nit.

4. OmniAuth.config.silence_get_warning = true in test support -- This silences warnings about GET requests in test mode, which is fine, but worth a comment explaining why (OmniAuth test_mode uses GET internally for redirects even when POST is enforced).

SOP COMPLIANCE

• Branch named after issue (115-keycloak-omniauth-login matches issue #115)
• PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
• Related references issue #115
• No secrets committed (env vars used for all credentials, ENV.fetch with test-only defaults)
• No unnecessary file changes (all 12 files are directly related to auth implementation + docs)
• Commit messages are descriptive (3 commits: feature, fix, docs)
• No Tailwind -- plain CSS with design tokens

PROCESS OBSERVATIONS

• Clean three-commit history: feature, blocker fix, docs update. Good separation of concerns.
• Cross-repo work tracked in keycloak-setup.md with specific PR numbers -- excellent traceability for a multi-repo change.
• Test coverage at 7 specs for a new auth flow is adequate for Phase 1. The missing logout-without-keycloak test is worth adding but not blocking.
• The PR body's manual test plan checkboxes are still unchecked -- staging verification should happen post-merge as part of validation.

VERDICT: APPROVED

## PR #134 Re-Review Re-review after two blocker fixes pushed in commits `c4dae6d` and `ff782e4`. ### PREVIOUS BLOCKERS -- STATUS **1. DRY violation: duplicate `keycloak_configured?` -- RESOLVED** `SessionsController` no longer defines its own `keycloak_configured?`. The method is defined once in `ApplicationController` (line 36 of the diff), and `SessionsController#destroy` correctly inherits it. No divergent logic. **2. CSRF risk: `:get` in `allowed_request_methods` -- RESOLVED** `config/initializers/omniauth.rb` now sets `OmniAuth.config.allowed_request_methods = %i[post]` (POST only). The view uses `button_to` with `method: :post` and `data: { turbo: false }`, which generates a proper form with a CSRF token. The `omniauth-rails_csrf_protection` gem provides additional CSRF verification on the request phase. Solid. ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8.1, OmniAuth OpenID Connect, RSpec, plain CSS. **OmniAuth / Auth patterns:** - Authorization Code + PKCE flow correctly configured (`response_type: :code`, `pkce: true`) - Confidential client with `KEYCLOAK_CLIENT_SECRET` -- appropriate for server-side Rails app - Session stores username, email, and realm roles from ID token -- no access token stored in session (good) - `authenticate_user!` is defined but NOT enforced globally via `before_action` -- intentional per PR description, correct for Phase 1 - Realm roles extracted via `auth.dig("extra", "raw_info")` with nil-safe fallback (`|| {}`, `|| []`) - Feature-flagged behind `KEYCLOAK_URL` presence -- app works without auth when env var is unset - Logout redirects through Keycloak end-session endpoint with `post_logout_redirect_uri` and `client_id` -- correct OIDC RP-initiated logout **Initializer guard logic:** - `keycloak_configured` checks all four env vars (URL, REALM, CLIENT_ID, CLIENT_SECRET) - Middleware only registered when configured OR in test mode -- clean separation - `discovery: keycloak_configured` skips OIDC discovery in test mode (no real IdP) -- correct **CSS:** - Uses existing design tokens throughout (`--color-surface`, `--color-border`, `--color-accent`, `--spacing-sm`, `--radius`, etc.) - No magic numbers, no Tailwind -- plain CSS with theme tokens. Consistent with codebase style. - `font-size: 0.85rem` and `0.8rem` are reasonable for a utility bar, not magic numbers in this context **Test coverage (7 specs):** - Happy path: callback creates session with correct fields - Role extraction: verifies realm_access roles from mock auth hash - Logout: session cleared, Keycloak redirect when configured - Failure: flash alert with error message - Unauthenticated access: root and properties accessible without login - Mock helper (`mock_keycloak_auth`) is reusable with keyword args for different user profiles - Missing: no test for logout when KEYCLOAK_URL is NOT set (the `else` branch in `destroy`). This is a gap but not a blocker since the logic is trivial (redirect with notice). **View (application.html.erb):** - `button_to` generates a form with CSRF token -- correct for POST-only OmniAuth - `data: { turbo: false }` prevents Turbo from intercepting the redirect to Keycloak -- correct - Conditional rendering gated on `keycloak_configured?` -- no auth UI leaks without Keycloak **Routes:** - Both GET and POST callbacks registered for `/auth/keycloak/callback` -- GET is needed for the OmniAuth redirect after the IdP callback, POST is the standard OmniAuth callback path. Correct. - DELETE `/logout` follows RESTful convention - `/auth/failure` captures OmniAuth errors **Docs (commit ff782e4):** - `keycloak-setup.md` cross-repo table updated from "New ticket" to specific PR numbers with "DONE" status -- good traceability - Realm count updated (4 to 5), client count updated (5 to 6), landscaping realm/client rows added - Follow-up tickets updated to reflect completed work - `user-stories-auth.md` corrected from "four test accounts" to "five test accounts" ### BLOCKERS None. Both previous blockers are resolved. No new blockers found. ### NITS 1. **Missing test for logout without Keycloak** -- The `else` branch in `SessionsController#destroy` (redirect to `root_path` with notice "Signed out") has no spec coverage. Consider adding a test where `KEYCLOAK_URL` is nil to cover both branches. 2. **`CGI.escape` in logout URL** -- `CGI.escape(root_url)` and `CGI.escape(ENV["KEYCLOAK_CLIENT_ID"])` are correct, but consider extracting the logout URL construction into a private method for readability, especially if id_token_hint is added later for full RP-initiated logout compliance. 3. **Blank line before `end` in SessionsController** -- Line 45 in the new file has a blank line before the final `end`. Minor style nit. 4. **`OmniAuth.config.silence_get_warning = true` in test support** -- This silences warnings about GET requests in test mode, which is fine, but worth a comment explaining why (OmniAuth test_mode uses GET internally for redirects even when POST is enforced). ### SOP COMPLIANCE - [x] Branch named after issue (`115-keycloak-omniauth-login` matches issue #115) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related references issue #115 - [x] No secrets committed (env vars used for all credentials, `ENV.fetch` with test-only defaults) - [x] No unnecessary file changes (all 12 files are directly related to auth implementation + docs) - [x] Commit messages are descriptive (3 commits: feature, fix, docs) - [x] No Tailwind -- plain CSS with design tokens ### PROCESS OBSERVATIONS - Clean three-commit history: feature, blocker fix, docs update. Good separation of concerns. - Cross-repo work tracked in `keycloak-setup.md` with specific PR numbers -- excellent traceability for a multi-repo change. - Test coverage at 7 specs for a new auth flow is adequate for Phase 1. The missing logout-without-keycloak test is worth adding but not blocking. - The PR body's manual test plan checkboxes are still unchecked -- staging verification should happen post-merge as part of validation. ### VERDICT: APPROVED

ldraney merged commit 18de386898 into main 2026-06-07 00:57:21 +00:00

ldraney deleted branch 115-keycloak-omniauth-login 2026-06-07 00:57:21 +00:00

ldraney referenced this pull request from a commit 2026-06-07 00:57:22 +00:00

Add Keycloak OmniAuth login with Authorization Code + PKCE (#134)

ldraney referenced this pull request 2026-06-07 01:01:12 +00:00

Fix session specs to use POST for OmniAuth request phase #135

ldraney referenced this pull request 2026-06-07 01:07:39 +00:00

Role-based tab visibility, route enforcement, and audit trail #107

ldraney referenced this pull request from a commit 2026-06-07 01:48:06 +00:00

Fix stale references in feature flags doc

ldraney referenced this pull request 2026-06-07 01:48:21 +00:00

Fix stale references in feature flags doc #141

ldraney referenced this pull request 2026-06-07 01:51:24 +00:00

Fix stale references in feature flags doc #141

ldraney referenced this pull request 2026-06-07 03:54:02 +00:00

Spike: Auth smoke test, /healthz endpoint, post-deploy verification #149

ldraney referenced this pull request 2026-06-07 03:55:12 +00:00

Bug: Production 404 — authenticate_user! redirects to GET /auth/keycloak which OmniAuth rejects #150

ldraney referenced this pull request 2026-06-07 04:33:44 +00:00

Replace OmniAuth redirect with Keycloak Direct Access Grants #157

ldraney referenced this pull request 2026-06-07 20:01:16 +00:00

OmniAuth sends no redirect_uri to Keycloak — login broken #166

ldraney referenced this pull request 2026-06-08 04:18:37 +00:00

docs: update keycloak-setup.md with login theme architecture #175

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!134

No description provided.