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 diff --git a/spec/engine/features/engine_mounted_login_spec.rb b/spec/engine/features/engine_mounted_login_spec.rb index 42a19e5..600affd 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,11 +65,23 @@ 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 (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