From d45682716bcddb06fbd2890d0b86b07eae292c71 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 27 May 2021 06:49:20 +1000 Subject: [PATCH] FIX: automatically expire bad push channels (#13156) Previously we would retry push notifications indefinitely for all errors except for ExpiredSubscription Under certain conditions other persistent errors may arise such as a persistent rate limit. If we track more than 3 errors in a period of time longer than a day we will delete the subscription Also performs a bit of internal cleanup to ensure protected methods really are private. --- app/models/push_subscription.rb | 16 ++-- app/services/push_notification_pusher.rb | 43 +++++++--- ...1_add_error_count_to_push_subscriptions.rb | 8 ++ .../services/push_notification_pusher_spec.rb | 78 ++++++++++++++++--- 4 files changed, 119 insertions(+), 26 deletions(-) create mode 100644 db/migrate/20210526053611_add_error_count_to_push_subscriptions.rb diff --git a/app/models/push_subscription.rb b/app/models/push_subscription.rb index 1c012d84522..dffdab102a7 100644 --- a/app/models/push_subscription.rb +++ b/app/models/push_subscription.rb @@ -2,15 +2,21 @@ class PushSubscription < ActiveRecord::Base belongs_to :user + + def parsed_data + JSON.parse(data) + end end # == Schema Information # # Table name: push_subscriptions # -# id :bigint not null, primary key -# user_id :integer not null -# data :string not null -# created_at :datetime not null -# updated_at :datetime not null +# id :bigint not null, primary key +# user_id :integer not null +# data :string not null +# created_at :datetime not null +# updated_at :datetime not null +# error_count :integer default(0), not null +# first_error_at :datetime # diff --git a/app/services/push_notification_pusher.rb b/app/services/push_notification_pusher.rb index 4e013a8efec..72000de01e6 100644 --- a/app/services/push_notification_pusher.rb +++ b/app/services/push_notification_pusher.rb @@ -22,7 +22,6 @@ class PushNotificationPusher } subscriptions(user).each do |subscription| - subscription = JSON.parse(subscription.data) send_notification(user, subscription, message) end end @@ -66,8 +65,6 @@ class PushNotificationPusher PushSubscription.find_by(user: user, data: subscription.to_json)&.destroy! end - protected - def self.get_badge if (url = SiteSetting.site_push_notifications_icon_url).present? url @@ -76,13 +73,30 @@ class PushNotificationPusher end end + MAX_ERRORS ||= 3 + MIN_ERROR_DURATION ||= 86400 # 1 day + + def self.handle_generic_error(subscription) + subscription.error_count += 1 + subscription.first_error_at ||= Time.zone.now + + delta = Time.zone.now - subscription.first_error_at + if subscription.error_count >= MAX_ERRORS && delta > MIN_ERROR_DURATION + subscription.destroy! + else + subscription.save! + end + end + def self.send_notification(user, subscription, message) - endpoint = subscription["endpoint"] - p256dh = subscription.dig("keys", "p256dh") - auth = subscription.dig("keys", "auth") + parsed_data = subscription.parsed_data + + endpoint = parsed_data["endpoint"] + p256dh = parsed_data.dig("keys", "p256dh") + auth = parsed_data.dig("keys", "auth") if (endpoint.blank? || p256dh.blank? || auth.blank?) - unsubscribe(user, subscription) + subscription.destroy! return end @@ -99,22 +113,31 @@ class PushNotificationPusher expiration: TOKEN_VALID_FOR_SECONDS } ) + + if subscription.first_error_at || subscription.error_count != 0 + subscription.update_columns(error_count: 0, first_error_at: nil) + end rescue Webpush::ExpiredSubscription - unsubscribe(user, subscription) + subscription.destroy! rescue Webpush::ResponseError => e if e.response.message == "MismatchSenderId" - unsubscribe(user, subscription) + subscription.destroy! else + handle_generic_error(subscription) Discourse.warn_exception( e, message: "Failed to send push notification", env: { user_id: user.id, - endpoint: subscription["endpoint"], + endpoint: endpoint, message: message.to_json } ) end end end + + private_class_method :send_notification + private_class_method :handle_generic_error + end diff --git a/db/migrate/20210526053611_add_error_count_to_push_subscriptions.rb b/db/migrate/20210526053611_add_error_count_to_push_subscriptions.rb new file mode 100644 index 00000000000..632b5103822 --- /dev/null +++ b/db/migrate/20210526053611_add_error_count_to_push_subscriptions.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddErrorCountToPushSubscriptions < ActiveRecord::Migration[6.1] + def change + add_column :push_subscriptions, :error_count, :integer, null: false, default: 0 + add_column :push_subscriptions, :first_error_at, :datetime + end +end diff --git a/spec/services/push_notification_pusher_spec.rb b/spec/services/push_notification_pusher_spec.rb index de1659204f8..f00d3ab2f16 100644 --- a/spec/services/push_notification_pusher_spec.rb +++ b/spec/services/push_notification_pusher_spec.rb @@ -19,18 +19,20 @@ RSpec.describe PushNotificationPusher do it "sends notification in user's locale" do SiteSetting.allow_user_locale = true user = Fabricate(:user, locale: 'pt_BR') - PushSubscription.create!(user_id: user.id, data: "{\"endpoint\": \"endpoint\"}") + data = <<~JSON + { + "endpoint": "endpoint", + "keys": { + "p256dh": "p256dh", + "auth": "auth" + } + } + JSON + PushSubscription.create!(user_id: user.id, data: data) - PushNotificationPusher.expects(:send_notification).with(user, { "endpoint" => "endpoint" }, { - title: "system mencionou você em \"Topic\" - Discourse", - body: "description", - badge: "/assets/push-notifications/discourse.png", - icon: "/assets/push-notifications/mentioned.png", - tag: "test.localhost-1", - base_url: "http://test.localhost", - url: "https://example.com/t/1/2", - hide_when_active: true - }).once + Webpush.expects(:payload_send).with do |*args| + args.to_s.include?("system mencionou") + end.once PushNotificationPusher.push(user, { topic_title: 'Topic', @@ -42,6 +44,60 @@ RSpec.describe PushNotificationPusher do }) end + it "deletes subscriptions which are erroring regularly" do + start = freeze_time + + user = Fabricate(:user) + + data = <<~JSON + { + "endpoint": "endpoint", + "keys": { + "p256dh": "p256dh", + "auth": "auth" + } + } + JSON + + sub = PushSubscription.create!(user_id: user.id, data: data) + + response = Struct.new(:body, :inspect, :message).new("test", "test", "failed") + error = Webpush::ResponseError.new(response, "localhost") + + Webpush.expects(:payload_send).raises(error).times(4) + + # 3 failures in more than 24 hours + 3.times do + PushNotificationPusher.push(user, { + topic_title: 'Topic', + username: 'system', + excerpt: 'description', + topic_id: 1, + post_url: "https://example.com/t/1/2", + notification_type: 1 + }) + + freeze_time 1.minute.from_now + end + + sub.reload + expect(sub.error_count).to eq(3) + expect(sub.first_error_at).to eq_time(start) + + freeze_time(2.days.from_now) + + PushNotificationPusher.push(user, { + topic_title: 'Topic', + username: 'system', + excerpt: 'description', + topic_id: 1, + post_url: "https://example.com/t/1/2", + notification_type: 1 + }) + + expect(PushSubscription.where(id: sub.id).exists?).to eq(false) + end + it "deletes invalid subscriptions during send" do user = Fabricate(:walter_white)