From ac8a08ff0479363502866ee03496707d2a88b17a Mon Sep 17 00:00:00 2001 From: Denis Talakevich Date: Thu, 1 Oct 2026 18:01:40 +0300 Subject: [PATCH] Replace omniauth_path_prefix_configured? with an adopt method The engine asked the predicate and then assigned the prefix, so the predicate answered true for every host after boot. One method that checks and assigns keeps the decision in Configuration and drops a public method before 3.0.0 makes it permanent. Co-Authored-By: Clanker --- lib/activeadmin/oidc/configuration.rb | 11 +++++----- lib/activeadmin/oidc/engine.rb | 17 +++------------- spec/unit/configuration_spec.rb | 29 ++++++++++++++++++--------- 3 files changed, 28 insertions(+), 29 deletions(-) diff --git a/lib/activeadmin/oidc/configuration.rb b/lib/activeadmin/oidc/configuration.rb index 2a85953..6d61bad 100644 --- a/lib/activeadmin/oidc/configuration.rb +++ b/lib/activeadmin/oidc/configuration.rb @@ -89,11 +89,12 @@ 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? + # A host that set only `Devise.omniauth_path_prefix` meant one + # prefix, so it becomes ours too. An explicit value set here still + # wins: engine-mounted hosts need the two to differ by the mount + # prefix. + def adopt_devise_omniauth_path_prefix(prefix) + @omniauth_path_prefix ||= prefix.presence end # What Devise declares its OmniAuth request/callback routes with. diff --git a/lib/activeadmin/oidc/engine.rb b/lib/activeadmin/oidc/engine.rb index 8f37854..9d19178 100644 --- a/lib/activeadmin/oidc/engine.rb +++ b/lib/activeadmin/oidc/engine.rb @@ -111,20 +111,9 @@ def controllers ::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 + # strategy's `path_prefix` decides where the middleware LISTENS, + # and the login view posts to it. If they differ, sign-in 404s. + cfg.adopt_devise_omniauth_path_prefix(host_prefix) ::Devise.setup do |devise| devise.omniauth :openid_connect, diff --git a/spec/unit/configuration_spec.rb b/spec/unit/configuration_spec.rb index b8bc4a6..dc37da1 100644 --- a/spec/unit/configuration_spec.rb +++ b/spec/unit/configuration_spec.rb @@ -317,19 +317,28 @@ def with_namespace(namespace) 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) + describe "#adopt_devise_omniauth_path_prefix" do + # The engine passes a host-set `Devise.omniauth_path_prefix` here. + # Getting it wrong draws the callback route and the middleware on + # different paths. + it "adopts the host's prefix while ours is only derived" do + config.adopt_devise_omniauth_path_prefix("/sso/auth") + + expect(config.omniauth_path_prefix).to eq("/sso/auth") + expect(config.login_submit_path).to eq("/sso/auth/oidc") + end + + it "keeps an explicit prefix" do + config.omniauth_path_prefix = "/admin/auth" + config.adopt_devise_omniauth_path_prefix("/auth") + + expect(config.omniauth_path_prefix).to eq("/admin/auth") end - it "is true once the host assigns one" do - config.omniauth_path_prefix = "/sso/auth" + it "keeps the derived default when Devise has no prefix" do + config.adopt_devise_omniauth_path_prefix(nil) - expect(config.omniauth_path_prefix_configured?).to be(true) + expect(config.omniauth_path_prefix).to eq("/admin/auth") end end