From 9a6893b81f4faeb0fe979f5a728239be00e234b2 Mon Sep 17 00:00:00 2001 From: Igor Fedoronchuk Date: Tue, 2 Jun 2026 16:28:13 +0200 Subject: [PATCH 1/3] Add failing tests for three security risks in UserProvisioner Each spec is RED today and documents one production bug. The next commit implements the fixes and turns them green. 1. on_login runs twice when two first sign-ins race. The loser hits RecordNotUnique, the provisioner retries, and the host's hook double-fires (audit log, webhook, welcome email all duplicated). 2. A disabled-by-hook user still gets a row in admin_users. The active_for_authentication? check runs in the controller AFTER the provisioner has already persisted. Hostile attempts grow the table. 3. A pre-seeded admin row (provider/uid still nil) gets adopted by anyone the IdP says owns that email - even when the IdP marks the email as unverified. Attacker registers ceo@example.com at a lax IdP, signs in, walks off with the CEO account. --- .../disabled_user_persistence_spec.rb | 73 ++++++++++++ spec/unit/user_provisioner_high_risk_spec.rb | 106 ++++++++++++++++++ 2 files changed, 179 insertions(+) create mode 100644 spec/requests/disabled_user_persistence_spec.rb create mode 100644 spec/unit/user_provisioner_high_risk_spec.rb diff --git a/spec/requests/disabled_user_persistence_spec.rb b/spec/requests/disabled_user_persistence_spec.rb new file mode 100644 index 0000000..2ede43b --- /dev/null +++ b/spec/requests/disabled_user_persistence_spec.rb @@ -0,0 +1,73 @@ +# 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 +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 From b40e24b3eea2fbf27aa5dd1a27f693d7082d38f3 Mon Sep 17 00:00:00 2001 From: Igor Fedoronchuk Date: Tue, 2 Jun 2026 16:28:50 +0200 Subject: [PATCH 2/3] Harden UserProvisioner against the three security risks Fix the three bugs covered by the failing specs in the previous commit. 1. Don't re-run on_login on the RecordNotUnique retry path. The flag set by save!'s rescue is now read in #call to short-circuit after the row is found: return the winner's record without re-invoking the host hook. Side effects fire exactly once. 2. Check active_for_authentication? inside the provisioner, between on_login and save!. If the hook flipped an inactivity flag, raise ProvisioningError BEFORE writing the row. 3. Refuse to adopt a legacy admin row when email_verified is explicitly false. IdPs that don't ship the claim keep the old behaviour - many never emit it, so a strict "must be true" check would break legitimate hosts. --- lib/activeadmin/oidc/user_provisioner.rb | 31 ++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lib/activeadmin/oidc/user_provisioner.rb b/lib/activeadmin/oidc/user_provisioner.rb index 2f7490d..a99a87d 100644 --- a/lib/activeadmin/oidc/user_provisioner.rb +++ b/lib/activeadmin/oidc/user_provisioner.rb @@ -35,11 +35,29 @@ 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. + unless admin_user.active_for_authentication? + raise ProvisioningError, admin_user.inactive_message.to_s + end + save!(admin_user) admin_user rescue RetryProvisioning @@ -84,6 +102,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 From 4e8ff0bd706b319888c780d07162b1578f02c77f Mon Sep 17 00:00:00 2001 From: Igor Fedoronchuk Date: Tue, 2 Jun 2026 16:36:55 +0200 Subject: [PATCH 3/3] Preserve the model's inactive_message in the disabled-user flash The previous commit moved the active_for_authentication? guard into the provisioner, but the controller's generic ProvisioningError rescue replaced the model's I18n-translated reason ("Your account is not activated yet.") with the catch-all access_denied_message. Disabled users lost the specific feedback. Add InactiveError (a ProvisioningError subclass that carries the inactive_message symbol) and rescue it separately in the callbacks controller, translating via I18n.t("devise.failure."). The generic rescue stays in place for every other denial path. Also drop the now-dead active_for_authentication? check in the controller - the provisioner runs the same guard earlier and never returns an inactive user. --- .../devise/omniauth_callbacks_controller.rb | 17 +++++------ lib/activeadmin-oidc.rb | 13 ++++++++ lib/activeadmin/oidc/user_provisioner.rb | 6 ++-- .../disabled_user_persistence_spec.rb | 30 +++++++++++++++++++ 4 files changed, 54 insertions(+), 12 deletions(-) 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/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 a99a87d..33509ac 100644 --- a/lib/activeadmin/oidc/user_provisioner.rb +++ b/lib/activeadmin/oidc/user_provisioner.rb @@ -53,9 +53,11 @@ def call # 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. + # 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 ProvisioningError, admin_user.inactive_message.to_s + raise InactiveError, admin_user.inactive_message end save!(admin_user) diff --git a/spec/requests/disabled_user_persistence_spec.rb b/spec/requests/disabled_user_persistence_spec.rb index 2ede43b..3ff6e1b 100644 --- a/spec/requests/disabled_user_persistence_spec.rb +++ b/spec/requests/disabled_user_persistence_spec.rb @@ -70,4 +70,34 @@ def build_auth_hash(uid:, email:) 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