diff --git a/.gitignore b/.gitignore index 34bd2a2..02b2a7a 100644 --- a/.gitignore +++ b/.gitignore @@ -21,9 +21,12 @@ /spec/dummy_engine/tmp/ /spec/dummy_isolated/log/ /spec/dummy_isolated/tmp/ +/spec/dummy_namespaced/log/ +/spec/dummy_namespaced/tmp/ # The dummy app's database.yml is checked in — override a global # ignore rule that excludes "database.yml" everywhere by default. !/spec/dummy/config/database.yml !/spec/dummy_engine/config/database.yml !/spec/dummy_isolated/config/database.yml +!/spec/dummy_namespaced/config/database.yml diff --git a/.rspec b/.rspec index 28e9c03..db77bd0 100644 --- a/.rspec +++ b/.rspec @@ -3,6 +3,6 @@ --color # `bundle exec rspec` runs the default suite (spec/dummy). The engine- # and isolated-engine suites each boot their own dummy Rails app and -# must run in separate processes — invoke them via `rake spec:engine`, -# `rake spec:isolated`, or all three with `rake spec:all`. +# must run in separate processes — invoke them via `rake spec:engine` +# and `rake spec:isolated`, or all three with `rake spec:all`. --exclude-pattern "spec/{engine,isolated,dummy_engine,dummy_isolated}/**/*" diff --git a/README.md b/README.md index 9951fe3..1e95c27 100644 --- a/README.md +++ b/README.md @@ -50,16 +50,25 @@ OIDC is the only authentication mechanism — `:database_authenticatable`, `encr If `devise_for :admin_users` lives inside a Rails engine (not the main app routes), set `Devise.router_name = :` in `config/initializers/devise.rb` and pass the same option to `devise_for`. The gem reads `Devise.available_router_name` and mounts its session routes inside that engine's route set, so `.routes.url_helpers.new__session_path` resolves correctly. -For **isolated** engines (`isolate_namespace ...`) mounted at a prefix (e.g. `mount AdminPanel::Engine => '/admin'`), the engine prepends its mount path to every internal route. The gem's default `login_path = '/admin/login'` would then become `/admin/admin/login`. Configure engine-relative paths in `config/initializers/activeadmin_oidc.rb`: +For **isolated** engines (`isolate_namespace ...`) mounted at a prefix (e.g. `mount AdminPanel::Engine => '/admin'`), the engine prepends its mount path to every internal route, so the derived `login_path` of `/admin/login` would become `/admin/admin/login`. Configure engine-relative paths in `config/initializers/activeadmin_oidc.rb`: ```ruby ActiveAdmin::Oidc.configure do |c| - c.login_path = '/login' - c.logout_path = '/logout' + c.login_path = '/login' + c.logout_path = '/logout' + c.omniauth_route_prefix = '/auth' end ``` -Non-isolated engines don't need this override. +`omniauth_route_prefix` is the same idea applied to the routes Devise draws for +OmniAuth. It is separate from `omniauth_path_prefix` because the two are not the +same string here: the OmniAuth middleware sits in the *application's* Rack stack +and sees `/admin/auth/oidc` with the mount prefix still attached, while the route +Devise declares for the callback is inside the engine and gets `/admin` prepended +to it. Set only `omniauth_path_prefix` and the callback route lands on +`/admin/admin/auth/oidc/callback`, so the redirect the middleware issues 404s. + +Non-isolated engines mounted at `/` don't need any of these overrides. ### 3. `config/initializers/activeadmin_oidc.rb` (generated) @@ -73,7 +82,7 @@ The gem's Rails engine handles several things so host apps don't have to: * **Callback controller** — the engine patches `ActiveAdmin::Devise.controllers` to route OmniAuth callbacks to the gem's controller. No manual `controllers: { omniauth_callbacks: ... }` needed in `routes.rb`. * **Login view override** — the engine prepends an SSO-only login page (no email/password fields) to the sessions controller's view path. If your host app ships its own `app/views/active_admin/devise/sessions/new.html.erb`, the gem detects it and backs off — your view wins. * **Session routes** — the engine mounts `GET /admin/login` (renders the SSO landing page) and `DELETE /admin/logout` under `devise_scope`, with the scope name derived from `config.admin_user_class`. Devise normally generates session routes as a side effect of `:database_authenticatable`; without that module the route helpers would not exist and ActiveAdmin's login redirect would 404. -* **Path prefix** — the engine sets `Devise.omniauth_path_prefix` and `OmniAuth.config.path_prefix` to `/admin/auth` so the middleware intercepts requests under ActiveAdmin's mount point. Compatible with Rails 7.2+ and Rails 8's lazy route loading. +* **Path prefix** — the engine registers the strategy with `path_prefix: '/admin/auth'` so the middleware intercepts requests under ActiveAdmin's mount point, and sets `Devise.omniauth_path_prefix` to the prefix Devise declares its routes with. Compatible with Rails 7.2+ and Rails 8's lazy route loading. * **Parameter filtering** — `code`, `id_token`, `access_token`, `refresh_token`, `state`, and `nonce` are added to `Rails.application.config.filter_parameters`. ## Configuration @@ -133,10 +142,39 @@ end | `identity_attribute` | `:email` | AdminUser column used for lookup/adoption | | `identity_claim` | `:email` | Claim key read from the id_token/userinfo | | `admin_user_class` | `"AdminUser"` | String or Class for the host's admin user model | +| `login_path` | `//login` | SSO landing page path; derived from ActiveAdmin's namespace | +| `logout_path` | `//logout` | Sign-out path; derived from ActiveAdmin's namespace | +| `omniauth_path_prefix` | `//auth` | Browser-visible path the OmniAuth middleware listens on; derived from ActiveAdmin's namespace | +| `omniauth_route_prefix` | `omniauth_path_prefix` | Prefix Devise declares its OmniAuth routes with; differs only for engine-mounted hosts | | `login_button_label` | `"Sign in with SSO"` | Label on the login-page button | | `access_denied_message` | generic | Flash shown on any denial | | `on_login` | — (required) | Authorization hook; see below | +## ActiveAdmin's namespace + +Everything the gem mounts hangs off ActiveAdmin's namespace, and all of it is +derived from `ActiveAdmin.application.default_namespace` rather than assumed to +be `admin`. A host that renames it: + +```ruby +# config/initializers/active_admin.rb +config.default_namespace = :backoffice +``` + +gets `/backoffice/login`, `/backoffice/logout`, the OmniAuth middleware at +`/backoffice/auth`, and a post-sign-in redirect to `/backoffice` — no gem +configuration needed. ActiveAdmin's root namespace (`config.default_namespace = +false`) mounts everything at the top level: `/login`, `/auth`, `/`. + +Each is still overridable. Isolated engines *have* to override them, since the +engine's mount prefix is prepended to every path declared inside it — see +[Engine-mounted Devise](#engine-mounted-devise). + +`omniauth_route_prefix` is what the gem assigns to `Devise.omniauth_path_prefix`, +and it is skipped entirely if your app already assigned that in +`config/initializers/devise.rb`. `omniauth_path_prefix` is passed to the OmniAuth +strategy directly, so it stays correct regardless. + ## The `on_login` hook `on_login` is the **only** place authorization lives. The gem handles authentication (the user proved who they are via the IdP); deciding whether that user is allowed into the admin panel — and what they can see once they are in — is the host application's problem. The gem does not ship a role model. @@ -241,7 +279,7 @@ AdminUser.last.oidc_raw_info * A login button is added to the ActiveAdmin sessions page via a prepended view override — no templates to edit. * Clicking it POSTs to `/admin/auth/oidc` with a Rails CSRF token. The gem loads `omniauth-rails_csrf_protection` so OmniAuth 2.x delegates its authenticity check to Rails' forgery protection and `button_to` just works. -* After a successful callback the user is signed in and redirected to `/admin` (not the host app's `/`, which may not exist). +* After a successful callback the user is signed in and redirected to ActiveAdmin's namespace root (not the host app's `/`, which may not exist). The path comes from ActiveAdmin's own route helper, so a renamed `config.default_namespace` or an engine-mounted ActiveAdmin lands correctly; `/admin` is only the fallback when that helper cannot be resolved. * **Disabled/locked users are rejected.** Devise's `active_for_authentication?` is checked after provisioning but before sign-in. If your model overrides this method (e.g. to check an `enabled` flag or Devise's `:lockable` module), the guard fires on OIDC sign-in too — the user sees an appropriate flash and is redirected to the login page. * Logout goes through Devise's stock session destroy. No RP-initiated single-logout ping to the IdP — override the destroy action in your host app if you need that. diff --git a/Rakefile b/Rakefile index 64ca094..67501d3 100644 --- a/Rakefile +++ b/Rakefile @@ -25,7 +25,7 @@ begin end desc "Run every spec suite (default + engine + isolated)" - task all: [:spec, :engine, :isolated] + task all: %i[spec engine isolated] end task default: :spec diff --git a/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb b/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb index a43bf23..1aeac9e 100644 --- a/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb +++ b/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb @@ -69,28 +69,53 @@ def failure # sign-in instead of Devise's default (host app root). Hosts # that don't define a `/` route would otherwise hit a routing # error immediately after login, and even when `/` does exist - # it's rarely what an admin user wants to see. ActiveAdmin - # always mounts at `/admin`, so we go there directly. + # it's rarely what an admin user wants to see. def after_sign_in_path_for(resource) - stored_location_for(resource) || '/admin' + stored_location_for(resource) || active_admin_root_path + end + + # Resolved from ActiveAdmin's own route helper rather than + # assumed to be '/admin'. Two things move it: a host can rename + # the namespace (`config.default_namespace = :backoffice`), and + # an engine-mounted ActiveAdmin prefixes every path it declares + # with the engine's mount point -- so the real root can be + # '/admin/admin' or anything else entirely. Routing a signed-in + # admin to a 404 is a poor reward for a successful login. + def active_admin_root_path + send(devise_router_name(resource_name)).public_send(active_admin_root_helper) + rescue NameError, ::ActionController::UrlGenerationError + # The host has ActiveAdmin's routes somewhere we can't see (or + # hasn't drawn them at all). Its namespace prefix is the best + # remaining guess at where the admin panel lives -- except for + # the root namespace, where that prefix is '' and would make an + # empty, unredirectable Location. + ActiveAdmin::Oidc.config.active_admin_namespace_prefix.presence || '/' + end + + # ActiveAdmin names its namespace root helper `_root_path`, + # except for the root namespace where it is plain `root_path`. + def active_admin_root_helper + namespace = ActiveAdmin::Oidc.config.active_admin_namespace + namespace ? :"#{namespace}_root_path" : :root_path end # Devise's `new_session_path(scope)` is only generated when # `:database_authenticatable` is in the mapping's `used_helpers`, # so an OIDC-only model never gets it. The engine mounts - # `new__session_path` itself, but the helper lives on - # whichever route set Devise's URL helper dispatcher points at: - # the per-mapping `router_name` (set by - # `devise_for :scope, router_name: :engine`) when present, - # otherwise the global `Devise.available_router_name` - # (set by `Devise.router_name = :engine`), which defaults to - # `:main_app`. Replicate that dispatcher here so the helper is - # resolved on the right context (Rails.application proxy or - # mounted engine proxy). + # `new__session_path` itself, but on whichever route set + # `devise_router_name` resolves to. def after_omniauth_failure_path_for(scope) - router_name = ::Devise.mappings[scope].router_name || - ::Devise.available_router_name - send(router_name).public_send(:"new_#{scope}_session_path") + send(devise_router_name(scope)).public_send(:"new_#{scope}_session_path") + end + + # Devise's URL helpers live on the per-mapping `router_name` (set + # by `devise_for :scope, router_name: :engine`) when present, + # otherwise on the global `Devise.available_router_name` (set by + # `Devise.router_name = :engine`), which defaults to `:main_app`. + # Replicate that dispatcher here so helpers resolve on the right + # context (Rails.application proxy or mounted engine proxy). + def devise_router_name(scope) + ::Devise.mappings[scope]&.router_name || ::Devise.available_router_name end end end diff --git a/app/views/active_admin/devise/sessions/new.html.erb b/app/views/active_admin/devise/sessions/new.html.erb index 700d875..bff44e3 100644 --- a/app/views/active_admin/devise/sessions/new.html.erb +++ b/app/views/active_admin/devise/sessions/new.html.erb @@ -5,7 +5,7 @@ <%= button_to ActiveAdmin::Oidc.config.login_button_label, - "#{OmniAuth.config.path_prefix}/oidc", + "#{ActiveAdmin::Oidc.config.omniauth_path_prefix}/oidc", method: :post, class: "activeadmin-oidc-login-button w-full", form_class: 'formtastic', @@ -15,7 +15,7 @@

