Catch host on_login hook exceptions and render generic denial

The on_login hook is host-app code. Before this change, if the host's
hook raised any non-ProvisioningError StandardError — a typo, a
NoMethodError on the claims hash, a failed external API call inside
the hook — the callback action crashed with a 500 instead of rendering
the existing access-denied flash.

UserProvisioner now wraps the hook invocation: gem-internal
ActiveAdmin::Oidc::Error subclasses still propagate untouched (so a
host can explicitly raise ProvisioningError with a custom message),
but any other StandardError is logged at :error level with the
original exception class and message, then re-raised as a generic
ProvisioningError. The controller already handles that — the user
sees the same clean denial flash as the 'hook returned false' path,
and ops gets the stack-level detail in the log.

Also introduces ActiveAdmin::Oidc.logger: a module-level logger
accessor that defaults to Rails.logger when Rails is booted and
falls back to a null logger otherwise. UserProvisioner uses it
instead of touching Rails.logger directly, which keeps the class
safe to call in unit-spec contexts that don't boot Rails and gives
tests a clean injection point (allow(ActiveAdmin::Oidc).to
receive(:logger)…).

Flips the existing 'propagates unrelated exceptions from the hook'
spec, which had calcified the old buggy behavior, and adds two new
examples covering the log line and the pass-through for gem-internal
errors.

Author: Fivell

lib/activeadmin-oidc.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "logger"
 require "activeadmin/oidc/version"
 
 # `omniauth-rails_csrf_protection` registers a Railtie that replaces
@@ -34,8 +35,30 @@ def configure
         config
       end
 
+      # Logger the gem uses for internal diagnostics (on_login hook
+      # failures, omniauth failures, etc). Defaults to Rails.logger when
+      # Rails is booted, falls back to a null logger otherwise so that
+      # library code is safe to call in non-Rails contexts (unit specs,
+      # scripts). Override by assigning directly — useful in tests.
+      def logger
+        @logger || default_logger
+      end
+
+      attr_writer :logger
+
       def reset!
         @config = Configuration.new
+        @logger = nil
+      end
+
+      private
+
+      def default_logger
+        if defined?(::Rails) && ::Rails.respond_to?(:logger) && ::Rails.logger
+          ::Rails.logger
+        else
+          @null_logger ||= ::Logger.new(IO::NULL)
+        end
       end
     end
   end

lib/activeadmin/oidc/user_provisioner.rb

@@ -37,7 +37,7 @@ def call
         admin_user = find_or_adopt_or_build
         assign_base_attributes(admin_user)
 
-        allowed = @config.on_login.call(admin_user, @claims)
+        allowed = invoke_on_login(admin_user)
         raise ProvisioningError, denial_message unless allowed
 
         save!(admin_user)
@@ -107,6 +107,25 @@ def save!(admin_user)
         raise ProvisioningError, e.message
       end
 
+      # The on_login hook is host-app code. If it raises a non-gem
+      # exception, we do NOT want the callback action to blow up with
+      # a 500 — the cleanest UX is the same generic denial flash the
+      # "hook returned false" path produces. We still log the original
+      # exception class + message at error level so ops can debug.
+      # Gem-internal exceptions (ActiveAdmin::Oidc::Error subclasses)
+      # are re-raised untouched so nested provisioning errors surface
+      # with their original messages.
+      def invoke_on_login(admin_user)
+        @config.on_login.call(admin_user, @claims)
+      rescue ActiveAdmin::Oidc::Error
+        raise
+      rescue StandardError => e
+        ActiveAdmin::Oidc.logger.error(
+          "[activeadmin-oidc] on_login hook raised #{e.class}: #{e.message}"
+        )
+        raise ProvisioningError, denial_message
+      end
+
       def denial_message
         @config.access_denied_message
       end

spec/unit/user_provisioner_spec.rb

@@ -141,9 +141,28 @@
       expect(existing.reload.email).to eq("old@example.com")
     end
 
-    it "propagates unrelated exceptions from the hook" do
+    it "converts unrelated exceptions from the hook into a ProvisioningError (generic denial)" do
+      # The on_login hook is host-app code. If it raises anything, the
+      # ideal UX is a clean denial flash (identical to the "false" case),
+      # not a 500 crash on the callback action. The original exception
+      # is logged for ops visibility; see next example.
       config.on_login = ->(_a, _c) { raise "boom" }
-      expect { provisioner.call }.to raise_error(RuntimeError, "boom")
+      expect { provisioner.call }.to raise_error(ActiveAdmin::Oidc::ProvisioningError)
+    end
+
+    it "logs the original exception class and message when the hook raises" do
+      config.on_login = ->(_a, _c) { raise ArgumentError, "bad claim shape" }
+      fake_logger = instance_double(Logger, error: nil)
+      allow(ActiveAdmin::Oidc).to receive(:logger).and_return(fake_logger)
+      provisioner.call rescue nil
+      expect(fake_logger).to have_received(:error).with(/on_login.*ArgumentError.*bad claim shape/)
+    end
+
+    it "does not swallow ActiveAdmin::Oidc::Error subclasses raised by the hook" do
+      # If host code explicitly raises a gem-internal error (e.g. from
+      # a nested provisioner), don't double-wrap it — let it surface.
+      config.on_login = ->(_a, _c) { raise ActiveAdmin::Oidc::ProvisioningError, "nested denial" }
+      expect { provisioner.call }.to raise_error(ActiveAdmin::Oidc::ProvisioningError, "nested denial")
     end
   end