Merge branch 'develop' into team-permissions-v2

Gemfile.lock

@@ -935,7 +935,7 @@ GEM
       unicode-emoji (~> 4.0, >= 4.0.4)
     unicode-emoji (4.0.4)
     uniform_notifier (1.17.0)
-    uri (1.0.4)
+    uri (1.0.3)
     uri_template (0.7.0)
     valid_email2 (5.2.6)
       activemodel (>= 3.2)

app/controllers/api/v1/accounts/conversations/base_controller.rb

@@ -5,6 +5,6 @@ class Api::V1::Accounts::Conversations::BaseController < Api::V1::Accounts::Base
 
   def conversation
     @conversation ||= Current.account.conversations.find_by!(display_id: params[:conversation_id])
-    authorize @conversation.inbox, :show?
+    authorize @conversation, :show?
   end
 end

app/controllers/api/v1/accounts/conversations_controller.rb

@@ -160,7 +160,7 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro
 
   def conversation
     @conversation ||= Current.account.conversations.find_by!(display_id: params[:id])
-    authorize @conversation.inbox, :show?
+    authorize @conversation, :show?
   end
 
   def inbox

app/controllers/devise_overrides/omniauth_callbacks_controller.rb

@@ -19,19 +19,6 @@ class DeviseOverrides::OmniauthCallbacksController < DeviseTokenAuth::OmniauthCa
     redirect_to login_page_url(email: encoded_email, sso_auth_token: @resource.generate_sso_auth_token)
   end
 
-  def sign_in_user_on_mobile
-    @resource.skip_confirmation! if confirmable_enabled?
-
-    # once the resource is found and verified
-    # we can just send them to the login page again with the SSO params
-    # that will log them in
-    encoded_email = ERB::Util.url_encode(@resource.email)
-    params = { email: encoded_email, sso_auth_token: @resource.generate_sso_auth_token }.to_query
-
-    mobile_deep_link_base = GlobalConfigService.load('MOBILE_DEEP_LINK_BASE', 'chatwootapp')
-    redirect_to "#{mobile_deep_link_base}://auth/saml?#{params}", allow_other_host: true
-  end
-
   def sign_up_user
     return redirect_to login_page_url(error: 'no-account-found') unless account_signup_allowed?
     return redirect_to login_page_url(error: 'business-account-only') unless validate_signup_email_is_business_domain?

app/finders/conversation_finder.rb

@@ -72,7 +72,7 @@ class ConversationFinder
 
   def set_inboxes
     @inbox_ids = if params[:inbox_id]
-                   @current_user.assigned_inboxes.where(id: params[:inbox_id])
+                   @current_user.assigned_inboxes.where(id: params[:inbox_id]).pluck(:id)
                  else
                    @current_user.assigned_inboxes.pluck(:id)
                  end

app/javascript/v3/views/login/Index.vue

@@ -22,8 +22,6 @@ import MfaVerification from 'dashboard/components/auth/MfaVerification.vue';
 const ERROR_MESSAGES = {
   'no-account-found': 'LOGIN.OAUTH.NO_ACCOUNT_FOUND',
   'business-account-only': 'LOGIN.OAUTH.BUSINESS_ACCOUNTS_ONLY',
-  'saml-authentication-failed': 'LOGIN.SAML.API.ERROR_MESSAGE',
-  'saml-not-enabled': 'LOGIN.SAML.API.ERROR_MESSAGE',
 };
 
 const IMPERSONATION_URL_SEARCH_KEY = 'impersonation';

app/javascript/v3/views/login/Saml.vue

@@ -15,10 +15,6 @@ const props = defineProps({
     type: String,
     default: '',
   },
-  target: {
-    type: String,
-    default: 'web',
-  },
 });
 
 const store = useStore();
