Replace OmniAuth redirect with Keycloak Direct Access Grants #158

Merged
ldraney merged 1 commit from 157-direct-access-grants into main 2026-06-07 16:47:36 +00:00
Owner

Summary

Remove OmniAuth gems and middleware entirely. Replace with Direct Access Grants (ROPC): the login form lives in the Rails app, SessionsController#create POSTs credentials to Keycloak's token endpoint server-side via Net::HTTP, decodes the JWT to extract roles, and stores them in the session. No redirect to Keycloak's UI.

Changes

  • Gemfile / Gemfile.lock: removed omniauth, omniauth_openid_connect, omniauth-rails_csrf_protection and all transitive deps
  • config/initializers/omniauth.rb: deleted (OmniAuth middleware no longer needed)
  • config/routes.rb: replaced /auth/keycloak/* callback routes with post "/login" direct route
  • app/controllers/sessions_controller.rb: rewritten -- create action accepts username/password, POSTs to Keycloak token endpoint, decodes JWT payload, stores session. Includes error handling for bad credentials, blank input, and Keycloak unavailability
  • app/views/sessions/new.html.erb: replaced Keycloak redirect button with username + password form fields
  • app/views/layouts/application.html.erb: changed auth-bar login from button_to "/auth/keycloak" to link_to login_path
  • app/assets/stylesheets/application.css: added styles for .login-form, .login-field, .login-label, .login-input
  • spec/support/omniauth.rb: rewritten -- sign_in_as now stubs Net::HTTP.post_form with a fake JWT and POSTs to /login
  • spec/requests/sessions_spec.rb: rewritten for direct grants flow (valid/invalid/blank credentials, Keycloak unavailable, logout)
  • spec/requests/role_access_spec.rb: fixed redirect expectation from /auth/keycloak to /login
  • spec/requests/crew_spec.rb: fixed redirect expectation from /auth/keycloak to /login
  • docs/app-architecture.md: updated auth flow diagram, description, and design decisions
  • docs/keycloak-setup.md: updated ROPC section to reflect the decision, updated client config and setup instructions

Test Plan

  • All 230 specs pass (bundle exec rspec)
  • Login page shows username/password fields (no Keycloak redirect button)
  • POST /login with valid credentials authenticates and redirects to app
  • POST /login with invalid credentials shows error on login page
  • All 5 test users can log in
  • Role-based tab visibility works after login
  • Logout clears session and redirects to root
  • No OmniAuth gems or config remain

Review Checklist

  • No OmniAuth gems or config remain
  • No secrets committed
  • No unrelated changes
  • All specs pass (230/230)
  • Docs updated (app-architecture.md, keycloak-setup.md)
  • Closes #157
  • Pre-req: pal-e-services PR to enable direct_access_grants_enabled on Keycloak client
## Summary Remove OmniAuth gems and middleware entirely. Replace with Direct Access Grants (ROPC): the login form lives in the Rails app, SessionsController#create POSTs credentials to Keycloak's token endpoint server-side via Net::HTTP, decodes the JWT to extract roles, and stores them in the session. No redirect to Keycloak's UI. ## Changes - `Gemfile` / `Gemfile.lock`: removed `omniauth`, `omniauth_openid_connect`, `omniauth-rails_csrf_protection` and all transitive deps - `config/initializers/omniauth.rb`: deleted (OmniAuth middleware no longer needed) - `config/routes.rb`: replaced `/auth/keycloak/*` callback routes with `post "/login"` direct route - `app/controllers/sessions_controller.rb`: rewritten -- `create` action accepts username/password, POSTs to Keycloak token endpoint, decodes JWT payload, stores session. Includes error handling for bad credentials, blank input, and Keycloak unavailability - `app/views/sessions/new.html.erb`: replaced Keycloak redirect button with username + password form fields - `app/views/layouts/application.html.erb`: changed auth-bar login from `button_to "/auth/keycloak"` to `link_to login_path` - `app/assets/stylesheets/application.css`: added styles for `.login-form`, `.login-field`, `.login-label`, `.login-input` - `spec/support/omniauth.rb`: rewritten -- `sign_in_as` now stubs `Net::HTTP.post_form` with a fake JWT and POSTs to `/login` - `spec/requests/sessions_spec.rb`: rewritten for direct grants flow (valid/invalid/blank credentials, Keycloak unavailable, logout) - `spec/requests/role_access_spec.rb`: fixed redirect expectation from `/auth/keycloak` to `/login` - `spec/requests/crew_spec.rb`: fixed redirect expectation from `/auth/keycloak` to `/login` - `docs/app-architecture.md`: updated auth flow diagram, description, and design decisions - `docs/keycloak-setup.md`: updated ROPC section to reflect the decision, updated client config and setup instructions ## Test Plan - [x] All 230 specs pass (`bundle exec rspec`) - [ ] Login page shows username/password fields (no Keycloak redirect button) - [ ] POST /login with valid credentials authenticates and redirects to app - [ ] POST /login with invalid credentials shows error on login page - [ ] All 5 test users can log in - [ ] Role-based tab visibility works after login - [ ] Logout clears session and redirects to root - [ ] No OmniAuth gems or config remain ## Review Checklist - [x] No OmniAuth gems or config remain - [x] No secrets committed - [x] No unrelated changes - [x] All specs pass (230/230) - [x] Docs updated (app-architecture.md, keycloak-setup.md) ## Related Notes - Closes #157 - Pre-req: pal-e-services PR to enable `direct_access_grants_enabled` on Keycloak client ## Related - Closes #157
Replace OmniAuth redirect flow with Keycloak Direct Access Grants (ROPC)
Some checks failed
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
CI / scan_ruby (pull_request) Has been cancelled
CI / scan_js (pull_request) Has been cancelled
CI / lint (pull_request) Has been cancelled
00a6d2ab03
Remove omniauth, omniauth_openid_connect, and omniauth-rails_csrf_protection
gems and all associated middleware/initializer/callback routes. Replace with
a direct grant flow: the login form lives in the Rails app, and
SessionsController#create POSTs credentials to Keycloak's token endpoint
server-side via Net::HTTP. JWT payload is decoded (Base64, no verification)
to extract username, email, and realm_access roles for the session.

Closes #157

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

QA Review -- PR #158

Scope Check

PR matches issue #157 requirements. All items from the acceptance criteria are addressed:

  • OmniAuth gems removed (omniauth, omniauth_openid_connect, omniauth-rails_csrf_protection)
  • OmniAuth initializer deleted
  • OmniAuth callback routes removed
  • SessionsController rewritten for Direct Access Grants
  • Login view has username + password fields
  • Specs rewritten and all 230 pass
  • Docs updated (app-architecture.md, keycloak-setup.md)

Findings

No blocking issues found.

Minor observations (non-blocking):

  1. spec/support/omniauth.rb filename: The file still carries the omniauth name despite no longer having any OmniAuth dependency. Consider renaming to auth_helpers.rb or keycloak_helpers.rb for clarity. Non-blocking -- the file works correctly.

  2. flash.now[:alert] in view template: The view checks flash.now[:alert] || flash[:alert]. In ERB templates, flash.now is only accessible during the current request (when render is called), while flash[:alert] persists across redirects. The flash.now[:alert] check in the view is technically redundant since Rails merges flash.now into the flash hash for the current render -- flash[:alert] alone would catch both cases. Non-blocking, works correctly as-is.

  3. Logout simplification: The old logout redirected through Keycloak's end-session endpoint to terminate the SSO session. The new logout only clears the Rails session. This is correct for ROPC (there is no SSO session to terminate since the user never authenticated with Keycloak's browser flow), but worth noting for reviewers.

  4. failure action removed: The old GET /auth/failure route and action are gone. Correct -- OmniAuth failure callbacks are no longer relevant. Error handling now lives inline in create.

Checklist

  • No OmniAuth gems or middleware remain
  • No secrets committed
  • No unrelated file changes (db/schema.rb correctly excluded from commit)
  • All 230 specs pass
  • Routes are clean (GET /login, POST /login, DELETE /logout)
  • Error handling covers: blank input, invalid credentials, Keycloak unavailable
  • JWT decoding handles Base64url padding correctly
  • Test helper (sign_in_as) exercises the full controller flow via stubbed HTTP
  • Docs updated consistently across both files
  • Layout auth-bar updated from button_to to link_to

VERDICT: APPROVE

## QA Review -- PR #158 ### Scope Check PR matches issue #157 requirements. All items from the acceptance criteria are addressed: - OmniAuth gems removed (omniauth, omniauth_openid_connect, omniauth-rails_csrf_protection) - OmniAuth initializer deleted - OmniAuth callback routes removed - SessionsController rewritten for Direct Access Grants - Login view has username + password fields - Specs rewritten and all 230 pass - Docs updated (app-architecture.md, keycloak-setup.md) ### Findings **No blocking issues found.** Minor observations (non-blocking): 1. **`spec/support/omniauth.rb` filename**: The file still carries the `omniauth` name despite no longer having any OmniAuth dependency. Consider renaming to `auth_helpers.rb` or `keycloak_helpers.rb` for clarity. Non-blocking -- the file works correctly. 2. **`flash.now[:alert]` in view template**: The view checks `flash.now[:alert] || flash[:alert]`. In ERB templates, `flash.now` is only accessible during the current request (when `render` is called), while `flash[:alert]` persists across redirects. The `flash.now[:alert]` check in the view is technically redundant since Rails merges `flash.now` into the flash hash for the current render -- `flash[:alert]` alone would catch both cases. Non-blocking, works correctly as-is. 3. **Logout simplification**: The old logout redirected through Keycloak's end-session endpoint to terminate the SSO session. The new logout only clears the Rails session. This is correct for ROPC (there is no SSO session to terminate since the user never authenticated with Keycloak's browser flow), but worth noting for reviewers. 4. **`failure` action removed**: The old `GET /auth/failure` route and action are gone. Correct -- OmniAuth failure callbacks are no longer relevant. Error handling now lives inline in `create`. ### Checklist - [x] No OmniAuth gems or middleware remain - [x] No secrets committed - [x] No unrelated file changes (db/schema.rb correctly excluded from commit) - [x] All 230 specs pass - [x] Routes are clean (GET /login, POST /login, DELETE /logout) - [x] Error handling covers: blank input, invalid credentials, Keycloak unavailable - [x] JWT decoding handles Base64url padding correctly - [x] Test helper (`sign_in_as`) exercises the full controller flow via stubbed HTTP - [x] Docs updated consistently across both files - [x] Layout auth-bar updated from button_to to link_to **VERDICT: APPROVE**
Author
Owner

PR #158 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, RSpec request specs, Keycloak OIDC, vanilla CSS, ERB views.

Security (auth rewrite -- critical path)

  1. Credentials not logged: The controller logs e.class and e.message on connection errors, but never logs username or password. The Keycloak error response body (error_description) is displayed to the user but does not contain credentials. PASS.

  2. Credentials not stored: Credentials are used only as parameters to Net::HTTP.post_form and are not persisted to session, database, or cache. PASS.

  3. CSRF protection: The login form uses form_with, which includes the Rails CSRF token by default. SessionsController inherits from ApplicationController < ActionController::Base, which has protect_from_forgery enabled by default. PASS.

  4. JWT decode approach: decode_jwt_payload does Base64url decode only, no signature verification. The comment explains the rationale -- the token comes from a direct Net::HTTP.post_form call to Keycloak, not from a client/browser, so it is trusted server-to-server communication. This is acceptable for ROPC where the server receives the token directly over HTTPS. PASS.

  5. JWT padding: The padding logic "=" * ((4 - len % 4) % 4) is correct for Base64url. PASS.

  6. No credentials in Gemfile/code: KEYCLOAK_CLIENT_SECRET and all credentials are read from ENV. No hardcoded values. PASS.

Session structure compatibility

The new session[:user] hash uses {username:, email:, roles:} with symbol keys, matching the existing current_user, current_user_has_role?, and visible_tabs methods in ApplicationController. The with_indifferent_access call on line 41 of ApplicationController means both string and symbol access works. PASS.

OmniAuth removal completeness

  • Gemfile: omniauth_openid_connect and omniauth-rails_csrf_protection removed. PASS.
  • Gemfile.lock: All transitive deps removed (faraday, hashie, json-jwt, openid_connect, rack-oauth2, rack-protection, swd, webfinger, etc.). PASS.
  • config/initializers/omniauth.rb: Deleted entirely. PASS.
  • config/routes.rb: /auth/keycloak/* and /auth/failure routes removed, replaced with post "/login". PASS.
  • SessionsController#failure action: Removed. PASS.
  • spec/support/omniauth.rb: Fully rewritten -- no OmniAuth references remain. No OmniAuth.config.test_mode, no OmniAuth::AuthHash. PASS.

Logout simplification

The old logout redirected to Keycloak's end-session endpoint to terminate the SSO session. With ROPC, there is no Keycloak browser session to terminate -- the user authenticated via server-side credential exchange, so a simple session.delete(:user) is correct. PASS.

Error handling

  • Blank credentials: returns 422 with message. PASS.
  • Invalid credentials (Keycloak returns error): returns 422 with Keycloak's error_description. PASS.
  • Keycloak unreachable: catches SocketError, Errno::ECONNREFUSED, Net::OpenTimeout, Net::ReadTimeout and returns 503. PASS.
  • JSON parse error: NOT caught. If Keycloak returns non-JSON (e.g., HTML error page from a proxy), JSON.parse(response.body) will raise JSON::ParserError and bubble up as a 500.

CSS

New login form styles use CSS custom properties (--spacing-md, --color-border, --radius, etc.) consistently with the existing design system. No magic numbers except the 0.15s transition and the rgba(37, 99, 235, 0.15) focus ring -- these are reasonable inline values for micro-interactions. PASS.

BLOCKERS

None. This is a clean, well-executed auth rewrite.

NITS

  1. flash.now in view template (app/views/sessions/new.html.erb): The condition flash.now[:alert] || flash[:alert] is misleading. When flash.now[:alert] = "..." is set in the controller, the value is accessible in the view via flash[:alert] (Rails merges flash.now into the flash for the current request render). Calling flash.now[:alert] in the view returns a FlashNow proxy lookup that is redundant. The simpler flash[:alert] alone covers both flash.now and regular flash. Not a bug (it works because flash.now in a view still delegates to the underlying hash), but it is confusing to future readers.

  2. JSON::ParserError not caught (app/controllers/sessions_controller.rb): If Keycloak returns non-JSON (e.g., an HTML error page from a reverse proxy), JSON.parse(response.body) will raise an unhandled exception and produce a 500. Consider adding JSON::ParserError to the rescue clause with a user-friendly message like "Unexpected response from authentication service."

  3. Stale comment in ApplicationController (line 48): # Redirect unauthenticated users to Keycloak. -- this comment still references Keycloak, but the redirect now goes to /login. The behavior is unchanged (still calls redirect_to login_path), but the comment is misleading. Consider updating to # Redirect unauthenticated users to the login page.

  4. with_keycloak helper duplication: The with_keycloak helper is defined identically in spec/requests/crew_spec.rb, spec/requests/role_access_spec.rb, and potentially other spec files. It could be extracted to spec/support/ as a shared helper. Not introduced by this PR (it existed before), but worth noting for future cleanup.

  5. net/http require: The controller uses Net::HTTP.post_form without an explicit require "net/http". Rails autoloads it in practice, but an explicit require at the top of the controller would be more defensive. Very minor -- Rails has always loaded net/http transitively.

SOP COMPLIANCE

  • Branch named after issue: 157-direct-access-grants follows {issue-number}-{kebab-case-purpose} convention
  • PR body follows template: has Summary, Changes, Test Plan, Related sections
  • Related references issue: Closes #157
  • No secrets committed: all credentials via ENV
  • No unrelated changes (scope is tight: auth rewrite + docs + tests)
  • Tests exist: sessions_spec covers valid/invalid/blank credentials, Keycloak unavailable, logout, login page rendering
  • Docs updated: app-architecture.md and keycloak-setup.md both updated comprehensively

PROCESS OBSERVATIONS

  • Change failure risk: LOW. The auth rewrite is self-contained. The session structure is unchanged, so all downstream role-checking code (require_role, visible_tabs, current_user_has_role?) continues to work without modification. The test suite covers the critical paths.
  • Pre-requisite dependency: The PR body correctly notes that direct_access_grants_enabled = true must be set on the Keycloak client in pal-e-services. This is a cross-repo dependency -- the Keycloak terraform change should land before or simultaneously with this PR.
  • Documentation quality: The docs updates are thorough. The keycloak-setup.md changes correctly reflect the decision reversal from "ROPC rejected" to "ROPC adopted" with clear rationale. The sequence diagram in app-architecture.md is updated accurately.
  • Net reduction: -311 lines, +254 lines. Removing OmniAuth and its transitive dependency tree is a meaningful simplification. The auth flow is now stdlib-only (Net::HTTP, Base64, JSON).

VERDICT: APPROVED

## PR #158 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails 8.1, RSpec request specs, Keycloak OIDC, vanilla CSS, ERB views. **Security (auth rewrite -- critical path)** 1. **Credentials not logged**: The controller logs `e.class` and `e.message` on connection errors, but never logs `username` or `password`. The Keycloak error response body (`error_description`) is displayed to the user but does not contain credentials. PASS. 2. **Credentials not stored**: Credentials are used only as parameters to `Net::HTTP.post_form` and are not persisted to session, database, or cache. PASS. 3. **CSRF protection**: The login form uses `form_with`, which includes the Rails CSRF token by default. `SessionsController` inherits from `ApplicationController < ActionController::Base`, which has `protect_from_forgery` enabled by default. PASS. 4. **JWT decode approach**: `decode_jwt_payload` does Base64url decode only, no signature verification. The comment explains the rationale -- the token comes from a direct `Net::HTTP.post_form` call to Keycloak, not from a client/browser, so it is trusted server-to-server communication. This is acceptable for ROPC where the server receives the token directly over HTTPS. PASS. 5. **JWT padding**: The padding logic `"=" * ((4 - len % 4) % 4)` is correct for Base64url. PASS. 6. **No credentials in Gemfile/code**: `KEYCLOAK_CLIENT_SECRET` and all credentials are read from ENV. No hardcoded values. PASS. **Session structure compatibility** The new `session[:user]` hash uses `{username:, email:, roles:}` with symbol keys, matching the existing `current_user`, `current_user_has_role?`, and `visible_tabs` methods in `ApplicationController`. The `with_indifferent_access` call on line 41 of `ApplicationController` means both string and symbol access works. PASS. **OmniAuth removal completeness** - `Gemfile`: `omniauth_openid_connect` and `omniauth-rails_csrf_protection` removed. PASS. - `Gemfile.lock`: All transitive deps removed (faraday, hashie, json-jwt, openid_connect, rack-oauth2, rack-protection, swd, webfinger, etc.). PASS. - `config/initializers/omniauth.rb`: Deleted entirely. PASS. - `config/routes.rb`: `/auth/keycloak/*` and `/auth/failure` routes removed, replaced with `post "/login"`. PASS. - `SessionsController#failure` action: Removed. PASS. - `spec/support/omniauth.rb`: Fully rewritten -- no `OmniAuth` references remain. No `OmniAuth.config.test_mode`, no `OmniAuth::AuthHash`. PASS. **Logout simplification** The old logout redirected to Keycloak's end-session endpoint to terminate the SSO session. With ROPC, there is no Keycloak browser session to terminate -- the user authenticated via server-side credential exchange, so a simple `session.delete(:user)` is correct. PASS. **Error handling** - Blank credentials: returns 422 with message. PASS. - Invalid credentials (Keycloak returns error): returns 422 with Keycloak's `error_description`. PASS. - Keycloak unreachable: catches `SocketError`, `Errno::ECONNREFUSED`, `Net::OpenTimeout`, `Net::ReadTimeout` and returns 503. PASS. - JSON parse error: NOT caught. If Keycloak returns non-JSON (e.g., HTML error page from a proxy), `JSON.parse(response.body)` will raise `JSON::ParserError` and bubble up as a 500. **CSS** New login form styles use CSS custom properties (`--spacing-md`, `--color-border`, `--radius`, etc.) consistently with the existing design system. No magic numbers except the `0.15s` transition and the `rgba(37, 99, 235, 0.15)` focus ring -- these are reasonable inline values for micro-interactions. PASS. ### BLOCKERS **None.** This is a clean, well-executed auth rewrite. ### NITS 1. **`flash.now` in view template** (`app/views/sessions/new.html.erb`): The condition `flash.now[:alert] || flash[:alert]` is misleading. When `flash.now[:alert] = "..."` is set in the controller, the value is accessible in the view via `flash[:alert]` (Rails merges `flash.now` into the flash for the current request render). Calling `flash.now[:alert]` in the view returns a `FlashNow` proxy lookup that is redundant. The simpler `flash[:alert]` alone covers both `flash.now` and regular flash. Not a bug (it works because `flash.now` in a view still delegates to the underlying hash), but it is confusing to future readers. 2. **`JSON::ParserError` not caught** (`app/controllers/sessions_controller.rb`): If Keycloak returns non-JSON (e.g., an HTML error page from a reverse proxy), `JSON.parse(response.body)` will raise an unhandled exception and produce a 500. Consider adding `JSON::ParserError` to the rescue clause with a user-friendly message like "Unexpected response from authentication service." 3. **Stale comment in `ApplicationController`** (line 48): `# Redirect unauthenticated users to Keycloak.` -- this comment still references Keycloak, but the redirect now goes to `/login`. The behavior is unchanged (still calls `redirect_to login_path`), but the comment is misleading. Consider updating to `# Redirect unauthenticated users to the login page.` 4. **`with_keycloak` helper duplication**: The `with_keycloak` helper is defined identically in `spec/requests/crew_spec.rb`, `spec/requests/role_access_spec.rb`, and potentially other spec files. It could be extracted to `spec/support/` as a shared helper. Not introduced by this PR (it existed before), but worth noting for future cleanup. 5. **`net/http` require**: The controller uses `Net::HTTP.post_form` without an explicit `require "net/http"`. Rails autoloads it in practice, but an explicit require at the top of the controller would be more defensive. Very minor -- Rails has always loaded `net/http` transitively. ### SOP COMPLIANCE - [x] Branch named after issue: `157-direct-access-grants` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body follows template: has Summary, Changes, Test Plan, Related sections - [x] Related references issue: `Closes #157` - [x] No secrets committed: all credentials via ENV - [x] No unrelated changes (scope is tight: auth rewrite + docs + tests) - [x] Tests exist: sessions_spec covers valid/invalid/blank credentials, Keycloak unavailable, logout, login page rendering - [x] Docs updated: `app-architecture.md` and `keycloak-setup.md` both updated comprehensively ### PROCESS OBSERVATIONS - **Change failure risk**: LOW. The auth rewrite is self-contained. The session structure is unchanged, so all downstream role-checking code (`require_role`, `visible_tabs`, `current_user_has_role?`) continues to work without modification. The test suite covers the critical paths. - **Pre-requisite dependency**: The PR body correctly notes that `direct_access_grants_enabled = true` must be set on the Keycloak client in `pal-e-services`. This is a cross-repo dependency -- the Keycloak terraform change should land before or simultaneously with this PR. - **Documentation quality**: The docs updates are thorough. The `keycloak-setup.md` changes correctly reflect the decision reversal from "ROPC rejected" to "ROPC adopted" with clear rationale. The sequence diagram in `app-architecture.md` is updated accurately. - **Net reduction**: -311 lines, +254 lines. Removing OmniAuth and its transitive dependency tree is a meaningful simplification. The auth flow is now stdlib-only (`Net::HTTP`, `Base64`, `JSON`). ### VERDICT: APPROVED
ldraney deleted branch 157-direct-access-grants 2026-06-07 16:47:36 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
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!158
No description provided.