From c0ccfec20773859b74f2ec53bb8fc57789f71640 Mon Sep 17 00:00:00 2001
From: Igor Fedoronchuk <fedoronchuk@gmail.com>
Date: Tue, 2 Jun 2026 16:55:50 +0200
Subject: [PATCH 1/2] Code-review followups: generator templates,
 inactive_message hardening

Follow-ups to PRs #9 and #10 surfaced by the code review.

- Apply the same OmniAuth.config.path_prefix substitution to both
  install-generator templates. New installs were still shipping the
  hardcoded path PR #10 fixed in the live view.
- Default the InactiveError key to :inactive when the model's
  inactive_message returns nil/blank. The empty key used to render
  an empty flash; now the user always sees a reason.
- Stop leaking custom inactive_message symbols. If the host returns
  a symbol like :locked_by_admin with no devise.failure translation,
  the raw symbol used to land in a public flash. Fall back to
  devise.failure.inactive instead.

The disabled-user request spec is consolidated via let helpers
(auth_hash, disabling_hook, post_callback) and now uses
new_admin_user_session_path / OmniAuth.config.path_prefix instead
of hardcoded URLs. Adds a regression spec for the retry leg + an
inactive winner row, documenting the expected redirect-and-no-
warden-session behaviour.
---
 .../devise/omniauth_callbacks_controller.rb   |   6 +-
 lib/activeadmin-oidc.rb                       |   8 +-
 .../install/templates/sessions_new.html.erb   |   2 +-
 .../templates/sessions_new_v4.html.erb        |   2 +-
 .../disabled_user_persistence_spec.rb         | 146 ++++++++++++------
 5 files changed, 108 insertions(+), 56 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 c4ed8b3..a43bf23 100644
--- a/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb
+++ b/app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb
@@ -42,9 +42,13 @@ def oidc
           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}")
+          # Fall back to the standard `inactive` translation rather
+          # than the raw symbol — custom keys like :locked_by_admin
+          # would otherwise leak host-internal state into a flash
+          # visible to unauthenticated visitors.
           flash[:alert] = I18n.t(
             "devise.failure.#{e.inactive_message_key}",
-            default: e.inactive_message_key.to_s
+            default: I18n.t("devise.failure.inactive")
           )
           redirect_to after_omniauth_failure_path_for(resource_name)
         rescue ActiveAdmin::Oidc::ProvisioningError => e
diff --git a/lib/activeadmin-oidc.rb b/lib/activeadmin-oidc.rb
index 7921ea9..7086095 100644
--- a/lib/activeadmin-oidc.rb
+++ b/lib/activeadmin-oidc.rb
@@ -35,9 +35,13 @@ class RetryProvisioning < Error; end
     class InactiveError < ProvisioningError
       attr_reader :inactive_message_key
 
+      # Devise's default inactive_message is :inactive. Hosts can
+      # override the method and legitimately return nil on some
+      # branches; fall back to :inactive so the controller never
+      # ends up with an empty flash.
       def initialize(inactive_message_key)
-        @inactive_message_key = inactive_message_key
-        super(inactive_message_key.to_s)
+        @inactive_message_key = inactive_message_key.presence || :inactive
+        super(@inactive_message_key.to_s)
       end
     end
 
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 0cd8d78..c7b277b 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 @@
 <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 %>
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 3140e29..875f972 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 @@
   </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',
diff --git a/spec/requests/disabled_user_persistence_spec.rb b/spec/requests/disabled_user_persistence_spec.rb
index 3ff6e1b..6f8c3c4 100644
--- a/spec/requests/disabled_user_persistence_spec.rb
+++ b/spec/requests/disabled_user_persistence_spec.rb
@@ -14,16 +14,9 @@
 # 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:)
+  let(:uid)   { "mallory-sub" }
+  let(:email) { "mallory@example.com" }
+  let(:auth_hash) do
     OmniAuth::AuthHash.new(
       provider: "oidc",
       uid:      uid,
@@ -32,43 +25,83 @@ def build_auth_hash(uid:, email:)
     )
   end
 
-  it "does NOT persist a row when on_login sets enabled=false and returns truthy" do
+  let(:disabling_hook) do
+    lambda do |admin_user, _claims|
+      admin_user.enabled = false
+      true # truthy → pre-fix code persisted the row
+    end
+  end
+
+  let(:noop_hook) { ->(*) { true } }
+
+  before do
+    OmniAuth.config.test_mode = true
+    OmniAuth.config.mock_auth[:oidc] = nil
+    AdminUser.delete_all
+
     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
+      c.on_login  = disabling_hook
     end
 
