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/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/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 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(