From 76706f91447de810f6aa3b618a2f5fe8c0924b4b Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Tue, 3 Oct 2017 10:13:19 +0200 Subject: [PATCH] FIX: don't create staged users when incoming email is rejected FIX: don't send subscription mail to new users --- config/locales/server.en.yml | 1 + lib/email/processor.rb | 3 +- lib/email/receiver.rb | 49 ++++++++++++++----- spec/components/email/receiver_spec.rb | 44 +++++++++++++++++ spec/fixtures/emails/unsubscribe_new_user.eml | 11 +++++ 5 files changed, 94 insertions(+), 14 deletions(-) create mode 100644 spec/fixtures/emails/unsubscribe_new_user.eml diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index dbcb7f8ba15..245b329690d 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -79,6 +79,7 @@ en: topic_closed_error: "Happens when a reply came in but the related topic has been closed." bounced_email_error: "Email is a bounced email report." screened_email_error: "Happens when the sender's email address was already screened." + unsubscribe_not_allowed: "Happens when unsubscribing via email is not allowed for this user." unrecognized_error: "Unrecognized Error" errors: &errors diff --git a/lib/email/processor.rb b/lib/email/processor.rb index 20d2704da6a..f1ee881531f 100644 --- a/lib/email/processor.rb +++ b/lib/email/processor.rb @@ -49,7 +49,8 @@ module Email when Email::Receiver::ReplyUserNotMatchingError then :email_reject_reply_user_not_matching when Email::Receiver::TopicNotFoundError then :email_reject_topic_not_found when Email::Receiver::TopicClosedError then :email_reject_topic_closed - when Email::Receiver::InvalidPost then :email_reject_invalid_post + when Email::Receiver::InvalidPost then :email_reject_invalid_pos + when Email::Receiver::UnsubscribeNotAllowed then :email_reject_invalid_post when ActiveRecord::Rollback then :email_reject_invalid_post when Email::Receiver::InvalidPostAction then :email_reject_invalid_post_action when Discourse::InvalidAccess then :email_reject_invalid_access diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 7fae5ec3b52..8405bde4a4a 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -30,6 +30,7 @@ module Email class TopicClosedError < ProcessingError; end class InvalidPost < ProcessingError; end class InvalidPostAction < ProcessingError; end + class UnsubscribeNotAllowed < ProcessingError; end attr_reader :incoming_email attr_reader :raw_email @@ -38,7 +39,7 @@ module Email def initialize(mail_string) raise EmptyEmailError if mail_string.blank? - @staged_users_created = 0 + @staged_users = [] @raw_email = try_to_encode(mail_string, "UTF-8") || try_to_encode(mail_string, "ISO-8859-1") || mail_string @mail = Mail.new(@raw_email) @message_id = @mail.message_id.presence || Digest::MD5.hexdigest(mail_string) @@ -82,14 +83,13 @@ module Email raise NoSenderDetectedError if @from_email.blank? raise ScreenedEmailError if ScreenedEmail.should_block?(@from_email) - user = find_or_create_user(@from_email, @from_display_name) + user = find_user(@from_email) - raise UserNotFoundError if user.nil? - - @incoming_email.update_columns(user_id: user.id) - - raise InactiveUserError if !user.active && !user.staged - raise BlockedUserError if user.blocked + if user.present? + process_user(user) + else + raise UserNotFoundError unless SiteSetting.enable_staged_users + end body, elided = select_body body ||= "" @@ -102,9 +102,17 @@ module Email end if action = subscription_action_for(body, subject) - message = SubscriptionMailer.send(action, user) - Email::Sender.new(message, :subscription).send - elsif post = find_related_post + raise UnsubscribeNotAllowed if user.nil? + send_subscription_mail(action, user) + 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. + user = find_or_create_user(@from_email, @from_display_name) if user.nil? + process_user(user) + + if post = find_related_post create_reply(user: user, raw: body, elided: elided, @@ -128,6 +136,13 @@ module Email end end + def process_user(user) + @incoming_email.update_columns(user_id: user.id) + + raise InactiveUserError if !user.active && !user.staged + raise BlockedUserError if user.blocked + end + def is_bounce? return false unless @mail.bounced? || verp @@ -310,6 +325,10 @@ module Email @suject ||= @mail.subject.presence || I18n.t("emails.incoming.default_subject", email: @from_email) end + def find_user(email) + User.find_by_email(email) + end + def find_or_create_user(email, display_name) user = nil @@ -325,7 +344,7 @@ module Email name: display_name.presence || User.suggest_name(email), staged: true ) - @staged_users_created += 1 + @staged_users << user end rescue user = nil @@ -693,7 +712,7 @@ module Email topic.add_small_action(sender, "invited_user", user.username) end # cap number of staged users created per email - if @staged_users_created > SiteSetting.maximum_staged_users_per_email + if @staged_users.count > SiteSetting.maximum_staged_users_per_email topic.add_moderator_post(sender, I18n.t("emails.incoming.maximum_staged_user_per_email_reached")) return end @@ -717,6 +736,10 @@ module Email !topic.topic_allowed_groups.where("group_id IN (SELECT group_id FROM group_users WHERE user_id = ?)", user.id).exists? end + def send_subscription_mail(action, user) + message = SubscriptionMailer.send(action, user) + Email::Sender.new(message, :subscription).send + end end end diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 4a13ac13b08..40dc01a0f3f 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -300,6 +300,12 @@ describe Email::Receiver do expect(before_deliveries).to eq ActionMailer::Base.deliveries.count end end + + it "raises an UnsubscribeNotAllowed and does not send an unsubscribe email" do + before_deliveries = ActionMailer::Base.deliveries.count + expect { process(:unsubscribe_new_user) }.to raise_error { Email::Receiver::UnsubscribeNotAllowed } + expect(before_deliveries).to eq ActionMailer::Base.deliveries.count + end end it "handles inline reply" do @@ -623,4 +629,42 @@ describe Email::Receiver do end end + context "no staged users on error" do + before do + SiteSetting.enable_staged_users = true + end + + shared_examples "no staged users" do |email_name| + it "does not create staged users" do + staged_user_count = User.where(staged: true).count + process(email_name) rescue nil + expect(User.where(staged: true).count).to eq(staged_user_count) + end + end + + context "when email address is screened" do + before do + ScreenedEmail.expects(:should_block?).with("screened@mail.com").returns(true) + end + + include_examples "no staged users", :screened_email + end + + context "when the mail is auto generated" do + include_examples "no staged users", :auto_generated_header + end + + context "when email is a bounced email" do + include_examples "no staged users", :bounced_email + end + + context "when the body is blank" do + include_examples "no staged users", :no_body + end + + context "when unsubscribe via email is not allowed" do + include_examples "no staged users", :unsubscribe_new_user + end + end + end diff --git a/spec/fixtures/emails/unsubscribe_new_user.eml b/spec/fixtures/emails/unsubscribe_new_user.eml new file mode 100644 index 00000000000..337f2c88b66 --- /dev/null +++ b/spec/fixtures/emails/unsubscribe_new_user.eml @@ -0,0 +1,11 @@ +Return-Path: +From: Foo Bar +To: reply@bar.com +Date: Thu, 13 Jun 2013 17:03:48 -0400 +Message-ID: <56@foo.bar.mail> +Subject: UnSuBScRiBe +Mime-Version: 1.0 +Content-Type: text/plain; +Content-Transfer-Encoding: 7bit + +I've basically had enough of your mailing list and would very much like it if you went away.