Replace OmniAuth redirect with Keycloak Direct Access Grants #158
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "157-direct-access-grants"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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: removedomniauth,omniauth_openid_connect,omniauth-rails_csrf_protectionand all transitive depsconfig/initializers/omniauth.rb: deleted (OmniAuth middleware no longer needed)config/routes.rb: replaced/auth/keycloak/*callback routes withpost "/login"direct routeapp/controllers/sessions_controller.rb: rewritten --createaction accepts username/password, POSTs to Keycloak token endpoint, decodes JWT payload, stores session. Includes error handling for bad credentials, blank input, and Keycloak unavailabilityapp/views/sessions/new.html.erb: replaced Keycloak redirect button with username + password form fieldsapp/views/layouts/application.html.erb: changed auth-bar login frombutton_to "/auth/keycloak"tolink_to login_pathapp/assets/stylesheets/application.css: added styles for.login-form,.login-field,.login-label,.login-inputspec/support/omniauth.rb: rewritten --sign_in_asnow stubsNet::HTTP.post_formwith a fake JWT and POSTs to/loginspec/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/keycloakto/loginspec/requests/crew_spec.rb: fixed redirect expectation from/auth/keycloakto/logindocs/app-architecture.md: updated auth flow diagram, description, and design decisionsdocs/keycloak-setup.md: updated ROPC section to reflect the decision, updated client config and setup instructionsTest Plan
bundle exec rspec)Review Checklist
Related Notes
direct_access_grants_enabledon Keycloak clientRelated
QA Review -- PR #158
Scope Check
PR matches issue #157 requirements. All items from the acceptance criteria are addressed:
Findings
No blocking issues found.
Minor observations (non-blocking):
spec/support/omniauth.rbfilename: The file still carries theomniauthname despite no longer having any OmniAuth dependency. Consider renaming toauth_helpers.rborkeycloak_helpers.rbfor clarity. Non-blocking -- the file works correctly.flash.now[:alert]in view template: The view checksflash.now[:alert] || flash[:alert]. In ERB templates,flash.nowis only accessible during the current request (whenrenderis called), whileflash[:alert]persists across redirects. Theflash.now[:alert]check in the view is technically redundant since Rails mergesflash.nowinto the flash hash for the current render --flash[:alert]alone would catch both cases. Non-blocking, works correctly as-is.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.
failureaction removed: The oldGET /auth/failureroute and action are gone. Correct -- OmniAuth failure callbacks are no longer relevant. Error handling now lives inline increate.Checklist
sign_in_as) exercises the full controller flow via stubbed HTTPVERDICT: APPROVE
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)
Credentials not logged: The controller logs
e.classande.messageon connection errors, but never logsusernameorpassword. The Keycloak error response body (error_description) is displayed to the user but does not contain credentials. PASS.Credentials not stored: Credentials are used only as parameters to
Net::HTTP.post_formand are not persisted to session, database, or cache. PASS.CSRF protection: The login form uses
form_with, which includes the Rails CSRF token by default.SessionsControllerinherits fromApplicationController < ActionController::Base, which hasprotect_from_forgeryenabled by default. PASS.JWT decode approach:
decode_jwt_payloaddoes Base64url decode only, no signature verification. The comment explains the rationale -- the token comes from a directNet::HTTP.post_formcall 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.JWT padding: The padding logic
"=" * ((4 - len % 4) % 4)is correct for Base64url. PASS.No credentials in Gemfile/code:
KEYCLOAK_CLIENT_SECRETand 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 existingcurrent_user,current_user_has_role?, andvisible_tabsmethods inApplicationController. Thewith_indifferent_accesscall on line 41 ofApplicationControllermeans both string and symbol access works. PASS.OmniAuth removal completeness
Gemfile:omniauth_openid_connectandomniauth-rails_csrf_protectionremoved. 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/failureroutes removed, replaced withpost "/login". PASS.SessionsController#failureaction: Removed. PASS.spec/support/omniauth.rb: Fully rewritten -- noOmniAuthreferences remain. NoOmniAuth.config.test_mode, noOmniAuth::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
error_description. PASS.SocketError,Errno::ECONNREFUSED,Net::OpenTimeout,Net::ReadTimeoutand returns 503. PASS.JSON.parse(response.body)will raiseJSON::ParserErrorand 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 the0.15stransition and thergba(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
flash.nowin view template (app/views/sessions/new.html.erb): The conditionflash.now[:alert] || flash[:alert]is misleading. Whenflash.now[:alert] = "..."is set in the controller, the value is accessible in the view viaflash[:alert](Rails mergesflash.nowinto the flash for the current request render). Callingflash.now[:alert]in the view returns aFlashNowproxy lookup that is redundant. The simplerflash[:alert]alone covers bothflash.nowand regular flash. Not a bug (it works becauseflash.nowin a view still delegates to the underlying hash), but it is confusing to future readers.JSON::ParserErrornot 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 addingJSON::ParserErrorto the rescue clause with a user-friendly message like "Unexpected response from authentication service."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 callsredirect_to login_path), but the comment is misleading. Consider updating to# Redirect unauthenticated users to the login page.with_keycloakhelper duplication: Thewith_keycloakhelper is defined identically inspec/requests/crew_spec.rb,spec/requests/role_access_spec.rb, and potentially other spec files. It could be extracted tospec/support/as a shared helper. Not introduced by this PR (it existed before), but worth noting for future cleanup.net/httprequire: The controller usesNet::HTTP.post_formwithout an explicitrequire "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 loadednet/httptransitively.SOP COMPLIANCE
157-direct-access-grantsfollows{issue-number}-{kebab-case-purpose}conventionCloses #157app-architecture.mdandkeycloak-setup.mdboth updated comprehensivelyPROCESS OBSERVATIONS
require_role,visible_tabs,current_user_has_role?) continues to work without modification. The test suite covers the critical paths.direct_access_grants_enabled = truemust be set on the Keycloak client inpal-e-services. This is a cross-repo dependency -- the Keycloak terraform change should land before or simultaneously with this PR.keycloak-setup.mdchanges correctly reflect the decision reversal from "ROPC rejected" to "ROPC adopted" with clear rationale. The sequence diagram inapp-architecture.mdis updated accurately.Net::HTTP,Base64,JSON).VERDICT: APPROVED