Follow a host-set Devise.omniauth_path_prefix in the strategy too - #21
Conversation
Devise's `omniauth_path_prefix` decides where the OmniAuth routes are
DRAWN; the strategy's own `path_prefix` decides where the middleware
LISTENS. The engine honoured a host-set value for the first and ignored
it for the second:
::Devise.omniauth_path_prefix ||= cfg.omniauth_route_prefix # host wins
devise.omniauth :openid_connect, path_prefix: cfg.omniauth_path_prefix
A host with `config.omniauth_path_prefix = '/sso/auth'` in its own
devise.rb — the line Devise's own generator template suggests — ends up
with the callback route at /sso/auth/oidc/callback and the middleware
listening on /admin/auth. Every SSO sign-in 404s after the IdP round
trip, and README.md documents that configuration as supported.
A host that pinned only Devise's setting meant one prefix, so follow it
for both. An explicit `c.omniauth_path_prefix` still wins, which is
what engine-mounted hosts need — there the two genuinely differ, by the
mount prefix.
Also here:
* `active_admin_namespace` now applies `.underscore`, matching
`ActiveAdmin::Namespace#initialize`. With
`default_namespace = :"admin-panel"` ActiveAdmin routes under
/admin_panel while the gem mounted /admin-panel/login and raised
looking up `:"admin-panel_root_path"`.
* The `#login_submit_path` example asserted against
`OmniAuth.config.path_prefix`, a global that tracks the same value
— it passed under the old implementation too, so it could not have
caught the regression it guards, and it only passed at all when
another spec had booted a dummy app first. Now a literal, plus a
second example for the derived default.
Rails 8 loads routes lazily in the test environment, so the draw happened
whenever the first example touched the route set -- and it inherited that
example's stubs. `spec/requests/after_sign_in_path_spec.rb` stubs
`ActiveAdmin.application.default_namespace` and then reads
`Devise.mappings`, which calls `reload_routes_unless_loaded` internally;
when that example ran first, the whole app got drawn under the stubbed
namespace and the bad route set leaked into every example after it.
That is what made the suite order-dependent. Two different symptoms, one
cause:
RuntimeError: Could not find a valid mapping for path
"/admin/auth/oidc/callback"
-- spec/requests/omniauth_callback_spec.rb, seed 18410
ActionController::RoutingError: No route matches
[POST] "/admin/login/stub"
-- spec/requests/stub_login_spec.rb:224, seed 21181
`rspec --bisect` on seed 18410 reduced it to two examples:
rspec ./spec/requests/after_sign_in_path_spec.rb[1:2] \
./spec/requests/omniauth_callback_spec.rb[1:7:1] --seed 18410
Note `reload_routes!` does not work here. On a not-yet-loaded app it
draws the routes and then resets the loaded flag back to false:
def reload_routes!
if routes_reloader.execute_unless_loaded
routes_reloader.loaded = false
else
routes_reloader.reload!
end
end
so the next `Devise.mappings` call redraws them anyway, which leaves the
original problem in place. `reload_routes_unless_loaded` draws them and
leaves them loaded. `try` keeps this a no-op on Rails 7.2, which has no
lazy route loading and no such method -- the same call Devise makes.
Seeds 6, 6233, 18410, 21181, 35323 and 62645 all pass after this.
|
Pushed one commit on top of this branch: a766de0 — "Draw the routes once before the suite runs", a single line in What it fixesThis branch already removed the first half of the order-dependence by replacing the mutable Rails 8 loads routes lazily in test, so the draw happened whenever the first example touched the route set — inheriting that example's stubs.
One gotcha worth recording
def reload_routes!
if routes_reloader.execute_unless_loaded
routes_reloader.loaded = false
else
routes_reloader.reload!
end
end…so the next Seed gridDefault suite, lockfiles deleted first,
166 examples each (8 pending on AA 4.0, 1 on AA 3.5). The engine-, isolated- and root-mounted suites were also run at seeds 6 / 18410 / 21181 / 62645 on both gemfiles — 7 / 8 / 6 examples, 0 failures throughout. They have their own helpers and never exhibited the problem, so they are left alone. Proof the commit is load-bearingReverting just
One more check worth calling out. CI here builds the merge commit, so its
Those 9 are exactly the cluster that has been showing up on CI on the merge commit: 6/6 legs pass (Ruby 3.3 / 3.4 / 4.0 × both gemfiles). Notes
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The login form still submits to the derived prefix when the middleware adopts Devise’s host-configured prefix.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates OIDC path handling to align Devise routes, middleware, and ActiveAdmin namespaces.
Changes:
- Honors host-configured Devise OmniAuth prefixes.
- Normalizes ActiveAdmin namespaces with
underscore. - Improves path tests and stabilizes Rails 8 route loading.
| File | Description |
|---|---|
lib/activeadmin/oidc/engine.rb |
Selects the middleware prefix. |
lib/activeadmin/oidc/configuration.rb |
Tracks explicit prefixes and normalizes namespaces. |
spec/unit/configuration_spec.rb |
Adds namespace and path-prefix coverage. |
spec/rails_helper.rb |
Loads routes before examples run. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The engine moved the middleware to a host-set Devise prefix but kept the value local, so `login_submit_path` still built /admin/auth/oidc and the login button hit a dead route. Writing it back to the config gives the view and the middleware one value. Co-Authored-By: Clanker
Follow-up to #17, found by an adversarial review of it.
The bug
Devise's
omniauth_path_prefixdecides where the OmniAuth routes are drawn. The strategy's ownpath_prefixdecides where the middleware listens.lib/activeadmin/oidc/engine.rbhonoured a host-set value for the first and ignored it for the second:Trigger:
config/initializers/devise.rbcontainsconfig.omniauth_path_prefix = '/sso/auth'— the line Devise's own generator template suggests — with the ActiveAdmin namespace left at:admin. Bootingspec/dummywith that value pre-set, before this change:Every SSO sign-in on such a host 404s after the IdP round trip, and the passthru route Devise drew is equally dead because the middleware never intercepts it.
README.mddocuments this configuration as supported and saysomniauth_path_prefix"stays correct regardless" — it is the one thing that is not.Nothing raises. Devise's
set_omniauth_path_prefix!only warns whenOmniAuth.config.path_prefixis already set, anddevise/omniauth.rbnils that global on load, so the split is silent.The fix
A host that pinned only Devise's setting meant one prefix, not two, so the engine writes that value back to
cfg.omniauth_path_prefix. The strategy and the login form'slogin_submit_pathboth read it, so the button, the middleware and the routes agree. An explicitc.omniauth_path_prefixstill wins — that is what engine-mounted hosts need, since there the two genuinely differ by the mount prefix.Same setup after the change, through the login page:
spec/requests/host_devise_prefix_spec.rbcovers this end to end. It bootsspec/dummyin a child process with the prefix preset, because the suite's own app is already booted without it.Also in this PR
active_admin_namespacenow applies.underscore.ActiveAdmin::Namespace#initializedoesname.to_s.underscore, soconfig.default_namespace = :"admin-panel"routes everything under/admin_panel. The gem derived from the raw value:so the login page mounted somewhere ActiveAdmin never routed, and
public_send(:"admin-panel_root_path")raised into the fallback. Covered by two new unit examples.#login_submit_pathspec had no discriminating power. It asserted againstOmniAuth.config.path_prefix— a global that tracks the same value, so it passed under the pre-#17 implementation too and could not have caught the regression it guards. It also only passed at all when another spec file had booted a dummy app first; run standalone it failed. Now a literal, plus a second example for the derived default.The suite no longer depends on the random seed. Rails 8 draws routes lazily, so the first example to touch them drew them under its own stubs.
spec/rails_helper.rbnow draws them once before any example runs. Details in the comment below.Suites