@@ -111,7 +107,6 @@ onMounted(async () => {
           name="authenticity_token"
           :value="csrfToken"
         />
-        <input type="hidden" class="h-0" name="target" :value="target" />
         <NextButton
           lg
           type="submit"

app/javascript/v3/views/routes.js

@@ -28,7 +28,6 @@ export default [
     meta: { requireEnterprise: true },
     props: route => ({
       authError: route.query.error,
-      target: route.query.target,
     }),
   },
   {

app/models/concerns/activity_message_handler.rb

@@ -106,7 +106,7 @@ module ActivityMessageHandler
   end
 
   def generate_assignee_change_activity_content(user_name)
-    params = { assignee_name: assignee&.name || '', user_name: user_name }
+    params = { assignee_name: assignee&.name, user_name: user_name }.compact
     key = assignee_id ? 'assigned' : 'removed'
     key = 'self_assigned' if self_assign? assignee_id
     I18n.t("conversations.activity.assignee.#{key}", **params)

app/policies/conversation_policy.rb

@@ -3,7 +3,27 @@ class ConversationPolicy < ApplicationPolicy
     true
   end
 
+  def show?
+    return true if @account_user&.administrator?
+
+    # Check if user has access to the conversation through inbox membership
+    return true if user_assigned_inboxes.include?(record.inbox)
+
+    # Check if user has access to the conversation through team membership
+    user_assigned_teams.include?(record.team) if record.team.present?
+  end
+
   def destroy?
     @account_user&.administrator?
   end
+
+  private
+
+  def user_assigned_inboxes
+    @user.inboxes.where(account_id: @account.id)
+  end
+
+  def user_assigned_teams
+    @user.teams.where(account_id: @account.id)
+  end
 end

app/services/conversations/permission_filter_service.rb

@@ -17,6 +17,7 @@ class Conversations::PermissionFilterService
 
   def accessible_conversations
     conversations.where(inbox: user.inboxes.where(account_id: account.id))
+                 .or(conversations.where(team: user.teams.where(account_id: account.id)))
   end
 
   def account_user

app/services/whatsapp/populate_template_parameters_service.rb

@@ -34,9 +34,8 @@ class Whatsapp::PopulateTemplateParametersService
     return nil if url.blank?
 
     sanitized_url = sanitize_parameter(url)
-    normalized_url = normalize_url(sanitized_url)
-    validate_url(normalized_url)
-    build_media_type_parameter(normalized_url, media_type.downcase, media_name)
+    validate_url(sanitized_url)
+    build_media_type_parameter(sanitized_url, media_type.downcase, media_name)
   end
 
   def build_named_parameter(parameter_name, value)
@@ -139,20 +138,9 @@ class Whatsapp::PopulateTemplateParametersService
     sanitized[0...1000] # Limit length to prevent DoS
   end
 
-  def normalize_url(url)
-    # Use Addressable::URI for better URL normalization
-    # It handles spaces, special characters, and encoding automatically
-    Addressable::URI.parse(url).normalize.to_s
-  rescue Addressable::URI::InvalidURIError
-    # Fallback: simple space encoding if Addressable fails
-    url.gsub(' ', '%20')
-  end
-
   def validate_url(url)
     return if url.blank?
 
-    # url is already normalized by the caller
-
     uri = URI.parse(url)
     raise ArgumentError, "Invalid URL scheme: #{uri.scheme}. Only http and https are allowed" unless %w[http https].include?(uri.scheme)
     raise ArgumentError, 'URL too long (max 2000 characters)' if url.length > 2000

enterprise/app/controllers/api/v1/auth_controller.rb

@@ -5,9 +5,7 @@ class Api::V1::AuthController < Api::BaseController
   def saml_login
     return if @account.nil?
 
-    relay_state = params[:target] || 'web'
-
-    saml_initiation_url = "/auth/saml?account_id=#{@account.id}&RelayState=#{relay_state}"
+    saml_initiation_url = "/auth/saml?account_id=#{@account.id}"
     redirect_to saml_initiation_url, status: :temporary_redirect
   end
 
@@ -46,18 +44,7 @@ class Api::V1::AuthController < Api::BaseController
   end
 
   def render_saml_error
-    error = 'saml-authentication-failed'
-
-    if mobile_target?
-      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
-    else
-      redirect_to sso_login_page_url(error: error)
-    end
-  end
-
-  def mobile_target?
-    params[:target]&.casecmp('mobile')&.zero?
+    redirect_to sso_login_page_url(error: 'saml-authentication-failed')
   end
 
   def sso_login_page_url(error: nil)

enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb

@@ -32,40 +32,17 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController
     end
   end
 
-  def omniauth_failure
-    return super unless params[:provider] == 'saml'
-
-    relay_state = saml_relay_state
-    error = params[:message] || 'authentication-failed'
-
-    if for_mobile?(relay_state)
-      redirect_to_mobile_error(error, relay_state)
-    else
-      redirect_to login_page_url(error: "saml-#{error}")
-    end
-  end
-
   private
 
   def handle_saml_auth
     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 redirect_to login_page_url(error: '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)
-
       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
   end
@@ -74,19 +51,6 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController
     params[:account_id] || session[:saml_account_id] || request.env['omniauth.params']&.dig('account_id')
   end
 
-  def saml_relay_state
-    session[:saml_relay_state] || request.env['omniauth.params']&.dig('RelayState')
-  end
-
-  def for_mobile?(relay_state)
-    relay_state.to_s.casecmp('mobile').zero?
-  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
-  end
-
   def saml_enabled_for_account?(account_id)
     return false if account_id.blank?
 

enterprise/app/services/captain/assistant/agent_runner_service.rb

@@ -23,7 +23,7 @@ class Captain::Assistant::AgentRunnerService
     message_to_process = extract_last_user_message(message_history)
     runner = Agents::Runner.with_agents(*agents)
     runner = add_callbacks_to_runner(runner) if @callbacks.any?
-    result = runner.run(message_to_process, context: context, max_turns: 6)
+    result = runner.run(message_to_process, context: context, max_turns: 100)
 
     process_agent_result(result)
   rescue StandardError => e

enterprise/config/initializers/omniauth_saml.rb

@@ -9,22 +9,18 @@ SAML_SETUP_PROC = proc do |env|
   account_id = request.params['account_id'] ||
                request.session[:saml_account_id] ||
                env['omniauth.params']&.dig('account_id')
-  relay_state = request.params['RelayState'] || ''
 
   if account_id
     # Store in session and omniauth params for callback
     request.session[:saml_account_id] = account_id
-    request.session[:saml_relay_state] = relay_state
     env['omniauth.params'] ||= {}
     env['omniauth.params']['account_id'] = account_id
-    env['omniauth.params']['RelayState'] = relay_state
 
     # Find SAML settings for this account
     settings = AccountSamlSettings.find_by(account_id: account_id)
 
     if settings
       # Configure the strategy options dynamically
-      env['omniauth.strategy'].options[:idp_sso_service_url_runtime_params] = { RelayState: :RelayState }
       env['omniauth.strategy'].options[:assertion_consumer_service_url] = "#{ENV.fetch('FRONTEND_URL', 'http://localhost:3000')}/omniauth/saml/callback?account_id=#{account_id}"
       env['omniauth.strategy'].options[:sp_entity_id] = settings.sp_entity_id
       env['omniauth.strategy'].options[:idp_entity_id] = settings.idp_entity_id

spec/enterprise/controllers/api/v1/auth_controller_spec.rb

@@ -36,12 +36,6 @@ RSpec.describe 'Api::V1::Auth', type: :request do
 
         expect(response.location).to eq('http://www.example.com/app/login/sso?error=saml-authentication-failed')
       end
-
-      it 'redirects to mobile deep link with error when target is mobile' do
-        post '/api/v1/auth/saml_login', params: { email: 'nonexistent@example.com', target: 'mobile' }
-
-        expect(response.location).to eq('chatwootapp://auth/saml?error=saml-authentication-failed')
-      end
     end
 
     context 'when user exists but has no SAML enabled accounts' do
@@ -54,12 +48,6 @@ RSpec.describe 'Api::V1::Auth', type: :request do
 
         expect(response.location).to eq('http://www.example.com/app/login/sso?error=saml-authentication-failed')
       end
-
-      it 'redirects to mobile deep link with error when target is mobile' do
-        post '/api/v1/auth/saml_login', params: { email: user.email, target: 'mobile' }
-
-        expect(response.location).to eq('chatwootapp://auth/saml?error=saml-authentication-failed')
-      end
     end
 
     context 'when user has account without SAML feature enabled' do
@@ -77,12 +65,6 @@ RSpec.describe 'Api::V1::Auth', type: :request do
 
         expect(response.location).to eq('http://www.example.com/app/login/sso?error=saml-authentication-failed')
       end
-
-      it 'redirects to mobile deep link with error when target is mobile' do
-        post '/api/v1/auth/saml_login', params: { email: user.email, target: 'mobile' }
-
-        expect(response.location).to eq('chatwootapp://auth/saml?error=saml-authentication-failed')
-      end
     end
 
     context 'when user has valid SAML configuration' do
@@ -100,12 +82,6 @@ RSpec.describe 'Api::V1::Auth', type: :request do
 
         expect(response.location).to include("/auth/saml?account_id=#{account.id}")
       end
-
-      it 'redirects to SAML initiation URL with mobile relay state' do
-        post '/api/v1/auth/saml_login', params: { email: user.email, target: 'mobile' }
-
-        expect(response.location).to include("/auth/saml?account_id=#{account.id}&RelayState=mobile")
-      end
     end
 
     context 'when user has multiple accounts with SAML' do

spec/policies/conversation_policy_spec.rb

@@ -31,4 +31,77 @@ RSpec.describe ConversationPolicy, type: :policy do
       end
     end
   end
+
+  permissions :show? do
+    let(:inbox) { create(:inbox, account: account) }
+    let(:team) { create(:team, account: account) }
+    let(:conversation_with_inbox) { create(:conversation, account: account, inbox: inbox) }
+    let(:conversation_with_team) { create(:conversation, account: account, inbox: other_inbox, team: team) }
+    let(:other_inbox) { create(:inbox, account: account) }
+    let(:inaccessible_conversation) { create(:conversation, account: account, inbox: other_inbox) }
+
+    context 'when user is an administrator' do
+      it 'allows show for any conversation' do
+        expect(subject).to permit(administrator_context, conversation_with_inbox)
+        expect(subject).to permit(administrator_context, conversation_with_team)
+        expect(subject).to permit(administrator_context, inaccessible_conversation)
+      end
+    end
+
+    context 'when user is an agent' do
+      context 'with inbox access' do
+        before do
+          create(:inbox_member, user: agent, inbox: inbox)
+        end
+
+        it 'allows show for conversations in assigned inbox' do
+          expect(subject).to permit(agent_context, conversation_with_inbox)
+        end
+
+        it 'denies show for conversations in non-assigned inbox' do
+          expect(subject).not_to permit(agent_context, conversation_with_team)
+          expect(subject).not_to permit(agent_context, inaccessible_conversation)
+        end
+      end
+
+      context 'with team access' do
+        before do
+          create(:team_member, user: agent, team: team)
+        end
+
+        it 'allows show for conversations assigned to user team' do
+          expect(subject).to permit(agent_context, conversation_with_team)
+        end
+
+        it 'denies show for conversations not assigned to user team' do
+          expect(subject).not_to permit(agent_context, conversation_with_inbox)
+          expect(subject).not_to permit(agent_context, inaccessible_conversation)
+        end
+      end
+
+      context 'with both inbox and team access' do
+        before do
+          create(:inbox_member, user: agent, inbox: inbox)
+          create(:team_member, user: agent, team: team)
+        end
+
+        it 'allows show for conversations from both inbox and team' do
+          expect(subject).to permit(agent_context, conversation_with_inbox)
+          expect(subject).to permit(agent_context, conversation_with_team)
+        end
+
+        it 'denies show for inaccessible conversations' do
+          expect(subject).not_to permit(agent_context, inaccessible_conversation)
+        end
+      end
+
+      context 'with no access' do
+        it 'denies show for all conversations' do
+          expect(subject).not_to permit(agent_context, conversation_with_inbox)
+          expect(subject).not_to permit(agent_context, conversation_with_team)
+          expect(subject).not_to permit(agent_context, inaccessible_conversation)
+        end
+      end
+    end
+  end
 end

spec/services/conversations/permission_filter_service_spec.rb

@@ -4,12 +4,20 @@ RSpec.describe Conversations::PermissionFilterService do
   let(:account) { create(:account) }
   let!(:conversation) { create(:conversation, account: account, inbox: inbox) }
   let!(:another_conversation) { create(:conversation, account: account, inbox: inbox) }
+  let!(:team_conversation) { create(:conversation, account: account, inbox: other_inbox, team: team) }
+  let!(:inaccessible_conversation) { create(:conversation, account: account, inbox: other_inbox) }
   let(:admin) { create(:user, account: account, role: :administrator) }
   let(:agent) { create(:user, account: account, role: :agent) }
+  let(:team_agent) { create(:user, account: account, role: :agent) }
   let!(:inbox) { create(:inbox, account: account) }
+  let!(:other_inbox) { create(:inbox, account: account) }
+  let!(:team) { create(:team, account: account) }
 
   # This inbox_member is used to establish the agent's access to the inbox
-  before { create(:inbox_member, user: agent, inbox: inbox) }
+  before do
+    create(:inbox_member, user: agent, inbox: inbox)
+    create(:team_member, user: team_agent, team: team)
+  end
 
   describe '#perform' do
     context 'when user is an administrator' do
@@ -22,25 +30,81 @@ RSpec.describe Conversations::PermissionFilterService do
 
         expect(result).to include(conversation)
         expect(result).to include(another_conversation)
-        expect(result.count).to eq(2)
+        expect(result).to include(team_conversation)
+        expect(result).to include(inaccessible_conversation)
+        expect(result.count).to eq(4)
       end
     end
 
     context 'when user is an agent' do
-      it 'returns all conversations with no further filtering' do
-        inbox_ids = agent.inboxes.where(account_id: account.id).pluck(:id)
+      context 'with inbox access only' do
+        it 'returns conversations from assigned inboxes' do
+          result = described_class.new(
+            account.conversations,
+            agent,
+            account
+          ).perform
 
-        # The base implementation returns all conversations
-        # expecting the caller to filter by assigned inboxes
-        result = described_class.new(
-          account.conversations.where(inbox_id: inbox_ids),
-          agent,
-          account
-        ).perform
+          expect(result).to include(conversation)
+          expect(result).to include(another_conversation)
+          expect(result).not_to include(team_conversation)
+          expect(result).not_to include(inaccessible_conversation)
+          expect(result.count).to eq(2)
+        end
+      end
 
-        expect(result).to include(conversation)
-        expect(result).to include(another_conversation)
-        expect(result.count).to eq(2)
+      context 'with team access only' do
+        it 'returns conversations from assigned teams' do
+          result = described_class.new(
+            account.conversations,
+            team_agent,
+            account
+          ).perform
+
+          expect(result).to include(team_conversation)
+          expect(result).not_to include(conversation)
+          expect(result).not_to include(another_conversation)
+          expect(result).not_to include(inaccessible_conversation)
+          expect(result.count).to eq(1)
+        end
+      end
+
+      context 'with both inbox and team access' do
+        let(:agent_with_both) { create(:user, account: account, role: :agent) }
+
+        before do
+          create(:inbox_member, user: agent_with_both, inbox: inbox)
+          create(:team_member, user: agent_with_both, team: team)
+        end
+
+        it 'returns conversations from both assigned inboxes and teams' do
+          result = described_class.new(
+            account.conversations,
+            agent_with_both,
+            account
+          ).perform
+
+          expect(result).to include(conversation)
+          expect(result).to include(another_conversation)
+          expect(result).to include(team_conversation)
+          expect(result).not_to include(inaccessible_conversation)
+          expect(result.count).to eq(3)
+        end
+      end
+
+      context 'with no access' do
+        let(:agent_no_access) { create(:user, account: account, role: :agent) }
+
+        it 'returns no conversations' do
+          result = described_class.new(
+            account.conversations,
+            agent_no_access,
+            account
+          ).perform
+
+          expect(result).to be_empty
+          expect(result.count).to eq(0)
+        end
       end
     end
   end

spec/services/whatsapp/populate_template_parameters_service_spec.rb

@@ -1,70 +0,0 @@
-require 'rails_helper'
-
-describe Whatsapp::PopulateTemplateParametersService do
-  let(:service) { described_class.new }
-
-  describe '#normalize_url' do
-    it 'normalizes URLs with spaces' do
-      url_with_spaces = 'https://example.com/path with spaces'
-      normalized = service.send(:normalize_url, url_with_spaces)
-
-      expect(normalized).to eq('https://example.com/path%20with%20spaces')
-    end
-
-    it 'handles URLs with special characters' do
-      url = 'https://example.com/path?query=test value'
-      normalized = service.send(:normalize_url, url)
-
-      expect(normalized).to include('https://example.com/path')
-      expect(normalized).not_to include(' ')
-    end
-
-    it 'returns valid URLs unchanged' do
-      url = 'https://example.com/valid-path'
-      normalized = service.send(:normalize_url, url)
-
-      expect(normalized).to eq(url)
-    end
-  end
-
-  describe '#build_media_parameter' do
-    context 'when URL contains spaces' do
-      it 'normalizes the URL before building media parameter' do
-        url_with_spaces = 'https://example.com/image with spaces.jpg'
-        result = service.build_media_parameter(url_with_spaces, 'IMAGE')
-
-        expect(result[:type]).to eq('image')
-        expect(result[:image][:link]).to eq('https://example.com/image%20with%20spaces.jpg')
-      end
-    end
-
-    context 'when URL contains special characters in query string' do
-      it 'normalizes the URL correctly' do
-        url = 'https://example.com/video.mp4?title=My Video'
-        result = service.build_media_parameter(url, 'VIDEO', 'test_video')
-
-        expect(result[:type]).to eq('video')
-        expect(result[:video][:link]).not_to include(' ')
-      end
-    end
-
-    context 'when URL is already valid' do
-      it 'builds media parameter without changing URL' do
-        url = 'https://example.com/document.pdf'
-        result = service.build_media_parameter(url, 'DOCUMENT', 'test.pdf')
-
-        expect(result[:type]).to eq('document')
-        expect(result[:document][:link]).to eq(url)
-        expect(result[:document][:filename]).to eq('test.pdf')
-      end
-    end
-
-    context 'when URL is blank' do
-      it 'returns nil' do
-        result = service.build_media_parameter('', 'IMAGE')
-
-        expect(result).to be_nil
-      end
-    end
-  end
-end