diff --git a/app/services/twilio/health_service.rb b/app/services/twilio/health_service.rb index d257dd76d..c6031a662 100644 --- a/app/services/twilio/health_service.rb +++ b/app/services/twilio/health_service.rb @@ -3,6 +3,9 @@ class Twilio::HealthService pattr_initialize [:channel!] + # Our Twilio routes are POST-only, so a matching URL on the wrong HTTP method never reaches us. + HTTP_METHOD = 'POST'.freeze + # Compares the webhooks Twilio actually has against the ones Chatwoot expects. # Errors (bad credentials, unknown number) bubble up to the controller as a 422. def perform @@ -18,35 +21,39 @@ class Twilio::HealthService def messaging_service_webhooks service = channel.client.messaging.services(channel.messaging_service_sid).fetch + # With use_inbound_webhook_on_number set, Twilio prefers the number's webhook over ours. + configured = service.inbound_method == HTTP_METHOD && !service.use_inbound_webhook_on_number - [webhook('messaging', twilio_callback_index_url, service.inbound_request_url)] + [webhook('messaging', twilio_callback_index_url, service.inbound_request_url, extra: configured)] end def phone_number_webhooks number = channel.client.incoming_phone_numbers.list(phone_number: channel.phone_number).first raise "Phone number #{channel.phone_number} was not found in the connected Twilio account" if number.nil? - webhooks = [webhook('messaging', twilio_callback_index_url, number.sms_url)] + webhooks = [webhook('messaging', twilio_callback_index_url, number.sms_url, extra: number.sms_method == HTTP_METHOD)] webhooks += voice_webhooks(number) if channel.voice_enabled? webhooks end def voice_webhooks(number) [ - webhook('voice', channel.voice_call_webhook_url, number.voice_url), - webhook('voice_status', channel.voice_status_webhook_url, number.status_callback), + webhook('voice', channel.voice_call_webhook_url, number.voice_url, extra: number.voice_method == HTTP_METHOD), + webhook('voice_status', channel.voice_status_webhook_url, number.status_callback, + extra: number.status_callback_method == HTTP_METHOD), # Outbound calls dial through the TwiML app, so a stale voice_url here breaks them silently. - webhook('voice_app', channel.voice_call_webhook_url, twiml_app_voice_url) + twiml_app_webhook ] end - def twiml_app_voice_url - return if channel.twiml_app_sid.blank? + def twiml_app_webhook + return webhook('voice_app', channel.voice_call_webhook_url, nil) if channel.twiml_app_sid.blank? - channel.client.applications(channel.twiml_app_sid).fetch.voice_url + app = channel.client.applications(channel.twiml_app_sid).fetch + webhook('voice_app', channel.voice_call_webhook_url, app.voice_url, extra: app.voice_method == HTTP_METHOD) end - def webhook(name, expected, actual) - { name: name, expected: expected, actual: actual.presence, configured: expected == actual } + def webhook(name, expected, actual, extra: false) + { name: name, expected: expected, actual: actual.presence, configured: expected == actual && extra } end end diff --git a/app/services/twilio/incoming_message_service.rb b/app/services/twilio/incoming_message_service.rb index 02a1636ed..94ba7d0a9 100644 --- a/app/services/twilio/incoming_message_service.rb +++ b/app/services/twilio/incoming_message_service.rb @@ -172,8 +172,8 @@ class Twilio::IncomingMessageService def download_with_auth(media_url) auth_credentials = if twilio_channel.api_key_sid.present? - # When using api_key_sid, the auth token should be the api_secret_key - [twilio_channel.api_key_sid, twilio_channel.auth_token] + # Voice channels keep the secret in api_key_secret; SMS channels store it in auth_token. + [twilio_channel.api_key_sid, twilio_channel.api_key_secret.presence || twilio_channel.auth_token] else # When using account_sid, the auth token is the account's auth token [twilio_channel.account_sid, twilio_channel.auth_token] diff --git a/spec/services/twilio/health_service_spec.rb b/spec/services/twilio/health_service_spec.rb index 19ab359a2..433ab944f 100644 --- a/spec/services/twilio/health_service_spec.rb +++ b/spec/services/twilio/health_service_spec.rb @@ -18,12 +18,13 @@ describe Twilio::HealthService do context 'with a phone number' do let(:channel) { create(:channel_twilio_sms, :with_phone_number) } let(:sms_url) { twilio_callback_index_url } + let(:sms_method) { 'POST' } before do - allow(numbers_list).to receive(:list).and_return([instance_double(NUMBER_INSTANCE, sms_url: sms_url)]) + allow(numbers_list).to receive(:list).and_return([instance_double(NUMBER_INSTANCE, sms_url: sms_url, sms_method: sms_method)]) end - it 'reports healthy when the messaging webhook points at chatwoot' do + it 'reports healthy when the messaging webhook points at chatwoot over POST' do result = described_class.new(channel: channel).perform expect(result[:status]).to eq('healthy') @@ -50,12 +51,46 @@ describe Twilio::HealthService do end end + context 'with a messaging service' do + let(:channel) { create(:channel_twilio_sms) } + let(:messaging) { instance_double(Twilio::REST::Messaging) } + let(:services) { instance_double(Twilio::REST::Messaging::V1::ServiceContext) } + let(:use_inbound_webhook_on_number) { false } + let(:service) do + instance_double(Twilio::REST::Messaging::V1::ServiceInstance, + inbound_request_url: twilio_callback_index_url, inbound_method: 'POST', + use_inbound_webhook_on_number: use_inbound_webhook_on_number) + end + + before do + allow(twilio_client).to receive(:messaging).and_return(messaging) + allow(messaging).to receive(:services).with(channel.messaging_service_sid).and_return(services) + allow(services).to receive(:fetch).and_return(service) + end + + it 'checks the inbound request url of the messaging service' do + result = described_class.new(channel: channel).perform + + expect(result[:status]).to eq('healthy') + expect(result[:webhooks].first).to include(name: 'messaging', configured: true) + end + + context 'when the service still defers to the number webhook' do + let(:use_inbound_webhook_on_number) { true } + + it 'reports misconfigured because twilio would bypass our inbound url' do + expect(described_class.new(channel: channel).perform[:status]).to eq('misconfigured') + end + end + end + context 'with voice enabled' do let(:channel) { create(:channel_twilio_sms, :with_voice) } let(:sms_url) { nil } let(:number) do - instance_double(NUMBER_INSTANCE, sms_url: sms_url, voice_url: channel.voice_call_webhook_url, - status_callback: channel.voice_status_webhook_url) + instance_double(NUMBER_INSTANCE, sms_url: sms_url, sms_method: 'POST', + voice_url: channel.voice_call_webhook_url, voice_method: 'POST', + status_callback: channel.voice_status_webhook_url, status_callback_method: 'POST') end let(:twiml_app) { instance_double(Twilio::REST::Api::V2010::AccountContext::ApplicationContext) } @@ -65,7 +100,8 @@ describe Twilio::HealthService do allow(numbers_list).to receive(:list).and_return([number]) allow(twilio_client).to receive(:applications).and_return(twiml_app) allow(twiml_app).to receive(:fetch).and_return( - instance_double(Twilio::REST::Api::V2010::AccountContext::ApplicationInstance, voice_url: twiml_app_voice_url) + instance_double(Twilio::REST::Api::V2010::AccountContext::ApplicationInstance, + voice_url: twiml_app_voice_url, voice_method: 'POST') ) end @@ -93,18 +129,6 @@ describe Twilio::HealthService do hash_including(name: 'voice_app', configured: false)) end end - - context 'when the messaging webhook is missing' do - let(:twiml_app_voice_url) { channel.voice_call_webhook_url } - - it 'flags messaging while voice stays configured' do - result = described_class.new(channel: channel).perform - - expect(result[:status]).to eq('misconfigured') - expect(result[:webhooks]).to include(hash_including(name: 'messaging', configured: false), - hash_including(name: 'voice', configured: true)) - end - end end end end diff --git a/spec/services/twilio/incoming_message_service_spec.rb b/spec/services/twilio/incoming_message_service_spec.rb index dc30f6e62..458a279a7 100644 --- a/spec/services/twilio/incoming_message_service_spec.rb +++ b/spec/services/twilio/incoming_message_service_spec.rb @@ -197,6 +197,43 @@ describe Twilio::IncomingMessageService do end end + # Voice channels keep the API key secret in api_key_secret, unlike SMS channels which reuse auth_token. + context 'when an attachment arrives on a voice enabled channel' do + before do + stub_request(:get, 'https://chatwoot-assets.local/sample.png') + .to_return(status: 200, body: 'image data', headers: { 'Content-Type' => 'image/png' }) + end + + let(:voice_channel) do + allow(Twilio::VoiceWebhookSetupService).to receive(:new) + .and_return(instance_double(Twilio::VoiceWebhookSetupService, perform: "AP#{SecureRandom.hex(16)}")) + create(:channel_twilio_sms, :with_voice) + end + let(:params_with_attachment) do + { + SmsSid: 'SMvoice', + From: '+12345', + To: voice_channel.phone_number, + AccountSid: voice_channel.account_sid, + Body: 'mms on a voice inbox', + NumMedia: '1', + MediaContentType0: 'image/jpeg', + MediaUrl0: 'https://chatwoot-assets.local/sample.png' + } + end + + it 'downloads the media using the api key secret, not the account auth token' do + allow(Down).to receive(:download).and_call_original + + described_class.new(params: params_with_attachment).perform + + expect(Down).to have_received(:download).with( + 'https://chatwoot-assets.local/sample.png', + http_basic_authentication: [voice_channel.api_key_sid, voice_channel.api_key_secret] + ) + end + end + context 'when there is an error downloading the attachment' do before do stub_request(:get, 'https://chatwoot-assets.local/sample.png')