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..8f37854 100644 --- a/lib/activeadmin/oidc/engine.rb +++ b/lib/activeadmin/oidc/engine.rb @@ -107,8 +107,25 @@ 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. + # + # 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, 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! 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[/