-    OmniAuth.config.mock_auth[:oidc] =
-      build_auth_hash(uid: "mallory-sub", email: "mallory@example.com")
+    OmniAuth.config.mock_auth[:oidc] = auth_hash
+  end
+
+  after { OmniAuth.config.mock_auth[:oidc] = nil }
 
-    expect {
-      post "/admin/auth/oidc"
-      follow_redirect! if response.redirect?
-    }.not_to change(AdminUser, :count),
+  def post_callback
+    post "#{OmniAuth.config.path_prefix}/oidc"
+    follow_redirect! if response.redirect?
+  end
+
+  it "does NOT persist a row when on_login sets enabled=false and returns truthy" do
+    expect { post_callback }.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
+    post_callback
+    expect(response).to redirect_to(new_admin_user_session_path)
+  end
+
+  # Code-review followup: the original retry short-circuit
+  # (`return admin_user if @retried`) skipped the
+  # `active_for_authentication?` guard. If a host-side trigger
+  # flips the winner's row to inactive between the concurrent
+  # insert and our retry read, the loser thread used to sign in
+  # silently. Enforce the same Devise inactivity guard on the
+  # retry leg.
+  it "rejects an inactive winner row on the retry leg" do
+    ActiveAdmin::Oidc.config.on_login = noop_hook
+
+    # First save! simulates a lost race: the "other thread" inserts
+    # the row as ACTIVE, then a host-side trigger flips it inactive
+    # before our retry read. RecordNotUnique sends us through the
+    # retry path, where find_by(provider, uid) returns the now-
+    # inactive row.
+    raise_once = true
+    allow_any_instance_of(AdminUser).to receive(:save!).and_wrap_original do |original, *args|
+      if raise_once
+        raise_once = false
+        winner = AdminUser.create!(provider: "oidc", uid: uid, email: email)
+        winner.update_column(:enabled, false)
+        raise ActiveRecord::RecordNotUnique, "duplicate (provider, uid)"
+      else
+        original.call(*args)
       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?
+    post_callback
 
-    expect(response).to redirect_to("/admin/login")
+    expect(response).to redirect_to(new_admin_user_session_path)
+    # Tighter than the redirect check: confirm the inactive winner
+    # did NOT end up in the Warden session. If they did, subsequent
+    # protected pages would honor the session until the next
+    # active_for_authentication? check, and the user would briefly
+    # appear signed-in.
+    expect(session.to_h.keys.grep(/warden/i)).to be_empty,
+      "retry leg signed in an inactive winner row — active_for_authentication? was skipped"
   end
 
   # The HIGH #2 fix raised ProvisioningError when the hook flipped
@@ -79,25 +112,36 @@ def build_auth_hash(uid:, email:)
   # 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),
+    post_callback
+    expect(flash[:alert]).to eq(I18n.t("devise.failure.inactive")),
       "expected the disabled user to see Devise's translated inactive " \
       "message, but the controller used the generic denial flash"
   end
+
+  # Code-review followup: if a host's inactive_message returns nil
+  # (legal Devise return shape — `super` returns :inactive but a
+  # subclass may legitimately return nil for other inactivity
+  # branches) the InactiveError used to carry "" as its key, which
+  # collapsed to I18n.t("devise.failure.") and rendered an empty
+  # flash. Default to :inactive so the user always sees a reason.
+  it "defaults to :inactive when the model's inactive_message is blank" do
+    allow_any_instance_of(AdminUser).to receive(:inactive_message).and_return(nil)
+    post_callback
+    expect(flash[:alert]).to eq(I18n.t("devise.failure.inactive")),
+      "blank inactive_message produced an empty flash"
+  end
+
+  # Code-review followup: when the model returns a custom symbol
+  # whose translation is missing (e.g. :locked_by_admin with no
+  # devise.failure.locked_by_admin key), the controller used to
+  # render the raw symbol name to an unauthenticated visitor —
+  # leaking host-internal state. Fall back to the standard inactive
+  # translation instead.
+  it "hides custom inactive_message symbols when the translation is missing" do
+    allow_any_instance_of(AdminUser).to receive(:inactive_message).and_return(:locked_by_admin)
+    post_callback
+    expect(flash[:alert]).not_to include("locked_by_admin"),
+      "raw inactive_message symbol leaked into the public flash"
+    expect(flash[:alert]).to eq(I18n.t("devise.failure.inactive"))
+  end
 end

From f59384f34e66a1fe83af8b584e737bf8456b4229 Mon Sep 17 00:00:00 2001
From: Igor Fedoronchuk <fedoronchuk@gmail.com>
Date: Tue, 2 Jun 2026 16:59:02 +0200
Subject: [PATCH 2/2] Drop review-process references from disabled-user spec
 comments

