PERF: Do not create staged users for most rejected incoming emails (#7301)

PERF: Do not create staged users for most rejected incoming emails (#7301)

Previously we would create users, then destroy them at the end of the job if the post was rejected. Now we do not create users unless required.

diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index cf0745a..5a10ebe 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2748,6 +2748,7 @@ en:
         %{post_error}
 
         If you can correct the problem, please try again.
+      date_invalid: "No post creation date found. Is the e-mail missing a Date: header?"
 
     email_reject_post_too_short:
       title: "Email Reject Post Too Short"
diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb
index 03e9c84..1e98de9 100644
--- a/lib/email/receiver.rb
+++ b/lib/email/receiver.rb
@@ -68,6 +68,7 @@ module Email
         begin
           return if IncomingEmail.exists?(message_id: @message_id)
           ensure_valid_address_lists
+          ensure_valid_date
           @from_email, @from_display_name = parse_from_field
           @from_user = User.find_by_email(@from_email)
           @incoming_email = create_incoming_email
@@ -93,6 +94,12 @@ module Email
       end
     end
 
+    def ensure_valid_date
+      if @mail.date.nil?
+        raise InvalidPost, I18n.t("system_messages.email_reject_invalid_post_specified.date_invalid")
+      end
+    end
+
     def is_blacklisted?
       return false if SiteSetting.ignore_by_title.blank?
       Regexp.new(SiteSetting.ignore_by_title, Regexp::IGNORECASE) =~ @mail.subject
@@ -142,14 +149,16 @@ module Email
         return
       end
 
-      # Lets create a staged user if there isn't one yet. We will try to
-      # delete staged users in process!() if something bad happens.
-      if user.nil?
-        user = find_or_create_user!(@from_email, @from_display_name)
-        log_and_validate_user(user)
-      end
-
       if post = find_related_post
+        # Most of the time, it is impossible to **reply** without a reply key, so exit early
+        if user.blank?
+          if sent_to_mailinglist_mirror? || !SiteSetting.find_related_post_with_key
+            user = stage_from_user
+          elsif user.blank?
+            raise BadDestinationAddress
+          end
+        end
+
         create_reply(user: user,
                      raw: body,
                      elided: elided,
@@ -171,6 +180,9 @@ module Email
 
         raise first_exception if first_exception
 
+        # We don't stage new users for emails to reply addresses, exit if user is nil
+        raise BadDestinationAddress if user.blank?
+
         post = find_related_post(force: true)
 
         if post && Guardian.new(user).can_see_post?(post)
@@ -627,13 +639,18 @@ module Email
 
       case destination[:type]
       when :group
+        user ||= stage_from_user
+
         group = destination[:obj]
         create_group_post(group, user, body, elided, hidden_reason_id)
 
       when :category
         category = destination[:obj]
 
-        raise StrangersNotAllowedError    if user.staged? && !category.email_in_allow_strangers
+        raise StrangersNotAllowedError    if (user.nil? || user.staged?) && !category.email_in_allow_strangers
+
+        user ||= stage_from_user
+
         raise InsufficientTrustLevelError if !user.has_trust_level?(SiteSetting.email_in_min_trust) && !sent_to_mailinglist_mirror?
 
         create_topic(user: user,
@@ -645,6 +662,9 @@ module Email
                      skip_validations: user.staged?)
 
       when :reply
+        # We don't stage new users for emails to reply addresses, exit if user is nil
+        raise BadDestinationAddress if user.blank?
+
         post_reply_key = destination[:obj]
 
         if post_reply_key.user_id != user.id && !forwarded_reply_key?(post_reply_key, user)
@@ -750,6 +770,7 @@ module Email
     end
 
     def process_forwarded_email(destination, user)
+      user ||= stage_from_user
       embedded = Mail.new(embedded_email_raw)
       email, display_name = parse_from_field(embedded)
 
@@ -1031,12 +1052,7 @@ module Email
       options[:via_email] = true
       options[:raw_email] = @raw_email
 
-      # ensure posts aren't created in the future
       options[:created_at] ||= @mail.date
-      if options[:created_at].nil?
-        raise InvalidPost, "No post creation date found. Is the e-mail missing a Date: header?"
-      end
-
       options[:created_at] = DateTime.now if options[:created_at] > DateTime.now
 
       is_private_message = options[:archetype] == Archetype.private_message ||
@@ -1136,9 +1152,15 @@ module Email
       Email::Sender.new(message, :subscription).send
     end
 
+    def stage_from_user
+      @from_user ||= find_or_create_user!(@from_email, @from_display_name).tap do |u|
+        log_and_validate_user(u)
+      end
+    end
+
     def delete_staged_users
       @staged_users.each do |user|
-        if @incoming_email.user.id == user.id
+        if @incoming_email.user&.id == user.id
           @incoming_email.update_columns(user_id: nil)
         end
 
diff --git a/spec/components/email/processor_spec.rb b/spec/components/email/processor_spec.rb
index c31a70f..ef33fd7 100644
--- a/spec/components/email/processor_spec.rb
+++ b/spec/components/email/processor_spec.rb
@@ -99,8 +99,8 @@ describe Email::Processor do
 
   context "unrecognized error" do
 
-    let(:mail) { "From: #{from}\nTo: bar@foo.com\nSubject: FOO BAR\n\nFoo foo bar bar?" }
-    let(:mail2) { "From: #{from}\nTo: foo@foo.com\nSubject: BAR BAR\n\nBar bar bar bar?" }
+    let(:mail) { "Date: Fri, 15 Jan 2016 00:12:43 +0100\nFrom: #{from}\nTo: bar@foo.com\nSubject: FOO BAR\n\nFoo foo bar bar?" }
+    let(:mail2) { "Date: Fri, 15 Jan 2016 00:12:43 +0100\nFrom: #{from}\nTo: foo@foo.com\nSubject: BAR BAR\n\nBar bar bar bar?" }
 
     it "sends a rejection email on an unrecognized error" do
       begin
@@ -144,7 +144,7 @@ describe Email::Processor do
 
   context "from reply to email address" do
 
-    let(:mail) { "From: reply@bar.com\nTo: reply@bar.com\nSubject: FOO BAR\n\nFoo foo bar bar?" }
+    let(:mail) { "Date: Fri, 15 Jan 2016 00:12:43 +0100\nFrom: reply@bar.com\nTo: reply@bar.com\nSubject: FOO BAR\n\nFoo foo bar bar?" }
 
     it "ignores the email" do
       Email::Receiver.any_instance.stubs(:process_internal).raises(Email::Receiver::FromReplyByAddressError.new)
@@ -177,7 +177,7 @@ describe Email::Processor do
 
   describe 'when replying to a post that is too old' do
     let(:mail) { file_from_fixtures("old_destination.eml", "emails").read }
-
+    let!(:user) { Fabricate(:user, email: "discourse@bar.com") }
     it 'rejects the email with the right response' do
       SiteSetting.disallow_reply_by_email_after_days = 2
 
diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb
index dbdc508..7d4568c 100644
--- a/spec/components/email/receiver_spec.rb
+++ b/spec/components/email/receiver_spec.rb
@@ -25,11 +25,13 @@ describe Email::Receiver do
 
   it "raises EmailNotAllowed when email address is not on whitelist" do
     SiteSetting.email_domains_whitelist = "example.com|bar.com"
+    Fabricate(:group, incoming_email: "some_group@bar.com")
     expect { process(:blacklist_whitelist_email) }.to raise_error(Email::Receiver::EmailNotAllowed)
   end
 
   it "raises EmailNotAllowed when email address is on blacklist" do
     SiteSetting.email_domains_blacklist = "email.com|mail.com"
+    Fabricate(:group, incoming_email: "some_group@bar.com")
     expect { process(:blacklist_whitelist_email) }.to raise_error(Email::Receiver::EmailNotAllowed)
   end
 
@@ -83,6 +85,7 @@ describe Email::Receiver do
 
     topic = Fabricate(:topic, id: 424242)
     post  = Fabricate(:post, topic: topic, id: 123456)
+    user  = Fabricate(:user, email: "discourse@bar.com")
 
     expect { process(:old_destination) }.to raise_error(
       Email::Receiver::BadDestinationAddress
@@ -262,6 +265,7 @@ describe Email::Receiver do
     end
 
     it "raises a ReplyUserNotMatchingError when the email address isn't matching the one we sent the notification to" do
+      Fabricate(:user, email: "someone_else@bar.com")
       expect { process(:reply_user_not_matching) }.to raise_error(Email::Receiver::ReplyUserNotMatchingError)
     end
 

[... diff too long, it was truncated ...]

GitHub sha: 6a05f190

1 Like