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/app/views/active_admin/devise/sessions/new.html.erb b/app/views/active_admin/devise/sessions/new.html.erb index efe7aa0..700d875 100644 --- a/app/views/active_admin/devise/sessions/new.html.erb +++ b/app/views/active_admin/devise/sessions/new.html.erb @@ -5,7 +5,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', @@ -15,7 +15,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/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 2f7490d..33509ac 100644 --- a/lib/activeadmin/oidc/user_provisioner.rb +++ b/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 diff --git a/spec/requests/disabled_user_persistence_spec.rb b/spec/requests/disabled_user_persistence_spec.rb new file mode 100644 index 0000000..3ff6e1b --- /dev/null +++ b/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 diff --git a/spec/requests/login_path_helper_spec.rb b/spec/requests/login_path_helper_spec.rb new file mode 100644 index 0000000..09e5d35 --- /dev/null +++ b/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 `/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 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