From 7a453f50f49e98a9596cfedf5342bb4fa1065e91 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Mon, 15 Sep 2025 21:20:22 +0530 Subject: [PATCH] feat: update users on SAML setup and destroy [CW-2958][CW-5612] (#12346) --- enterprise/app/builders/saml_user_builder.rb | 8 +++ .../saml/update_account_users_provider_job.rb | 33 ++++++++++ .../app/models/account_saml_settings.rb | 11 ++++ .../builders/saml_user_builder_spec.rb | 50 ++++++++++++++ .../update_account_users_provider_job_spec.rb | 65 +++++++++++++++++++ .../models/account_saml_settings_spec.rb | 17 +++++ 6 files changed, 184 insertions(+) create mode 100644 enterprise/app/jobs/saml/update_account_users_provider_job.rb create mode 100644 spec/enterprise/jobs/saml/update_account_users_provider_job_spec.rb diff --git a/enterprise/app/builders/saml_user_builder.rb b/enterprise/app/builders/saml_user_builder.rb index bd0059d07..97a287462 100644 --- a/enterprise/app/builders/saml_user_builder.rb +++ b/enterprise/app/builders/saml_user_builder.rb @@ -17,6 +17,7 @@ class SamlUserBuilder user = User.from_email(auth_attribute('email')) if user + confirm_user_if_required(user) convert_existing_user_to_saml(user) return user end @@ -24,6 +25,13 @@ class SamlUserBuilder create_user end + def confirm_user_if_required(user) + return if user.confirmed? + + user.skip_confirmation! + user.save! + end + def convert_existing_user_to_saml(user) return if user.provider == 'saml' diff --git a/enterprise/app/jobs/saml/update_account_users_provider_job.rb b/enterprise/app/jobs/saml/update_account_users_provider_job.rb new file mode 100644 index 000000000..46f47829d --- /dev/null +++ b/enterprise/app/jobs/saml/update_account_users_provider_job.rb @@ -0,0 +1,33 @@ +class Saml::UpdateAccountUsersProviderJob < ApplicationJob + queue_as :default + + # Updates the authentication provider for users in an account + # This job is triggered when SAML settings are created or destroyed + def perform(account_id, provider) + account = Account.find(account_id) + account.users.find_each(batch_size: 1000) do |user| + next unless should_update_user_provider?(user, provider) + + # rubocop:disable Rails/SkipsModelValidations + user.update_column(:provider, provider) + # rubocop:enable Rails/SkipsModelValidations + end + end + + private + + # Determines if a user's provider should be updated based on their multi-account status + # When resetting to 'email', only update users who don't have SAML enabled on other accounts + # This prevents breaking SAML authentication for users who belong to multiple accounts + def should_update_user_provider?(user, provider) + return !user_has_other_saml_accounts?(user) if provider == 'email' + + true + end + + # Checks if the user belongs to any other accounts that have SAML configured + # Used to preserve SAML authentication when one account disables SAML but others still use it + def user_has_other_saml_accounts?(user) + user.accounts.joins(:saml_settings).exists? + end +end diff --git a/enterprise/app/models/account_saml_settings.rb b/enterprise/app/models/account_saml_settings.rb index b2f7bfe7b..9e37f1c82 100644 --- a/enterprise/app/models/account_saml_settings.rb +++ b/enterprise/app/models/account_saml_settings.rb @@ -27,6 +27,9 @@ class AccountSamlSettings < ApplicationRecord before_validation :set_sp_entity_id, if: :sp_entity_id_needs_generation? + after_create_commit :update_account_users_provider + after_destroy_commit :reset_account_users_provider + def saml_enabled? sso_url.present? && certificate.present? end @@ -58,6 +61,14 @@ class AccountSamlSettings < ApplicationRecord GlobalConfigService.load('INSTALLATION_NAME', 'Chatwoot') end + def update_account_users_provider + Saml::UpdateAccountUsersProviderJob.perform_later(account_id, 'saml') + end + + def reset_account_users_provider + Saml::UpdateAccountUsersProviderJob.perform_later(account_id, 'email') + end + def certificate_must_be_valid_x509 return if certificate.blank? diff --git a/spec/enterprise/builders/saml_user_builder_spec.rb b/spec/enterprise/builders/saml_user_builder_spec.rb index 63fd65fd3..434beda16 100644 --- a/spec/enterprise/builders/saml_user_builder_spec.rb +++ b/spec/enterprise/builders/saml_user_builder_spec.rb @@ -109,6 +109,56 @@ RSpec.describe SamlUserBuilder do expect { builder.perform }.not_to change(AccountUser, :count) end + + context 'when user is not confirmed' do + let(:unconfirmed_email) { 'unconfirmed_saml_user@example.com' } + let(:unconfirmed_auth_hash) do + { + 'provider' => 'saml', + 'uid' => 'saml-uid-123', + 'info' => { + 'email' => unconfirmed_email, + 'name' => 'SAML User', + 'first_name' => 'SAML', + 'last_name' => 'User' + }, + 'extra' => { + 'raw_info' => { + 'groups' => %w[Administrators Users] + } + } + } + end + let(:unconfirmed_builder) { described_class.new(unconfirmed_auth_hash, account.id) } + let!(:existing_user) do + user = build(:user, email: unconfirmed_email) + user.confirmed_at = nil + user.save!(validate: false) + user + end + + it 'confirms unconfirmed user after SAML authentication' do + expect(existing_user.confirmed?).to be false + + unconfirmed_builder.perform + + expect(existing_user.reload.confirmed?).to be true + end + end + + context 'when user is already confirmed' do + let!(:existing_user) { create(:user, email: email, confirmed_at: Time.current) } + + it 'keeps already confirmed user confirmed' do + expect(existing_user.confirmed?).to be true + original_confirmed_at = existing_user.confirmed_at + + builder.perform + + expect(existing_user.reload.confirmed?).to be true + expect(existing_user.reload.confirmed_at).to be_within(2.seconds).of(original_confirmed_at) + end + end end context 'with role mappings' do diff --git a/spec/enterprise/jobs/saml/update_account_users_provider_job_spec.rb b/spec/enterprise/jobs/saml/update_account_users_provider_job_spec.rb new file mode 100644 index 000000000..82bb6b015 --- /dev/null +++ b/spec/enterprise/jobs/saml/update_account_users_provider_job_spec.rb @@ -0,0 +1,65 @@ +require 'rails_helper' + +RSpec.describe Saml::UpdateAccountUsersProviderJob, type: :job do + let(:account) { create(:account) } + let!(:user1) { create(:user, accounts: [account], provider: 'email') } + let!(:user2) { create(:user, accounts: [account], provider: 'email') } + let!(:user3) { create(:user, accounts: [account], provider: 'google') } + + describe '#perform' do + context 'when setting provider to saml' do + it 'updates all account users to saml provider' do + described_class.new.perform(account.id, 'saml') + + expect(user1.reload.provider).to eq('saml') + expect(user2.reload.provider).to eq('saml') + expect(user3.reload.provider).to eq('saml') + end + end + + context 'when resetting provider to email' do + before do + # rubocop:disable Rails/SkipsModelValidations + user1.update_column(:provider, 'saml') + user2.update_column(:provider, 'saml') + user3.update_column(:provider, 'saml') + # rubocop:enable Rails/SkipsModelValidations + end + + context 'when users have no other SAML accounts' do + it 'updates all account users to email provider' do + described_class.new.perform(account.id, 'email') + + expect(user1.reload.provider).to eq('email') + expect(user2.reload.provider).to eq('email') + expect(user3.reload.provider).to eq('email') + end + end + + context 'when users belong to other accounts with SAML enabled' do + let(:other_account) { create(:account) } + + before do + create(:account_saml_settings, account: other_account) + user1.account_users.create!(account: other_account, role: :agent) + end + + it 'preserves SAML provider for users with other SAML accounts' do + described_class.new.perform(account.id, 'email') + + expect(user1.reload.provider).to eq('saml') + expect(user2.reload.provider).to eq('email') + expect(user3.reload.provider).to eq('email') + end + end + end + + context 'when account does not exist' do + it 'raises ActiveRecord::RecordNotFound' do + expect do + described_class.new.perform(999_999, 'saml') + end.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end diff --git a/spec/enterprise/models/account_saml_settings_spec.rb b/spec/enterprise/models/account_saml_settings_spec.rb index 65d5119d7..ac35eb486 100644 --- a/spec/enterprise/models/account_saml_settings_spec.rb +++ b/spec/enterprise/models/account_saml_settings_spec.rb @@ -114,4 +114,21 @@ RSpec.describe AccountSamlSettings, type: :model do expect(fingerprint.count(':')).to eq(19) # 20 bytes = 19 colons end end + + describe 'callbacks' do + describe 'after_create_commit' do + it 'queues job to set account users to saml provider' do + expect(Saml::UpdateAccountUsersProviderJob).to receive(:perform_later).with(account.id, 'saml') + create(:account_saml_settings, account: account) + end + end + + describe 'after_destroy_commit' do + it 'queues job to reset account users provider' do + settings = create(:account_saml_settings, account: account) + expect(Saml::UpdateAccountUsersProviderJob).to receive(:perform_later).with(account.id, 'email') + settings.destroy + end + end + end end