<%= active_admin_application.site_title(self) %>

- <%= form_tag "#{OmniAuth.config.path_prefix}/oidc", + <%= form_tag "#{ActiveAdmin::Oidc.config.omniauth_path_prefix}/oidc", method: :post, class: "activeadmin-oidc-login-form formtastic", data: { turbo: false } do %> diff --git a/lib/activeadmin/oidc/configuration.rb b/lib/activeadmin/oidc/configuration.rb index 5bdda41..9dcd4ef 100644 --- a/lib/activeadmin/oidc/configuration.rb +++ b/lib/activeadmin/oidc/configuration.rb @@ -11,15 +11,19 @@ class Configuration DEFAULT_ADMIN_USER_CLASS = 'AdminUser' DEFAULT_ACCESS_DENIED_MESSAGE = 'Your account has no permission to access this admin panel.' - DEFAULT_LOGIN_PATH = '/admin/login' - DEFAULT_LOGOUT_PATH = '/admin/logout' + # Stands in for ActiveAdmin's namespace when ActiveAdmin is not + # loaded (plain unit specs, scripts) and it therefore cannot be + # read. Everything else derives from + # `ActiveAdmin.application.default_namespace`. + FALLBACK_NAMESPACE = :admin attr_accessor :issuer, :client_id, :client_secret, :scope, :redirect_uri, :login_button_label, :timeout, :identity_attribute, :identity_claim, - :access_denied_message, :on_login, :admin_user_class, - :login_path, :logout_path + :access_denied_message, :on_login, :admin_user_class + attr_writer :login_path, :logout_path, + :omniauth_path_prefix, :omniauth_route_prefix def initialize reset! @@ -37,13 +41,79 @@ def reset! @identity_claim = DEFAULT_IDENTITY_CLAIM @access_denied_message = DEFAULT_ACCESS_DENIED_MESSAGE @admin_user_class = DEFAULT_ADMIN_USER_CLASS - @login_path = DEFAULT_LOGIN_PATH - @logout_path = DEFAULT_LOGOUT_PATH + @login_path = nil + @logout_path = nil + @omniauth_path_prefix = nil + @omniauth_route_prefix = nil @on_login = nil @pkce_override = nil self end + # The paths below all hang off ActiveAdmin's namespace, which the + # host can rename (`config.default_namespace = :backoffice`) -- in + # which case there is no /admin anywhere in the app and every + # hardcoded one would 404. They are computed on read rather than in + # `reset!` because the gem's own initializer may run before the + # host's `ActiveAdmin.setup` block. + # + # `login_path` and `logout_path` are declared inside whichever route + # set holds the host's Devise mapping, so an engine-mounted host has + # to override them engine-relative -- the mount prefix is prepended + # on top of whatever is written here. + def login_path + @login_path || "#{active_admin_namespace_prefix}/login" + end + + def logout_path + @logout_path || "#{active_admin_namespace_prefix}/logout" + end + + # Where the OmniAuth middleware listens. This one is a real, + # browser-visible path: the middleware sits in the application's + # Rack stack and sees the URL before any engine mount prefix has + # been stripped. + def omniauth_path_prefix + @omniauth_path_prefix || "#{active_admin_namespace_prefix}/auth" + 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 + # mounted engine -- so an engine-mounted host sets this + # engine-relative ('/auth'), the same way it does `login_path`. + def omniauth_route_prefix + @omniauth_route_prefix || omniauth_path_prefix + end + + # ActiveAdmin's namespace as a Symbol, or nil for the root + # namespace (`default_namespace = false`), which mounts everything + # at the top level. + def active_admin_namespace + namespace = active_admin_default_namespace + return nil if namespace.blank? || namespace.to_sym == :root + + namespace.to_sym + end + + # Narrow on purpose: `NoMethodError` is what "ActiveAdmin is not + # loaded, or not set up yet" surfaces as. Anything else -- a host + # initializer blowing up inside its own `default_namespace` + # override, say -- is a real misconfiguration and must not be + # quietly turned into a wrong path. + def active_admin_default_namespace + return FALLBACK_NAMESPACE unless defined?(::ActiveAdmin) && ::ActiveAdmin.respond_to?(:application) + + ::ActiveAdmin.application.default_namespace + rescue NoMethodError + FALLBACK_NAMESPACE + end + + def active_admin_namespace_prefix + namespace = active_admin_namespace + namespace ? "/#{namespace}" : '' + end + def pkce return @pkce_override unless @pkce_override.nil? diff --git a/lib/activeadmin/oidc/engine.rb b/lib/activeadmin/oidc/engine.rb index 98d880b..1cffc67 100644 --- a/lib/activeadmin/oidc/engine.rb +++ b/lib/activeadmin/oidc/engine.rb @@ -7,6 +7,12 @@ module Oidc class Engine < ::Rails::Engine PROVIDER_NAME = :oidc + # True once the host has supplied the minimum OIDC credentials. + def self.configured? + cfg = ActiveAdmin::Oidc.config + cfg.issuer.present? && cfg.client_id.present? + end + # True when the host's AdminUser model includes :omniauthable. # Used to gate controller registration and view overrides so the # gem is a no-op when OIDC is not enabled on the model. @@ -77,21 +83,36 @@ def controllers # Automatically register the OmniAuth :openid_connect strategy with # Devise when the gem is configured, so host apps don't have to # duplicate the config.omniauth boilerplate in devise.rb. - # Runs before Devise's own initializer so the strategy is available - # when the model calls `devise :omniauthable`. - initializer 'activeadmin_oidc.register_omniauth_strategy', before: 'devise.omniauth' do + # + # Ordered after the host's config/initializers (where + # `ActiveAdmin.setup` and `Devise.setup` live, so the namespace and + # any explicit host overrides are readable) and before + # `devise.omniauth`, which both needs the strategy registered and + # builds the middleware from these args. + # + # `path_prefix` is passed per-strategy rather than left to + # `OmniAuth.config.path_prefix`, because Devise overwrites that + # global at route-draw time with `Devise.omniauth_path_prefix` -- + # the value it *declares its routes* with, which for a mounted + # engine is the same path minus the mount prefix. Setting the two + # independently through the global is impossible: Devise raises + # "Wrong OmniAuth configuration" whenever they disagree. + initializer 'activeadmin_oidc.register_omniauth_strategy', + after: :load_config_initializers, before: 'devise.omniauth' do cfg = ActiveAdmin::Oidc.config - next if cfg.issuer.blank? || cfg.client_id.blank? + next unless Engine.configured? require 'omniauth_openid_connect' - ::Devise.setup do |devise| - # ActiveAdmin mounts Devise under /admin, so OmniAuth middleware - # must intercept /admin/auth/:provider. - devise.omniauth_path_prefix ||= '/admin/auth' + # `||=` 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. + ::Devise.omniauth_path_prefix ||= cfg.omniauth_route_prefix + ::Devise.setup do |devise| devise.omniauth :openid_connect, name: PROVIDER_NAME, + path_prefix: cfg.omniauth_path_prefix, scope: (cfg.scope || 'openid email profile').split, response_type: :code, issuer: cfg.issuer, @@ -106,15 +127,6 @@ def controllers host: nil }.compact end - - # Devise propagates omniauth_path_prefix to - # OmniAuth.config.path_prefix during route generation - # (set_omniauth_path_prefix!). On Rails 8 routes load lazily, - # so the OmniAuth middleware may process requests before routes - # are drawn and miss the prefix. Set it eagerly here. - # Must happen AFTER `devise.omniauth` because that call - # triggers autoload of devise/omniauth which nils the value. - ::OmniAuth.config.path_prefix = ::Devise.omniauth_path_prefix end initializer 'activeadmin_oidc.filter_parameters' do |app| diff --git a/lib/generators/active_admin/oidc/install/templates/sessions_new.html.erb b/lib/generators/active_admin/oidc/install/templates/sessions_new.html.erb index c7b277b..c3353d6 100644 --- a/lib/generators/active_admin/oidc/install/templates/sessions_new.html.erb +++ b/lib/generators/active_admin/oidc/install/templates/sessions_new.html.erb @@ -1,7 +1,7 @@