---
 .../disabled_user_persistence_spec.rb         | 63 ++++++++-----------
 1 file changed, 27 insertions(+), 36 deletions(-)

diff --git a/spec/requests/disabled_user_persistence_spec.rb b/spec/requests/disabled_user_persistence_spec.rb
index 6f8c3c4..f47ce68 100644
--- a/spec/requests/disabled_user_persistence_spec.rb
+++ b/spec/requests/disabled_user_persistence_spec.rb
@@ -2,17 +2,13 @@
 
 require "rails_helper"
 
-# Failing spec for HIGH #2 — disabled user persistence.
+# Disabled-user provisioning behaviour.
 #
-# 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.
+# UserProvisioner must enforce `active_for_authentication?` BEFORE
+# `save!`. If the on_login hook flips a user's inactivity flag (e.g.
+# `enabled = false`) and returns truthy, the gem must NOT persist
+# the record — otherwise repeated hostile attempts grow the table
+# with provisional rows that can never sign in.
 RSpec.describe "OIDC callback: disabled user not persisted", type: :request do
   let(:uid)   { "mallory-sub" }
   let(:email) { "mallory@example.com" }
@@ -65,13 +61,14 @@ def post_callback
     expect(response).to redirect_to(new_admin_user_session_path)
   end
 
-  # Code-review followup: the original retry short-circuit
-  # (`return admin_user if @retried`) skipped the
-  # `active_for_authentication?` guard. If a host-side trigger
-  # flips the winner's row to inactive between the concurrent
-  # insert and our retry read, the loser thread used to sign in
-  # silently. Enforce the same Devise inactivity guard on the
-  # retry leg.
+  # The retry short-circuit (`return admin_user if @retried`) skips
+  # the `active_for_authentication?` guard in the provisioner. If a
+  # host-side trigger flips the winner's row to inactive between the
+  # concurrent insert and our retry read, the loser thread would
+  # sign in silently — except Devise's after_set_user callback also
+  # checks active_for_authentication? and intercepts. This spec
+  # pins that safety net so a future refactor can't quietly remove
+  # it.
   it "rejects an inactive winner row on the retry leg" do
     ActiveAdmin::Oidc.config.on_login = noop_hook
 
@@ -104,13 +101,10 @@ def post_callback
       "retry leg signed in an inactive winner row — active_for_authentication? was skipped"
   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.
+  # The flash for a disabled user must carry Devise's translated
+  # inactive_message ("Your account is not activated yet."), not the
+  # gem's generic access_denied_message — otherwise users lose the
+  # specific reason for the rejection.
   it "shows the model's I18n-translated inactive_message in the flash" do
     post_callback
     expect(flash[:alert]).to eq(I18n.t("devise.failure.inactive")),
@@ -118,12 +112,11 @@ def post_callback
       "message, but the controller used the generic denial flash"
   end
 
-  # Code-review followup: if a host's inactive_message returns nil
-  # (legal Devise return shape — `super` returns :inactive but a
-  # subclass may legitimately return nil for other inactivity
-  # branches) the InactiveError used to carry "" as its key, which
-  # collapsed to I18n.t("devise.failure.") and rendered an empty
-  # flash. Default to :inactive so the user always sees a reason.
+  # A host's override may legitimately return nil from
+  # inactive_message on some branches (Devise itself returns :inactive
+  # from the base, but a subclass overriding for custom branches can
+  # return nil). The flash must still carry a reason, not collapse to
+  # I18n.t("devise.failure.") = "".
   it "defaults to :inactive when the model's inactive_message is blank" do
     allow_any_instance_of(AdminUser).to receive(:inactive_message).and_return(nil)
     post_callback
@@ -131,12 +124,10 @@ def post_callback
       "blank inactive_message produced an empty flash"
   end
 
-  # Code-review followup: when the model returns a custom symbol
-  # whose translation is missing (e.g. :locked_by_admin with no
-  # devise.failure.locked_by_admin key), the controller used to
-  # render the raw symbol name to an unauthenticated visitor —
-  # leaking host-internal state. Fall back to the standard inactive
-  # translation instead.
+  # If the model returns a custom symbol with no translation (e.g.
+  # :locked_by_admin without a devise.failure.locked_by_admin key),
+  # the raw symbol name must NOT land in the flash visible to
+  # unauthenticated visitors — it would leak host-internal state.
   it "hides custom inactive_message symbols when the translation is missing" do
     allow_any_instance_of(AdminUser).to receive(:inactive_message).and_return(:locked_by_admin)
     post_callback