From 39c9eb58c397c8249f5b05da635aa4905eff720b Mon Sep 17 00:00:00 2001 From: Igor Fedoronchuk Date: Tue, 2 Jun 2026 12:10:57 +0200 Subject: [PATCH 1/3] Fall back to Devise.available_router_name when mapping has none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hosts that mount Devise inside an engine commonly pin URL helpers via the GLOBAL setting (Devise.router_name = :engine in devise.rb initializer) rather than passing router_name: in devise_for. In that case Devise.mappings[scope].router_name is nil, but Devise.available_router_name returns :engine — Devise's own URL-helper dispatcher uses that fallback, and we have to too. Without the fallback the failure handler dropped through to `self`, which doesn't carry the engine-scoped session helper, and raised NoMethodError on hosts using the global pattern. The dummy_engine app's routes drop the per-mapping `router_name:` option so the regression test now exercises the global-only setup (matches the host-app pattern that surfaced the bug). --- .../devise/omniauth_callbacks_controller.rb | 18 +++++++++++------- .../lib/admin_panel/config/routes.rb | 8 ++++++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb b/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb index 160db64..5c49ecf 100644 --- a/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb +++ b/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb @@ -78,14 +78,18 @@ def after_sign_in_path_for(resource) # `:database_authenticatable` is in the mapping's `used_helpers`, # so an OIDC-only model never gets it. The engine mounts # `new__session_path` itself, but the helper lives on - # whichever route set the mapping's `router_name` points at — - # main_app by default, or `` for hosts that mount - # Devise inside a Rails engine. Replicate Devise's own dispatch - # so the right context is asked. + # whichever route set Devise's URL helper dispatcher points at: + # the per-mapping `router_name` (set by + # `devise_for :scope, router_name: :engine`) when present, + # otherwise the global `Devise.available_router_name` + # (set by `Devise.router_name = :engine`), which defaults to + # `:main_app`. Replicate that dispatcher here so the helper is + # resolved on the right context (Rails.application proxy or + # mounted engine proxy). def after_omniauth_failure_path_for(scope) - router_name = ::Devise.mappings[scope].router_name - context = router_name ? send(router_name) : self - context.public_send(:"new_#{scope}_session_path") + router_name = ::Devise.mappings[scope].router_name || + ::Devise.available_router_name + send(router_name).public_send(:"new_#{scope}_session_path") end end end diff --git a/spec/dummy_engine/lib/admin_panel/config/routes.rb b/spec/dummy_engine/lib/admin_panel/config/routes.rb index 7caa389..8843f9c 100644 --- a/spec/dummy_engine/lib/admin_panel/config/routes.rb +++ b/spec/dummy_engine/lib/admin_panel/config/routes.rb @@ -1,8 +1,12 @@ # frozen_string_literal: true # Engine-mounted-Devise layout: devise_for + ActiveAdmin routes live -# inside the engine, pinned to it via `router_name`. +# inside the engine. The engine is pinned via the GLOBAL +# `Devise.router_name = :admin_panel` set in config/initializers/devise.rb +# rather than a per-mapping `router_name:` option here — this is the +# pattern hosts use most often (and the pattern under which the bug in +# OmniauthCallbacksController#after_omniauth_failure_path_for surfaces). AdminPanel::Engine.routes.draw do - devise_for :admin_users, ActiveAdmin::Devise.config.merge(router_name: :admin_panel) + devise_for :admin_users, ActiveAdmin::Devise.config ActiveAdmin.routes(self) end From 6b6a2ae6ce07cb25fa371bf24180a656576763ed Mon Sep 17 00:00:00 2001 From: Igor Fedoronchuk Date: Tue, 2 Jun 2026 14:25:10 +0200 Subject: [PATCH 2/3] Make the global-Devise.router_name regression test explicit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #7 reconfigured spec/dummy_engine to use global-only Devise.router_name (drops the per-mapping router_name: option) so the existing OmniAuth-failure scenario in spec/engine/features/engine_mounted_login_spec.rb covers the new fallback code path. That coverage was implicit — someone reading the diff sees a config change + a method change without an obvious connection. Spell it out: top comment now documents that the dummy uses global-only router_name and why the fallback matters; the failure-path context is renamed to 'OmniAuth failure path under global Devise.router_name'; a sanity check asserts Devise.mappings[:admin_user].router_name is nil and Devise.available_router_name == :admin_panel so a future flip back to the per-mapping form is loud. --- .../features/engine_mounted_login_spec.rb | 41 ++++++++++++------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/spec/engine/features/engine_mounted_login_spec.rb b/spec/engine/features/engine_mounted_login_spec.rb index 42a19e5..1e6aadd 100644 --- a/spec/engine/features/engine_mounted_login_spec.rb +++ b/spec/engine/features/engine_mounted_login_spec.rb @@ -5,19 +5,18 @@ # Engine-mounted Devise + OIDC scenario: # # - AdminPanel::Engine is a non-isolated Rails engine -# - `devise_for :admin_users, ..., router_name: :admin_panel` lives -# inside the engine's routes -# - `Devise.router_name = :admin_panel` pins Devise's URL helpers to -# `AdminPanel::Engine.routes` +# - `devise_for :admin_users` (NO `router_name:` option) lives inside +# the engine's routes +# - `Devise.router_name = :admin_panel` is set globally in the +# devise initializer — this pins Devise's URL helpers to the +# engine via `Devise.available_router_name` rather than the +# per-mapping option # -# With AdminUser also OIDC-only (`devise :omniauthable`, no -# `:database_authenticatable`), Devise generates no session routes. -# activeadmin-oidc has to mount `/admin/login` and `/admin/logout` -# *inside the engine's route set*, otherwise -# `AdminPanel::Engine.routes.url_helpers.new_admin_user_session_path` -# stays undefined and host-side failure apps (Devise::FailureApp -# subclasses that redirect via the engine's helpers) blow up with -# NoMethodError. +# The global-only setup is the more common pattern in real hosts. The +# OmniauthCallbacks failure handler must therefore fall back to +# `Devise.available_router_name` when `Devise.mappings[scope].router_name` +# is nil — calling the helper directly on the controller blows up +# because the helper lives on the engine proxy, not on main_app. RSpec.feature "Engine-mounted OIDC sessions", type: :feature do it "AdminPanel::Engine.routes.url_helpers exposes new_admin_user_session_path" do expect { @@ -47,7 +46,15 @@ expect(page).to have_current_path("/admin/login") end - context "OmniAuth failure path" do + context "OmniAuth failure path under global Devise.router_name" do + # Sanity-check the dummy setup so a future refactor that flips it + # back to per-mapping `router_name:` makes the regression scenario + # below stop covering the global-only code path explicitly. + it "dummy_engine has no per-mapping router_name (relies on the global setting)" do + expect(Devise.mappings[:admin_user].router_name).to be_nil + expect(Devise.available_router_name.to_sym).to eq(:admin_panel) + end + around do |ex| saved = OmniAuth.config.mock_auth[:oidc] OmniAuth.config.mock_auth[:oidc] = :invalid_credentials @@ -58,8 +65,12 @@ scenario "failed OmniAuth callback lands back on the SSO landing page" do # OmniauthCallbacksController#after_omniauth_failure_path_for has # to resolve `new__session_path` against the engine's - # route set (Devise.router_name pins helpers there) — calling the - # helper directly on the controller would raise NoMethodError. + # route set. When the host pins helpers via the GLOBAL + # `Devise.router_name = :admin_panel` (no per-mapping option), + # `Devise.mappings[scope].router_name` is nil and the handler has + # to fall back to `Devise.available_router_name` to find the + # engine proxy — otherwise it tries the helper on the controller + # itself and raises NoMethodError. visit "/admin/login" click_button ActiveAdmin::Oidc.config.login_button_label expect(page).to have_current_path("/admin/login") From a5b3af5a8deb60efe179291490b974c56203d109 Mon Sep 17 00:00:00 2001 From: Igor Fedoronchuk Date: Tue, 2 Jun 2026 14:29:05 +0200 Subject: [PATCH 3/3] Assert rendered DOM state in the failure-path Capybara scenario MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scenario now checks not just have_current_path but also have_button (the SSO landing renders again) and have_content for the access-denied flash. If the failure handler ever silently redirects to the wrong place — or if the SSO landing view itself regresses — the test catches it; previously only the path was checked, which would have passed a redirect to an empty AA index page that happens to share /admin/login. --- spec/engine/features/engine_mounted_login_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/engine/features/engine_mounted_login_spec.rb b/spec/engine/features/engine_mounted_login_spec.rb index 1e6aadd..600affd 100644 --- a/spec/engine/features/engine_mounted_login_spec.rb +++ b/spec/engine/features/engine_mounted_login_spec.rb @@ -70,10 +70,18 @@ # `Devise.mappings[scope].router_name` is nil and the handler has # to fall back to `Devise.available_router_name` to find the # engine proxy — otherwise it tries the helper on the controller - # itself and raises NoMethodError. + # itself and raises NoMethodError (500 response → driver + # exception, not a clean redirect). visit "/admin/login" click_button ActiveAdmin::Oidc.config.login_button_label + + # Full Capybara assertions: real path + rendered DOM state. If + # the failure handler had raised, Capybara would never reach the + # `page` assertions — but we also check the SSO button + flash + # so a silent redirect to the wrong place can't pass either. expect(page).to have_current_path("/admin/login") + expect(page).to have_button(ActiveAdmin::Oidc.config.login_button_label) + expect(page).to have_content(ActiveAdmin::Oidc.config.access_denied_message) end end end