Follow a host-set Devise.omniauth_path_prefix in the strategy too

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

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,

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!

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

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