From 6a05f190c684ec71fa13cdc02c92ddea0b532adb Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 8 Apr 2019 10:36:39 +0100 Subject: [PATCH] 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 | 1 + lib/email/receiver.rb | 50 +++++++++++++++------ spec/components/email/processor_spec.rb | 8 ++-- spec/components/email/receiver_spec.rb | 57 +++++++++++++++++------- spec/fixtures/emails/old_destination.eml | 4 +- 5 files changed, 84 insertions(+), 36 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index cf0745a6d8f..5a10ebeab0b 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 03e9c845ef5..1e98de90ac3 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 c31a70fa7b2..ef33fd7d8f8 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 dbdc5082fc8..7d4568c2f3c 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 @@ -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: @@ -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 diff --git a/spec/fixtures/emails/old_destination.eml b/spec/fixtures/emails/old_destination.eml index fb21321d32c..ee51811d05e 100644 --- a/spec/fixtures/emails/old_destination.eml +++ b/spec/fixtures/emails/old_destination.eml @@ -1,5 +1,5 @@ -Return-Path: -From: Foo Bar +Return-Path: +From: Foo Bar To: reply+4f97315cc828096c9cb34c6f1a0d6fe8@bar.com Cc: foofoo@bar.com Date: Fri, 15 Jan 2018 00:12:43 +0100