Restore orphaned PR #10 merge: source SSO login path from OmniAuth.config.path_prefix

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

app/views/active_admin/devise/sessions/new.html.erb

@@ -5,7 +5,7 @@
     </h2>
 
     <%= 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 @@
   <div id="login">
     <h2><%= active_admin_application.site_title(self) %></h2>
 
-    <%= form_tag "/admin/auth/oidc",
+    <%= form_tag "#{OmniAuth.config.path_prefix}/oidc",
                  method: :post,
                  class: "activeadmin-oidc-login-form formtastic",
                  data: { turbo: false } do %>

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.<symbol>) 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

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

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

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 `<sentinel>/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

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