fix: incorrect scope across controllers (#14459)

Co-authored-by: Sojan Jose <sojan@pepalo.com>

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

@@ -87,7 +87,7 @@ class Api::V1::Accounts::InboxesController < Api::V1::Accounts::BaseController
   end
 
   def fetch_agent_bot
-    @agent_bot = AgentBot.find(params[:agent_bot]) if params[:agent_bot]
+    @agent_bot = AgentBot.accessible_to(Current.account).find(params[:agent_bot]) if params[:agent_bot]
   end
 
   def create_channel

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

@@ -41,9 +41,9 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro
 
   def destroy_all
     if params[:type] == 'read'
-      ::Notification::DeleteNotificationJob.perform_later(Current.user, type: :read)
+      ::Notification::DeleteNotificationJob.perform_later(Current.user, Current.account, type: :read)
     else
-      ::Notification::DeleteNotificationJob.perform_later(Current.user, type: :all)
+      ::Notification::DeleteNotificationJob.perform_later(Current.user, Current.account, type: :all)
     end
     head :ok
   end
@@ -69,7 +69,7 @@ class Api::V1::Accounts::NotificationsController < Api::V1::Accounts::BaseContro
   end
 
   def fetch_notification
-    @notification = current_user.notifications.find(params[:id])
+    @notification = current_user.notifications.where(account_id: Current.account.id).find(params[:id])
   end
 
   def set_current_page

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

@@ -87,7 +87,7 @@ class Api::V1::Accounts::PortalsController < Api::V1::Accounts::BaseController
     return {} unless permitted_params.key?(:inbox_id)
     return { channel_web_widget_id: nil } if permitted_params[:inbox_id].blank?
 
-    inbox = Inbox.find(permitted_params[:inbox_id])
+    inbox = Current.account.inboxes.find(permitted_params[:inbox_id])
     return {} unless inbox.web_widget?
 
     { channel_web_widget_id: inbox.channel.id }

app/controllers/api/v1/notification_subscriptions_controller.rb

@@ -8,7 +8,8 @@ class Api::V1::NotificationSubscriptionsController < Api::BaseController
   end
 
   def destroy
-    notification_subscription = NotificationSubscription.where(["subscription_attributes->>'push_token' = ?", params[:push_token]]).first
+    notification_subscription = current_user.notification_subscriptions
+                                            .where(["subscription_attributes->>'push_token' = ?", params[:push_token]]).first
     notification_subscription.destroy! if notification_subscription.present?
     head :ok
   end

app/controllers/api/v1/widget/messages_controller.rb

@@ -83,6 +83,10 @@ class Api::V1::Widget::MessagesController < Api::V1::Widget::BaseController
   end
 
   def set_message
-    @message = @web_widget.inbox.messages.find(permitted_params[:id])
+    # `conversation.messages.find` would be simpler, but `conversation` is `conversations.last`,
+    # which means a visitor with more than one open thread could not edit a message in any
+    # but their most recent one. Scoping across all of the visitor's conversations keeps the
+    # happy path correct for that future multi-conversation widget flow.
+    @message = Message.where(conversation_id: conversations.select(:id)).find(permitted_params[:id])
   end
 end

app/controllers/public/api/v1/inboxes_controller.rb

@@ -24,6 +24,10 @@ class Public::Api::V1::InboxesController < PublicController
   def set_conversation
     return if params[:conversation_id].blank?
 
-    @conversation = @contact_inbox.contact.conversations.find_by!(display_id: params[:conversation_id])
+    @conversation = if @contact_inbox.hmac_verified?
+                      @contact_inbox.contact.conversations.find_by!(display_id: params[:conversation_id])
+                    else
+                      @contact_inbox.conversations.find_by!(display_id: params[:conversation_id])
+                    end
   end
 end

app/jobs/bulk_actions_job.rb

@@ -8,6 +8,7 @@ class BulkActionsJob < ApplicationJob
 
   def perform(account:, params:, user:)
     @account = account
+    @user = user
     Current.user = user
     @params = params
     @records = records_to_updated(params[:ids])
@@ -61,6 +62,7 @@ class BulkActionsJob < ApplicationJob
     current_model = @params[:type].camelcase
     return unless MODEL_TYPE.include?(current_model)
 
-    current_model.constantize&.where(account_id: @account.id, display_id: ids)
+    scope = current_model.constantize.where(account_id: @account.id, display_id: ids)
+    Conversations::PermissionFilterService.new(scope, @user, @account).perform
   end
 end

app/jobs/notification/delete_notification_job.rb

@@ -1,14 +1,14 @@
 class Notification::DeleteNotificationJob < ApplicationJob
   queue_as :low
 
-  def perform(user, type: :all)
+  def perform(user, account, type: :all)
+    notifications = user.notifications.where(account_id: account.id)
+
     ActiveRecord::Base.transaction do
       if type == :all
-        # Delete all notifications
-        user.notifications.destroy_all
+        notifications.destroy_all
       elsif type == :read
-        # Delete only read notifications
-        user.notifications.where.not(read_at: nil).destroy_all
+        notifications.where.not(read_at: nil).destroy_all
       end
     end
   end

spec/controllers/api/v1/accounts/bulk_actions_controller_spec.rb

@@ -34,6 +34,10 @@ RSpec.describe 'Api::V1::Accounts::BulkActionsController', type: :request do
     context 'when it is an authenticated user' do
       let!(:agent) { create(:user, account: account, role: :agent) }
 
+      before do
+        Conversation.all.find_each { |conversation| create(:inbox_member, inbox: conversation.inbox, user: agent) }
+      end
+
       it 'Ignores bulk_actions for wrong type' do
         post "/api/v1/accounts/#{account.id}/bulk_actions",
              headers: agent.create_new_auth_token,
@@ -202,6 +206,10 @@ RSpec.describe 'Api::V1::Accounts::BulkActionsController', type: :request do
     context 'when it is an authenticated user' do
       let!(:agent) { create(:user, account: account, role: :agent) }
 
+      before do
+        Conversation.all.find_each { |conversation| create(:inbox_member, inbox: conversation.inbox, user: agent) }
+      end
+
       it 'Bulk delete conversation labels' do
         Conversation.first.add_labels(%w[support priority_customer])
         Conversation.second.add_labels(%w[support priority_customer])

spec/controllers/api/v1/accounts/inboxes_controller_spec.rb

@@ -1008,6 +1008,19 @@ RSpec.describe 'Inboxes API', type: :request do
 
         expect(response).to have_http_status(:unauthorized)
       end
+
+      it 'does not allow binding an agent bot from another account' do
+        other_account = create(:account)
+        foreign_bot = create(:agent_bot, account: other_account)
+
+        post "/api/v1/accounts/#{account.id}/inboxes/#{inbox.id}/set_agent_bot",
+             headers: admin.create_new_auth_token,
+             params: { agent_bot: foreign_bot.id },
+             as: :json
+
+        expect(response).to have_http_status(:not_found)
+        expect(inbox.reload.agent_bot).to be_nil
+      end
     end
   end
 

spec/controllers/api/v1/accounts/notifications_controller_spec.rb

@@ -101,6 +101,20 @@ RSpec.describe 'Notifications API', type: :request do
         expect(response).to have_http_status(:success)
         expect(notification.reload.read_at).not_to eq('')
       end
+
+      it 'does not update a notification reached via a different account that the user belongs to' do
+        other_account = create(:account)
+        create(:account_user, account: other_account, user: admin, role: :administrator)
+        original_read_at = notification.read_at
+
+        patch "/api/v1/accounts/#{other_account.id}/notifications/#{notification.id}",
+              headers: admin.create_new_auth_token,
+              params: { read_at: true },
+              as: :json
+
+        expect(response).to have_http_status(:not_found)
+        expect(notification.reload.read_at).to eq(original_read_at)
+      end
     end
   end
 
@@ -227,7 +241,7 @@ RSpec.describe 'Notifications API', type: :request do
       let(:admin) { create(:user, account: account, role: :administrator) }
 
       it 'deletes all the read notifications' do
-        expect(Notification::DeleteNotificationJob).to receive(:perform_later).with(admin, type: :read)
+        expect(Notification::DeleteNotificationJob).to receive(:perform_later).with(admin, account, type: :read)
 
         post "/api/v1/accounts/#{account.id}/notifications/destroy_all",
              headers: admin.create_new_auth_token,
@@ -238,7 +252,7 @@ RSpec.describe 'Notifications API', type: :request do
       end
 
       it 'deletes all the notifications' do
-        expect(Notification::DeleteNotificationJob).to receive(:perform_later).with(admin, type: :all)
+        expect(Notification::DeleteNotificationJob).to receive(:perform_later).with(admin, account, type: :all)
 
         post "/api/v1/accounts/#{account.id}/notifications/destroy_all",
              headers: admin.create_new_auth_token,

spec/controllers/api/v1/accounts/portals_controller_spec.rb

@@ -192,6 +192,21 @@ RSpec.describe 'Api::V1::Accounts::Portals', type: :request do
         expect(portal.reload.logo).to be_attached
       end
 
+      it 'does not allow associating an inbox from another account' do
+        other_account = create(:account)
+        foreign_inbox = create(:inbox, account: other_account)
+
+        put "/api/v1/accounts/#{account.id}/portals/#{portal.slug}",
+            params: {
+              portal: { name: portal.name },
+              inbox_id: foreign_inbox.id
+            },
+            headers: admin.create_new_auth_token
+
+        expect(response).to have_http_status(:not_found)
+        expect(portal.reload.channel_web_widget_id).to be_nil
+      end
+
       it 'clears associated web widget when inbox selection is blank' do
         web_widget_inbox = create(:inbox, account: account)
         portal.update!(channel_web_widget: web_widget_inbox.channel)

spec/controllers/api/v1/notification_subscriptions_controller_spec.rb

@@ -106,6 +106,21 @@ RSpec.describe 'Notifications Subscriptions API', type: :request do
         expect(response).to have_http_status(:success)
         expect { subscription.reload }.to raise_exception(ActiveRecord::RecordNotFound)
       end
+
+      it 'does not delete another user notification subscription with the same push token' do
+        victim = create(:user, account: account, role: :agent)
+        victim_subscription = create(:notification_subscription, subscription_type: 'fcm',
+                                                                 subscription_attributes: { push_token: 'victimToken' },
+                                                                 user: victim)
+
+        delete '/api/v1/notification_subscriptions',
+               params: { push_token: 'victimToken' },
+               headers: agent.create_new_auth_token,
+               as: :json
+
+        expect(response).to have_http_status(:success)
+        expect { victim_subscription.reload }.not_to raise_error
+      end
     end
   end
 end

spec/controllers/api/v1/widget/messages_controller_spec.rb

@@ -212,6 +212,26 @@ RSpec.describe '/api/v1/widget/messages', type: :request do
   end
 
   describe 'PUT /api/v1/widget/messages' do
+    context 'when put request targets a message from another visitor in the same inbox' do
+      it 'does not update the foreign message' do
+        other_contact = create(:contact, account: account, email: nil)
+        other_contact_inbox = create(:contact_inbox, contact: other_contact, inbox: web_widget.inbox)
+        other_conversation = create(:conversation, contact: other_contact, account: account,
+                                                   inbox: web_widget.inbox, contact_inbox: other_contact_inbox)
+        foreign_message = create(:message, content_type: 'input_email', account: account,
+                                           inbox: web_widget.inbox, conversation: other_conversation)
+        original_email = foreign_message.submitted_email
+
+        put api_v1_widget_message_url(foreign_message.id),
+            params: { website_token: web_widget.website_token, contact: { email: Faker::Internet.email } },
+            headers: { 'X-Auth-Token' => token },
+            as: :json
+
+        expect(response).to have_http_status(:not_found)
+        expect(foreign_message.reload.submitted_email).to eq(original_email)
+      end
+    end
+
     context 'when put request is made with non existing email' do
       it 'updates message in conversation and creates a new contact' do
         message = create(:message, content_type: 'input_email', account: account, inbox: web_widget.inbox, conversation: conversation)

spec/controllers/public/api/v1/inbox/messages_controller_spec.rb

@@ -16,6 +16,19 @@ RSpec.describe 'Public Inbox Contact Conversation Messages API', type: :request
       data = response.parsed_body
       expect(data.length).to eq 2
     end
+
+    it 'does not return messages from a conversation in another inbox even when both share the same contact' do
+      other_channel = create(:channel_api, account: conversation.account)
+      other_contact_inbox = create(:contact_inbox, contact: contact, inbox: other_channel.inbox)
+      foreign_conversation = create(:conversation, contact: contact, account: conversation.account,
+                                                   inbox: other_channel.inbox, contact_inbox: other_contact_inbox)
+      create(:message, account: foreign_conversation.account, inbox: foreign_conversation.inbox, conversation: foreign_conversation)
+
+      get "/public/api/v1/inboxes/#{api_channel.identifier}/contacts/#{contact_inbox.source_id}/conversations/" \
+          "#{foreign_conversation.display_id}/messages"
+
+      expect(response).to have_http_status(:not_found)
+    end
   end
 
   describe 'POST /public/api/v1/inboxes/{identifier}/contact/{source_id}/conversations/{conversation_id}/messages' do

spec/jobs/bulk_actions_job_spec.rb

@@ -1,12 +1,6 @@
 require 'rails_helper'
 
 RSpec.describe BulkActionsJob do
-  params = {
-    type: 'Conversation',
-    fields: { status: 'snoozed' },
-    ids: Conversation.first(3).pluck(:display_id)
-  }
-
   subject(:job) { described_class.perform_later(account: account, params: params, user: agent) }
 
   let(:account) { create(:account) }
@@ -14,9 +8,11 @@ RSpec.describe BulkActionsJob do
   let!(:conversation_1) { create(:conversation, account_id: account.id, status: :open) }
   let!(:conversation_2) { create(:conversation, account_id: account.id, status: :open) }
   let!(:conversation_3) { create(:conversation, account_id: account.id, status: :open) }
+  let(:conversation_ids) { [conversation_1.display_id, conversation_2.display_id, conversation_3.display_id] }
+  let(:params) { { type: 'Conversation', fields: { status: 'snoozed' }, ids: conversation_ids } }
 
   before do
-    Conversation.all.find_each do |conversation|
+    [conversation_1, conversation_2, conversation_3].each do |conversation|
       create(:inbox_member, inbox: conversation.inbox, user: agent)
     end
   end
@@ -38,10 +34,10 @@ RSpec.describe BulkActionsJob do
       params = {
         type: 'Conversation',
         fields: { status: 'snoozed', assignee_id: agent.id },
-        ids: Conversation.first(3).pluck(:display_id)
+        ids: conversation_ids
       }
 
-      expect(Conversation.first.status).to eq('open')
+      expect(conversation_1.status).to eq('open')
 
       described_class.perform_now(account: account, params: params, user: agent)
 
@@ -54,32 +50,46 @@ RSpec.describe BulkActionsJob do
       params = {
         type: 'Conversation',
         fields: { status: 'snoozed', assignee_id: agent.id },
-        ids: Conversation.first(3).pluck(:display_id)
+        ids: conversation_ids
       }
 
-      expect(Conversation.first.assignee_id).to be_nil
+      expect(conversation_1.assignee_id).to be_nil
 
       described_class.perform_now(account: account, params: params, user: agent)
 
-      expect(Conversation.first.assignee_id).to eq(agent.id)
-      expect(Conversation.second.assignee_id).to eq(agent.id)
-      expect(Conversation.third.assignee_id).to eq(agent.id)
+      expect(conversation_1.reload.assignee_id).to eq(agent.id)
+      expect(conversation_2.reload.assignee_id).to eq(agent.id)
+      expect(conversation_3.reload.assignee_id).to eq(agent.id)
     end
 
     it 'bulk updates the snoozed_until' do
       params = {
         type: 'Conversation',
         fields: { status: 'snoozed', snoozed_until: Time.zone.now },
-        ids: Conversation.first(3).pluck(:display_id)
+        ids: conversation_ids
       }
 
-      expect(Conversation.first.snoozed_until).to be_nil
+      expect(conversation_1.snoozed_until).to be_nil
 
       described_class.perform_now(account: account, params: params, user: agent)
 
-      expect(Conversation.first.snoozed_until).to be_present
-      expect(Conversation.second.snoozed_until).to be_present
-      expect(Conversation.third.snoozed_until).to be_present
+      expect(conversation_1.reload.snoozed_until).to be_present
+      expect(conversation_2.reload.snoozed_until).to be_present
+      expect(conversation_3.reload.snoozed_until).to be_present
+    end
+
+    it 'skips conversations whose inbox the agent does not belong to' do
+      forbidden_conversation = create(:conversation, account_id: account.id, status: :open)
+      params = {
+        type: 'Conversation',
+        fields: { status: 'resolved' },
+        ids: [conversation_1.display_id, forbidden_conversation.display_id]
+      }
+
+      described_class.perform_now(account: account, params: params, user: agent)
+
+      expect(conversation_1.reload.status).to eq('resolved')
+      expect(forbidden_conversation.reload.status).to eq('open')
     end
   end
 end

spec/jobs/notification/delete_notification_job_spec.rb

@@ -2,37 +2,45 @@ require 'rails_helper'
 
 RSpec.describe Notification::DeleteNotificationJob do
   let(:user) { create(:user) }
+  let(:account) { create(:account) }
   let(:conversation) { create(:conversation) }
 
   context 'when enqueuing the job' do
     it 'enqueues the job to delete all notifications' do
       expect do
-        described_class.perform_later(user.id, type: :all)
+        described_class.perform_later(user, account, type: :all)
       end.to have_enqueued_job(described_class).on_queue('low')
     end
 
     it 'enqueues the job to delete read notifications' do
       expect do
-        described_class.perform_later(user.id, type: :read)
+        described_class.perform_later(user, account, type: :read)
       end.to have_enqueued_job(described_class).on_queue('low')
     end
   end
 
   context 'when performing the job' do
+    let(:other_account) { create(:account) }
+
     before do
-      create(:notification, user: user, read_at: nil)
-      create(:notification, user: user, read_at: Time.current)
+      create(:notification, account: account, user: user, read_at: nil)
+      create(:notification, account: account, user: user, read_at: Time.current)
+      create(:notification, account: other_account, user: user, read_at: Time.current)
     end
 
-    it 'deletes all notifications' do
-      described_class.perform_now(user, type: :all)
-      expect(user.notifications.count).to eq(0)
+    it 'deletes all notifications for the requested account' do
+      described_class.perform_now(user, account, type: :all)
+
+      expect(user.notifications.where(account_id: account.id).count).to eq(0)
+      expect(user.notifications.where(account_id: other_account.id).count).to eq(1)
     end
 
-    it 'deletes only read notifications' do
-      described_class.perform_now(user, type: :read)
-      expect(user.notifications.count).to eq(1)
-      expect(user.notifications.where(read_at: nil).count).to eq(1)
+    it 'deletes only read notifications for the requested account' do
+      described_class.perform_now(user, account, type: :read)
+
+      expect(user.notifications.where(account_id: account.id).count).to eq(1)
+      expect(user.notifications.where(account_id: account.id, read_at: nil).count).to eq(1)
+      expect(user.notifications.where(account_id: other_account.id).count).to eq(1)
     end
   end
 end