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 @@
<%%= 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/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 @@
<%%= 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..f47ce68 100644
--- a/spec/requests/disabled_user_persistence_spec.rb
+++ b/spec/requests/disabled_user_persistence_spec.rb
@@ -2,28 +2,17 @@
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
- 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,72 +21,118 @@ def build_auth_hash(uid:, 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
+ let(:disabling_hook) do
+ lambda do |admin_user, _claims|
+ admin_user.enabled = false
+ true # truthy → pre-fix code persisted the row
end
+ end
- OmniAuth.config.mock_auth[:oidc] =
- build_auth_hash(uid: "mallory-sub", email: "mallory@example.com")
+ let(:noop_hook) { ->(*) { true } }
- 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
+ before do
+ OmniAuth.config.test_mode = true
+ OmniAuth.config.mock_auth[:oidc] = nil
+ AdminUser.delete_all
- 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
+ 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 }
- post "/admin/auth/oidc"
+ def post_callback
+ post "#{OmniAuth.config.path_prefix}/oidc"
follow_redirect! if response.redirect?
+ end
- expect(response).to redirect_to("/admin/login")
+ 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
- # 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
+ it "still redirects the disabled user to the login page" do
+ post_callback
+ expect(response).to redirect_to(new_admin_user_session_path)
+ end
+
+ # 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
+
+ # 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")
-
- expected = I18n.t("devise.failure.inactive")
+ post_callback
- post "/admin/auth/oidc"
- follow_redirect! if response.redirect? # OmniAuth → /callback → our controller
+ 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
- expect(flash[:alert]).to eq(expected),
+ # 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")),
"expected the disabled user to see Devise's translated inactive " \
"message, but the controller used the generic denial flash"
end
+
+ # 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
+ expect(flash[:alert]).to eq(I18n.t("devise.failure.inactive")),
+ "blank inactive_message produced an empty flash"
+ end
+
+ # 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
+ 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