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.

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"

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
+      if post = find_related_post
-      # delete staged users in process!() if something bad happens.
+        # Most of the time, it is impossible to **reply** without a reply key, so exit early
-      if user.nil?
+        if user.blank?
-        user = find_or_create_user!(@from_email, @from_display_name)
+          if sent_to_mailinglist_mirror? || !SiteSetting.find_related_post_with_key
-        log_and_validate_user(user)
+            user = stage_from_user
+          elsif user.blank?
+            raise BadDestinationAddress
+          end
         end
 
-      if post = find_related_post
         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
 

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(: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) { "From: #{from}\nTo: foo@foo.com\nSubject: BAR BAR\n\nBar bar 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
 

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
 
@@ -606,6 +610,7 @@ describe Email::Receiver do
     end
 
     it "accepts emails with wrong reply key if the system knows about the forwarded email" do
+      Fabricate(:user, email: "bob@bar.com")
       Fabricate(:incoming_email,
                 raw: <<~RAW,
                   Return-Path: <discourse@bar.com>
@@ -750,7 +755,10 @@ describe Email::Receiver do
     end
 
     context "with forwarded emails enabled" do
-      before { SiteSetting.enable_forwarded_emails = true }
+      before do
+        Fabricate(:group, incoming_email: "some_group@bar.com")
+        SiteSetting.enable_forwarded_emails = true
+      end
 
       it "handles forwarded emails" do
         expect { process(:forwarded_email_1) }.to change(Topic, :count)
@@ -1020,8 +1028,18 @@ describe Email::Receiver do
       SiteSetting.enable_staged_users = true
     end
 
-    shared_examples "no staged users" do |email_name, expected_exception|
+    shared_examples "does not create staged users" do |email_name, expected_exception|
       it "does not create staged users" do
+        staged_user_count = User.where(staged: true).count
+        User.expects(:create).never
+        User.expects(:create!).never
+        expect { process(email_name) }.to raise_error(expected_exception)
+        expect(User.where(staged: true).count).to eq(staged_user_count)
+      end
+    end
+
+    shared_examples "cleans up staged users" do |email_name, expected_exception|
+      it "cleans up staged users" do
         staged_user_count = User.where(staged: true).count
         expect { process(email_name) }.to raise_error(expected_exception)
         expect(User.where(staged: true).count).to eq(staged_user_count)
@@ -1033,39 +1051,41 @@ describe Email::Receiver do
         ScreenedEmail.expects(:should_block?).with("screened@mail.com").returns(true)
       end
 
-      include_examples "no staged users", :screened_email, Email::Receiver::ScreenedEmailError
+      include_examples "does not create staged users", :screened_email, Email::Receiver::ScreenedEmailError
     end
 
     context "when the mail is auto generated" do
-      include_examples "no staged users", :auto_generated_header, Email::Receiver::AutoGeneratedEmailError
+      include_examples "does not create staged users", :auto_generated_header, Email::Receiver::AutoGeneratedEmailError
     end
 
     context "when email is a bounced email" do
-      include_examples "no staged users", :bounced_email, Email::Receiver::BouncedEmailError
+      include_examples "does not create staged users", :bounced_email, Email::Receiver::BouncedEmailError
     end
 
     context "when the body is blank" do
-      include_examples "no staged users", :no_body, Email::Receiver::NoBodyDetectedError
+      include_examples "does not create staged users", :no_body, Email::Receiver::NoBodyDetectedError
     end
 
     context "when unsubscribe via email is not allowed" do
-      include_examples "no staged users", :unsubscribe_new_user, Email::Receiver::UnsubscribeNotAllowed
+      include_examples "does not create staged users", :unsubscribe_new_user, Email::Receiver::UnsubscribeNotAllowed
     end
 
     context "when From email address is not on whitelist" do
       before do
         SiteSetting.email_domains_whitelist = "example.com|bar.com"
+        Fabricate(:group, incoming_email: "some_group@bar.com")
       end
 
-      include_examples "no staged users", :blacklist_whitelist_email, Email::Receiver::EmailNotAllowed
+      include_examples "does not create staged users", :blacklist_whitelist_email, Email::Receiver::EmailNotAllowed
     end
 
     context "when From email address is on blacklist" do
       before do
         SiteSetting.email_domains_blacklist = "email.com|mail.com"
+        Fabricate(:group, incoming_email: "some_group@bar.com")
       end
 
-      include_examples "no staged users", :blacklist_whitelist_email, Email::Receiver::EmailNotAllowed
+      include_examples "does not create staged users", :blacklist_whitelist_email, Email::Receiver::EmailNotAllowed
     end
 
     context "blacklist and whitelist for To and Cc" do
@@ -1093,20 +1113,24 @@ describe Email::Receiver do
     end
 
     context "when destinations aren't matching any of the incoming emails" do
-      include_examples "no staged users", :bad_destinations, Email::Receiver::BadDestinationAddress
+      include_examples "does not create staged users", :bad_destinations, Email::Receiver::BadDestinationAddress
     end
 
     context "when email is sent to category" do
       context "when email is sent by a new user and category does not allow strangers" do
         let!(:category) { Fabricate(:category, email_in: "category@foo.com", email_in_allow_strangers: false) }
 
-        include_examples "no staged users", :new_user, Email::Receiver::StrangersNotAllowedError
+        include_examples "does not create staged users", :new_user, Email::Receiver::StrangersNotAllowedError
       end
 
       context "when email has no date" do
         let!(:category) { Fabricate(:category, email_in: "category@foo.com", email_in_allow_strangers: true) }
 
-        include_examples "no staged users", :no_date, Email::Receiver::InvalidPost
+        it "includes the translated string in the error" do
+          expect { process(:no_date) }.to raise_error(Email::Receiver::InvalidPost).with_message(I18n.t("system_messages.email_reject_invalid_post_specified.date_invalid"))
+        end
+
+        include_examples "does not create staged users", :no_date, Email::Receiver::InvalidPost
       end
     end
 
@@ -1114,6 +1138,7 @@ describe Email::Receiver do
       let(:reply_key) { "4f97315cc828096c9cb34c6f1a0d6fe8" }
       let(:category) { Fabricate(:category) }
       let(:user) { Fabricate(:user, email: "discourse@bar.com") }
+      let!(:user2) { Fabricate(:user, email: "someone_else@bar.com") }
       let(:topic) { create_topic(category: category, user: user) }
       let(:post) { create_post(topic: topic, user: user) }
 
@@ -1122,7 +1147,7 @@ describe Email::Receiver do
       end
 
       context "when the email address isn't matching the one we sent the notification to" do
-        include_examples "no staged users", :reply_user_not_matching, Email::Receiver::ReplyUserNotMatchingError
+        include_examples "does not create staged users", :reply_user_not_matching, Email::Receiver::ReplyUserNotMatchingError
       end
     end
 
@@ -1139,7 +1164,7 @@ describe Email::Receiver do
           topic.update_columns(deleted_at: 1.day.ago)
         end
 
-        include_examples "no staged users", :email_reply_staged, Email::Receiver::TopicNotFoundError
+        include_examples "cleans up staged users", :email_reply_staged, Email::Receiver::TopicNotFoundError
       end
 
       context "when the topic was closed" do
@@ -1147,7 +1172,7 @@ describe Email::Receiver do
           topic.update_columns(closed: true)
         end
 
-        include_examples "no staged users", :email_reply_staged, Email::Receiver::TopicClosedError
+        include_examples "cleans up staged users", :email_reply_staged, Email::Receiver::TopicClosedError
       end
 
       context "when they aren't allowed to like a post" do
@@ -1155,7 +1180,7 @@ describe Email::Receiver do
           topic.update_columns(archived: true)
         end
 
-        include_examples "no staged users", :email_reply_like, Email::Receiver::InvalidPostAction
+        include_examples "cleans up staged users", :email_reply_like, Email::Receiver::InvalidPostAction
       end
     end
 

spec/fixtures/emails/old_destination.eml

@@ -1,5 +1,5 @@
-Return-Path: <staged@bar.com>
+Return-Path: <discourse@bar.com>
-From: Foo Bar <staged@bar.com>
+From: Foo Bar <discourse@bar.com>
 To: reply+4f97315cc828096c9cb34c6f1a0d6fe8@bar.com
 Cc: foofoo@bar.com
 Date: Fri, 15 Jan 2018 00:12:43 +0100