From eabdfc81684b467c478bbf6ee238f5fa4ee8f7ec Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Wed, 22 Oct 2025 20:20:37 -0700 Subject: [PATCH 1/2] chore(sidekiq): log ActiveJob class and job_id on dequeue (#12704) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Context Sidekiq logs only showed the Sidekiq wrapper class and JID, which wasn’t helpful when debugging ActiveJobs. ## Changes - Updated `ChatwootDequeuedLogger` to log the actual `ActiveJob class` and `job_id` instead of the generic Sidekiq wrapper and JID. > Example > ``` > Dequeued ActionMailer::MailDeliveryJob 123e4567-e89b-12d3-a456-426614174000 from default > ``` - Remove sidekiq worker and unify everything to `ActiveJob` --- app/jobs/conversation_reply_email_job.rb | 15 ++++++ .../send_email_notification_service.rb | 2 +- .../conversation_reply_email_worker.rb | 29 ------------ config/initializers/sidekiq.rb | 3 +- ...nding_conversations_resolution_job_spec.rb | 27 ++++++----- .../jobs/conversation_reply_email_job_spec.rb | 33 +++++++++++++ spec/models/message_spec.rb | 33 ++++--------- .../send_email_notification_service_spec.rb | 46 +++++++------------ .../conversation_reply_email_worker_spec.rb | 46 ------------------- 9 files changed, 92 insertions(+), 142 deletions(-) create mode 100644 app/jobs/conversation_reply_email_job.rb delete mode 100644 app/workers/conversation_reply_email_worker.rb create mode 100644 spec/jobs/conversation_reply_email_job_spec.rb delete mode 100644 spec/workers/conversation_reply_email_worker_spec.rb diff --git a/app/jobs/conversation_reply_email_job.rb b/app/jobs/conversation_reply_email_job.rb new file mode 100644 index 000000000..5d186bf29 --- /dev/null +++ b/app/jobs/conversation_reply_email_job.rb @@ -0,0 +1,15 @@ +class ConversationReplyEmailJob < ApplicationJob + queue_as :mailers + + def perform(conversation_id, last_queued_id) + conversation = Conversation.find(conversation_id) + + if conversation.messages.incoming&.last&.content_type == 'incoming_email' + ConversationReplyMailer.with(account: conversation.account).reply_without_summary(conversation, last_queued_id).deliver_later + else + ConversationReplyMailer.with(account: conversation.account).reply_with_summary(conversation, last_queued_id).deliver_later + end + + Redis::Alfred.delete(format(::Redis::Alfred::CONVERSATION_MAILER_KEY, conversation_id: conversation.id)) + end +end diff --git a/app/services/messages/send_email_notification_service.rb b/app/services/messages/send_email_notification_service.rb index 87ba1a797..25a77b0d5 100644 --- a/app/services/messages/send_email_notification_service.rb +++ b/app/services/messages/send_email_notification_service.rb @@ -12,7 +12,7 @@ class Messages::SendEmailNotificationService # the worker never manages to clean up. return unless Redis::Alfred.set(conversation_mail_key, message.id, nx: true, ex: 1.hour.to_i) - ConversationReplyEmailWorker.perform_in(2.minutes, conversation.id, message.id) + ConversationReplyEmailJob.set(wait: 2.minutes).perform_later(conversation.id, message.id) end private diff --git a/app/workers/conversation_reply_email_worker.rb b/app/workers/conversation_reply_email_worker.rb deleted file mode 100644 index 0eddecaf7..000000000 --- a/app/workers/conversation_reply_email_worker.rb +++ /dev/null @@ -1,29 +0,0 @@ -# TODO: lets move this to active job, since thats what we use over all -class ConversationReplyEmailWorker - include Sidekiq::Worker - sidekiq_options queue: :mailers - - def perform(conversation_id, last_queued_id) - @conversation = Conversation.find(conversation_id) - - # send the email - if @conversation.messages.incoming&.last&.content_type == 'incoming_email' - ConversationReplyMailer.with(account: @conversation.account).reply_without_summary(@conversation, last_queued_id).deliver_later - else - ConversationReplyMailer.with(account: @conversation.account).reply_with_summary(@conversation, last_queued_id).deliver_later - end - - # delete the redis set from the first new message on the conversation - Redis::Alfred.delete(conversation_mail_key) - end - - private - - def email_inbox? - @conversation.inbox&.inbox_type == 'Email' - end - - def conversation_mail_key - format(::Redis::Alfred::CONVERSATION_MAILER_KEY, conversation_id: @conversation.id) - end -end diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index 1ada67cc1..04d605c2e 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -9,7 +9,8 @@ end # Logs whenever a job is pulled off Redis for execution. class ChatwootDequeuedLogger def call(_worker, job, queue) - Sidekiq.logger.info("Dequeued #{job['class']} #{job['jid']} from #{queue}") + payload = job['args'].first + Sidekiq.logger.info("Dequeued #{job['wrapped']} #{payload['job_id']} from #{queue}") yield end end diff --git a/spec/enterprise/jobs/captain/inbox_pending_conversations_resolution_job_spec.rb b/spec/enterprise/jobs/captain/inbox_pending_conversations_resolution_job_spec.rb index 40f1ea294..1a8a5a342 100644 --- a/spec/enterprise/jobs/captain/inbox_pending_conversations_resolution_job_spec.rb +++ b/spec/enterprise/jobs/captain/inbox_pending_conversations_resolution_job_spec.rb @@ -1,8 +1,6 @@ require 'rails_helper' RSpec.describe Captain::InboxPendingConversationsResolutionJob, type: :job do - include ActiveJob::TestHelper - let!(:inbox) { create(:inbox) } let!(:resolvable_pending_conversation) { create(:conversation, inbox: inbox, last_activity_at: 2.hours.ago, status: :pending) } @@ -14,6 +12,7 @@ RSpec.describe Captain::InboxPendingConversationsResolutionJob, type: :job do before do create(:captain_inbox, inbox: inbox, captain_assistant: captain_assistant) stub_const('Limits::BULK_ACTIONS_LIMIT', 2) + inbox.reload end it 'queues the job' do @@ -22,7 +21,7 @@ RSpec.describe Captain::InboxPendingConversationsResolutionJob, type: :job do end it 'resolves only the eligible pending conversations' do - perform_enqueued_jobs { described_class.perform_later(inbox) } + described_class.perform_now(inbox) expect(resolvable_pending_conversation.reload.status).to eq('resolved') expect(recent_pending_conversation.reload.status).to eq('pending') @@ -34,7 +33,7 @@ RSpec.describe Captain::InboxPendingConversationsResolutionJob, type: :job do captain_assistant.update!(config: { 'resolution_message' => custom_message }) expect do - perform_enqueued_jobs { described_class.perform_later(inbox) } + described_class.perform_now(inbox) end.to change { resolvable_pending_conversation.messages.outgoing.reload.count }.by(1) outgoing_message = resolvable_pending_conversation.messages.outgoing.last @@ -44,7 +43,7 @@ RSpec.describe Captain::InboxPendingConversationsResolutionJob, type: :job do it 'creates an outgoing message with default auto resolution message if not configured' do captain_assistant.update!(config: {}) - perform_enqueued_jobs { described_class.perform_later(inbox) } + described_class.perform_now(inbox) outgoing_message = resolvable_pending_conversation.messages.outgoing.last expect(outgoing_message.content).to eq( I18n.t('conversations.activity.auto_resolution_message') @@ -52,11 +51,17 @@ RSpec.describe Captain::InboxPendingConversationsResolutionJob, type: :job do end it 'adds the correct activity message after resolution by Captain' do - perform_enqueued_jobs { described_class.perform_later(inbox) } - activity_message = resolvable_pending_conversation.messages.activity.last - expect(activity_message).not_to be_nil - expect(activity_message.content).to eq( - I18n.t('conversations.activity.captain.resolved', user_name: captain_assistant.name) - ) + described_class.perform_now(inbox) + expected_content = I18n.t('conversations.activity.captain.resolved', user_name: captain_assistant.name) + expect(Conversations::ActivityMessageJob) + .to have_been_enqueued.with( + resolvable_pending_conversation, + { + account_id: resolvable_pending_conversation.account_id, + inbox_id: resolvable_pending_conversation.inbox_id, + message_type: :activity, + content: expected_content + } + ) end end diff --git a/spec/jobs/conversation_reply_email_job_spec.rb b/spec/jobs/conversation_reply_email_job_spec.rb new file mode 100644 index 000000000..32758c48e --- /dev/null +++ b/spec/jobs/conversation_reply_email_job_spec.rb @@ -0,0 +1,33 @@ +require 'rails_helper' + +RSpec.describe ConversationReplyEmailJob, type: :job do + let(:conversation) { create(:conversation) } + let(:mailer) { double } + let(:mailer_action) { double } + + before do + allow(Conversation).to receive(:find).and_return(conversation) + allow(ConversationReplyMailer).to receive(:with).and_return(mailer) + allow(mailer).to receive(:reply_with_summary).and_return(mailer_action) + allow(mailer).to receive(:reply_without_summary).and_return(mailer_action) + allow(mailer_action).to receive(:deliver_later).and_return(true) + end + + it 'enqueues on mailers queue' do + ActiveJob::Base.queue_adapter = :test + expect do + described_class.perform_later(conversation.id, 123) + end.to have_enqueued_job(described_class).on_queue('mailers') + end + + it 'calls reply_with_summary when last incoming message was not email' do + described_class.perform_now(conversation.id, 123) + expect(mailer).to have_received(:reply_with_summary) + end + + it 'calls reply_without_summary when last incoming message was email' do + create(:message, conversation: conversation, message_type: :incoming, content_type: 'incoming_email') + described_class.perform_now(conversation.id, 123) + expect(mailer).to have_received(:reply_without_summary) + end +end diff --git a/spec/models/message_spec.rb b/spec/models/message_spec.rb index 49a1d1e51..9cc43d0fd 100644 --- a/spec/models/message_spec.rb +++ b/spec/models/message_spec.rb @@ -327,14 +327,11 @@ RSpec.describe Message do message.conversation.contact.update!(email: 'test@example.com') message.message_type = 'outgoing' - # Perform jobs inline to test full integration - perform_enqueued_jobs do - message.save! + ActiveJob::Base.queue_adapter = :test + allow(Redis::Alfred).to receive(:set).and_return(true) + perform_enqueued_jobs(only: SendReplyJob) do + expect { message.save! }.to have_enqueued_job(ConversationReplyEmailJob).with(message.conversation.id, kind_of(Integer)).on_queue('mailers') end - - # Verify the email worker is eventually scheduled through the service - jobs_for_conversation_count = ConversationReplyEmailWorker.jobs.count { |job| job['args'].first == message.conversation.id } - expect(jobs_for_conversation_count).to eq(1) end it 'does not schedule email for website channel if continuity is disabled' do @@ -345,15 +342,8 @@ RSpec.describe Message do message.conversation.contact.update!(email: 'test@example.com') message.message_type = 'outgoing' - initial_job_count = ConversationReplyEmailWorker.jobs.count { |job| job['args'].first == message.conversation.id } - - perform_enqueued_jobs do - message.save! - end - - # No new jobs should be scheduled for this conversation - jobs_for_conversation_count = ConversationReplyEmailWorker.jobs.count { |job| job['args'].first == message.conversation.id } - expect(jobs_for_conversation_count).to eq(initial_job_count) + ActiveJob::Base.queue_adapter = :test + expect { message.save! }.not_to have_enqueued_job(ConversationReplyEmailJob) end it 'does not schedule email for private notes' do @@ -363,15 +353,8 @@ RSpec.describe Message do message.private = true message.message_type = 'outgoing' - initial_job_count = ConversationReplyEmailWorker.jobs.count { |job| job['args'].first == message.conversation.id } - - perform_enqueued_jobs do - message.save! - end - - # No new jobs should be scheduled for this conversation - jobs_for_conversation_count = ConversationReplyEmailWorker.jobs.count { |job| job['args'].first == message.conversation.id } - expect(jobs_for_conversation_count).to eq(initial_job_count) + ActiveJob::Base.queue_adapter = :test + expect { message.save! }.not_to have_enqueued_job(ConversationReplyEmailJob) end it 'calls SendReplyJob for all channels' do diff --git a/spec/services/messages/send_email_notification_service_spec.rb b/spec/services/messages/send_email_notification_service_spec.rb index cda728f21..7c0970fe1 100644 --- a/spec/services/messages/send_email_notification_service_spec.rb +++ b/spec/services/messages/send_email_notification_service_spec.rb @@ -14,17 +14,11 @@ describe Messages::SendEmailNotificationService do before do conversation.contact.update!(email: 'test@example.com') allow(Redis::Alfred).to receive(:set).and_return(true) - allow(ConversationReplyEmailWorker).to receive(:perform_in) + ActiveJob::Base.queue_adapter = :test end - it 'schedules ConversationReplyEmailWorker' do - service.perform - - expect(ConversationReplyEmailWorker).to have_received(:perform_in).with( - 2.minutes, - conversation.id, - message.id - ) + it 'enqueues ConversationReplyEmailJob' do + expect { service.perform }.to have_enqueued_job(ConversationReplyEmailJob).with(conversation.id, message.id).on_queue('mailers') end it 'atomically sets redis key to prevent duplicate emails' do @@ -40,10 +34,8 @@ describe Messages::SendEmailNotificationService do allow(Redis::Alfred).to receive(:set).and_return(false) end - it 'does not schedule worker' do - service.perform - - expect(ConversationReplyEmailWorker).not_to have_received(:perform_in) + it 'does not enqueue job' do + expect { service.perform }.not_to have_enqueued_job(ConversationReplyEmailJob) end it 'attempts atomic set once' do @@ -62,7 +54,7 @@ describe Messages::SendEmailNotificationService do conversation.contact.update!(email: 'test@example.com') end - it 'prevents duplicate workers under race conditions' do + it 'prevents duplicate jobs under race conditions' do # Create 5 threads that simultaneously try to enqueue workers for the same conversation threads = Array.new(5) do Thread.new do @@ -73,24 +65,24 @@ describe Messages::SendEmailNotificationService do threads.each(&:join) - # Only ONE worker should be scheduled despite 5 concurrent attempts - jobs_for_conversation = ConversationReplyEmailWorker.jobs.select { |job| job['args'].first == conversation.id } + # Only ONE job should be scheduled despite 5 concurrent attempts + jobs_for_conversation = ActiveJob::Base.queue_adapter.enqueued_jobs.select do |job| + job[:job] == ConversationReplyEmailJob && job[:args].first == conversation.id + end expect(jobs_for_conversation.size).to eq(1) end end context 'when email notification should not be sent' do before do - allow(ConversationReplyEmailWorker).to receive(:perform_in) + ActiveJob::Base.queue_adapter = :test end context 'when message is not email notifiable' do let(:message) { create(:message, conversation: conversation, message_type: 'incoming') } - it 'does not schedule worker' do - service.perform - - expect(ConversationReplyEmailWorker).not_to have_received(:perform_in) + it 'does not enqueue job' do + expect { service.perform }.not_to have_enqueued_job(ConversationReplyEmailJob) end end @@ -102,10 +94,8 @@ describe Messages::SendEmailNotificationService do conversation.contact.update!(email: nil) end - it 'does not schedule worker' do - service.perform - - expect(ConversationReplyEmailWorker).not_to have_received(:perform_in) + it 'does not enqueue job' do + expect { service.perform }.not_to have_enqueued_job(ConversationReplyEmailJob) end end @@ -117,10 +107,8 @@ describe Messages::SendEmailNotificationService do conversation.contact.update!(email: 'test@example.com') end - it 'does not schedule worker' do - service.perform - - expect(ConversationReplyEmailWorker).not_to have_received(:perform_in) + it 'does not enqueue job' do + expect { service.perform }.not_to have_enqueued_job(ConversationReplyEmailJob) end end end diff --git a/spec/workers/conversation_reply_email_worker_spec.rb b/spec/workers/conversation_reply_email_worker_spec.rb deleted file mode 100644 index 7b28eedde..000000000 --- a/spec/workers/conversation_reply_email_worker_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -require 'rails_helper' - -Sidekiq::Testing.fake! -RSpec.describe ConversationReplyEmailWorker, type: :worker do - let(:conversation) { build(:conversation, display_id: nil) } - let(:message) { build(:message, conversation: conversation, content_type: 'incoming_email', inbox: conversation.inbox) } - let(:mailer) { double } - let(:mailer_action) { double } - - describe 'testing ConversationSummaryEmailWorker' do - before do - conversation.save! - allow(Conversation).to receive(:find).and_return(conversation) - allow(ConversationReplyMailer).to receive(:with).and_return(mailer) - allow(ConversationReplyMailer).to receive(:with).and_return(mailer) - allow(mailer).to receive(:reply_with_summary).and_return(mailer_action) - allow(mailer).to receive(:reply_without_summary).and_return(mailer_action) - allow(mailer_action).to receive(:deliver_later).and_return(true) - end - - it 'worker jobs are enqueued in the mailers queue' do - described_class.perform_async - expect(described_class.queue).to eq(:mailers) - end - - it 'goes into the jobs array for testing environment' do - expect do - described_class.perform_async - end.to change(described_class.jobs, :size).by(1) - described_class.new.perform(1, message.id) - end - - context 'with actions performed by the worker' do - it 'calls ConversationSummaryMailer#reply_with_summary when last incoming message was not email' do - described_class.new.perform(1, message.id) - expect(mailer).to have_received(:reply_with_summary) - end - - it 'calls ConversationSummaryMailer#reply_without_summary when last incoming message was from email' do - message.save! - described_class.new.perform(1, message.id) - expect(mailer).to have_received(:reply_without_summary) - end - end - end -end From 9898ccee9eb5fb887b88d6a76063fb687f28940a Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Wed, 22 Oct 2025 20:23:37 -0700 Subject: [PATCH 2/2] chore: Enforce custom role permissions on conversation access (#12583) ## Summary - ensure conversation lookup uses the permission filter before fetching records - add request specs covering custom role access to unassigned conversations ## Testing - bundle exec rspec spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb ------ https://chatgpt.com/codex/tasks/task_e_68de1f62b9b883268a54882e608a8bb8 --- .../accounts/conversations/base_controller.rb | 2 +- .../v1/accounts/conversations_controller.rb | 2 +- .../accounts/integrations/dyte_controller.rb | 2 +- .../concerns/access_token_auth_helper.rb | 1 + app/policies/conversation_policy.rb | 40 ++++++++++- .../enterprise/conversation_policy.rb | 42 +++++++++++ spec/controllers/api/base_controller_spec.rb | 23 ++++++ .../accounts/conversations_controller_spec.rb | 71 +++++++++++++++++++ .../policies/conversation_policy_spec.rb | 65 +++++++++++++++++ spec/policies/conversation_policy_spec.rb | 45 +++++++++++- 10 files changed, 286 insertions(+), 7 deletions(-) create mode 100644 enterprise/app/policies/enterprise/conversation_policy.rb create mode 100644 spec/enterprise/policies/conversation_policy_spec.rb diff --git a/app/controllers/api/v1/accounts/conversations/base_controller.rb b/app/controllers/api/v1/accounts/conversations/base_controller.rb index 500c7772f..223530e27 100644 --- a/app/controllers/api/v1/accounts/conversations/base_controller.rb +++ b/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 diff --git a/app/controllers/api/v1/accounts/conversations_controller.rb b/app/controllers/api/v1/accounts/conversations_controller.rb index e27869d82..4301eaa4a 100644 --- a/app/controllers/api/v1/accounts/conversations_controller.rb +++ b/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 diff --git a/app/controllers/api/v1/accounts/integrations/dyte_controller.rb b/app/controllers/api/v1/accounts/integrations/dyte_controller.rb index c5f795d34..845caab5e 100644 --- a/app/controllers/api/v1/accounts/integrations/dyte_controller.rb +++ b/app/controllers/api/v1/accounts/integrations/dyte_controller.rb @@ -22,7 +22,7 @@ class Api::V1::Accounts::Integrations::DyteController < Api::V1::Accounts::BaseC private def authorize_request - authorize @conversation.inbox, :show? + authorize @conversation, :show? end def render_response(response) diff --git a/app/controllers/concerns/access_token_auth_helper.rb b/app/controllers/concerns/access_token_auth_helper.rb index 9b0f9021f..338b290da 100644 --- a/app/controllers/concerns/access_token_auth_helper.rb +++ b/app/controllers/concerns/access_token_auth_helper.rb @@ -14,6 +14,7 @@ module AccessTokenAuthHelper ensure_access_token render_unauthorized('Invalid Access Token') && return if @access_token.blank? + # NOTE: This ensures that current_user is set and available for the rest of the controller actions @resource = @access_token.owner Current.user = @resource if allowed_current_user_type?(@resource) end diff --git a/app/policies/conversation_policy.rb b/app/policies/conversation_policy.rb index 931e17435..d23f29e54 100644 --- a/app/policies/conversation_policy.rb +++ b/app/policies/conversation_policy.rb @@ -4,6 +4,44 @@ class ConversationPolicy < ApplicationPolicy end def destroy? - @account_user&.administrator? + administrator? + end + + def show? + administrator? || agent_bot? || agent_can_view_conversation? + end + + private + + def agent_can_view_conversation? + inbox_access? || team_access? + end + + def administrator? + account_user&.administrator? + end + + def agent_bot? + user.is_a?(AgentBot) + end + + def inbox_access? + user.inboxes.where(account_id: account&.id).exists?(id: record.inbox_id) + end + + def team_access? + return false if record.team_id.blank? + + user.teams.where(account_id: account&.id).exists?(id: record.team_id) + end + + def assigned_to_user? + record.assignee_id == user.id + end + + def participant? + record.conversation_participants.exists?(user_id: user.id) end end + +ConversationPolicy.prepend_mod_with('ConversationPolicy') diff --git a/enterprise/app/policies/enterprise/conversation_policy.rb b/enterprise/app/policies/enterprise/conversation_policy.rb new file mode 100644 index 000000000..d956db2d7 --- /dev/null +++ b/enterprise/app/policies/enterprise/conversation_policy.rb @@ -0,0 +1,42 @@ +module Enterprise::ConversationPolicy + def show? + return false unless super + return true unless custom_role_permissions? + + permissions = custom_role_permissions + return true if manage_all_conversations?(permissions) + return true if permits_unassigned_manage?(permissions) + + permits_participating?(permissions) + end + + private + + def manage_all_conversations?(permissions) + permissions.include?('conversation_manage') + end + + def permits_unassigned_manage?(permissions) + return false unless permissions.include?('conversation_unassigned_manage') + + unassigned_conversation? || assigned_to_user? + end + + def permits_participating?(permissions) + return false unless permissions.include?('conversation_participating_manage') + + assigned_to_user? || participant? + end + + def unassigned_conversation? + record.assignee_id.nil? + end + + def custom_role_permissions? + account_user&.custom_role_id.present? + end + + def custom_role_permissions + account_user&.custom_role&.permissions || [] + end +end diff --git a/spec/controllers/api/base_controller_spec.rb b/spec/controllers/api/base_controller_spec.rb index ac2306d14..69e4f5cae 100644 --- a/spec/controllers/api/base_controller_spec.rb +++ b/spec/controllers/api/base_controller_spec.rb @@ -5,6 +5,29 @@ RSpec.describe 'API Base', type: :request do let!(:user) { create(:user, account: account) } describe 'request with api_access_token for user' do + context 'when accessing an account scoped resource' do + let!(:admin) { create(:user, :administrator, account: account) } + let!(:conversation) { create(:conversation, account: account) } + + it 'sets Current attributes for the request and then returns the response' do + # expect Current.account_user is set to the admin's account_user + allow(Current).to receive(:user=).and_call_original + allow(Current).to receive(:account=).and_call_original + allow(Current).to receive(:account_user=).and_call_original + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", + headers: { api_access_token: admin.access_token.token }, + as: :json + + expect(Current).to have_received(:user=).with(admin).at_least(:once) + expect(Current).to have_received(:account=).with(account).at_least(:once) + expect(Current).to have_received(:account_user=).with(admin.account_users.first).at_least(:once) + + expect(response).to have_http_status(:success) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + end + context 'when it is an invalid api_access_token' do it 'returns unauthorized' do get '/api/v1/profile', diff --git a/spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb b/spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb index 4d5269cde..bbb3f1eb3 100644 --- a/spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb +++ b/spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb @@ -30,5 +30,76 @@ RSpec.describe 'Conversations API', type: :request do expect(response.parsed_body.keys).not_to include('applied_sla') expect(response.parsed_body.keys).not_to include('sla_events') end + + context 'when agent has team access' do + let(:agent) { create(:user, account: account, role: :agent) } + let(:team) { create(:team, account: account) } + let(:conversation) { create(:conversation, account: account, team: team) } + + before do + create(:team_member, team: team, user: agent) + end + + it 'allows accessing the conversation via team membership' do + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + end + + context 'when agent has a custom role' do + let(:agent) { create(:user, account: account, role: :agent) } + let(:conversation) { create(:conversation, account: account) } + + before do + create(:inbox_member, user: agent, inbox: conversation.inbox) + end + + it 'returns unauthorized for unassigned conversation without permission' do + custom_role = create(:custom_role, account: account, permissions: ['conversation_participating_manage']) + account.account_users.find_by(user_id: agent.id).update!(custom_role: custom_role) + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:unauthorized) + end + + it 'returns the conversation when permission allows managing unassigned conversations, including when assigned to agent' do + custom_role = create(:custom_role, account: account, permissions: ['conversation_unassigned_manage']) + account_user = account.account_users.find_by(user_id: agent.id) + account_user.update!(custom_role: custom_role) + conversation.update!(assignee: agent) + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + + it 'returns the conversation when permission allows managing assigned conversations' do + custom_role = create(:custom_role, account: account, permissions: ['conversation_participating_manage']) + account_user = account.account_users.find_by(user_id: agent.id) + account_user.update!(custom_role: custom_role) + conversation.update!(assignee: agent) + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + + it 'returns the conversation when permission allows managing participating conversations' do + custom_role = create(:custom_role, account: account, permissions: ['conversation_participating_manage']) + account_user = account.account_users.find_by(user_id: agent.id) + account_user.update!(custom_role: custom_role) + create(:conversation_participant, conversation: conversation, account: account, user: agent) + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + end end end diff --git a/spec/enterprise/policies/conversation_policy_spec.rb b/spec/enterprise/policies/conversation_policy_spec.rb new file mode 100644 index 000000000..e48a84852 --- /dev/null +++ b/spec/enterprise/policies/conversation_policy_spec.rb @@ -0,0 +1,65 @@ +require 'rails_helper' + +RSpec.describe ConversationPolicy, type: :policy do + subject { described_class } + + let(:account) { create(:account) } + let(:agent) { create(:user, account: account, role: :agent) } + let(:inbox) { create(:inbox, account: account) } + let(:agent_account_user) { agent.account_users.find_by(account: account) } + let(:context) { { user: agent, account: account, account_user: agent_account_user } } + + before do + create(:inbox_member, user: agent, inbox: inbox) + end + + permissions :show? do + context 'when role grants conversation_unassigned_manage' do + let(:custom_role) { create(:custom_role, account: account, permissions: ['conversation_unassigned_manage']) } + + before do + agent_account_user.update!(role: :agent, custom_role: custom_role) + end + + it 'allows access to conversations assigned to the agent' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: agent) + + expect(subject).to permit(context, conversation) + end + + it 'denies access to conversations assigned to someone else' do + other_agent = create(:user, account: account, role: :agent) + conversation = create(:conversation, account: account, inbox: inbox, assignee: other_agent) + + expect(subject).not_to permit(context, conversation) + end + end + + context 'when role grants conversation_participating_manage' do + let(:custom_role) { create(:custom_role, account: account, permissions: ['conversation_participating_manage']) } + + before do + agent_account_user.update!(role: :agent, custom_role: custom_role) + end + + it 'allows access to conversations assigned to the agent' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: agent) + + expect(subject).to permit(context, conversation) + end + + it 'allows access to conversations where the agent is a participant' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: nil) + create(:conversation_participant, conversation: conversation, account: account, user: agent) + + expect(subject).to permit(context, conversation) + end + + it 'denies access to unrelated conversations' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: nil) + + expect(subject).not_to permit(context, conversation) + end + end + end +end diff --git a/spec/policies/conversation_policy_spec.rb b/spec/policies/conversation_policy_spec.rb index ecc3134fc..d75a6bc3a 100644 --- a/spec/policies/conversation_policy_spec.rb +++ b/spec/policies/conversation_policy_spec.rb @@ -4,11 +4,12 @@ RSpec.describe ConversationPolicy, type: :policy do subject { described_class } let(:account) { create(:account) } - let(:conversation) { create(:conversation, account: account) } let(:administrator) { create(:user, account: account, role: :administrator) } let(:agent) { create(:user, account: account, role: :agent) } - let(:administrator_context) { { user: administrator, account: account, account_user: administrator.account_users.first } } - let(:agent_context) { { user: agent, account: account, account_user: agent.account_users.first } } + let(:administrator_context) { { user: administrator, account: account, account_user: administrator.account_users.find_by(account: account) } } + let(:agent_context) { { user: agent, account: account, account_user: agent.account_users.find_by(account: account) } } + + let(:conversation) { create(:conversation, account: account) } permissions :destroy? do context 'when user is an administrator' do @@ -31,4 +32,42 @@ RSpec.describe ConversationPolicy, type: :policy do end end end + + permissions :show? do + context 'when user is an administrator' do + it 'allows access' do + expect(subject).to permit(administrator_context, conversation) + end + end + + context 'when agent has inbox access' do + let(:inbox) { create(:inbox, account: account) } + let(:conversation) { create(:conversation, account: account, inbox: inbox) } + + before { create(:inbox_member, user: agent, inbox: inbox) } + + it 'allows access' do + expect(subject).to permit(agent_context, conversation) + end + end + + context 'when agent has team access' do + let(:team) { create(:team, account: account) } + let(:conversation) { create(:conversation, :with_team, account: account, team: team) } + + before { create(:team_member, team: team, user: agent) } + + it 'allows access' do + expect(subject).to permit(agent_context, conversation) + end + end + + context 'when agent lacks inbox and team access' do + let(:conversation) { create(:conversation, account: account) } + + it 'denies access' do + expect(subject).not_to permit(agent_context, conversation) + end + end + end end