mirror of
https://github.com/lingble/chatwoot.git
synced 2025-10-30 18:47:51 +00:00
feat: update users on SAML setup and destroy [CW-2958][CW-5612] (#12346)
This commit is contained in:
@@ -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'
|
||||
|
||||
|
||||
@@ -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
|
||||
@@ -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?
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user