diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 61a81a9..e545741 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,5 +26,14 @@ jobs: with: ruby-version: ${{ matrix.ruby }} bundler-cache: true - - name: Run all spec suites (default + engine + isolated) - run: bundle exec rake spec:all + # One step per dummy app: each boots a different Rails app, so they + # cannot share a process — and a separate step names which host + # shape broke instead of burying it in a combined run. + - name: Default suite (spec/dummy — ActiveAdmin at /admin) + run: bundle exec rake spec + - name: Engine-mounted Devise suite (spec/dummy_engine) + run: bundle exec rake spec:engine + - name: Isolated-engine suite (spec/dummy_isolated) + run: bundle exec rake spec:isolated + - name: Root-mounted suite (spec/dummy_root — ActiveAdmin at /) + run: bundle exec rake spec:root diff --git a/.gitignore b/.gitignore index 34bd2a2..92d4eea 100644 --- a/.gitignore +++ b/.gitignore @@ -21,9 +21,12 @@ /spec/dummy_engine/tmp/ /spec/dummy_isolated/log/ /spec/dummy_isolated/tmp/ +/spec/dummy_root/log/ +/spec/dummy_root/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_root/config/database.yml diff --git a/Rakefile b/Rakefile index 64ca094..dfdeb5e 100644 --- a/Rakefile +++ b/Rakefile @@ -7,7 +7,7 @@ begin # Default spec suite — boots spec/dummy/ (main-app OIDC-only setup). RSpec::Core::RakeTask.new(:spec) do |t| - t.exclude_pattern = "spec/{engine,isolated,dummy_engine,dummy_isolated}/**/*" + t.exclude_pattern = "spec/{engine,isolated,root,dummy_engine,dummy_isolated,dummy_root}/**/*" end namespace :spec do @@ -24,8 +24,13 @@ begin sh "bundle exec rspec --options /dev/null --require spec_helper -I spec/isolated spec/isolated" end - desc "Run every spec suite (default + engine + isolated)" - task all: [:spec, :engine, :isolated] + desc "Run root-mounted-ActiveAdmin specs (boots spec/dummy_root/)" + task :root do + sh "bundle exec rspec --options /dev/null --require spec_helper -I spec/root spec/root" + end + + desc "Run every spec suite (default + engine + isolated + root)" + task all: [:spec, :engine, :isolated, :root] 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 271b0cd..a5d98ef 100644 --- a/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb +++ b/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'devise' +require 'active_admin/devise' module ActiveAdmin module Oidc @@ -17,6 +18,9 @@ module Devise # The action name matches the provider name registered with Devise # (`:oidc`, from ActiveAdmin::Oidc::Engine::PROVIDER_NAME). class OmniauthCallbacksController < ::Devise::OmniauthCallbacksController + # For `#root_path` — ActiveAdmin's namespace-aware landing path. + include ::ActiveAdmin::Devise::Controller + def oidc auth = request.env['omniauth.auth'] || {} info = auth['info'] || {} @@ -105,10 +109,16 @@ def provision_and_sign_in(claims, kind: 'OIDC') # 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. + # + # `root_path` comes from ActiveAdmin::Devise::Controller and + # follows the host's `config.default_namespace`: `/admin` by + # default, but `/` for hosts that set `default_namespace = false` + # (or `/foo` for a custom namespace). Hardcoding `/admin` 404s on + # those hosts whenever Devise has no stored location — i.e. every + # sign-in that did not start from a protected page. def after_sign_in_path_for(resource) - stored_location_for(resource) || '/admin' + stored_location_for(resource) || root_path end # Devise's `new_session_path(scope)` is only generated when diff --git a/gemfiles/activeadmin_3.5.gemfile b/gemfiles/activeadmin_3.5.gemfile index a81b26f..2fdf8c4 100644 --- a/gemfiles/activeadmin_3.5.gemfile +++ b/gemfiles/activeadmin_3.5.gemfile @@ -14,3 +14,11 @@ gem "omniauth_openid_connect", "~> 0.6.0" gem "sprockets-rails", ">= 3.4" gem "sassc-rails", ">= 2.1" + +# json 3.0 dropped the `quirks_mode` keyword that ActiveSupport::JSON +# still passes to JSON.parse/JSON.generate (activesupport 7.2 and 8.0, +# lib/active_support/json/{decoding,encoding}.rb). CI has no lockfile, so +# it picks up json 3.x and every request spec dies with +# `ArgumentError: unknown keyword: quirks_mode`. Drop this once Rails +# ships a version that no longer passes it. +gem "json", "< 3" diff --git a/gemfiles/activeadmin_4.0.gemfile b/gemfiles/activeadmin_4.0.gemfile index dc65df7..538c275 100644 --- a/gemfiles/activeadmin_4.0.gemfile +++ b/gemfiles/activeadmin_4.0.gemfile @@ -17,3 +17,7 @@ gem "propshaft" gem "importmap-rails" gem "cssbundling-rails" gem "tailwindcss-rails", "~> 4.0" + +# See the note in activeadmin_3.5.gemfile: json 3.0 removed the +# `quirks_mode` keyword ActiveSupport::JSON still passes. +gem "json", "< 3" diff --git a/spec/dummy_root/Rakefile b/spec/dummy_root/Rakefile new file mode 100644 index 0000000..91e7ad7 --- /dev/null +++ b/spec/dummy_root/Rakefile @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +require_relative "config/application" +Rails.application.load_tasks diff --git a/spec/dummy_root/app/admin/dashboard.rb b/spec/dummy_root/app/admin/dashboard.rb new file mode 100644 index 0000000..06b6306 --- /dev/null +++ b/spec/dummy_root/app/admin/dashboard.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +ActiveAdmin.register_page "Dashboard" do + menu priority: 1, label: proc { I18n.t("active_admin.dashboard") } + + content title: proc { I18n.t("active_admin.dashboard") } do + para "Dummy dashboard." + end +end diff --git a/spec/dummy_root/app/assets/config/manifest.js b/spec/dummy_root/app/assets/config/manifest.js new file mode 100644 index 0000000..aebeee2 --- /dev/null +++ b/spec/dummy_root/app/assets/config/manifest.js @@ -0,0 +1,3 @@ +//= link active_admin.css +//= link active_admin.js + diff --git a/spec/dummy_root/app/assets/javascripts/active_admin.js b/spec/dummy_root/app/assets/javascripts/active_admin.js new file mode 100644 index 0000000..a7f3bdd --- /dev/null +++ b/spec/dummy_root/app/assets/javascripts/active_admin.js @@ -0,0 +1 @@ +// stub for specs — ActiveAdmin layout references this file diff --git a/spec/dummy_root/app/assets/stylesheets/active_admin.css b/spec/dummy_root/app/assets/stylesheets/active_admin.css new file mode 100644 index 0000000..7ba4ac9 --- /dev/null +++ b/spec/dummy_root/app/assets/stylesheets/active_admin.css @@ -0,0 +1 @@ +/* stub for specs — ActiveAdmin layout references this file */ diff --git a/spec/dummy_root/app/controllers/application_controller.rb b/spec/dummy_root/app/controllers/application_controller.rb new file mode 100644 index 0000000..7944f9f --- /dev/null +++ b/spec/dummy_root/app/controllers/application_controller.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +class ApplicationController < ActionController::Base +end diff --git a/spec/dummy_root/app/models/admin_user.rb b/spec/dummy_root/app/models/admin_user.rb new file mode 100644 index 0000000..09e66eb --- /dev/null +++ b/spec/dummy_root/app/models/admin_user.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class AdminUser < ApplicationRecord + devise :omniauthable, + omniauth_providers: [:oidc] + + validates :email, presence: true + + serialize :oidc_raw_info, coder: JSON + + def active_for_authentication? + super && enabled? + end +end diff --git a/spec/dummy_root/app/models/application_record.rb b/spec/dummy_root/app/models/application_record.rb new file mode 100644 index 0000000..71fbba5 --- /dev/null +++ b/spec/dummy_root/app/models/application_record.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class ApplicationRecord < ActiveRecord::Base + self.abstract_class = true +end diff --git a/spec/dummy_root/config/application.rb b/spec/dummy_root/config/application.rb new file mode 100644 index 0000000..703d870 --- /dev/null +++ b/spec/dummy_root/config/application.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require_relative "boot" + +require "rails" +require "active_record/railtie" +require "action_controller/railtie" +require "action_view/railtie" +require "action_mailer/railtie" +begin + require "sprockets/railtie" +rescue LoadError + # sprockets-rails is optional; dummy app uses ActiveAdmin's cssbundling/importmap defaults +end + +Bundler.require(*Rails.groups) + +require "devise" +require "activeadmin" + +# Load the gem under test. +require "activeadmin-oidc" + +# Host app that mounts ActiveAdmin at `/` instead of `/admin` +# (`config.default_namespace = false`). Identical to spec/dummy in every +# other respect — see config/initializers/active_admin.rb for the one +# line that differs. +module DummyRoot + class Application < Rails::Application + rails_gem_version = Gem::Version.new(Rails.version) + config.load_defaults(rails_gem_version >= Gem::Version.new("8.0") ? 8.0 : 7.2) + config.eager_load = false + config.root = File.expand_path("..", __dir__) + + config.secret_key_base = "test-secret-key-base-#{"x" * 64}" + config.hosts.clear + + config.action_controller.allow_forgery_protection = false + config.session_store :cookie_store, key: "_dummy_root_session" + + config.action_dispatch.show_exceptions = :none + config.consider_all_requests_local = true + config.active_support.to_time_preserves_timezone = :zone if config.active_support.respond_to?(:to_time_preserves_timezone=) + + config.logger = Logger.new($stdout) + config.log_level = ENV.fetch("DUMMY_LOG_LEVEL", "fatal").to_sym + end +end diff --git a/spec/dummy_root/config/boot.rb b/spec/dummy_root/config/boot.rb new file mode 100644 index 0000000..7865da2 --- /dev/null +++ b/spec/dummy_root/config/boot.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../../../Gemfile", __dir__) + +require "bundler/setup" diff --git a/spec/dummy_root/config/database.yml b/spec/dummy_root/config/database.yml new file mode 100644 index 0000000..57ed80b --- /dev/null +++ b/spec/dummy_root/config/database.yml @@ -0,0 +1,11 @@ +test: + adapter: sqlite3 + database: ":memory:" + pool: 5 + timeout: 5000 + +development: + adapter: sqlite3 + database: db/development.sqlite3 + pool: 5 + timeout: 5000 diff --git a/spec/dummy_root/config/environment.rb b/spec/dummy_root/config/environment.rb new file mode 100644 index 0000000..b3a30d6 --- /dev/null +++ b/spec/dummy_root/config/environment.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +require_relative "application" +DummyRoot::Application.initialize! diff --git a/spec/dummy_root/config/environments/test.rb b/spec/dummy_root/config/environments/test.rb new file mode 100644 index 0000000..ca39b0e --- /dev/null +++ b/spec/dummy_root/config/environments/test.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +Rails.application.configure do + config.cache_classes = true + config.eager_load = false + config.public_file_server.enabled = true + config.consider_all_requests_local = true + config.action_controller.perform_caching = false + config.action_dispatch.show_exceptions = :none + config.action_controller.allow_forgery_protection = false + config.active_support.deprecation = :stderr + config.active_support.disallowed_deprecation = :raise + config.action_mailer.delivery_method = :test + config.action_mailer.default_url_options = { host: "www.example.com" } + config.i18n.raise_on_missing_translations = false + config.log_level = :fatal +end diff --git a/spec/dummy_root/config/initializers/active_admin.rb b/spec/dummy_root/config/initializers/active_admin.rb new file mode 100644 index 0000000..71b4a80 --- /dev/null +++ b/spec/dummy_root/config/initializers/active_admin.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +ActiveAdmin.setup do |config| + config.site_title = "Dummy Root" + config.authentication_method = :authenticate_admin_user! + config.current_user_method = :current_admin_user + config.logout_link_path = :destroy_admin_user_session_path + + config.root_to = "dashboard#index" + config.comments = false + + # The whole point of this dummy app: ActiveAdmin mounted at `/`, not + # `/admin`. Real hosts do this when the admin panel *is* the app. + config.default_namespace = false +end diff --git a/spec/dummy_root/config/initializers/activeadmin_oidc.rb b/spec/dummy_root/config/initializers/activeadmin_oidc.rb new file mode 100644 index 0000000..d764381 --- /dev/null +++ b/spec/dummy_root/config/initializers/activeadmin_oidc.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +ActiveAdmin::Oidc.configure do |c| + c.issuer = "https://idp.example.com" + c.client_id = "client-abc" + # client_secret intentionally blank so PKCE auto-enables in specs. + c.on_login = ->(admin_user, claims) { + # Default dummy on_login: accept, set department if provided. + admin_user.department = claims["department"] if claims.key?("department") + true + } +end diff --git a/spec/dummy_root/config/initializers/devise.rb b/spec/dummy_root/config/initializers/devise.rb new file mode 100644 index 0000000..6948f9f --- /dev/null +++ b/spec/dummy_root/config/initializers/devise.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +Devise.setup do |config| + config.mailer_sender = 'please-change-me@example.com' + require 'devise/orm/active_record' + + config.case_insensitive_keys = [:email] + config.strip_whitespace_keys = [:email] + config.skip_session_storage = [:http_auth] + config.stretches = Rails.env.test? ? 1 : 11 + config.reconfirmable = true + config.expire_all_remember_me_on_sign_out = true + config.password_length = 6..128 + config.email_regexp = /\A[^@\s]+@[^@\s]+\z/ + config.reset_password_within = 6.hours + config.sign_out_via = :delete + + # OmniAuth strategy registration and path prefix are handled automatically + # by the gem's engine (see lib/activeadmin/oidc/engine.rb) based on the + # ActiveAdmin::Oidc configuration in config/initializers/activeadmin_oidc.rb. +end diff --git a/spec/dummy_root/config/routes.rb b/spec/dummy_root/config/routes.rb new file mode 100644 index 0000000..a2e66cc --- /dev/null +++ b/spec/dummy_root/config/routes.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +Rails.application.routes.draw do + devise_for :admin_users, ActiveAdmin::Devise.config + ActiveAdmin.routes(self) + # No host-defined `root` route: ActiveAdmin's own `root_to` supplies + # `/`. Anything the gem redirects to outside the ActiveAdmin route + # table therefore 404s, which is what these specs are here to catch. +end diff --git a/spec/dummy_root/db/schema.rb b/spec/dummy_root/db/schema.rb new file mode 100644 index 0000000..0cab485 --- /dev/null +++ b/spec/dummy_root/db/schema.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +ActiveRecord::Schema[7.2].define(version: 0) do + create_table :admin_users, force: :cascade do |t| + t.string :email + t.string :username + t.string :provider + t.string :uid + t.text :oidc_raw_info + t.string :department + t.boolean :enabled, default: true + t.timestamps + end + + add_index :admin_users, :email, unique: true + add_index :admin_users, %i[provider uid], unique: true, + where: "provider IS NOT NULL AND uid IS NOT NULL", + name: "index_admin_users_on_provider_and_uid" +end diff --git a/spec/root/requests/root_mounted_redirect_spec.rb b/spec/root/requests/root_mounted_redirect_spec.rb new file mode 100644 index 0000000..92c56fa --- /dev/null +++ b/spec/root/requests/root_mounted_redirect_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require "root_rails_helper" + +# Regression suite for hosts that mount ActiveAdmin at `/` via +# `config.default_namespace = false` (see spec/dummy_root/). +# +# After a successful SSO round-trip the gem has to land the user on the +# ActiveAdmin namespace root. When Devise has a stored location that is +# used, and the bug is invisible. When it does not — every sign-in that +# did not start from a protected page: a bookmarked login page, a session +# cookie rotated during the IdP round-trip — the fallback fires, and a +# fallback that assumes `/admin` sends these hosts to a path that does +# not exist. +RSpec.describe "SSO sign-in on a host mounted at /", type: :request do + before do + OmniAuth.config.test_mode = true + OmniAuth.config.mock_auth[:oidc] = OmniAuth::AuthHash.new( + provider: "oidc", + uid: "sub-root", + info: { "email" => "root@example.com" }, + extra: { "raw_info" => { "sub" => "sub-root", "email" => "root@example.com" } } + ) + + ActiveAdmin::Oidc.configure do |c| + c.issuer = "https://idp.example.com" + c.client_id = "client-abc" + c.on_login = ->(_admin_user, _claims) { true } + end + + AdminUser.delete_all + end + + after { OmniAuth.config.mock_auth[:oidc] = nil } + + # POST the OmniAuth request phase and follow it into the callback, so + # `response` ends up on whatever the gem's controller redirected to. + def sign_in_via_sso + post "/admin/auth/oidc" + follow_redirect! + end + + describe "the dummy host itself" do + it "mounts ActiveAdmin at / and has no /admin" do + expect(ActiveAdmin.application.default_namespace).to be(false) + expect(Rails.application.routes.recognize_path("/")).to include(action: "index") + expect { + Rails.application.routes.recognize_path("/admin") + }.to raise_error(ActionController::RoutingError) + end + end + + context "with no stored location (sign-in did not start from a protected page)" do + it "redirects to /" do + sign_in_via_sso + + expect(response).to be_redirect + expect(URI(response.location).path).to eq("/") + end + + it "lands on a page that actually exists" do + sign_in_via_sso + follow_redirect! + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Dashboard") + end + end + + context "with a stored location (bounced off a protected page)" do + it "returns to where the user was headed" do + get "/" # unauthenticated → Devise stores admin_user_return_to + expect(response).to be_redirect + + sign_in_via_sso + + expect(URI(response.location).path).to eq("/") + end + end +end diff --git a/spec/root/root_rails_helper.rb b/spec/root/root_rails_helper.rb new file mode 100644 index 0000000..f3ddb0d --- /dev/null +++ b/spec/root/root_rails_helper.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +ENV["RAILS_ENV"] ||= "test" + +require "spec_helper" +require_relative "../dummy_root/config/environment" + +abort("Rails not in test mode") if Rails.env.production? + +require "rspec/rails" +require "webmock/rspec" + +ActiveRecord::Schema.verbose = false +load File.expand_path("../dummy_root/db/schema.rb", __dir__) + +RSpec.configure do |config| + config.use_transactional_fixtures = true + config.infer_spec_type_from_file_location! + config.filter_rails_from_backtrace! +end + +OmniAuth.config.test_mode = true +OmniAuth.config.logger = Logger.new(File::NULL) +OmniAuth.config.request_validation_phase = ->(_env) { } +OmniAuth.config.allowed_request_methods = %i[get post] +OmniAuth.config.silence_get_warning = true if OmniAuth.config.respond_to?(:silence_get_warning=)