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[/]*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 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