Revert PR #158: restore OmniAuth to fix broken prod auth

ldraney commented

2026-06-07 17:11:55 +00:00

Owner

Summary

Emergency revert of PR #158 which replaced OmniAuth with Keycloak Direct Access Grants (ROPC). The Keycloak terraform was never applied, so Keycloak does not accept grant_type=password, breaking production auth for all users. This revert restores OmniAuth Authorization Code flow which Keycloak IS configured for.

Changes

Gemfile / Gemfile.lock -- Restore omniauth, omniauth_openid_connect, omniauth-rails_csrf_protection gems
config/initializers/omniauth.rb -- Restore OmniAuth initializer (was deleted by #158)
config/routes.rb -- Restore /auth/keycloak/callback GET/POST routes
app/controllers/sessions_controller.rb -- Restore OmniAuth callback handling
app/views/sessions/new.html.erb -- Restore login page with Keycloak redirect button
app/views/layouts/application.html.erb -- Restore layout changes
app/assets/stylesheets/application.css -- Remove ROPC login form styles
docs/ -- Revert documentation changes
spec/ -- Restore OmniAuth test helpers and specs

Test Plan

This is a clean git revert of commit af9dc69 -- no manual edits
Verify login works via OmniAuth redirect flow in production after merge
Existing test suite should pass as specs are reverted to their pre-#158 state

Review Checklist

Clean git revert, no manual edits
OmniAuth initializer restored
OmniAuth gems restored in Gemfile/Gemfile.lock
/auth/keycloak/callback routes restored
Login page restored with Keycloak redirect button

None -- this is a straight revert to fix prod.

Reverts: PR #158
Closes #160

## Summary Emergency revert of PR #158 which replaced OmniAuth with Keycloak Direct Access Grants (ROPC). The Keycloak terraform was never applied, so Keycloak does not accept `grant_type=password`, breaking production auth for all users. This revert restores OmniAuth Authorization Code flow which Keycloak IS configured for. ## Changes - **Gemfile / Gemfile.lock** -- Restore omniauth, omniauth_openid_connect, omniauth-rails_csrf_protection gems - **config/initializers/omniauth.rb** -- Restore OmniAuth initializer (was deleted by #158) - **config/routes.rb** -- Restore `/auth/keycloak/callback` GET/POST routes - **app/controllers/sessions_controller.rb** -- Restore OmniAuth callback handling - **app/views/sessions/new.html.erb** -- Restore login page with Keycloak redirect button - **app/views/layouts/application.html.erb** -- Restore layout changes - **app/assets/stylesheets/application.css** -- Remove ROPC login form styles - **docs/** -- Revert documentation changes - **spec/** -- Restore OmniAuth test helpers and specs ## Test Plan - This is a clean `git revert` of commit af9dc69 -- no manual edits - Verify login works via OmniAuth redirect flow in production after merge - Existing test suite should pass as specs are reverted to their pre-#158 state ## Review Checklist - [x] Clean git revert, no manual edits - [x] OmniAuth initializer restored - [x] OmniAuth gems restored in Gemfile/Gemfile.lock - [x] /auth/keycloak/callback routes restored - [x] Login page restored with Keycloak redirect button ## Related Notes None -- this is a straight revert to fix prod. ## Related - Reverts: PR #158 - Closes #160

ldraney added 1 commit

2026-06-07 17:11:56 +00:00

Revert "Replace OmniAuth redirect with Keycloak Direct Access Grants (#158 )"

ci/woodpecker/push/woodpecker Pipeline failed

Details

ci/woodpecker/pr/woodpecker Pipeline failed

Details

CI / scan_ruby (pull_request) Has been cancelled

Details

CI / scan_js (pull_request) Has been cancelled

Details

CI / lint (pull_request) Has been cancelled

Details

abbec2019c

This reverts commit af9dc69ff8.

ldraney commented

2026-06-07 17:15:15 +00:00

Author

Owner

PR #161 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, OmniAuth OpenID Connect, Keycloak SSO, RSpec request specs.

This is an emergency revert of PR #158 (which replaced OmniAuth Authorization Code + PKCE flow with ROPC Direct Access Grants). The revert restores the OmniAuth-based auth stack that Keycloak is actually configured for in production.

Revert completeness -- all critical components verified:

Gemfile/Gemfile.lock: omniauth_openid_connect (~> 0.8) and omniauth-rails_csrf_protection (~> 1.0) restored with full dependency tree (faraday, json-jwt, rack-oauth2, openid_connect, etc.). Checksums present. Clean.
config/initializers/omniauth.rb: Restored as a new file. Configures OmniAuth::Builder with :openid_connect strategy, PKCE enabled (pkce: true), Authorization Code flow (response_type: :code), and proper guards for test vs production vs dev-without-Keycloak. allowed_request_methods = %i[post] enforces POST-only OmniAuth requests (CSRF protection). Clean.
config/routes.rb: Restored OmniAuth callback routes (GET /auth/keycloak/callback, POST /auth/keycloak/callback, GET /auth/failure). The ROPC POST /login route is removed, GET /login retained for the login page. DELETE /logout retained. Clean.
SessionsController: ROPC flow (Net::HTTP POST to Keycloak token endpoint, manual JWT decoding, credential validation) replaced with OmniAuth callback pattern (request.env["omniauth.auth"]). The destroy action restores Keycloak end-session redirect via keycloak_configured? guard. failure action restored for OmniAuth error callback. The unsafe decode_jwt_payload method (Base64 decode without signature verification) is removed -- OmniAuth handles token validation properly. Clean.
Login view (sessions/new.html.erb): Username/password form fields removed, replaced with button_to "/auth/keycloak" redirect button. flash.now[:alert] simplified to flash[:alert] (correct for redirect-based flow). Clean.
Layout (application.html.erb): Login link changed from link_to login_path to button_to "/auth/keycloak", method: :post with turbo: false. This ensures the OmniAuth POST requirement is met and CSRF protection works. Clean.
CSS (application.css): ROPC login form styles (.login-form, .login-field, .login-label, .login-input) removed. These are no longer needed since the login page uses a single redirect button instead of a form. Clean.
Documentation: docs/app-architecture.md and docs/keycloak-setup.md reverted from ROPC descriptions back to Authorization Code + PKCE descriptions. The ROPC flow decision section is reverted to the "ROPC Flow Validation" analysis explaining why ROPC is wrong. Client table shows direct_access_grants_enabled = false. Clean.
Test support (spec/support/omniauth.rb): stub_keycloak_token (which fabricated fake JWT tokens for Net::HTTP stubs) replaced with mock_keycloak_auth using OmniAuth::AuthHash. sign_in_as helper now uses OmniAuth test mode flow (post "/auth/keycloak" + follow redirects). RSpec.configure block sets default mock auth before each test. This is the standard OmniAuth testing pattern. Clean.
Test specs: sessions_spec.rb reverted from ROPC-specific tests (invalid credentials, blank credentials, Keycloak unavailable) to OmniAuth callback tests. Keycloak logout redirect test preserved. auth/failure test added. role_access_spec.rb and crew_spec.rb redirect expectations updated from /login to /auth/keycloak where appropriate.

PR #146 (crew tab) preservation: Verified. The crew controller, crew views, crew routes, and crew index specs are untouched by this revert. The only crew_spec changes are in the auth redirect expectations for crew member show pages (changing /login to /auth/keycloak), which is the correct auth behavior change.

Security observations (positive):

The removed decode_jwt_payload method did Base64 JWT decoding without signature verification -- the comment said "trusted server-to-server" but this is a bad practice. OmniAuth handles token validation properly through the openid_connect library.
ROPC flow removed means the Rails app no longer handles raw user credentials, reducing attack surface.
OmniAuth CSRF protection gem restored.
POST-only OmniAuth requests enforced.

BLOCKERS

None.

NITS

The diff is +311/-254 lines across 14 files, which is large for a revert. However, reviewing the content confirms this is a clean reversal of PR #158's changes -- every addition restores pre-#158 code and every deletion removes #158's additions. No manual edits or additional changes detected beyond what git revert would produce.
The crew_spec.rb diff shows changes to crew member show specs (lines 90+) that reference active_member.id. These lines exist in the PR branch but not on main currently -- this suggests the branch was based on a commit that included crew member show changes. Since mergeable: true per Forgejo, there should be no conflict, but worth noting.

SOP COMPLIANCE

Branch named after issue: 160-urgent-revert-pr-158-prod-auth-broken-ke references issue #160
PR body follows template: Summary, Changes, Test Plan, Related sections all present
Related references: Reverts PR #158, Closes #160
Related references plan slug: No plan slug referenced -- acceptable for an emergency revert
No secrets committed: No API keys, passwords, or credentials in the diff. Env vars referenced by name only.
No scope creep: All changes directly relate to reverting the ROPC auth back to OmniAuth
Tests exist: OmniAuth test helpers and session specs restored, covering happy path, role extraction, logout, and failure callback

PROCESS OBSERVATIONS

Change failure risk: LOW. This is a straight revert to a known-working auth configuration. The OmniAuth flow was the original implementation before PR #158 changed it.
Deployment frequency impact: This unblocks production. Users cannot currently log in. Merging this restores auth immediately.
Root cause: PR #158 changed the auth flow to ROPC but the corresponding Keycloak terraform (direct_access_grants_enabled = true) was never applied. This is a deployment coordination gap -- the app change shipped without the infrastructure change it depended on.
Follow-up needed: When ROPC is reconsidered (if ever), the Keycloak terraform must be applied BEFORE the Rails app change ships. The ROPC analysis in docs/keycloak-setup.md recommends against ROPC entirely, which this revert correctly restores.

VERDICT: APPROVED

## PR #161 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails 8.1, OmniAuth OpenID Connect, Keycloak SSO, RSpec request specs. This is an emergency revert of PR #158 (which replaced OmniAuth Authorization Code + PKCE flow with ROPC Direct Access Grants). The revert restores the OmniAuth-based auth stack that Keycloak is actually configured for in production. **Revert completeness -- all critical components verified:** 1. **Gemfile/Gemfile.lock**: `omniauth_openid_connect (~> 0.8)` and `omniauth-rails_csrf_protection (~> 1.0)` restored with full dependency tree (faraday, json-jwt, rack-oauth2, openid_connect, etc.). Checksums present. Clean. 2. **config/initializers/omniauth.rb**: Restored as a new file. Configures `OmniAuth::Builder` with `:openid_connect` strategy, PKCE enabled (`pkce: true`), Authorization Code flow (`response_type: :code`), and proper guards for test vs production vs dev-without-Keycloak. `allowed_request_methods = %i[post]` enforces POST-only OmniAuth requests (CSRF protection). Clean. 3. **config/routes.rb**: Restored OmniAuth callback routes (`GET /auth/keycloak/callback`, `POST /auth/keycloak/callback`, `GET /auth/failure`). The ROPC `POST /login` route is removed, `GET /login` retained for the login page. `DELETE /logout` retained. Clean. 4. **SessionsController**: ROPC flow (Net::HTTP POST to Keycloak token endpoint, manual JWT decoding, credential validation) replaced with OmniAuth callback pattern (`request.env["omniauth.auth"]`). The `destroy` action restores Keycloak end-session redirect via `keycloak_configured?` guard. `failure` action restored for OmniAuth error callback. The unsafe `decode_jwt_payload` method (Base64 decode without signature verification) is removed -- OmniAuth handles token validation properly. Clean. 5. **Login view (sessions/new.html.erb)**: Username/password form fields removed, replaced with `button_to "/auth/keycloak"` redirect button. `flash.now[:alert]` simplified to `flash[:alert]` (correct for redirect-based flow). Clean. 6. **Layout (application.html.erb)**: Login link changed from `link_to login_path` to `button_to "/auth/keycloak", method: :post` with `turbo: false`. This ensures the OmniAuth POST requirement is met and CSRF protection works. Clean. 7. **CSS (application.css)**: ROPC login form styles (`.login-form`, `.login-field`, `.login-label`, `.login-input`) removed. These are no longer needed since the login page uses a single redirect button instead of a form. Clean. 8. **Documentation**: `docs/app-architecture.md` and `docs/keycloak-setup.md` reverted from ROPC descriptions back to Authorization Code + PKCE descriptions. The ROPC flow decision section is reverted to the "ROPC Flow Validation" analysis explaining why ROPC is wrong. Client table shows `direct_access_grants_enabled = false`. Clean. 9. **Test support (spec/support/omniauth.rb)**: `stub_keycloak_token` (which fabricated fake JWT tokens for Net::HTTP stubs) replaced with `mock_keycloak_auth` using `OmniAuth::AuthHash`. `sign_in_as` helper now uses OmniAuth test mode flow (`post "/auth/keycloak"` + follow redirects). `RSpec.configure` block sets default mock auth before each test. This is the standard OmniAuth testing pattern. Clean. 10. **Test specs**: `sessions_spec.rb` reverted from ROPC-specific tests (invalid credentials, blank credentials, Keycloak unavailable) to OmniAuth callback tests. Keycloak logout redirect test preserved. `auth/failure` test added. `role_access_spec.rb` and `crew_spec.rb` redirect expectations updated from `/login` to `/auth/keycloak` where appropriate. **PR #146 (crew tab) preservation**: Verified. The crew controller, crew views, crew routes, and crew index specs are untouched by this revert. The only crew_spec changes are in the auth redirect expectations for crew member show pages (changing `/login` to `/auth/keycloak`), which is the correct auth behavior change. **Security observations (positive)**: - The removed `decode_jwt_payload` method did Base64 JWT decoding without signature verification -- the comment said "trusted server-to-server" but this is a bad practice. OmniAuth handles token validation properly through the openid_connect library. - ROPC flow removed means the Rails app no longer handles raw user credentials, reducing attack surface. - OmniAuth CSRF protection gem restored. - POST-only OmniAuth requests enforced. ### BLOCKERS None. ### NITS 1. The diff is +311/-254 lines across 14 files, which is large for a revert. However, reviewing the content confirms this is a clean reversal of PR #158's changes -- every addition restores pre-#158 code and every deletion removes #158's additions. No manual edits or additional changes detected beyond what `git revert` would produce. 2. The `crew_spec.rb` diff shows changes to crew member show specs (lines 90+) that reference `active_member.id`. These lines exist in the PR branch but not on `main` currently -- this suggests the branch was based on a commit that included crew member show changes. Since `mergeable: true` per Forgejo, there should be no conflict, but worth noting. ### SOP COMPLIANCE - [x] Branch named after issue: `160-urgent-revert-pr-158-prod-auth-broken-ke` references issue #160 - [x] PR body follows template: Summary, Changes, Test Plan, Related sections all present - [x] Related references: Reverts PR #158, Closes #160 - [ ] Related references plan slug: No plan slug referenced -- acceptable for an emergency revert - [x] No secrets committed: No API keys, passwords, or credentials in the diff. Env vars referenced by name only. - [x] No scope creep: All changes directly relate to reverting the ROPC auth back to OmniAuth - [x] Tests exist: OmniAuth test helpers and session specs restored, covering happy path, role extraction, logout, and failure callback ### PROCESS OBSERVATIONS - **Change failure risk**: LOW. This is a straight revert to a known-working auth configuration. The OmniAuth flow was the original implementation before PR #158 changed it. - **Deployment frequency impact**: This unblocks production. Users cannot currently log in. Merging this restores auth immediately. - **Root cause**: PR #158 changed the auth flow to ROPC but the corresponding Keycloak terraform (`direct_access_grants_enabled = true`) was never applied. This is a deployment coordination gap -- the app change shipped without the infrastructure change it depended on. - **Follow-up needed**: When ROPC is reconsidered (if ever), the Keycloak terraform must be applied BEFORE the Rails app change ships. The ROPC analysis in `docs/keycloak-setup.md` recommends against ROPC entirely, which this revert correctly restores. ### VERDICT: APPROVED