From 2e18084a04edef0ce092b5da1c214a22695a090e Mon Sep 17 00:00:00 2001 From: mohammed-guiga Date: Mon, 8 Jun 2026 12:29:33 +0300 Subject: [PATCH] Fix Clerk SSO auto-admin: fail closed + explicit admin allowlist (BLO-287) clerk_email_allowed? failed OPEN on an empty CLERK_ALLOWED_EMAIL_DOMAINS (empty env = anyone could sign in), and both SSO entrypoints (from_clerk_oidc and the apex-cookie ClerkDeviseBridge) auto-provisioned every first-time user as ADMIN_ROLE. - Fail closed: an unset/empty allowlist now matches no one. - New CLERK_ADMIN_EMAIL_DOMAINS allowlist gates admin provisioning; SSO never silently mints an admin. - Single chokepoint User.provision_clerk_admin used by both SSO paths. - Add RSpec coverage (none existed). --- .../concerns/clerk_devise_bridge.rb | 12 +- app/models/user.rb | 28 ++++- lib/docuseal.rb | 27 ++++- spec/models/user_spec.rb | 108 ++++++++++++++++++ 4 files changed, 154 insertions(+), 21 deletions(-) create mode 100644 spec/models/user_spec.rb diff --git a/app/controllers/concerns/clerk_devise_bridge.rb b/app/controllers/concerns/clerk_devise_bridge.rb index 4a1c735d..b1294df8 100644 --- a/app/controllers/concerns/clerk_devise_bridge.rb +++ b/app/controllers/concerns/clerk_devise_bridge.rb @@ -47,20 +47,10 @@ module ClerkDeviseBridge end def provision_user_for_clerk(email, clerk_user) - account = Account.first - return nil unless account - first_name = clerk_user.respond_to?(:first_name) ? clerk_user.first_name : nil last_name = clerk_user.respond_to?(:last_name) ? clerk_user.last_name : nil - User.create!( - account: account, - email: email, - first_name: first_name.presence, - last_name: last_name.presence, - role: User::ADMIN_ROLE, - password: Devise.friendly_token(40) - ) + User.provision_clerk_admin(email: email, first_name: first_name, last_name: last_name) rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique => e Rails.logger.warn("[clerk-bridge] provision failed for #{email}: #{e.message}") User.active.find_by(email: email) diff --git a/app/models/user.rb b/app/models/user.rb index f6bd157c..e06d410c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -132,19 +132,37 @@ class User < ApplicationRecord return nil if email.blank? return nil unless Docuseal.clerk_email_allowed?(email) - account = Account.first - return nil if account.blank? - user = active.find_by(email:) return user if user first, *rest = auth.info.name.to_s.split(' ', 2) + provision_clerk_admin( + email:, + first_name: auth.info.first_name.presence || first, + last_name: auth.info.last_name.presence || rest.join(' ') + ) + end + + # The single chokepoint for auto-provisioning a Devise user from Clerk SSO, + # used by every SSO entrypoint (the OIDC callback and the apex-cookie bridge). + # Fails closed: it refuses to create anyone unless their domain is on the + # explicit admin allowlist, so SSO can never silently mint an admin. docuseal + # has no non-admin console role, so an allowed-but-not-admin email gets no + # account rather than an elevated one. + def self.provision_clerk_admin(email:, first_name:, last_name:) + email = email.to_s.downcase + return nil if email.blank? + return nil unless Docuseal.clerk_email_admin?(email) + + account = Account.first + return nil if account.blank? + create!( account:, email:, - first_name: auth.info.first_name.presence || first, - last_name: auth.info.last_name.presence || rest.join(' '), + first_name: first_name.presence, + last_name: last_name.presence, role: ADMIN_ROLE, password: Devise.friendly_token(40) ) diff --git a/lib/docuseal.rb b/lib/docuseal.rb index c03fde2d..ad4f8d48 100644 --- a/lib/docuseal.rb +++ b/lib/docuseal.rb @@ -38,11 +38,15 @@ module Docuseal CERTS = JSON.parse(ENV.fetch('CERTS', '{}')) TIMESERVER_URL = ENV.fetch('TIMESERVER_URL', nil) + def self.parse_email_domains(env_key) + ENV.fetch(env_key, '').split(',').map { |d| d.strip.downcase }.compact_blank.freeze + end + CLERK_DISCOVERY_URL = ENV.fetch('CLERK_DISCOVERY_URL', nil) CLERK_CLIENT_ID = ENV.fetch('CLERK_CLIENT_ID', nil) CLERK_CLIENT_SECRET = ENV.fetch('CLERK_CLIENT_SECRET', nil) - CLERK_ALLOWED_EMAIL_DOMAINS = ENV.fetch('CLERK_ALLOWED_EMAIL_DOMAINS', '') - .split(',').map { |d| d.strip.downcase }.compact_blank.freeze + CLERK_ALLOWED_EMAIL_DOMAINS = parse_email_domains('CLERK_ALLOWED_EMAIL_DOMAINS') + CLERK_ADMIN_EMAIL_DOMAINS = parse_email_domains('CLERK_ADMIN_EMAIL_DOMAINS') VERSION_FILE_PATH = Rails.root.join('.version') VERSION_FILE2_PATH = Rails.public_path.join('version') @@ -71,11 +75,24 @@ module Docuseal CLERK_DISCOVERY_URL.present? && CLERK_CLIENT_ID.present? && CLERK_CLIENT_SECRET.present? end - def clerk_email_allowed?(email) - return true if CLERK_ALLOWED_EMAIL_DOMAINS.empty? + # Fail closed: an unset/empty allowlist matches no one rather than everyone. + def email_in_domains?(email, allowed_domains) + return false if allowed_domains.empty? domain = email.to_s.split('@').last.to_s.downcase - CLERK_ALLOWED_EMAIL_DOMAINS.include?(domain) + return false if domain.blank? + + allowed_domains.include?(domain) + end + + def clerk_email_allowed?(email) + email_in_domains?(email, CLERK_ALLOWED_EMAIL_DOMAINS) + end + + # Admin must be granted explicitly via its own allowlist; never inferred from + # mere sign-in eligibility. + def clerk_email_admin?(email) + email_in_domains?(email, CLERK_ADMIN_EMAIL_DOMAINS) end def advanced_formats? diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb new file mode 100644 index 00000000..e459dabd --- /dev/null +++ b/spec/models/user_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe User do + describe '.from_clerk_oidc' do + let!(:account) { create(:account) } + + def auth_for(email, name: 'Jane Doe') + OmniAuth::AuthHash.new( + provider: 'clerk_oidc', + info: { email:, name:, first_name: nil, last_name: nil } + ) + end + + before do + stub_const('Docuseal::CLERK_ALLOWED_EMAIL_DOMAINS', ['bloombilt.com']) + stub_const('Docuseal::CLERK_ADMIN_EMAIL_DOMAINS', ['bloombilt.com']) + end + + it 'returns nil for a blank email' do + expect(described_class.from_clerk_oidc(auth_for(''))).to be_nil + end + + it 'rejects an email whose domain is not on the allowlist' do + expect { described_class.from_clerk_oidc(auth_for('intruder@evil.com')) } + .not_to change(described_class, :count) + expect(described_class.from_clerk_oidc(auth_for('intruder@evil.com'))).to be_nil + end + + it 'provisions a first-time admin-allowlisted user as admin' do + user = described_class.from_clerk_oidc(auth_for('founder@bloombilt.com')) + + expect(user).to be_persisted + expect(user.role).to eq(User::ADMIN_ROLE) + expect(user.email).to eq('founder@bloombilt.com') + end + + it 'refuses to provision an allowed-but-not-admin user instead of minting an admin' do + stub_const('Docuseal::CLERK_ADMIN_EMAIL_DOMAINS', ['admins.bloombilt.com']) + + expect { described_class.from_clerk_oidc(auth_for('staff@bloombilt.com')) } + .not_to change(described_class, :count) + expect(described_class.from_clerk_oidc(auth_for('staff@bloombilt.com'))).to be_nil + end + + it 'returns an existing active user without creating a new one' do + existing = create(:user, account:, email: 'founder@bloombilt.com', role: User::ADMIN_ROLE) + + expect { described_class.from_clerk_oidc(auth_for('FOUNDER@bloombilt.com')) } + .not_to change(described_class, :count) + expect(described_class.from_clerk_oidc(auth_for('FOUNDER@bloombilt.com'))).to eq(existing) + end + end + + describe '.provision_clerk_admin' do + before { stub_const('Docuseal::CLERK_ADMIN_EMAIL_DOMAINS', ['bloombilt.com']) } + + it 'creates an admin for an admin-allowlisted email' do + account = create(:account) + user = described_class.provision_clerk_admin( + email: 'Founder@Bloombilt.com', first_name: 'Founder', last_name: 'One' + ) + + expect(user).to be_persisted + expect(user.role).to eq(User::ADMIN_ROLE) + expect(user.email).to eq('founder@bloombilt.com') + expect(user.account).to eq(account) + end + + it 'refuses to provision an email that is not admin-allowlisted' do + create(:account) + expect { described_class.provision_clerk_admin(email: 'rando@evil.com', first_name: 'R', last_name: 'O') } + .not_to change(described_class, :count) + end + + it 'returns nil when no account exists' do + expect(described_class.provision_clerk_admin(email: 'founder@bloombilt.com', first_name: 'F', last_name: 'O')) + .to be_nil + end + end + + describe '.clerk_email_allowed? (via Docuseal)' do + it 'fails closed when the allowlist is empty' do + stub_const('Docuseal::CLERK_ALLOWED_EMAIL_DOMAINS', []) + expect(Docuseal.clerk_email_allowed?('anyone@anywhere.com')).to be(false) + end + + it 'allows a listed domain and rejects an unlisted one' do + stub_const('Docuseal::CLERK_ALLOWED_EMAIL_DOMAINS', ['bloombilt.com']) + expect(Docuseal.clerk_email_allowed?('a@bloombilt.com')).to be(true) + expect(Docuseal.clerk_email_allowed?('a@evil.com')).to be(false) + end + end + + describe '.clerk_email_admin? (via Docuseal)' do + it 'grants no one when the admin allowlist is empty' do + stub_const('Docuseal::CLERK_ADMIN_EMAIL_DOMAINS', []) + expect(Docuseal.clerk_email_admin?('founder@bloombilt.com')).to be(false) + end + + it 'grants only listed domains' do + stub_const('Docuseal::CLERK_ADMIN_EMAIL_DOMAINS', ['bloombilt.com']) + expect(Docuseal.clerk_email_admin?('founder@bloombilt.com')).to be(true) + expect(Docuseal.clerk_email_admin?('founder@other.com')).to be(false) + end + end +end