From 31113f12b48639800985c652b554430b147cd878 Mon Sep 17 00:00:00 2001 From: Igor Fedoronchuk Date: Wed, 30 Sep 2026 14:53:46 +0200 Subject: [PATCH 1/3] Follow a host-set Devise.omniauth_path_prefix in the strategy too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devise's `omniauth_path_prefix` decides where the OmniAuth routes are DRAWN; the strategy's own `path_prefix` decides where the middleware LISTENS. The engine honoured a host-set value for the first and ignored it for the second: ::Devise.omniauth_path_prefix ||= cfg.omniauth_route_prefix # host wins devise.omniauth :openid_connect, path_prefix: cfg.omniauth_path_prefix A host with `config.omniauth_path_prefix = '/sso/auth'` in its own devise.rb — the line Devise's own generator template suggests — ends up with the callback route at /sso/auth/oidc/callback and the middleware listening on /admin/auth. Every SSO sign-in 404s after the IdP round trip, and README.md documents that configuration as supported. A host that pinned only Devise's setting meant one prefix, so follow it for both. An explicit `c.omniauth_path_prefix` still wins, which is what engine-mounted hosts need — there the two genuinely differ, by the mount prefix. Also here: * `active_admin_namespace` now applies `.underscore`, matching `ActiveAdmin::Namespace#initialize`. With `default_namespace = :"admin-panel"` ActiveAdmin routes under /admin_panel while the gem mounted /admin-panel/login and raised looking up `:"admin-panel_root_path"`. * The `#login_submit_path` example asserted against `OmniAuth.config.path_prefix`, a global that tracks the same value — it passed under the old implementation too, so it could not have caught the regression it guards, and it only passed at all when another spec had booted a dummy app first. Now a literal, plus a second example for the derived default. --- lib/activeadmin/oidc/configuration.rb | 13 ++++++- lib/activeadmin/oidc/engine.rb | 19 +++++++++- spec/unit/configuration_spec.rb | 50 +++++++++++++++++++++++++-- 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/lib/activeadmin/oidc/configuration.rb b/lib/activeadmin/oidc/configuration.rb index c491e4a..2a85953 100644 --- a/lib/activeadmin/oidc/configuration.rb +++ b/lib/activeadmin/oidc/configuration.rb @@ -89,6 +89,13 @@ def omniauth_path_prefix @omniauth_path_prefix || "#{active_admin_namespace_prefix}/auth" end + # Whether the host pinned this itself. The engine needs to tell an + # explicit choice apart from the derived default before it decides + # whether a host-set `Devise.omniauth_path_prefix` should win. + def omniauth_path_prefix_configured? + !@omniauth_path_prefix.nil? + end + # What Devise declares its OmniAuth request/callback routes with. # Devise reuses a single setting for both jobs, and the two differ # by exactly the mount prefix when `devise_for` lives inside a @@ -105,7 +112,11 @@ def active_admin_namespace namespace = active_admin_default_namespace return nil if namespace.blank? || namespace.to_sym == :root - namespace.to_sym + # `.underscore` to match ActiveAdmin: `Namespace#initialize` does + # `name.to_s.underscore`, so `default_namespace = :"admin-panel"` + # routes under /admin_panel. Deriving from the raw value would + # mount the login page at /admin-panel and 404 after sign-in. + namespace.to_s.underscore.to_sym end # Narrow on purpose: `NoMethodError` is what "ActiveAdmin is not diff --git a/lib/activeadmin/oidc/engine.rb b/lib/activeadmin/oidc/engine.rb index 22f871d..b9d367b 100644 --- a/lib/activeadmin/oidc/engine.rb +++ b/lib/activeadmin/oidc/engine.rb @@ -107,12 +107,29 @@ def controllers # `||=` semantics preserved: a host that set the prefix in its # own devise.rb keeps it. Read by Devise when the routes are # drawn, which happens later still. + host_prefix = ::Devise.omniauth_path_prefix ::Devise.omniauth_path_prefix ||= cfg.omniauth_route_prefix + # Devise's setting decides where the routes are DRAWN; the + # strategy's `path_prefix` decides where the middleware LISTENS. + # A host that pinned Devise's value but left ours alone meant one + # prefix, not two -- following only the derived default there + # would put the callback route and the middleware on different + # paths, and every sign-in would 404 after the IdP round trip. + # An explicit `c.omniauth_path_prefix` still wins, which is what + # engine-mounted hosts need: there the two genuinely differ, by + # the mount prefix. + middleware_prefix = + if host_prefix.present? && !cfg.omniauth_path_prefix_configured? + host_prefix + else + cfg.omniauth_path_prefix + end + ::Devise.setup do |devise| devise.omniauth :openid_connect, name: PROVIDER_NAME, - path_prefix: cfg.omniauth_path_prefix, + path_prefix: middleware_prefix, scope: (cfg.scope || 'openid email profile').split, response_type: :code, issuer: cfg.issuer, diff --git a/spec/unit/configuration_spec.rb b/spec/unit/configuration_spec.rb index 06f2ebf..b8bc4a6 100644 --- a/spec/unit/configuration_spec.rb +++ b/spec/unit/configuration_spec.rb @@ -201,8 +201,19 @@ describe "#login_submit_path" do it "points at the OmniAuth entry point when stub login is off" do - expect(config.login_submit_path) - .to eq("#{OmniAuth.config.path_prefix}/oidc") + # A literal, not `OmniAuth.config.path_prefix`: that global tracks + # the same value, so comparing against it passed under the old + # implementation too and could not have caught the regression this + # example exists for. It also made the example order-dependent -- + # Devise nils the global on load, and only a booted dummy app + # writes it back. + config.omniauth_path_prefix = "/sentinel/auth" + + expect(config.login_submit_path).to eq("/sentinel/auth/oidc") + end + + it "derives the entry point from ActiveAdmin's namespace by default" do + expect(config.login_submit_path).to eq("/admin/auth/oidc") end it "points at the stub route, derived from login_path, when stub login is on" do @@ -287,6 +298,41 @@ def with_namespace(namespace) end end + describe "#active_admin_namespace" do + # ActiveAdmin::Namespace#initialize does `name.to_s.underscore`, so + # every path it draws is underscored. Deriving from the raw value + # would put the login page somewhere ActiveAdmin never routed. + it "underscores the namespace the way ActiveAdmin does" do + allow(ActiveAdmin.application).to receive(:default_namespace).and_return(:"admin-panel") + + expect(config.active_admin_namespace).to eq(:admin_panel) + expect(config.login_path).to eq("/admin_panel/login") + expect(config.omniauth_path_prefix).to eq("/admin_panel/auth") + end + + it "underscores a CamelCase namespace too" do + allow(ActiveAdmin.application).to receive(:default_namespace).and_return(:AdminPanel) + + expect(config.active_admin_namespace).to eq(:admin_panel) + end + end + + describe "#omniauth_path_prefix_configured?" do + # The engine reads this to tell a host that pinned + # `Devise.omniauth_path_prefix` (and meant one prefix) apart from a + # host that pinned ours too (and meant two). Getting it wrong draws + # the callback route and the middleware on different paths. + it "is false while the prefix is only derived" do + expect(config.omniauth_path_prefix_configured?).to be(false) + end + + it "is true once the host assigns one" do + config.omniauth_path_prefix = "/sso/auth" + + expect(config.omniauth_path_prefix_configured?).to be(true) + end + end + describe "#pkce" do it "defaults to true when client_secret is blank" do config.client_secret = nil From a766de05149706d0b4dfd3683e9a10360cbaba41 Mon Sep 17 00:00:00 2001 From: Igor Fedoronchuk Date: Thu, 1 Oct 2026 15:05:44 +0200 Subject: [PATCH 2/3] Draw the routes once before the suite runs Rails 8 loads routes lazily in the test environment, so the draw happened whenever the first example touched the route set -- and it inherited that example's stubs. `spec/requests/after_sign_in_path_spec.rb` stubs `ActiveAdmin.application.default_namespace` and then reads `Devise.mappings`, which calls `reload_routes_unless_loaded` internally; when that example ran first, the whole app got drawn under the stubbed namespace and the bad route set leaked into every example after it. That is what made the suite order-dependent. Two different symptoms, one cause: RuntimeError: Could not find a valid mapping for path "/admin/auth/oidc/callback" -- spec/requests/omniauth_callback_spec.rb, seed 18410 ActionController::RoutingError: No route matches [POST] "/admin/login/stub" -- spec/requests/stub_login_spec.rb:224, seed 21181 `rspec --bisect` on seed 18410 reduced it to two examples: rspec ./spec/requests/after_sign_in_path_spec.rb[1:2] \ ./spec/requests/omniauth_callback_spec.rb[1:7:1] --seed 18410 Note `reload_routes!` does not work here. On a not-yet-loaded app it draws the routes and then resets the loaded flag back to false: def reload_routes! if routes_reloader.execute_unless_loaded routes_reloader.loaded = false else routes_reloader.reload! end end so the next `Devise.mappings` call redraws them anyway, which leaves the original problem in place. `reload_routes_unless_loaded` draws them and leaves them loaded. `try` keeps this a no-op on Rails 7.2, which has no lazy route loading and no such method -- the same call Devise makes. Seeds 6, 6233, 18410, 21181, 35323 and 62645 all pass after this. --- spec/rails_helper.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index a1a8456..9f18f18 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -14,6 +14,24 @@ ActiveRecord::Schema.verbose = false load File.expand_path("dummy/db/schema.rb", __dir__) +# Draw the routes once, up front, and leave them marked as loaded. +# +# Rails 8 loads routes lazily in test, so without this the draw happens +# whenever the first example touches the route set -- and it inherits +# whatever stubs that example has installed. An example that stubs +# `ActiveAdmin.application.default_namespace` and then reads +# `Devise.mappings` (which calls `reload_routes_unless_loaded` internally) +# would draw the whole app under the stubbed namespace and leak that route +# set into every later example. That is what made the suite depend on the +# random seed. +# +# `reload_routes!` is deliberately NOT used here: on a not-yet-loaded app +# it draws the routes and then resets the loaded flag back to false, so +# the next `Devise.mappings` call would redraw them anyway. `try` keeps +# this working on Rails 7.2, which has no lazy route loading and no such +# method -- the same call Devise itself makes. +Rails.application.try(:reload_routes_unless_loaded) + RSpec.configure do |config| config.use_transactional_fixtures = true config.infer_spec_type_from_file_location! From 45872ce83ec64c54227b313dfb7bf210d530319f Mon Sep 17 00:00:00 2001 From: Denis Talakevich Date: Thu, 1 Oct 2026 17:25:32 +0300 Subject: [PATCH 3/3] Point the login form at the host-set OmniAuth prefix too The engine moved the middleware to a host-set Devise prefix but kept the value local, so `login_submit_path` still built /admin/auth/oidc and the login button hit a dead route. Writing it back to the config gives the view and the middleware one value. Co-Authored-By: Clanker --- lib/activeadmin/oidc/engine.rb | 14 +++---- spec/requests/host_devise_prefix_spec.rb | 48 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) create mode 100644 spec/requests/host_devise_prefix_spec.rb diff --git a/lib/activeadmin/oidc/engine.rb b/lib/activeadmin/oidc/engine.rb index b9d367b..8f37854 100644 --- a/lib/activeadmin/oidc/engine.rb +++ b/lib/activeadmin/oidc/engine.rb @@ -119,17 +119,17 @@ def controllers # An explicit `c.omniauth_path_prefix` still wins, which is what # engine-mounted hosts need: there the two genuinely differ, by # the mount prefix. - middleware_prefix = - if host_prefix.present? && !cfg.omniauth_path_prefix_configured? - host_prefix - else - cfg.omniauth_path_prefix - end + # + # Written back to the config rather than kept local: the login + # view posts to `login_submit_path`, which reads the same value. + if host_prefix.present? && !cfg.omniauth_path_prefix_configured? + cfg.omniauth_path_prefix = host_prefix + end ::Devise.setup do |devise| devise.omniauth :openid_connect, name: PROVIDER_NAME, - path_prefix: middleware_prefix, + path_prefix: cfg.omniauth_path_prefix, scope: (cfg.scope || 'openid email profile').split, response_type: :code, issuer: cfg.issuer, diff --git a/spec/requests/host_devise_prefix_spec.rb b/spec/requests/host_devise_prefix_spec.rb new file mode 100644 index 0000000..a0262da --- /dev/null +++ b/spec/requests/host_devise_prefix_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require "open3" +require "rbconfig" + +# A host that sets `config.omniauth_path_prefix` in its own devise.rb +# fixes that value before the engine's initializer runs, so this suite's +# already-booted app cannot exercise it. The example boots spec/dummy in +# a child process with the value preset, the way such a host would. +RSpec.describe "Host-set Devise.omniauth_path_prefix" do + let(:boot_script) do + <<~'RUBY' + ENV["RAILS_ENV"] = "test" + require "./spec/dummy/config/application" + Devise.omniauth_path_prefix = "/sso/auth" + Dummy::Application.initialize! + ActiveRecord::Schema.verbose = false + load "spec/dummy/db/schema.rb" + + OmniAuth.config.test_mode = true + OmniAuth.config.logger = Logger.new(File::NULL) + OmniAuth.config.request_validation_phase = ->(_env) { } + + request = ->(method, path) do + Rails.application.call(Rack::MockRequest.env_for(path, method: method, "HTTP_HOST" => "example.com")) + end + + _, _, body = request.call("GET", "/admin/login") + html = +"" + body.each { |chunk| html << chunk } + action = html[/]*action="([^"]+)"/, 1] + + status, headers, = request.call("POST", action) + puts action, status, headers["Location"] + RUBY + end + + it "points the login form and the middleware at the host's prefix" do + out, err, status = Open3.capture3(RbConfig.ruby, "-e", boot_script, + chdir: File.expand_path("../..", __dir__)) + expect(status).to be_success, err + + action, post_status, location = out.lines.last(3).map(&:chomp) + expect(action).to eq("/sso/auth/oidc") + expect(post_status).to eq("302") + expect(location).to eq("http://example.com/sso/auth/oidc/callback") + end +end