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