From 36a7028f19b2b511862eb753b3ec72fccaffd2d7 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 21 Aug 2018 16:17:08 +0800 Subject: [PATCH] FEATURE: Clean up `PostReplyKey` records. * Default retention of 90 days. --- .../scheduled/clean_up_post_reply_keys.rb | 14 +++++++++++ config/locales/server.en.yml | 3 ++- config/site_settings.yml | 3 +++ lib/email/processor.rb | 1 + lib/email/receiver.rb | 8 ++++-- spec/components/email/processor_spec.rb | 23 +++++++++++++++++ spec/components/email/receiver_spec.rb | 24 +++++++++++++++--- spec/fixtures/emails/old_destination.eml | 3 ++- spec/jobs/clean_up_post_reply_keys_spec.rb | 25 +++++++++++++++++++ 9 files changed, 97 insertions(+), 7 deletions(-) create mode 100644 app/jobs/scheduled/clean_up_post_reply_keys.rb create mode 100644 spec/jobs/clean_up_post_reply_keys_spec.rb diff --git a/app/jobs/scheduled/clean_up_post_reply_keys.rb b/app/jobs/scheduled/clean_up_post_reply_keys.rb new file mode 100644 index 00000000000..e308b079e95 --- /dev/null +++ b/app/jobs/scheduled/clean_up_post_reply_keys.rb @@ -0,0 +1,14 @@ +module Jobs + class CleanUpPostReplyKeys < Jobs::Scheduled + every 1.day + + def execute(_) + return if SiteSetting.disallow_reply_by_email_after_days <= 0 + + PostReplyKey.where( + "created_at < ?", + SiteSetting.disallow_reply_by_email_after_days.days.ago + ).delete_all + end + end +end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 991b7631f2b..bf769a1e33d 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1565,6 +1565,7 @@ en: unsubscribe_via_email_footer: "Attach an unsubscribe via email mailto: link to the footer of sent emails" delete_email_logs_after_days: "Delete email logs after (N) days. 0 to keep indefinitely" + disallow_reply_by_email_after_days: "Disallow reply by email after (N) days. 0 to keep indefinitely" max_emails_per_day_per_user: "Maximum number of emails to send users per day. 0 to disable the limit" enable_staged_users: "Automatically create staged users when processing incoming emails." maximum_staged_users_per_email: "Maximum number of staged users created when processing an incoming email." @@ -2554,7 +2555,7 @@ en: text_body_template: | We're sorry, but your email message to %{destination} (titled %{former_title}) didn't work. - For security reasons we only accept replies to original notifications for 90 days. Please [visit the topic](%{short_url}) to continue the conversation. + For security reasons we only accept replies to original notifications for %{number_of_days} days. Please [visit the topic](%{short_url}) to continue the conversation. email_reject_topic_not_found: title: "Email Reject Topic Not Found" diff --git a/config/site_settings.yml b/config/site_settings.yml index a27fb7a668b..f2956bd4588 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -816,6 +816,9 @@ email: default: true unsubscribe_via_email_footer: default: false + disallow_reply_by_email_after_days: + default: 90 + shadowed_by_global: true delete_email_logs_after_days: default: 90 shadowed_by_global: true diff --git a/lib/email/processor.rb b/lib/email/processor.rb index ad43efeda92..27ae09b1b91 100644 --- a/lib/email/processor.rb +++ b/lib/email/processor.rb @@ -85,6 +85,7 @@ module Email if message_template == :email_reject_old_destination template_args[:short_url] = e.message + template_args[:number_of_days] = SiteSetting.disallow_reply_by_email_after_days end if message_template diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 2ca5c315d57..898838faad1 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -171,8 +171,12 @@ module Email raise first_exception if first_exception - if post = find_related_post(force: true) - if Guardian.new(user).can_see_post?(post) && post.created_at < 90.days.ago + post = find_related_post(force: true) + + if post && Guardian.new(user).can_see_post?(post) + num_of_days = SiteSetting.disallow_reply_by_email_after_days + + if num_of_days > 0 && post.created_at < num_of_days.days.ago raise OldDestinationError.new("#{Discourse.base_url}/p/#{post.id}") end end diff --git a/spec/components/email/processor_spec.rb b/spec/components/email/processor_spec.rb index 0a0a0c7b4cc..c953400f524 100644 --- a/spec/components/email/processor_spec.rb +++ b/spec/components/email/processor_spec.rb @@ -161,4 +161,27 @@ describe Email::Processor do expect { Email::Processor.process!(email) }.to_not change { EmailLog.count } end end + + describe 'when replying to a post that is too old' do + let(:mail) { file_from_fixtures("old_destination.eml", "emails").read } + + it 'rejects the email with the right response' do + SiteSetting.disallow_reply_by_email_after_days = 2 + + topic = Fabricate(:topic, id: 424242) + post = Fabricate(:post, topic: topic, id: 123456, created_at: 3.days.ago) + + processor = Email::Processor.new(mail) + processor.process! + + rejection_raw = ActionMailer::Base.deliveries.first.body.to_s + + expect(rejection_raw).to eq(I18n.t("system_messages.email_reject_old_destination.text_body_template", + destination: '["reply+4f97315cc828096c9cb34c6f1a0d6fe8@bar.com"]', + former_title: 'Some Old Post', + short_url: "#{Discourse.base_url}/p/#{post.id}", + number_of_days: 2 + )) + end + end end diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 7289aa92f16..6d92858fd15 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -79,10 +79,28 @@ describe Email::Receiver do end it "raises an OldDestinationError when notification is too old" do - topic = Fabricate(:topic, id: 424242) - post = Fabricate(:post, topic: topic, id: 123456, created_at: 1.year.ago) + SiteSetting.disallow_reply_by_email_after_days = 2 - expect { process(:old_destination) }.to raise_error(Email::Receiver::OldDestinationError) + topic = Fabricate(:topic, id: 424242) + post = Fabricate(:post, topic: topic, id: 123456) + + expect { process(:old_destination) }.to raise_error( + Email::Receiver::BadDestinationAddress + ) + + IncomingEmail.destroy_all + post.update!(created_at: 3.days.ago) + + expect { process(:old_destination) }.to raise_error( + Email::Receiver::OldDestinationError + ) + + SiteSetting.disallow_reply_by_email_after_days = 0 + IncomingEmail.destroy_all + + expect { process(:old_destination) }.to raise_error( + Email::Receiver::BadDestinationAddress + ) end it "raises a BouncerEmailError when email is a bounced email" do diff --git a/spec/fixtures/emails/old_destination.eml b/spec/fixtures/emails/old_destination.eml index eb847ead444..fb21321d32c 100644 --- a/spec/fixtures/emails/old_destination.eml +++ b/spec/fixtures/emails/old_destination.eml @@ -1,6 +1,6 @@ Return-Path: From: Foo Bar -To: wat@bar.com +To: reply+4f97315cc828096c9cb34c6f1a0d6fe8@bar.com Cc: foofoo@bar.com Date: Fri, 15 Jan 2018 00:12:43 +0100 Message-ID: <999@foo.bar.mail> @@ -8,5 +8,6 @@ In-Reply-To: Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit +Subject: Some Old Post Lorem ipsum dolor sit amet, consectetur adipiscing elit. diff --git a/spec/jobs/clean_up_post_reply_keys_spec.rb b/spec/jobs/clean_up_post_reply_keys_spec.rb new file mode 100644 index 00000000000..97b3cc3a178 --- /dev/null +++ b/spec/jobs/clean_up_post_reply_keys_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' + +RSpec.describe Jobs::CleanUpPostReplyKeys do + it 'removes old post_reply_keys' do + freeze_time + + reply_key1 = Fabricate(:post_reply_key, created_at: 1.day.ago) + reply_key2 = Fabricate(:post_reply_key, created_at: 2.days.ago) + Fabricate(:post_reply_key, created_at: 3.days.ago) + + SiteSetting.disallow_reply_by_email_after_days = 0 + + expect { Jobs::CleanUpPostReplyKeys.new.execute({}) } + .to change { PostReplyKey.count }.by(0) + + SiteSetting.disallow_reply_by_email_after_days = 2 + + expect { Jobs::CleanUpPostReplyKeys.new.execute({}) } + .to change { PostReplyKey.count }.by(-1) + + expect(PostReplyKey.all).to contain_exactly( + reply_key1, reply_key2 + ) + end +end