diff --git a/app/controllers/api/v1/accounts/inboxes_controller.rb b/app/controllers/api/v1/accounts/inboxes_controller.rb index b7c28c1c3..757af9b62 100644 --- a/app/controllers/api/v1/accounts/inboxes_controller.rb +++ b/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 diff --git a/app/controllers/api/v1/accounts/notifications_controller.rb b/app/controllers/api/v1/accounts/notifications_controller.rb index 52035ce64..bb5c57ca6 100644 --- a/app/controllers/api/v1/accounts/notifications_controller.rb +++ b/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 diff --git a/app/controllers/api/v1/accounts/portals_controller.rb b/app/controllers/api/v1/accounts/portals_controller.rb index ade83d8ec..6d3332d08 100644 --- a/app/controllers/api/v1/accounts/portals_controller.rb +++ b/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 } diff --git a/app/controllers/api/v1/notification_subscriptions_controller.rb b/app/controllers/api/v1/notification_subscriptions_controller.rb index 1a797a74d..bb20c64ae 100644 --- a/app/controllers/api/v1/notification_subscriptions_controller.rb +++ b/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 diff --git a/app/controllers/api/v1/widget/messages_controller.rb b/app/controllers/api/v1/widget/messages_controller.rb index 83b3dc8b1..8b1e8ef48 100644 --- a/app/controllers/api/v1/widget/messages_controller.rb +++ b/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 diff --git a/app/controllers/public/api/v1/inboxes_controller.rb b/app/controllers/public/api/v1/inboxes_controller.rb index 65fad57b1..5679a4a3e 100644 --- a/app/controllers/public/api/v1/inboxes_controller.rb +++ b/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 diff --git a/app/jobs/bulk_actions_job.rb b/app/jobs/bulk_actions_job.rb index 73fa1b151..c12a507a3 100644 --- a/app/jobs/bulk_actions_job.rb +++ b/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 diff --git a/app/jobs/notification/delete_notification_job.rb b/app/jobs/notification/delete_notification_job.rb index 9efaf472c..2791b6c71 100644 --- a/app/jobs/notification/delete_notification_job.rb +++ b/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 diff --git a/spec/controllers/api/v1/accounts/bulk_actions_controller_spec.rb b/spec/controllers/api/v1/accounts/bulk_actions_controller_spec.rb index 1ab490a96..7ee763dbc 100644 --- a/spec/controllers/api/v1/accounts/bulk_actions_controller_spec.rb +++ b/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]) diff --git a/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb b/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb index 0372bb141..9e03e2587 100644 --- a/spec/controllers/api/v1/accounts/inboxes_controller_spec.rb +++ b/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 diff --git a/spec/controllers/api/v1/accounts/notifications_controller_spec.rb b/spec/controllers/api/v1/accounts/notifications_controller_spec.rb index 9b16e48fb..4fb1e217f 100644 --- a/spec/controllers/api/v1/accounts/notifications_controller_spec.rb +++ b/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, diff --git a/spec/controllers/api/v1/accounts/portals_controller_spec.rb b/spec/controllers/api/v1/accounts/portals_controller_spec.rb index 9c780c00a..4c45e4f0f 100644 --- a/spec/controllers/api/v1/accounts/portals_controller_spec.rb +++ b/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) diff --git a/spec/controllers/api/v1/notification_subscriptions_controller_spec.rb b/spec/controllers/api/v1/notification_subscriptions_controller_spec.rb index cb4366019..ea74a871c 100644 --- a/spec/controllers/api/v1/notification_subscriptions_controller_spec.rb +++ b/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 diff --git a/spec/controllers/api/v1/widget/messages_controller_spec.rb b/spec/controllers/api/v1/widget/messages_controller_spec.rb index 902b4ec01..3d4ec83ca 100644 --- a/spec/controllers/api/v1/widget/messages_controller_spec.rb +++ b/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) diff --git a/spec/controllers/public/api/v1/inbox/messages_controller_spec.rb b/spec/controllers/public/api/v1/inbox/messages_controller_spec.rb index a7e055b19..c6650511b 100644 --- a/spec/controllers/public/api/v1/inbox/messages_controller_spec.rb +++ b/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 diff --git a/spec/jobs/bulk_actions_job_spec.rb b/spec/jobs/bulk_actions_job_spec.rb index 3b0ff8133..ad8ebee6a 100644 --- a/spec/jobs/bulk_actions_job_spec.rb +++ b/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 diff --git a/spec/jobs/notification/delete_notification_job_spec.rb b/spec/jobs/notification/delete_notification_job_spec.rb index 194a24732..a61ef5cb9 100644 --- a/spec/jobs/notification/delete_notification_job_spec.rb +++ b/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