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 5c49ecf..c4ed8b3 100644
--- a/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb
+++ b/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb
@@ -38,18 +38,15 @@ def oidc
provider: ActiveAdmin::Oidc::Engine::PROVIDER_NAME.to_s
).call
- # Devise checks active_for_authentication? on session
- # deserialization but NOT on initial OmniAuth sign-in.
- # Guard here so disabled/locked users are rejected immediately.
- unless admin_user.active_for_authentication?
- message = admin_user.inactive_message
- flash[:alert] = I18n.t("devise.failure.#{message}", default: message.to_s)
- redirect_to after_omniauth_failure_path_for(resource_name)
- return
- end
-
sign_in_and_redirect admin_user, event: :authentication
set_flash_message(:notice, :success, kind: 'OIDC') if is_navigational_format?
+ rescue ActiveAdmin::Oidc::InactiveError => e
+ Rails.logger.warn("[activeadmin-oidc] inactive: #{e.inactive_message_key}")
+ flash[:alert] = I18n.t(
+ "devise.failure.#{e.inactive_message_key}",
+ default: e.inactive_message_key.to_s
+ )
+ redirect_to after_omniauth_failure_path_for(resource_name)
rescue ActiveAdmin::Oidc::ProvisioningError => e
Rails.logger.warn("[activeadmin-oidc] denial: #{e.message}")
flash[:alert] = ActiveAdmin::Oidc.config.access_denied_message
diff --git a/app/views/active_admin/devise/sessions/new.html.erb b/app/views/active_admin/devise/sessions/new.html.erb
index efe7aa0..700d875 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,
- "/admin/auth/oidc",
+ "#{OmniAuth.config.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 "/admin/auth/oidc",
+ <%= form_tag "#{OmniAuth.config.path_prefix}/oidc",
method: :post,
class: "activeadmin-oidc-login-form formtastic",
data: { turbo: false } do %>
diff --git a/lib/activeadmin-oidc.rb b/lib/activeadmin-oidc.rb
index dc6544b..7921ea9 100644
--- a/lib/activeadmin-oidc.rb
+++ b/lib/activeadmin-oidc.rb
@@ -28,6 +28,19 @@ class ConfigurationError < Error; end
class ProvisioningError < Error; end
class RetryProvisioning < Error; end
+ # Raised when active_for_authentication? rejects the user. Carries
+ # the model's inactive_message symbol so the controller can
+ # translate it via I18n (devise.failure.) instead of
+ # falling back to the generic denial flash.
+ class InactiveError < ProvisioningError
+ attr_reader :inactive_message_key
+
+ def initialize(inactive_message_key)
+ @inactive_message_key = inactive_message_key
+ super(inactive_message_key.to_s)
+ end
+ end
+
class << self
def config
@config ||= Configuration.new
diff --git a/lib/activeadmin/oidc/user_provisioner.rb b/lib/activeadmin/oidc/user_provisioner.rb
index 2f7490d..33509ac 100644
--- a/lib/activeadmin/oidc/user_provisioner.rb
+++ b/lib/activeadmin/oidc/user_provisioner.rb
@@ -35,11 +35,31 @@ def call
validate_claims!
admin_user = find_or_adopt_or_build
+
+ # Retry path: a concurrent first sign-in inserted between our
+ # initial miss-and-build and our failed save. Return the
+ # winner's row verbatim — on_login already ran on our (now
+ # discarded) in-memory build, and re-firing it would double
+ # any host-side side effects (audit log, webhook, email).
+ return admin_user if @retried
+
assign_base_attributes(admin_user)
allowed = invoke_on_login(admin_user)
raise ProvisioningError, denial_message unless allowed
+ # Devise's `active_for_authentication?` guard runs in the
+ # controller post-sign-in, but by then we've already saved
+ # the record. Hostile attempts where on_login flips an
+ # inactivity flag (e.g. enabled=false) would otherwise leave
+ # provisional rows in the DB on every try. Refuse before
+ # persisting. Raise the dedicated InactiveError so the
+ # controller can surface the model's I18n inactive_message
+ # instead of the generic denial flash.
+ unless admin_user.active_for_authentication?
+ raise InactiveError, admin_user.inactive_message
+ end
+
save!(admin_user)
admin_user
rescue RetryProvisioning
@@ -84,6 +104,19 @@ def find_or_adopt_or_build
"Identity #{identity_value.inspect} is already linked to a different account (takeover guard)"
end
+ # Adoption of a pre-existing row (provider/uid still nil) by
+ # an IdP-supplied identity value is a privilege-escalation
+ # vector when the IdP allows external / unverified emails:
+ # an attacker registers `ceo@example.com` at the IdP and
+ # adopts the seeded admin row. Refuse when the IdP explicitly
+ # marks the claim as unverified. (Absent claim → unchanged
+ # behaviour, since many IdPs don't ship `email_verified` at
+ # all.)
+ if @claims["email_verified"] == false
+ raise ProvisioningError,
+ "Identity #{identity_value.inspect} is not verified by the IdP — refusing adoption"
+ end
+
identity_match.provider = @provider
identity_match.uid = uid
return identity_match
diff --git a/spec/requests/disabled_user_persistence_spec.rb b/spec/requests/disabled_user_persistence_spec.rb
new file mode 100644
index 0000000..3ff6e1b
--- /dev/null
+++ b/spec/requests/disabled_user_persistence_spec.rb
@@ -0,0 +1,103 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+# Failing spec for HIGH #2 — disabled user persistence.
+#
+# UserProvisioner#save! runs BEFORE OmniauthCallbacksController#oidc
+# checks `active_for_authentication?`. If the on_login hook flips a
+# user's `enabled` flag (or any other Devise inactivity guard) and
+# returns truthy, the gem still persists the record — only then does
+# the controller reject the sign-in. Repeated hostile attempts leave
+# a growing pile of provisional AdminUser rows.
+#
+# Acceptance criterion: a user the host hook marks inactive must NOT
+# be persisted to the database.
+RSpec.describe "OIDC callback: disabled user not persisted", type: :request do
+ before do
+ OmniAuth.config.test_mode = true
+ OmniAuth.config.mock_auth[:oidc] = nil
+
+ AdminUser.delete_all
+ end
+
+ after { OmniAuth.config.mock_auth[:oidc] = nil }
+
+ def build_auth_hash(uid:, email:)
+ OmniAuth::AuthHash.new(
+ provider: "oidc",
+ uid: uid,
+ info: { "email" => email, "name" => "Mallory" },
+ extra: { "raw_info" => { "sub" => uid, "email" => email } }
+ )
+ end
+
+ it "does NOT persist a row when on_login sets enabled=false and returns truthy" do
+ ActiveAdmin::Oidc.configure do |c|
+ c.issuer = "https://idp.example.com"
+ c.client_id = "client-abc"
+ c.on_login = lambda do |admin_user, _claims|
+ admin_user.enabled = false
+ true # truthy → current code persists the row
+ end
+ end
+
+ OmniAuth.config.mock_auth[:oidc] =
+ build_auth_hash(uid: "mallory-sub", email: "mallory@example.com")
+
+ expect {
+ post "/admin/auth/oidc"
+ follow_redirect! if response.redirect?
+ }.not_to change(AdminUser, :count),
+ "disabled-by-hook user was persisted to AdminUser — repeated attempts grow the table"
+ end
+
+ it "still redirects the disabled user to the login page" do
+ ActiveAdmin::Oidc.configure do |c|
+ c.issuer = "https://idp.example.com"
+ c.client_id = "client-abc"
+ c.on_login = lambda do |admin_user, _claims|
+ admin_user.enabled = false
+ true
+ end
+ end
+
+ OmniAuth.config.mock_auth[:oidc] =
+ build_auth_hash(uid: "mallory-sub", email: "mallory@example.com")
+
+ post "/admin/auth/oidc"
+ follow_redirect! if response.redirect?
+
+ expect(response).to redirect_to("/admin/login")
+ end
+
+ # The HIGH #2 fix raised ProvisioningError when the hook flipped
+ # the inactivity flag, but the controller's generic rescue replaced
+ # the model's I18n-translated inactive_message with the generic
+ # access_denied_message. The disabled user lost the specific reason
+ # ("Your account has not been activated yet") and saw the catch-all
+ # denial flash instead. Surface the original reason via a dedicated
+ # error class.
+ it "shows the model's I18n-translated inactive_message in the flash" do
+ ActiveAdmin::Oidc.configure do |c|
+ c.issuer = "https://idp.example.com"
+ c.client_id = "client-abc"
+ c.on_login = lambda do |admin_user, _claims|
+ admin_user.enabled = false
+ true
+ end
+ end
+
+ OmniAuth.config.mock_auth[:oidc] =
+ build_auth_hash(uid: "mallory-sub", email: "mallory@example.com")
+
+ expected = I18n.t("devise.failure.inactive")
+
+ post "/admin/auth/oidc"
+ follow_redirect! if response.redirect? # OmniAuth → /callback → our controller
+
+ expect(flash[:alert]).to eq(expected),
+ "expected the disabled user to see Devise's translated inactive " \
+ "message, but the controller used the generic denial flash"
+ end
+end
diff --git a/spec/requests/login_path_helper_spec.rb b/spec/requests/login_path_helper_spec.rb
new file mode 100644
index 0000000..09e5d35
--- /dev/null
+++ b/spec/requests/login_path_helper_spec.rb
@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+# Regression spec for MEDIUM #4 — login view must derive the OmniAuth
+# callback path from `OmniAuth.config.path_prefix`, not hardcode it.
+#
+# `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.
+RSpec.describe "Login view OmniAuth path", type: :request do
+ before do
+ ActiveAdmin::Oidc.configure do |c|
+ c.issuer = "https://idp.example.com"
+ 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.
+ sentinel = "/sentinel-omniauth-prefix"
+ allow(OmniAuth.config).to receive(:path_prefix).and_return(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"
+ end
+end
diff --git a/spec/unit/user_provisioner_high_risk_spec.rb b/spec/unit/user_provisioner_high_risk_spec.rb
new file mode 100644
index 0000000..752b3c7
--- /dev/null
+++ b/spec/unit/user_provisioner_high_risk_spec.rb
@@ -0,0 +1,106 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+require_relative "_active_record_setup"
+
+# Failing specs for the HIGH-priority production risks the code review
+# surfaced. Each example here is RED today; flipping it green is the
+# acceptance criterion for the corresponding fix.
+RSpec.describe "UserProvisioner — HIGH-risk regressions" do
+ let(:config) do
+ ActiveAdmin::Oidc::Configuration.new.tap do |c|
+ c.issuer = "https://idp.example.com"
+ c.client_id = "client-abc"
+ c.on_login = ->(_user, _claims) { true }
+ end
+ end
+
+ let(:provider) { "oidc" }
+ let(:uid) { "sub-123" }
+ let(:email) { "alice@example.com" }
+
+ let(:claims) do
+ { "sub" => uid, "email" => email, "name" => "Alice" }
+ end
+
+ before { AdminUser.delete_all }
+
+ subject(:provisioner) { ActiveAdmin::Oidc::UserProvisioner.new(config, claims: claims, provider: provider) }
+
+ # -------------------------------------------------------------------
+ # HIGH #1 — concurrent JIT provisioning re-runs on_login
+ # -------------------------------------------------------------------
+ # Two simultaneous first sign-ins for the same (provider, uid):
+ # the losing thread hits RecordNotUnique, the provisioner retries,
+ # finds the now-persisted row, and runs `on_login` AGAIN. Any
+ # non-idempotent side effects in the host's hook (audit log row,
+ # webhook, email) double-fire.
+ describe "concurrent first sign-in race" do
+ it "does NOT invoke on_login twice when RecordNotUnique triggers a retry" do
+ call_count = 0
+ config.on_login = lambda do |_user, _claims|
+ call_count += 1
+ true
+ end
+
+ # Simulate the race: the first save! raises RecordNotUnique (as
+ # if the other thread won the insert), and that other thread's
+ # row is now visible to find_by on retry.
+ raise_once = true
+ allow_any_instance_of(AdminUser).to receive(:save!).and_wrap_original do |original, *args|
+ if raise_once
+ raise_once = false
+ # The "other thread" inserted concurrently.
+ AdminUser.create!(provider: provider, uid: uid, email: email)
+ raise ActiveRecord::RecordNotUnique, "duplicate (provider, uid)"
+ else
+ original.call(*args)
+ end
+ end
+
+ provisioner.call
+
+ expect(call_count).to eq(1),
+ "on_login was invoked #{call_count}x — host hook side effects would double-fire"
+ end
+ end
+
+ # -------------------------------------------------------------------
+ # HIGH #3 — identity-claim adoption without email_verified
+ # -------------------------------------------------------------------
+ # A pre-existing AdminUser row with the configured identity_attribute
+ # set (e.g. seeded "ceo@example.com") and no (provider, uid) yet is
+ # adopted by anyone the IdP says owns that email. When the IdP allows
+ # unverified emails (guest/external tenants), an attacker who
+ # registers `ceo@example.com` at the IdP takes over the row.
+ describe "identity adoption guarded by email_verified" do
+ let!(:legacy_admin) do
+ AdminUser.create!(email: "ceo@example.com", provider: nil, uid: nil)
+ end
+
+ let(:unverified_claims) do
+ {
+ "sub" => "attacker-sub",
+ "email" => "ceo@example.com",
+ "email_verified" => false
+ }
+ end
+
+ subject(:attacker_provisioner) do
+ ActiveAdmin::Oidc::UserProvisioner.new(config, claims: unverified_claims, provider: provider)
+ end
+
+ it "refuses to adopt the legacy row when the email claim is not verified" do
+ expect { attacker_provisioner.call }
+ .to raise_error(ActiveAdmin::Oidc::ProvisioningError, /verified|adoption/i)
+ end
+
+ it "does NOT link the legacy row to the attacker's (provider, uid)" do
+ attacker_provisioner.call rescue nil
+
+ legacy_admin.reload
+ expect(legacy_admin.provider).to be_nil
+ expect(legacy_admin.uid).to be_nil
+ end
+ end
+end