Merge pull request #6289 from tgxworld/clean_up_post_reply_keys

FEATURE: Clean up `PostReplyKey` records.

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

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"

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

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

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

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

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

spec/fixtures/emails/old_destination.eml

@@ -1,6 +1,6 @@
 Return-Path: <staged@bar.com>
 From: Foo Bar <staged@bar.com>
-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: <topic/424242/123456@test.localhost>
 Mime-Version: 1.0
 Content-Type: text/plain
 Content-Transfer-Encoding: 7bit
+Subject: Some Old Post
 
 Lorem ipsum dolor sit amet, consectetur adipiscing elit.

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