From d35580155506075ed55c86b923f91799162041bc Mon Sep 17 00:00:00 2001 From: Pranav Date: Fri, 21 Mar 2025 18:04:46 -0700 Subject: [PATCH] fix: Do not allow sending messages if merged contact has a duplicate session (#11152) In this PR https://github.com/chatwoot/chatwoot/pull/11139, if there is an attempt to create a duplication session for the contact in the same inbox, we will anonymize the old session. This PR would prevent sending messages to the older sessions. The support agents will have to create a new conversation to continue messages with customer. --- app/builders/contact_inbox_builder.rb | 51 +------- .../contacts/contact_inboxes_controller.rb | 2 + .../helpers/composeConversationHelper.js | 5 + .../specs/composeConversationHelper.spec.js | 19 ++- app/services/base/send_on_channel_service.rb | 25 ++++ .../contact_inbox/source_id_service.rb | 55 ++++++++ config/locales/en.yml | 3 + spec/builders/contact_inbox_builder_spec.rb | 10 +- .../contact_inbox/source_id_service_spec.rb | 117 ++++++++++++++++++ spec/services/sms/send_on_sms_service_spec.rb | 5 +- .../twilio/send_on_twilio_service_spec.rb | 8 +- .../whatsapp/send_on_whatsapp_service_spec.rb | 32 +++-- 12 files changed, 260 insertions(+), 72 deletions(-) create mode 100644 app/services/contact_inbox/source_id_service.rb create mode 100644 spec/services/contact_inbox/source_id_service_spec.rb diff --git a/app/builders/contact_inbox_builder.rb b/app/builders/contact_inbox_builder.rb index ffa45db2e..4d51700a8 100644 --- a/app/builders/contact_inbox_builder.rb +++ b/app/builders/contact_inbox_builder.rb @@ -12,50 +12,11 @@ class ContactInboxBuilder private def generate_source_id - case @inbox.channel_type - when 'Channel::TwilioSms' - twilio_source_id - when 'Channel::Whatsapp' - wa_source_id - when 'Channel::Email' - email_source_id - when 'Channel::Sms' - phone_source_id - when 'Channel::Api', 'Channel::WebWidget' - SecureRandom.uuid - else - raise "Unsupported operation for this channel: #{@inbox.channel_type}" - end - end - - def email_source_id - raise ActionController::ParameterMissing, 'contact email' unless @contact.email - - @contact.email - end - - def phone_source_id - raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number - - @contact.phone_number - end - - def wa_source_id - raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number - - # whatsapp doesn't want the + in e164 format - @contact.phone_number.delete('+').to_s - end - - def twilio_source_id - raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number - - case @inbox.channel.medium - when 'sms' - @contact.phone_number - when 'whatsapp' - "whatsapp:#{@contact.phone_number}" - end + ContactInbox::SourceIdService.new( + contact: @contact, + channel_type: @inbox.channel_type, + medium: @inbox.channel.try(:medium) + ).generate end def create_contact_inbox @@ -91,7 +52,7 @@ class ContactInboxBuilder def new_source_id if @inbox.whatsapp? || @inbox.sms? || @inbox.twilio? - "whatsapp:#{@source_id}#{rand(100)}" + "#{@source_id}#{rand(100)}" else "#{rand(10)}#{@source_id}" end diff --git a/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb b/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb index d985c8a73..bde9a4f0c 100644 --- a/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb +++ b/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb @@ -9,6 +9,8 @@ class Api::V1::Accounts::Contacts::ContactInboxesController < Api::V1::Accounts: source_id: params[:source_id], hmac_verified: hmac_verified? ).perform + rescue ArgumentError => e + render json: { error: e.message }, status: :unprocessable_entity end private diff --git a/app/javascript/dashboard/components-next/NewConversation/helpers/composeConversationHelper.js b/app/javascript/dashboard/components-next/NewConversation/helpers/composeConversationHelper.js index 57e819245..45abef1e0 100644 --- a/app/javascript/dashboard/components-next/NewConversation/helpers/composeConversationHelper.js +++ b/app/javascript/dashboard/components-next/NewConversation/helpers/composeConversationHelper.js @@ -25,6 +25,11 @@ export const generateLabelForContactableInboxesList = ({ channelType === INBOX_TYPES.TWILIO || channelType === INBOX_TYPES.WHATSAPP ) { + // Handled separately for Twilio Inbox where phone number is not mandatory. + // You can send message to a contact with Messaging Service Id. + if (!phoneNumber) { + return name; + } return `${name} (${phoneNumber})`; } return name; diff --git a/app/javascript/dashboard/components-next/NewConversation/helpers/specs/composeConversationHelper.spec.js b/app/javascript/dashboard/components-next/NewConversation/helpers/specs/composeConversationHelper.spec.js index 3de1fad0d..37ae5fe29 100644 --- a/app/javascript/dashboard/components-next/NewConversation/helpers/specs/composeConversationHelper.spec.js +++ b/app/javascript/dashboard/components-next/NewConversation/helpers/specs/composeConversationHelper.spec.js @@ -8,8 +8,8 @@ vi.mock('dashboard/api/contacts'); describe('composeConversationHelper', () => { describe('generateLabelForContactableInboxesList', () => { const contact = { - name: 'John Doe', - email: 'john@example.com', + name: 'Priority Inbox', + email: 'hello@example.com', phoneNumber: '+1234567890', }; @@ -19,7 +19,7 @@ describe('composeConversationHelper', () => { ...contact, channelType: INBOX_TYPES.EMAIL, }) - ).toBe('John Doe (john@example.com)'); + ).toBe('Priority Inbox (hello@example.com)'); }); it('generates label for twilio inbox', () => { @@ -28,7 +28,14 @@ describe('composeConversationHelper', () => { ...contact, channelType: INBOX_TYPES.TWILIO, }) - ).toBe('John Doe (+1234567890)'); + ).toBe('Priority Inbox (+1234567890)'); + + expect( + helpers.generateLabelForContactableInboxesList({ + name: 'Priority Inbox', + channelType: INBOX_TYPES.TWILIO, + }) + ).toBe('Priority Inbox'); }); it('generates label for whatsapp inbox', () => { @@ -37,7 +44,7 @@ describe('composeConversationHelper', () => { ...contact, channelType: INBOX_TYPES.WHATSAPP, }) - ).toBe('John Doe (+1234567890)'); + ).toBe('Priority Inbox (+1234567890)'); }); it('generates label for other inbox types', () => { @@ -46,7 +53,7 @@ describe('composeConversationHelper', () => { ...contact, channelType: 'Channel::Api', }) - ).toBe('John Doe'); + ).toBe('Priority Inbox'); }); }); diff --git a/app/services/base/send_on_channel_service.rb b/app/services/base/send_on_channel_service.rb index 298f60962..58f225ef0 100644 --- a/app/services/base/send_on_channel_service.rb +++ b/app/services/base/send_on_channel_service.rb @@ -12,8 +12,10 @@ class Base::SendOnChannelService def perform validate_target_channel + return unless outgoing_message? return if invalid_message? + return if invalid_source_id? perform_reply end @@ -49,6 +51,29 @@ class Base::SendOnChannelService message.private? || outgoing_message_originated_from_channel? end + def invalid_source_id? + return false unless channels_to_validate? + + return false if contact_inbox.source_id == expected_source_id + + message.update!(status: :failed, external_error: I18n.t('errors.channel_service.invalid_source_id')) + true + end + + def expected_source_id + ContactInbox::SourceIdService.new( + contact: contact, + channel_type: inbox.channel_type, + medium: inbox.channel.try(:medium) + ).generate + rescue ArgumentError + nil + end + + def channels_to_validate? + inbox.sms? || inbox.whatsapp? || inbox.email? || inbox.twilio? + end + def validate_target_channel raise 'Invalid channel service was called' if inbox.channel.class != channel_class end diff --git a/app/services/contact_inbox/source_id_service.rb b/app/services/contact_inbox/source_id_service.rb new file mode 100644 index 000000000..995bc37a9 --- /dev/null +++ b/app/services/contact_inbox/source_id_service.rb @@ -0,0 +1,55 @@ +class ContactInbox::SourceIdService + pattr_initialize [:contact!, :channel_type!, { medium: nil }] + + def generate + case channel_type + when 'Channel::TwilioSms' + twilio_source_id + when 'Channel::Whatsapp' + wa_source_id + when 'Channel::Email' + email_source_id + when 'Channel::Sms' + phone_source_id + when 'Channel::Api', 'Channel::WebWidget' + SecureRandom.uuid + else + raise ArgumentError, "Unsupported operation for this channel: #{channel_type}" + end + end + + private + + def email_source_id + raise ArgumentError, 'contact email required' unless contact.email + + contact.email + end + + def phone_source_id + raise ArgumentError, 'contact phone number required' unless contact.phone_number + + contact.phone_number + end + + def wa_source_id + raise ArgumentError, 'contact phone number required' unless contact.phone_number + + # whatsapp doesn't want the + in e164 format + contact.phone_number.delete('+').to_s + end + + def twilio_source_id + raise ArgumentError, 'contact phone number required' unless contact.phone_number + raise ArgumentError, 'medium required for Twilio channel' if medium.blank? + + case medium + when 'sms' + contact.phone_number + when 'whatsapp' + "whatsapp:#{contact.phone_number}" + else + raise ArgumentError, "Unsupported Twilio medium: #{medium}" + end + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 4a66a4bc2..27ba48cba 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -67,6 +67,9 @@ en: invalid_message_type: 'Invalid message type. Action not permitted' slack: invalid_channel_id: 'Invalid slack channel. Please try again' + channel_service: + invalid_source_id: "This conversation may have originally belonged to a different contact but is now showing here due to a merge or update. You won't be able to continue this conversation. Please create a new conversation to proceed." + inboxes: imap: socket_error: Please check the network connection, IMAP address and try again. diff --git a/spec/builders/contact_inbox_builder_spec.rb b/spec/builders/contact_inbox_builder_spec.rb index f0e3deb31..b6519f130 100644 --- a/spec/builders/contact_inbox_builder_spec.rb +++ b/spec/builders/contact_inbox_builder_spec.rb @@ -59,7 +59,7 @@ describe ContactInboxBuilder do contact: contact, inbox: twilio_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact phone number') + end.to raise_error(ArgumentError, 'contact phone number required') end end @@ -117,7 +117,7 @@ describe ContactInboxBuilder do contact: contact, inbox: twilio_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact phone number') + end.to raise_error(ArgumentError, 'contact phone number required') end end @@ -174,7 +174,7 @@ describe ContactInboxBuilder do contact: contact, inbox: whatsapp_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact phone number') + end.to raise_error(ArgumentError, 'contact phone number required') end end @@ -232,7 +232,7 @@ describe ContactInboxBuilder do contact: contact, inbox: sms_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact phone number') + end.to raise_error(ArgumentError, 'contact phone number required') end end @@ -290,7 +290,7 @@ describe ContactInboxBuilder do contact: contact, inbox: email_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact email') + end.to raise_error(ArgumentError, 'contact email required') end end diff --git a/spec/services/contact_inbox/source_id_service_spec.rb b/spec/services/contact_inbox/source_id_service_spec.rb new file mode 100644 index 000000000..94fe8a384 --- /dev/null +++ b/spec/services/contact_inbox/source_id_service_spec.rb @@ -0,0 +1,117 @@ +require 'rails_helper' + +RSpec.describe ContactInbox::SourceIdService do + let(:contact) { create(:contact, email: 'test@example.com', phone_number: '+1234567890') } + + describe '#generate' do + context 'when channel is TwilioSms' do + let(:channel_type) { 'Channel::TwilioSms' } + + context 'with SMS medium' do + subject { described_class.new(contact: contact, channel_type: channel_type, medium: 'sms') } + + it 'returns phone number as source id' do + expect(subject.generate).to eq(contact.phone_number) + end + end + + context 'with WhatsApp medium' do + subject { described_class.new(contact: contact, channel_type: channel_type, medium: 'whatsapp') } + + it 'returns whatsapp prefixed phone number as source id' do + expect(subject.generate).to eq("whatsapp:#{contact.phone_number}") + end + end + + context 'with invalid medium' do + subject { described_class.new(contact: contact, channel_type: channel_type, medium: 'invalid') } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'Unsupported Twilio medium: invalid') + end + end + + context 'without medium' do + subject { described_class.new(contact: contact, channel_type: channel_type) } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'medium required for Twilio channel') + end + end + end + + context 'when channel is Whatsapp' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Whatsapp') } + + it 'returns phone number without + as source id' do + expect(subject.generate).to eq(contact.phone_number.delete('+')) + end + + context 'when contact has no phone number' do + let(:contact) { create(:contact, phone_number: nil) } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'contact phone number required') + end + end + end + + context 'when channel is Email' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Email') } + + it 'returns email as source id' do + expect(subject.generate).to eq(contact.email) + end + + context 'when contact has no email' do + let(:contact) { create(:contact, email: nil) } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'contact email required') + end + end + end + + context 'when channel is SMS' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Sms') } + + it 'returns phone number as source id' do + expect(subject.generate).to eq(contact.phone_number) + end + + context 'when contact has no phone number' do + let(:contact) { create(:contact, phone_number: nil) } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'contact phone number required') + end + end + end + + context 'when channel is Api' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Api') } + + it 'returns a UUID as source id' do + allow(SecureRandom).to receive(:uuid).and_return('uuid-123') + expect(subject.generate).to eq('uuid-123') + end + end + + context 'when channel is WebWidget' do + subject { described_class.new(contact: contact, channel_type: 'Channel::WebWidget') } + + it 'returns a UUID as source id' do + allow(SecureRandom).to receive(:uuid).and_return('uuid-123') + expect(subject.generate).to eq('uuid-123') + end + end + + context 'when channel is unsupported' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Unknown') } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'Unsupported operation for this channel: Channel::Unknown') + end + end + end +end diff --git a/spec/services/sms/send_on_sms_service_spec.rb b/spec/services/sms/send_on_sms_service_spec.rb index d2dc1f24a..52c5d89b1 100644 --- a/spec/services/sms/send_on_sms_service_spec.rb +++ b/spec/services/sms/send_on_sms_service_spec.rb @@ -5,8 +5,9 @@ describe Sms::SendOnSmsService do context 'when a valid message' do let(:sms_request) { double } let!(:sms_channel) { create(:channel_sms) } - let!(:contact_inbox) { create(:contact_inbox, inbox: sms_channel.inbox, source_id: '+123456789') } - let!(:conversation) { create(:conversation, contact_inbox: contact_inbox, inbox: sms_channel.inbox) } + let!(:contact) { create(:contact, phone_number: '+123456789') } + let!(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: sms_channel.inbox, source_id: '+123456789') } + let!(:conversation) { create(:conversation, contact_inbox: contact_inbox, inbox: sms_channel.inbox, contact: contact) } it 'calls channel.send_message' do message = create(:message, message_type: :outgoing, content: 'test', diff --git a/spec/services/twilio/send_on_twilio_service_spec.rb b/spec/services/twilio/send_on_twilio_service_spec.rb index beae54b57..af74a46e7 100644 --- a/spec/services/twilio/send_on_twilio_service_spec.rb +++ b/spec/services/twilio/send_on_twilio_service_spec.rb @@ -13,9 +13,11 @@ describe Twilio::SendOnTwilioService do let!(:twilio_whatsapp) { create(:channel_twilio_sms, medium: :whatsapp, account: account) } let!(:twilio_inbox) { create(:inbox, channel: twilio_sms, account: account) } let!(:twilio_whatsapp_inbox) { create(:inbox, channel: twilio_whatsapp, account: account) } - let!(:contact) { create(:contact, account: account) } - let(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: twilio_inbox) } + let!(:contact) { create(:contact, account: account, phone_number: '+123456789') } + let(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: twilio_inbox, source_id: '+123456789') } + let(:whatsapp_contact_inbox) { create(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:+123456789') } let(:conversation) { create(:conversation, contact: contact, inbox: twilio_inbox, contact_inbox: contact_inbox) } + let(:whatsapp_conversation) { create(:conversation, contact: contact, inbox: twilio_whatsapp_inbox, contact_inbox: whatsapp_contact_inbox) } before do allow(Twilio::REST::Client).to receive(:new).and_return(twilio_client) @@ -71,7 +73,7 @@ describe Twilio::SendOnTwilioService do allow(message_record_double).to receive(:sid).and_return('1234') message = build( - :message, message_type: 'outgoing', inbox: twilio_whatsapp_inbox, account: account, conversation: conversation + :message, message_type: 'outgoing', inbox: twilio_whatsapp_inbox, account: account, conversation: whatsapp_conversation ) attachment = message.attachments.new(account_id: message.account_id, file_type: :image) attachment.file.attach(io: Rails.root.join('spec/assets/avatar.png').open, filename: 'avatar.png', content_type: 'image/png') diff --git a/spec/services/whatsapp/send_on_whatsapp_service_spec.rb b/spec/services/whatsapp/send_on_whatsapp_service_spec.rb index 88f3f7740..deb7d6a3f 100644 --- a/spec/services/whatsapp/send_on_whatsapp_service_spec.rb +++ b/spec/services/whatsapp/send_on_whatsapp_service_spec.rb @@ -18,8 +18,9 @@ describe Whatsapp::SendOnWhatsappService do context 'when a valid message' do let(:whatsapp_request) { instance_double(HTTParty::Response) } let!(:whatsapp_channel) { create(:channel_whatsapp, sync_templates: false) } - let!(:contact_inbox) { create(:contact_inbox, inbox: whatsapp_channel.inbox, source_id: '123456789') } - let!(:conversation) { create(:conversation, contact_inbox: contact_inbox, inbox: whatsapp_channel.inbox) } + let!(:contact) { create(:contact, phone_number: '+123456789') } + let!(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: whatsapp_channel.inbox, source_id: '123456789') } + let!(:conversation) { create(:conversation, contact: contact, contact_inbox: contact_inbox, inbox: whatsapp_channel.inbox) } let(:api_key) { 'test_key' } let(:headers) { { 'D360-API-KEY' => api_key, 'Content-Type' => 'application/json' } } let(:template_body) do @@ -35,14 +36,12 @@ describe Whatsapp::SendOnWhatsappService do } end - let(:success_response) { { 'messages' => [{ 'id' => '123456789' }] }.to_json } + let(:success_response) { { 'messages' => [{ 'id' => 'message-123456789' }] }.to_json } it 'calls channel.send_message when with in 24 hour limit' do # to handle the case of 24 hour window limit. - create(:message, message_type: :incoming, content: 'test', - conversation: conversation) - message = create(:message, message_type: :outgoing, content: 'test', - conversation: conversation) + create(:message, message_type: :incoming, content: 'test', conversation: conversation) + message = create(:message, message_type: :outgoing, content: 'test', conversation: conversation) stub_request(:post, 'https://waba.360dialog.io/v1/messages') .with( @@ -52,7 +51,7 @@ describe Whatsapp::SendOnWhatsappService do .to_return(status: 200, body: success_response, headers: { 'content-type' => 'application/json' }) described_class.new(message: message).perform - expect(message.reload.source_id).to eq('123456789') + expect(message.reload.source_id).to eq('message-123456789') end it 'calls channel.send_template when after 24 hour limit' do @@ -66,7 +65,7 @@ describe Whatsapp::SendOnWhatsappService do ).to_return(status: 200, body: success_response, headers: { 'content-type' => 'application/json' }) described_class.new(message: message).perform - expect(message.reload.source_id).to eq('123456789') + expect(message.reload.source_id).to eq('message-123456789') end it 'calls channel.send_template if template_params are present' do @@ -79,7 +78,7 @@ describe Whatsapp::SendOnWhatsappService do ).to_return(status: 200, body: success_response, headers: { 'content-type' => 'application/json' }) described_class.new(message: message).perform - expect(message.reload.source_id).to eq('123456789') + expect(message.reload.source_id).to eq('message-123456789') end it 'calls channel.send_template when template has regexp characters' do @@ -106,7 +105,18 @@ describe Whatsapp::SendOnWhatsappService do ).to_return(status: 200, body: success_response, headers: { 'content-type' => 'application/json' }) described_class.new(message: message).perform - expect(message.reload.source_id).to eq('123456789') + expect(message.reload.source_id).to eq('message-123456789') + end + + context 'when source_id validation is required' do + let(:message) { create(:message, conversation: conversation, message_type: :outgoing) } + + it 'marks message as failed when source_ids do not match' do + contact_inbox.update!(source_id: '1234567890') + described_class.new(message: message).perform + expect(message.reload.status).to eq('failed') + expect(message.external_error).to include('This conversation may have originally belonged to a different contact') + end end end end