<%%= active_admin_application.site_title(self) %>

- <%%= form_tag "#{OmniAuth.config.path_prefix}/oidc", + <%%= form_tag "#{ActiveAdmin::Oidc.config.omniauth_path_prefix}/oidc", method: :post, class: "activeadmin-oidc-login-form formtastic", data: { turbo: false } do %> diff --git a/lib/generators/active_admin/oidc/install/templates/sessions_new_v4.html.erb b/lib/generators/active_admin/oidc/install/templates/sessions_new_v4.html.erb index 875f972..2e98060 100644 --- a/lib/generators/active_admin/oidc/install/templates/sessions_new_v4.html.erb +++ b/lib/generators/active_admin/oidc/install/templates/sessions_new_v4.html.erb @@ -4,7 +4,7 @@ <%%= button_to ActiveAdmin::Oidc.config.login_button_label, - "#{OmniAuth.config.path_prefix}/oidc", + "#{ActiveAdmin::Oidc.config.omniauth_path_prefix}/oidc", method: :post, class: "activeadmin-oidc-login-button w-full", form_class: 'formtastic', diff --git a/spec/dummy_isolated/config/initializers/activeadmin_oidc.rb b/spec/dummy_isolated/config/initializers/activeadmin_oidc.rb index 819f45c..e76d11b 100644 --- a/spec/dummy_isolated/config/initializers/activeadmin_oidc.rb +++ b/spec/dummy_isolated/config/initializers/activeadmin_oidc.rb @@ -10,4 +10,10 @@ # would become `/admin/admin/login`. Use relative paths instead. c.login_path = "/login" c.logout_path = "/logout" + + # Same reason, for the routes Devise draws for OmniAuth. The + # middleware still listens on the browser-visible `/admin/auth` + # (the derived `omniauth_path_prefix`); only the route declaration + # is engine-relative. + c.omniauth_route_prefix = "/auth" end diff --git a/spec/isolated/requests/isolated_engine_callback_spec.rb b/spec/isolated/requests/isolated_engine_callback_spec.rb new file mode 100644 index 0000000..fca4387 --- /dev/null +++ b/spec/isolated/requests/isolated_engine_callback_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require "isolated_rails_helper" + +# The OmniAuth round trip for an isolated engine mounted at a prefix. +# +# Devise reuses one setting -- `Devise.omniauth_path_prefix` -- both to +# declare its OmniAuth routes and to tell the OmniAuth middleware where +# to listen. Those two are not the same string here: routes declared +# inside AdminPanel::Engine get `/admin` prepended by the mount, while +# the middleware sits in the application's Rack stack and sees the URL +# with `/admin` still on it. Feeding the browser-visible `/admin/auth` +# to both puts the callback route at `/admin/admin/auth/oidc/callback`, +# so the redirect the middleware issues 404s. +# +# The gem therefore drives them separately: `omniauth_route_prefix` +# ('/auth' here) for the route declaration, `omniauth_path_prefix` +# ('/admin/auth', derived) as a per-strategy middleware option. +RSpec.describe "Isolated engine OIDC callback", type: :request do + before do + # spec_helper resets the singleton config before every example, so + # restore the parts the callback action reads at request time. + ActiveAdmin::Oidc.configure do |c| + c.issuer = "https://idp.example.com" + c.client_id = "client-abc" + c.on_login = ->(_admin_user, _claims) { true } + end + + OmniAuth.config.mock_auth[:oidc] = OmniAuth::AuthHash.new( + provider: "oidc", + uid: "sub-isolated", + info: { "email" => "isolated@example.com" }, + extra: { "raw_info" => { "sub" => "sub-isolated", "email" => "isolated@example.com" } } + ) + AdminUser.delete_all + end + + after { OmniAuth.config.mock_auth[:oidc] = nil } + + it "declares the callback route engine-relative so the mount prefix lands it on /admin/auth" do + paths = AdminPanel::Engine.routes.routes.map { |r| r.path.spec.to_s } + expect(paths).to include("/auth/oidc/callback(.:format)") + end + + it "keeps the middleware listening on the browser-visible /admin/auth" do + post "/admin/auth/oidc" + + expect(response).to redirect_to("http://www.example.com/admin/auth/oidc/callback") + end + + it "completes the round trip and signs the admin user in" do + post "/admin/auth/oidc" + follow_redirect! + + expect(AdminUser.find_by(uid: "sub-isolated")).to be_present + # Not bounced back to the SSO landing page, which is where every + # failure path ends up. + expect(response.headers["Location"]).not_to include("/admin/login") + end +end diff --git a/spec/requests/after_sign_in_path_spec.rb b/spec/requests/after_sign_in_path_spec.rb new file mode 100644 index 0000000..4c9163d --- /dev/null +++ b/spec/requests/after_sign_in_path_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Where a successful callback lands. The path used to be the literal +# '/admin', which is wrong for any host whose ActiveAdmin is not mounted +# exactly there -- a renamed `config.default_namespace`, or ActiveAdmin +# drawn inside a mounted engine, whose mount prefix is prepended to +# every path it declares. It is now read from ActiveAdmin's own route +# helper. +# +# This dummy app uses the default :admin namespace in the main app, so +# the resolved path and the old hardcoded one coincide; what these specs +# pin down is that the value is *derived* rather than written down. +RSpec.describe "Post-sign-in landing path" do + let(:admin_user) do + AdminUser.create!(email: "alice@example.com", provider: "oidc", uid: "sub-123") + end + + let(:controller) do + ActiveAdmin::Oidc::Devise::OmniauthCallbacksController.new.tap do |c| + request = ActionDispatch::TestRequest.create + request.env["devise.mapping"] = Devise.mappings[:admin_user] + request.env["rack.session"] = ActionController::TestSession.new + c.set_request!(request) + c.set_response!(ActionDispatch::TestResponse.create) + end + end + + before { AdminUser.delete_all } + + it "uses ActiveAdmin's namespace root helper" do + expect(controller.send(:after_sign_in_path_for, admin_user)) + .to eq(Rails.application.routes.url_helpers.admin_root_path) + end + + it "builds the helper name from ActiveAdmin's configured namespace" do + allow(ActiveAdmin.application).to receive(:default_namespace).and_return(:backoffice) + + # No :backoffice routes exist here, so stand in for the url-helper + # proxy the controller resolves through: this pins down that the + # helper NAME follows the setting, not that the route exists. + allow(controller).to receive(:main_app) + .and_return(double("main_app", backoffice_root_path: "/backoffice")) + + expect(controller.send(:after_sign_in_path_for, admin_user)).to eq("/backoffice") + end + + it "falls back to the namespace prefix when the helper cannot be resolved" do + allow(ActiveAdmin.application).to receive(:default_namespace).and_return(:nowhere) + + expect(controller.send(:after_sign_in_path_for, admin_user)).to eq("/nowhere") + end + + # ActiveAdmin's root namespace has no prefix at all, so the fallback + # has to name a real path rather than the empty string, which would + # render an unredirectable Location header. + it "falls back to / for ActiveAdmin's root namespace" do + allow(ActiveAdmin.application).to receive(:default_namespace).and_return(false) + + expect(controller.send(:after_sign_in_path_for, admin_user)).to eq("/") + end + + it "still honours a stored location" do + controller.session["admin_user_return_to"] = "/admin/admin_users" + + expect(controller.send(:after_sign_in_path_for, admin_user)).to eq("/admin/admin_users") + end +end diff --git a/spec/requests/disabled_user_persistence_spec.rb b/spec/requests/disabled_user_persistence_spec.rb index f47ce68..d0aa21f 100644 --- a/spec/requests/disabled_user_persistence_spec.rb +++ b/spec/requests/disabled_user_persistence_spec.rb @@ -47,7 +47,7 @@ after { OmniAuth.config.mock_auth[:oidc] = nil } def post_callback - post "#{OmniAuth.config.path_prefix}/oidc" + post "#{ActiveAdmin::Oidc.config.omniauth_path_prefix}/oidc" follow_redirect! if response.redirect? end diff --git a/spec/requests/login_path_helper_spec.rb b/spec/requests/login_path_helper_spec.rb index 09e5d35..964b6a7 100644 --- a/spec/requests/login_path_helper_spec.rb +++ b/spec/requests/login_path_helper_spec.rb @@ -2,22 +2,17 @@ require "rails_helper" -# Regression spec for MEDIUM #4 — login view must derive the OmniAuth -# callback path from `OmniAuth.config.path_prefix`, not hardcode it. +# Regression spec for MEDIUM #4 — the login view must derive the OmniAuth +# request path from configuration, not hardcode `/admin/auth/oidc`. Hosts +# that rename ActiveAdmin's namespace, or set `omniauth_path_prefix` +# explicitly, otherwise get a button POSTing to a dead URL. # -# `app/views/active_admin/devise/sessions/new.html.erb` used to hardcode -# `"/admin/auth/oidc"` in the form action. Hosts that customise -# `Devise.omniauth_path_prefix` (mount Devise at a non-`/admin` path, -# or use a different sub-prefix for SSO) ended up with a button POSTing -# to a dead URL — the gem's strategy is registered at the configured -# prefix, not the hardcoded one. -# -# We can't use Devise's `omniauth_authorize_path` helper here because -# the OmniAuth middleware lives at the Rack level (global path prefix), -# while Devise route helpers resolve through the engine and get -# re-prefixed by the engine mount — producing e.g. `/admin/admin/auth/oidc` -# when Devise is engine-mounted. `OmniAuth.config.path_prefix` is the -# single source of truth for where the middleware actually listens. +# The source of truth is `ActiveAdmin::Oidc.config.omniauth_path_prefix`: +# the browser-visible path the OmniAuth middleware listens on. Neither +# `Devise.omniauth_path_prefix` nor `OmniAuth.config.path_prefix` works +# here — Devise sets both to the prefix it *declares its routes* with, +# which for an engine-mounted host is the same path minus the engine's +# mount prefix. RSpec.describe "Login view OmniAuth path", type: :request do before do ActiveAdmin::Oidc.configure do |c| @@ -25,32 +20,19 @@ c.client_id = "client-abc" c.on_login = ->(*) { true } end - - # Force routes to load NOW. Otherwise Rails 8 lazy loading defers - # `devise_for` until the first request — at which point the stub - # below is active, Devise's "OmniAuth.config.path_prefix matches - # Devise.omniauth_path_prefix" guard sees the sentinel, and raises. - # `execute_unless_loaded` is Rails 8+; fall back for 7.x. - reloader = Rails.application.routes_reloader - if reloader.respond_to?(:execute_unless_loaded) - reloader.execute_unless_loaded - else - Rails.application.reload_routes! - end end - it "renders the form action from OmniAuth.config.path_prefix (no hardcoded literal)" do - # Stub the OmniAuth path prefix to a sentinel value the hardcoded - # string could never match. If the view actually reads the prefix, - # the rendered form action will be `/oidc`; if it - # hardcodes the path, the literal "/admin/auth/oidc" stays. + it "renders the form action from the configured prefix (no hardcoded literal)" do + # A sentinel the hardcoded string could never match: if the view + # reads the config, the action is `/oidc`; if it hardcodes + # the path, the literal "/admin/auth/oidc" stays. sentinel = "/sentinel-omniauth-prefix" - allow(OmniAuth.config).to receive(:path_prefix).and_return(sentinel) + ActiveAdmin::Oidc.config.omniauth_path_prefix = sentinel get "/admin/login" expect(response.body).to include(%(action="#{sentinel}/oidc")), - "form action ignores Devise.omniauth_path_prefix — hosts that " \ - "customise the prefix get a button POSTing to a dead URL" + "form action ignores ActiveAdmin::Oidc.config.omniauth_path_prefix — " \ + "hosts that customise the prefix get a button POSTing to a dead URL" end end diff --git a/spec/unit/configuration_spec.rb b/spec/unit/configuration_spec.rb index 5b4e7ee..a15aee7 100644 --- a/spec/unit/configuration_spec.rb +++ b/spec/unit/configuration_spec.rb @@ -115,6 +115,80 @@ end end + describe "paths derived from ActiveAdmin's namespace" do + # `default_namespace` is one of ActiveAdmin's dynamically defined + # settings, so a verifying double refuses it. + def with_namespace(namespace) + without_partial_double_verification do + allow(ActiveAdmin).to receive(:application) + .and_return(double("ActiveAdmin::Application", default_namespace: namespace)) + yield + end + end + + it "follows a renamed default_namespace" do + with_namespace(:backoffice) do + expect(config.login_path).to eq("/backoffice/login") + expect(config.logout_path).to eq("/backoffice/logout") + expect(config.omniauth_path_prefix).to eq("/backoffice/auth") + end + end + + it "mounts at the top level for ActiveAdmin's root namespace" do + with_namespace(:root) do + expect(config.login_path).to eq("/login") + expect(config.omniauth_path_prefix).to eq("/auth") + end + end + + it "treats a blank namespace as the root namespace" do + with_namespace(false) { expect(config.login_path).to eq("/login") } + end + + it "keeps an explicit override, which isolated engines depend on" do + config.login_path = "/login" + config.logout_path = "/logout" + + with_namespace(:backoffice) do + expect(config.login_path).to eq("/login") + expect(config.logout_path).to eq("/logout") + # ...without dragging the derived prefix along with it. + expect(config.omniauth_path_prefix).to eq("/backoffice/auth") + end + end + + it "defaults omniauth_route_prefix to omniauth_path_prefix" do + with_namespace(:backoffice) do + expect(config.omniauth_route_prefix).to eq("/backoffice/auth") + end + end + + # Engine-mounted hosts split the two: the middleware sees the + # browser-visible path, Devise declares its routes engine-relative. + it "lets omniauth_route_prefix be overridden independently" do + config.omniauth_route_prefix = "/auth" + + with_namespace(:admin) do + expect(config.omniauth_path_prefix).to eq("/admin/auth") + expect(config.omniauth_route_prefix).to eq("/auth") + end + end + + # Library code has to stay callable with no ActiveAdmin around at + # all (unit specs, scripts). Driven by raising rather than by the + # absence of the constant, so it holds whether or not another spec + # in the same process has booted the dummy app. + it "falls back to /admin when ActiveAdmin cannot be consulted" do + without_partial_double_verification do + allow(ActiveAdmin).to receive(:application).and_raise(NoMethodError) + end + + expect(config.login_path).to eq("/admin/login") + expect(config.logout_path).to eq("/admin/logout") + expect(config.omniauth_path_prefix).to eq("/admin/auth") + end + end + describe "#pkce" do it "defaults to true when client_secret is blank" do config.client_secret = nil