Merge branch 'develop' into feat/rollup/1-model-write-path

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)

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

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

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

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

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(