From ee2844877cba441c12d80fd2c402f8771d4ec5c0 Mon Sep 17 00:00:00 2001 From: Pranav Date: Fri, 21 Jun 2024 14:58:36 -0700 Subject: [PATCH] fix: Add catch for additional webpush errors (#9662) Webpush gem throws errors such as `WebPush::ExpiredSubscription`, `WebPush::InvalidSubscription`, `WebPush::Unauthorized`. We handled only ExpiredSubscription. If the SDK threw any other errors, it would pause sending the notification to all other devices for that user. This change would update the logic to remove the expired subscription and handle the rest of the errors gracefully. Fixes https://linear.app/chatwoot/issue/CW-3399/webpushinvalidsubscription-host-fcmgoogleapiscom-nethttpnotfound-404 --- .../notification/push_notification_service.rb | 29 ++++++--- spec/factories/notification_subscriptions.rb | 8 +++ .../push_notification_service_spec.rb | 65 +++++++++++++------ 3 files changed, 74 insertions(+), 28 deletions(-) diff --git a/app/services/notification/push_notification_service.rb b/app/services/notification/push_notification_service.rb index 3c7ed52e7..263800d4a 100644 --- a/app/services/notification/push_notification_service.rb +++ b/app/services/notification/push_notification_service.rb @@ -42,14 +42,12 @@ class Notification::PushNotificationService app_account_conversation_url(account_id: conversation.account_id, id: conversation.display_id) end - def send_browser_push?(subscription) + def can_send_browser_push?(subscription) VapidService.public_key && subscription.browser_push? end - def send_browser_push(subscription) - return unless send_browser_push?(subscription) - - WebPush.payload_send( + def browser_push_payload(subscription) + { message: JSON.generate(push_message), endpoint: subscription.subscription_attributes['endpoint'], p256dh: subscription.subscription_attributes['p256dh'], @@ -62,11 +60,22 @@ class Notification::PushNotificationService ssl_timeout: 5, open_timeout: 5, read_timeout: 5 - ) - rescue WebPush::ExpiredSubscription + } + end + + def send_browser_push(subscription) + return unless can_send_browser_push?(subscription) + + WebPush.payload_send(browser_push_payload(subscription)) + Rails.logger.info("Browser push sent to #{user.email} with title #{push_message[:title]}") + rescue WebPush::ExpiredSubscription, WebPush::InvalidSubscription, WebPush::Unauthorized => e + Rails.logger.info "WebPush subscription expired: #{e.message}" subscription.destroy! rescue Errno::ECONNRESET, Net::OpenTimeout, Net::ReadTimeout => e Rails.logger.error "WebPush operation error: #{e.message}" + rescue StandardError => e + ChatwootExceptionTracker.new(e, account: notification.account).capture_exception + true end def send_fcm_push(subscription) @@ -98,7 +107,11 @@ class Notification::PushNotificationService end def remove_subscription_if_error(subscription, response) - subscription.destroy! if JSON.parse(response[:body])['results']&.first&.keys&.include?('error') + if JSON.parse(response[:body])['results']&.first&.keys&.include?('error') + subscription.destroy! + else + Rails.logger.info("FCM push sent to #{user.email} with title #{push_message[:title]}") + end end def fcm_options(subscription) diff --git a/spec/factories/notification_subscriptions.rb b/spec/factories/notification_subscriptions.rb index 581c198a6..c886a12c0 100644 --- a/spec/factories/notification_subscriptions.rb +++ b/spec/factories/notification_subscriptions.rb @@ -6,5 +6,13 @@ FactoryBot.define do identifier { 'test' } subscription_type { 'browser_push' } subscription_attributes { { endpoint: 'test', auth: 'test' } } + + trait :browser_push do + subscription_type { 'browser_push' } + end + + trait :fcm do + subscription_type { 'fcm' } + end end end diff --git a/spec/services/notification/push_notification_service_spec.rb b/spec/services/notification/push_notification_service_spec.rb index bb8ed5c66..bba912715 100644 --- a/spec/services/notification/push_notification_service_spec.rb +++ b/spec/services/notification/push_notification_service_spec.rb @@ -7,33 +7,58 @@ describe Notification::PushNotificationService do let(:fcm_double) { instance_double(FCM) } let(:fcm_service_double) { instance_double(Notification::FcmService, fcm_client: fcm_double) } - before do - allow(WebPush).to receive(:payload_send).and_return(true) - allow(Notification::FcmService).to receive(:new).and_return(fcm_service_double) - allow(fcm_double).to receive(:send_v1).and_return({ body: { 'results': [] }.to_json }) - allow(GlobalConfigService).to receive(:load).with('FIREBASE_PROJECT_ID', nil).and_return('test_project_id') - allow(GlobalConfigService).to receive(:load).with('FIREBASE_CREDENTIALS', nil).and_return('test_credentials') - end - describe '#perform' do - it 'sends webpush notifications for webpush subscription' do - with_modified_env VAPID_PUBLIC_KEY: 'test' do - create(:notification_subscription, user: notification.user) + context 'when the push server returns success' do + before do + allow(WebPush).to receive(:payload_send).and_return(true) + allow(Rails.logger).to receive(:info) + allow(Notification::FcmService).to receive(:new).and_return(fcm_service_double) + allow(fcm_double).to receive(:send_v1).and_return({ body: { 'results': [] }.to_json }) + allow(GlobalConfigService).to receive(:load).with('FIREBASE_PROJECT_ID', nil).and_return('test_project_id') + allow(GlobalConfigService).to receive(:load).with('FIREBASE_CREDENTIALS', nil).and_return('test_credentials') + end - described_class.new(notification: notification).perform - expect(WebPush).to have_received(:payload_send) - expect(Notification::FcmService).not_to have_received(:new) + it 'sends webpush notifications for webpush subscription' do + with_modified_env VAPID_PUBLIC_KEY: 'test' do + create(:notification_subscription, user: notification.user) + + described_class.new(notification: notification).perform + expect(WebPush).to have_received(:payload_send) + expect(Notification::FcmService).not_to have_received(:new) + expect(Rails.logger).to have_received(:info).with("Browser push sent to #{user.email} with title #{notification.push_message_title}") + end + end + + it 'sends a fcm notification for firebase subscription' do + with_modified_env ENABLE_PUSH_RELAY_SERVER: 'false' do + create(:notification_subscription, user: notification.user, subscription_type: 'fcm') + + described_class.new(notification: notification).perform + expect(Notification::FcmService).to have_received(:new) + expect(fcm_double).to have_received(:send_v1) + expect(WebPush).not_to have_received(:payload_send) + expect(Rails.logger).to have_received(:info).with("FCM push sent to #{user.email} with title #{notification.push_message_title}") + end end end + end - it 'sends a fcm notification for firebase subscription' do - with_modified_env ENABLE_PUSH_RELAY_SERVER: 'false' do - create(:notification_subscription, user: notification.user, subscription_type: 'fcm') + context 'when the push server returns error' do + it 'sends webpush notifications for webpush subscription' do + with_modified_env VAPID_PUBLIC_KEY: 'test' do + mock_response = instance_double(Net::HTTPResponse, body: 'Subscription is invalid') + mock_host = 'fcm.googleapis.com' + + allow(WebPush).to receive(:payload_send).and_raise(WebPush::InvalidSubscription.new(mock_response, mock_host)) + allow(Rails.logger).to receive(:info) + + create(:notification_subscription, :browser_push, user: notification.user) + + expect(Rails.logger).to receive(:info) do |message| + expect(message).to include('WebPush subscription expired:') + end described_class.new(notification: notification).perform - expect(Notification::FcmService).to have_received(:new) - expect(fcm_double).to have_received(:send_v1) - expect(WebPush).not_to have_received(:payload_send) end end end