From 550b408656269d3abadb490136ded11a028d8373 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Fri, 13 Mar 2026 12:22:25 +0530 Subject: [PATCH 1/2] fix: restrict existing user sign-in to account members (#13793) SAML sign-in now only links an existing user when that user already belongs to the account that initiated SSO. New users can still be created for SAML-enabled accounts, and invited members can continue to sign in through their IdP, but SAML will no longer auto-attach an unrelated existing user record during login. **What changed** - Added an account-membership check before SAML reuses an existing user by email. - Kept first-time SAML user creation unchanged for valid new users. - Added builder and request specs covering the allowed and rejected login paths. --- enterprise/app/builders/saml_user_builder.rb | 21 +++++++--- .../omniauth_callbacks_controller.rb | 32 ++++++++------- .../builders/saml_user_builder_spec.rb | 39 ++++++++++++++++--- .../omniauth_callbacks_controller_spec.rb | 17 ++++++++ 4 files changed, 83 insertions(+), 26 deletions(-) diff --git a/enterprise/app/builders/saml_user_builder.rb b/enterprise/app/builders/saml_user_builder.rb index 97a287462..568225201 100644 --- a/enterprise/app/builders/saml_user_builder.rb +++ b/enterprise/app/builders/saml_user_builder.rb @@ -1,4 +1,6 @@ class SamlUserBuilder + class AuthenticationFailed < StandardError; end + def initialize(auth_hash, account_id) @auth_hash = auth_hash @account_id = account_id @@ -16,13 +18,20 @@ class SamlUserBuilder def find_or_create_user user = User.from_email(auth_attribute('email')) - if user - confirm_user_if_required(user) - convert_existing_user_to_saml(user) - return user - end + return create_user unless user + return existing_user_for_account(user) if user_belongs_to_account?(user) - create_user + raise AuthenticationFailed, I18n.t('auth.saml.authentication_failed') + end + + def existing_user_for_account(user) + confirm_user_if_required(user) + convert_existing_user_to_saml(user) + user + end + + def user_belongs_to_account?(user) + user.account_users.exists?(account_id: @account_id) end def confirm_user_if_required(user) diff --git a/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb b/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb index 4856ca443..3dea12888 100644 --- a/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb +++ b/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb @@ -39,7 +39,7 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController error = params[:message] || 'authentication-failed' if for_mobile?(relay_state) - redirect_to_mobile_error(error, relay_state) + redirect_to_mobile_error(error) else redirect_to login_page_url(error: "saml-#{error}") end @@ -51,23 +51,15 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController account_id = extract_saml_account_id relay_state = saml_relay_state - unless saml_enabled_for_account?(account_id) - return redirect_to_mobile_error('saml-not-enabled') if for_mobile?(relay_state) - - return redirect_to login_page_url(error: 'saml-not-enabled') - end + return handle_saml_auth_error(relay_state, 'saml-not-enabled') unless saml_enabled_for_account?(account_id) @resource = SamlUserBuilder.new(auth_hash, account_id).perform - if @resource.persisted? - return sign_in_user_on_mobile if for_mobile?(relay_state) + return sign_in_saml_user(relay_state) if @resource.persisted? - sign_in_user - else - return redirect_to_mobile_error('saml-authentication-failed') if for_mobile?(relay_state) - - redirect_to login_page_url(error: 'saml-authentication-failed') - end + handle_saml_auth_error(relay_state, 'saml-authentication-failed') + rescue SamlUserBuilder::AuthenticationFailed + handle_saml_auth_error(relay_state, 'saml-authentication-failed') end def extract_saml_account_id @@ -82,6 +74,18 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController relay_state.to_s.casecmp('mobile').zero? end + def sign_in_saml_user(relay_state) + return sign_in_user_on_mobile if for_mobile?(relay_state) + + sign_in_user + end + + def handle_saml_auth_error(relay_state, error) + return redirect_to_mobile_error(error) if for_mobile?(relay_state) + + redirect_to login_page_url(error: error) + end + def redirect_to_mobile_error(error) mobile_deep_link_base = GlobalConfigService.load('MOBILE_DEEP_LINK_BASE', 'chatwootapp') redirect_to "#{mobile_deep_link_base}://auth/saml?error=#{ERB::Util.url_encode(error)}", allow_other_host: true diff --git a/spec/enterprise/builders/saml_user_builder_spec.rb b/spec/enterprise/builders/saml_user_builder_spec.rb index 434beda16..9ebaf3a55 100644 --- a/spec/enterprise/builders/saml_user_builder_spec.rb +++ b/spec/enterprise/builders/saml_user_builder_spec.rb @@ -74,7 +74,7 @@ RSpec.describe SamlUserBuilder do end context 'when user already exists' do - let!(:existing_user) { create(:user, email: email) } + let!(:existing_user) { create(:user, email: email, account: account) } it 'does not create a new user' do expect { builder.perform }.not_to change(User, :count) @@ -85,7 +85,7 @@ RSpec.describe SamlUserBuilder do expect(user).to eq(existing_user) end - it 'adds existing user to the account if not already added' do + it 'keeps the existing user in the account' do user = builder.perform expect(user.accounts).to include(account) end @@ -105,11 +105,38 @@ RSpec.describe SamlUserBuilder do end it 'does not duplicate account association' do - existing_user.account_users.create!(account: account, role: 'agent') - expect { builder.perform }.not_to change(AccountUser, :count) end + context 'when the user does not belong to the target account' do + let!(:other_account) { create(:account) } + let!(:existing_user) { create(:user, email: email, account: other_account) } + + it 'raises an authentication failure' do + expect { builder.perform }.to raise_error do |error| + expect(error.class.name).to eq('SamlUserBuilder::AuthenticationFailed') + expect(error.message).to eq(I18n.t('auth.saml.authentication_failed')) + end + end + + it 'does not add the user to the target account' do + expect do + builder.perform + rescue SamlUserBuilder::AuthenticationFailed + nil + end.not_to change(AccountUser, :count) + expect(existing_user.reload.accounts).not_to include(account) + end + + it 'does not convert the user provider to saml' do + expect do + builder.perform + rescue SamlUserBuilder::AuthenticationFailed + nil + end.not_to(change { existing_user.reload.provider }) + end + end + context 'when user is not confirmed' do let(:unconfirmed_email) { 'unconfirmed_saml_user@example.com' } let(:unconfirmed_auth_hash) do @@ -131,7 +158,7 @@ RSpec.describe SamlUserBuilder do end let(:unconfirmed_builder) { described_class.new(unconfirmed_auth_hash, account.id) } let!(:existing_user) do - user = build(:user, email: unconfirmed_email) + user = build(:user, email: unconfirmed_email, account: account) user.confirmed_at = nil user.save!(validate: false) user @@ -147,7 +174,7 @@ RSpec.describe SamlUserBuilder do end context 'when user is already confirmed' do - let!(:existing_user) { create(:user, email: email, confirmed_at: Time.current) } + let!(:existing_user) { create(:user, email: email, account: account, confirmed_at: Time.current) } it 'keeps already confirmed user confirmed' do expect(existing_user.confirmed?).to be true diff --git a/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb b/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb index af67e3009..8eb61fd8f 100644 --- a/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb +++ b/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb @@ -57,5 +57,22 @@ RSpec.describe 'Enterprise SAML OmniAuth Callbacks', type: :request do expect(response).to redirect_to(%r{/app/login\?email=.+&sso_auth_token=.+$}) end end + + it 'rejects an existing user from another account' do + with_modified_env FRONTEND_URL: 'http://www.example.com' do + other_account = create(:account) + existing_user = create(:user, email: 'existing@example.com', account: other_account, provider: 'email') + set_saml_config('existing@example.com') + + get "/omniauth/saml/callback?account_id=#{account.id}" + + expect(response).to redirect_to('http://www.example.com/auth/saml/callback') + follow_redirect! + + expect(response).to redirect_to('http://www.example.com/app/login?error=saml-authentication-failed') + expect(existing_user.reload.provider).to eq('email') + expect(existing_user.accounts).not_to include(account) + end + end end end From 8aa49f69d239f6ff56bf929a34fd636fc85c7118 Mon Sep 17 00:00:00 2001 From: Aakash Bakhle <48802744+aakashb95@users.noreply.github.com> Date: Fri, 13 Mar 2026 13:10:10 +0530 Subject: [PATCH 2/2] fix: prefer system API key for completion service (#13799) # Pull Request Template ## Description Add an api_key override so internal conversation completions prefers using the system API key and do not consume customer OpenAI credits. ## Type of change Please delete options that are not relevant. - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. specs and locally ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] Any dependent changes have been merged and published in downstream modules --- .../conversation_completion_service.rb | 8 ++++++ .../conversation_completion_service_spec.rb | 27 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/enterprise/lib/captain/conversation_completion_service.rb b/enterprise/lib/captain/conversation_completion_service.rb index 6ac45421c..e9cdc8937 100644 --- a/enterprise/lib/captain/conversation_completion_service.rb +++ b/enterprise/lib/captain/conversation_completion_service.rb @@ -56,6 +56,14 @@ class Captain::ConversationCompletionService < Captain::BaseTaskService { complete: false, reason: reason } end + # Prefer the system API key over the account's OpenAI hook key. + # This is an internal operational evaluation, not a customer-triggered feature, + # so it should not consume the customer's OpenAI credits on hosted platforms. + # Falls back to the account hook for self-hosted deployments without a system key. + def api_key + @api_key ||= system_api_key.presence || openai_hook&.settings&.dig('api_key') + end + def event_name 'captain.conversation_completion' end diff --git a/spec/enterprise/lib/captain/conversation_completion_service_spec.rb b/spec/enterprise/lib/captain/conversation_completion_service_spec.rb index 51d7fa807..58b2b2ce6 100644 --- a/spec/enterprise/lib/captain/conversation_completion_service_spec.rb +++ b/spec/enterprise/lib/captain/conversation_completion_service_spec.rb @@ -126,6 +126,33 @@ RSpec.describe Captain::ConversationCompletionService do end end + context 'when account has its own OpenAI hook' do + before do + create(:message, conversation: conversation, message_type: :incoming, content: 'Hello') + create(:integrations_hook, :openai, account: account, settings: { 'api_key' => 'customer-own-key' }) + end + + it 'uses the system API key instead of the account hook key' do + expect(Llm::Config).to receive(:with_api_key).with('test-key', api_base: anything).and_yield(mock_context) + allow(mock_chat).to receive(:ask).and_return( + instance_double(RubyLLM::Message, content: { 'complete' => true, 'reason' => 'Done' }, input_tokens: 10, output_tokens: 5) + ) + + service.perform + end + + it 'falls back to the account hook key when no system key exists' do + InstallationConfig.find_by(name: 'CAPTAIN_OPEN_AI_API_KEY').update!(value: nil) + + expect(Llm::Config).to receive(:with_api_key).with('customer-own-key', api_base: anything).and_yield(mock_context) + allow(mock_chat).to receive(:ask).and_return( + instance_double(RubyLLM::Message, content: { 'complete' => true, 'reason' => 'Done' }, input_tokens: 10, output_tokens: 5) + ) + + service.perform + end + end + context 'when customer quota is exhausted' do let(:mock_response) do instance_double(