diff --git a/app/jobs/scheduled/poll_mailbox.rb b/app/jobs/scheduled/poll_mailbox.rb index 4dcf3f2882c..552833731ab 100644 --- a/app/jobs/scheduled/poll_mailbox.rb +++ b/app/jobs/scheduled/poll_mailbox.rb @@ -62,7 +62,6 @@ module Jobs message_template = :email_reject_post_error_specified template_args[:post_error] = e.message end - else message_template = nil end diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 7ad4e0049e3..c2c89d2288f 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -18,6 +18,8 @@ module Email class AutoGeneratedEmailError < ProcessingError; end class EmailLogNotFound < ProcessingError; end class InvalidPost < ProcessingError; end + class ReplyUserNotFoundError < ProcessingError; end + class ReplyUserNotMatchingError < ProcessingError; end attr_reader :body, :email_log @@ -29,71 +31,78 @@ module Email def process raise EmptyEmailError if @raw.blank? - message = Mail.new(@raw) + @message = Mail.new(@raw) - body = parse_body(message) + raise AutoGeneratedEmailError if @message.header.to_s =~ /auto-(replied|generated)/ + + @body = parse_body(@message) - dest_info = { type: :invalid, obj: nil } # 'smtp_envelope_to' is a combination of: to, cc and bcc fields - message.smtp_envelope_to.each do |to_address| - dest_info = check_address(to_address) - break if dest_info[:type] != :invalid - end + # prioriziting the `:reply` types + dest_infos = @message.smtp_envelope_to + .map { |to_address| check_address(to_address) } + .compact + .sort do |a, b| + if a[:type] == :reply && b[:type] != :reply + 1 + elsif a[:type] != :reply && b[:type] == :reply + -1 + else + 0 + end + end - raise BadDestinationAddress if dest_info[:type] == :invalid - raise AutoGeneratedEmailError if message.header.to_s =~ /auto-generated/ || message.header.to_s =~ /auto-replied/ - - # TODO get to a state where we can remove this - @message = message - @body = body + raise BadDestinationAddress if dest_infos.empty? from = @message[:from].address_list.addresses.first user_email = "#{from.local}@#{from.domain}" user_name = from.display_name - @user = User.find_by_email(user_email) + # TODO: deal with suspended/inactive users + user = User.find_by_email(user_email) + # TODO: take advantage of all the "TO"s + dest_info = dest_infos[0] case dest_info[:type] when :group group = dest_info[:obj] - if @user.blank? + if user.blank? if SiteSetting.allow_staged_accounts - @user = create_staged_account(user_email, user_name) + user = create_staged_account(user_email, user_name) else wrap_body_in_quote(user_email) - @user = Discourse.system_user + user = Discourse.system_user end end - raise UserNotFoundError if @user.blank? - raise UserNotSufficientTrustLevelError.new(@user) unless @user.has_trust_level?(TrustLevel[SiteSetting.email_in_min_trust.to_i]) - - create_new_topic(archetype: Archetype.private_message, target_group_names: [group.name]) + create_new_topic(user, archetype: Archetype.private_message, target_group_names: [group.name]) when :category category = dest_info[:obj] - if @user.blank? && category.email_in_allow_strangers + if user.blank? && category.email_in_allow_strangers if SiteSetting.allow_staged_accounts - @user = create_staged_account(user_email) + user = create_staged_account(user_email) else wrap_body_in_quote(user_email) - @user = Discourse.system_user + user = Discourse.system_user end end - raise UserNotFoundError if @user.blank? - raise UserNotSufficientTrustLevelError.new(@user) unless category.email_in_allow_strangers || @user.has_trust_level?(TrustLevel[SiteSetting.email_in_min_trust.to_i]) + raise UserNotFoundError if user.blank? + raise UserNotSufficientTrustLevelError.new(user) unless category.email_in_allow_strangers || user.has_trust_level?(TrustLevel[SiteSetting.email_in_min_trust.to_i]) - create_new_topic(category: category.id) + create_new_topic(user, category: category.id) when :reply @email_log = dest_info[:obj] - raise EmailLogNotFound if @email_log.blank? - raise TopicNotFoundError if Topic.find_by_id(@email_log.topic_id).nil? - raise TopicClosedError if Topic.find_by_id(@email_log.topic_id).closed? + raise EmailLogNotFound if @email_log.blank? + raise TopicNotFoundError if Topic.find_by_id(@email_log.topic_id).nil? + raise TopicClosedError if Topic.find_by_id(@email_log.topic_id).closed? + raise ReplyUserNotFoundError if user.blank? + raise ReplyUserNotMatchingError if @email_log.user_id != user.id - create_reply + create_reply(@email_log) end rescue Encoding::UndefinedConversionError, Encoding::InvalidByteSequenceError => e @@ -110,37 +119,36 @@ module Email end def check_address(address) - # only check groups/categories when 'email_in' is enabled + # only check for a group/category when 'email_in' is enabled if SiteSetting.email_in group = Group.find_by_email(address) - return { type: :group, obj: group } if group + return { address: address, type: :group, obj: group } if group category = Category.find_by_email(address) - return { type: :category, obj: category } if category + return { address: address, type: :category, obj: category } if category end - regex = Regexp.escape(SiteSetting.reply_by_email_address) - regex = regex.gsub(Regexp.escape('%{reply_key}'), "(.*)") - regex = Regexp.new(regex) - match = regex.match address + match = reply_by_email_address_regex.match(address) if match && match[1].present? - reply_key = match[1] - email_log = EmailLog.for(reply_key) - return { type: :reply, obj: email_log } + email_log = EmailLog.for(match[1]) + return { address: address, type: :reply, obj: email_log } end + end - { type: :invalid, obj: nil } + def reply_by_email_address_regex + @reply_by_email_address_regex ||= Regexp.new Regexp.escape(SiteSetting.reply_by_email_address) + .gsub(Regexp.escape("%{reply_key}"), "([[:xdigit:]]{32})") end def parse_body(message) - body = select_body message + body = select_body(message) encoding = body.encoding raise EmptyEmailError if body.strip.blank? - body = discourse_email_trimmer body + body = discourse_email_trimmer(body) raise EmptyEmailError if body.strip.blank? - body = DiscourseEmailParser.parse_reply body + body = DiscourseEmailParser.parse_reply(body) raise EmptyEmailError if body.strip.blank? body.force_encoding(encoding).encode("UTF-8") @@ -187,7 +195,7 @@ module Email nil end - REPLYING_HEADER_LABELS = ['From', 'Sent', 'To', 'Subject', 'Reply To', 'Cc', 'Bcc', 'Date'] + REPLYING_HEADER_LABELS = ['From', 'Sent', 'To', 'Subject', 'In-Reply-To', 'Cc', 'Bcc', 'Date'] REPLYING_HEADER_REGEX = Regexp.union(REPLYING_HEADER_LABELS.map { |lbl| "#{lbl}:" }) def line_is_quote?(l) @@ -230,25 +238,25 @@ module Email @body = "[quote=\"#{user_email}\"]\n#{@body}\n[/quote]" end - def create_reply - create_post_with_attachments(@email_log.user, + def create_reply(email_log) + create_post_with_attachments(email_log.user, raw: @body, - topic_id: @email_log.topic_id, - reply_to_post_number: @email_log.post.post_number) + topic_id: email_log.topic_id, + reply_to_post_number: email_log.post.post_number) end - def create_new_topic(topic_options={}) + def create_new_topic(user, topic_options={}) topic_options[:raw] = @body topic_options[:title] = @message.subject - result = create_post_with_attachments(@user, topic_options) + result = create_post_with_attachments(user, topic_options) topic_id = result.post.present? ? result.post.topic_id : nil EmailLog.create( email_type: "topic_via_incoming_email", - to_address: @message.from.first, # pick from address because we want the user's email + to_address: user.email, topic_id: topic_id, - user_id: @user.id, + user_id: user.id, ) result diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 35a94c14c5e..97096d3f480 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -262,6 +262,7 @@ This is a link http://example.com" let(:reply_key) { raise "Override this in a lower describe block" } let(:email_raw) { raise "Override this in a lower describe block" } # ---- + let(:to) { SiteSetting.reply_by_email_address.gsub("%{reply_key}", reply_key) } let(:receiver) { Email::Receiver.new(email_raw) } let(:post) { create_post } let(:topic) { post.topic } @@ -286,7 +287,7 @@ This is a link http://example.com" describe "valid_reply.eml" do let!(:reply_key) { '59d8df8370b7e95c5a49fbf86aeb2c93' } - let!(:email_raw) { fixture_file("emails/valid_reply.eml") } + let!(:email_raw) { fill_email(fixture_file("emails/valid_reply.eml"), replying_user_email, to) } it "creates a post with the correct content" do start_count = topic.posts.count @@ -296,7 +297,6 @@ This is a link http://example.com" expect(topic.posts.count).to eq(start_count + 1) created_post = topic.posts.last expect(created_post.via_email).to eq(true) - expect(created_post.raw_email).to eq(fixture_file("emails/valid_reply.eml")) expect(created_post.cooked.strip).to eq(fixture_file("emails/valid_reply.cooked").strip) end end @@ -386,6 +386,7 @@ This is a link http://example.com" describe "posting reply to a closed topic" do let(:reply_key) { raise "Override this in a lower describe block" } let(:email_raw) { raise "Override this in a lower describe block" } + let(:to) { SiteSetting.reply_by_email_address.gsub("%{reply_key}", reply_key) } let(:receiver) { Email::Receiver.new(email_raw) } let(:topic) { Fabricate(:topic, closed: true) } let(:post) { Fabricate(:post, topic: topic, post_number: 1) } @@ -407,7 +408,7 @@ This is a link http://example.com" describe "should not create post" do let!(:reply_key) { '59d8df8370b7e95c5a49fbf86aeb2c93' } - let!(:email_raw) { fixture_file("emails/valid_reply.eml") } + let!(:email_raw) { fill_email(fixture_file("emails/valid_reply.eml"), replying_user_email, to) } it "raises a TopicClosedError" do expect { receiver.process }.to raise_error(Email::Receiver::TopicClosedError) end @@ -417,6 +418,7 @@ This is a link http://example.com" describe "posting reply to a deleted topic" do let(:reply_key) { raise "Override this in a lower describe block" } let(:email_raw) { raise "Override this in a lower describe block" } + let(:to) { SiteSetting.reply_by_email_address.gsub("%{reply_key}", reply_key) } let(:receiver) { Email::Receiver.new(email_raw) } let(:deleted_topic) { Fabricate(:deleted_topic) } let(:post) { Fabricate(:post, topic: deleted_topic, post_number: 1) } @@ -438,7 +440,7 @@ This is a link http://example.com" describe "should not create post" do let!(:reply_key) { '59d8df8370b7e95c5a49fbf86aeb2c93' } - let!(:email_raw) { fixture_file("emails/valid_reply.eml") } + let!(:email_raw) { fill_email(fixture_file("emails/valid_reply.eml"), replying_user_email, to) } it "raises a TopicNotFoundError" do expect { receiver.process }.to raise_error(Email::Receiver::TopicNotFoundError) end @@ -499,16 +501,16 @@ This is a link http://example.com" describe "with a valid email" do let(:reply_key) { "59d8df8370b7e95c5a49fbf86aeb2c93" } let(:to) { SiteSetting.reply_by_email_address.gsub("%{reply_key}", reply_key) } + let(:user_email) { "test@test.com" } + let(:user) { Fabricate(:user, email: user_email, trust_level: 2)} + let(:post) { create_post(user: user) } let(:valid_reply) { reply = fixture_file("emails/valid_reply.eml") - to = SiteSetting.reply_by_email_address.gsub("%{reply_key}", reply_key) - fill_email(reply, "test@test.com", to) + fill_email(reply, user.email, to) } let(:receiver) { Email::Receiver.new(valid_reply) } - let(:post) { create_post } - let(:user) { post.user } let(:email_log) { EmailLog.new(reply_key: reply_key, post_id: post.id, topic_id: post.topic_id, @@ -516,7 +518,7 @@ This is a link http://example.com" post: post, user: user, email_type: 'test', - to_address: 'test@test.com' + to_address: user.email ) } let(:reply_body) { "I could not disagree more. I am obviously biased but adventure time is the @@ -527,7 +529,7 @@ greatest show ever created. Everyone should watch it. describe "with an email log" do it "extracts data" do - expect{ receiver.process }.to raise_error(Email::Receiver::EmailLogNotFound) + expect { receiver.process }.to raise_error(Email::Receiver::EmailLogNotFound) email_log.save! receiver.process @@ -540,6 +542,34 @@ greatest show ever created. Everyone should watch it. end + describe "with a valid email from a different user" do + let(:reply_key) { SecureRandom.hex(16) } + let(:to) { SiteSetting.reply_by_email_address.gsub("%{reply_key}", reply_key) } + let(:user) { Fabricate(:user, email: "test@test.com", trust_level: 2)} + let(:post) { create_post(user: user) } + let!(:email_log) { EmailLog.create(reply_key: reply_key, + post_id: post.id, + topic_id: post.topic_id, + user_id: post.user_id, + post: post, + user: user, + email_type: 'test', + to_address: user.email) } + + it "raises ReplyUserNotFoundError when user doesn't exist" do + reply = fill_email(fixture_file("emails/valid_reply.eml"), "unknown@user.com", to) + receiver = Email::Receiver.new(reply) + expect { receiver.process }.to raise_error(Email::Receiver::ReplyUserNotFoundError) + end + + it "raises ReplyUserNotMatchingError when user is not matching the reply key" do + another_user = Fabricate(:user, email: "existing@user.com") + reply = fill_email(fixture_file("emails/valid_reply.eml"), another_user.email, to) + receiver = Email::Receiver.new(reply) + expect { receiver.process }.to raise_error(Email::Receiver::ReplyUserNotMatchingError) + end + end + describe "processes an email to a category" do let(:to) { "some@email.com" } diff --git a/spec/fixtures/emails/valid_reply.eml b/spec/fixtures/emails/valid_reply.eml index 1e696389954..786b1d85169 100644 --- a/spec/fixtures/emails/valid_reply.eml +++ b/spec/fixtures/emails/valid_reply.eml @@ -1,11 +1,11 @@ -Return-Path: +Return-Path: Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400 Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for ; Thu, 13 Jun 2013 17:03:50 -0400 Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for ; Thu, 13 Jun 2013 14:03:48 -0700 Received: by 10.0.0.1 with HTTP; Thu, 13 Jun 2013 14:03:48 -0700 Date: Thu, 13 Jun 2013 17:03:48 -0400 -From: Jake the Dog -To: reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo +From: FROM +To: TO Message-ID: Subject: re: [Discourse Meta] eviltrout posted in 'Adventure Time Sux' Mime-Version: 1.0 @@ -37,4 +37,4 @@ On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta > Please visit this link to respond: http://localhost:3000/t/adventure-time-sux/1234/3 > > To unsubscribe from these emails, visit your [user preferences](http://localhost:3000/user_preferences). -> \ No newline at end of file +>