github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-01-30 03:21:02 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

FIX: Ensure group SMTP and message builder always uses from address for Reply-To when IMAP is enabled (#11037)

Browse Source 
There is a site setting reply_by_email_enabled which when combined with reply_by_email_address creates a Reply-To header in emails in the format "test+%{reply_key}@test.com" along with a PostReplyKey record, so when replying Discourse knows where to route the reply.

However this conflicts with the IMAP implementation. Since we are sending the email for a group via SMTP and from their actual email account, we want all replys to go to that email account as well so the IMAP sync job can pick them up and put them in the correct place. So if the group has IMAP enabled and configured, then the reply-to header will be correct.

This PR also makes a further fix to 64b0b50 by using the correct recipient user for the PostReplyKey record. If the post user is used we encounter this error:

if destination.user_id != user.id && !forwarded_reply_key?(destination, user)
  raise ReplyUserNotMatchingError, "post_reply_key.user_id => #{destination.user_id.inspect}, user.id => #{user.id.inspect}"
end
This is because the user above is found from the from_address, but the destination which is the PostReplyKey is made by the post.user, which will be different people.
This commit is contained in:
Martin Brennan 2020-10-28 07:01:58 +10:00 committed by GitHub
parent 12724ac6e4
commit 632942e697
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 120 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
app/jobs/regular/group_smtp_email.rb
View File

@ -10,10 +10,11 @@ module Jobs
group = Group.find_by(id: args[:group_id])
post = Post.find_by(id: args[:post_id])
email = args[:email]
recipient_user = ::UserEmail.find_by(email: email, primary: true)&.user
Rails.logger.debug("[IMAP] Sending email for group #{group.name} and post #{post.id}")
message = GroupSmtpMailer.send_mail(group, email, post)
Email::Sender.new(message, :group_smtp, post.user).send
Email::Sender.new(message, :group_smtp, recipient_user).send
# Create an incoming email record to avoid importing again from IMAP
# server.

1
app/mailers/group_smtp_mailer.rb
View File

@ -48,6 +48,7 @@ class GroupSmtpMailer < ActionMailer::Base
group_name: from_group.name,
allow_reply_by_email: true,
only_reply_by_email: true,
use_from_address_for_reply_to: from_group.imap_enabled?,
private_reply: post.topic.private_message?,
participants: participants(post),
include_respond_instructions: true,

5
app/models/group.rb
View File

@ -817,6 +817,11 @@ class Group < ActiveRecord::Base
}
end
def imap_enabled?
return false if !SiteSetting.enable_imap
imap_config.values.compact.length == imap_config.keys.length
end
def email_username_regex
user, domain = email_username.split('@')
if user.present? && domain.present?

2
lib/email/message_builder.rb
View File

@ -153,7 +153,7 @@ module Email
# please, don't send us automatic responses...
result['X-Auto-Response-Suppress'] = 'All'
if allow_reply_by_email?
if allow_reply_by_email? && !@opts[:use_from_address_for_reply_to]
result[ALLOW_REPLY_BY_EMAIL_HEADER] = true
result['Reply-To'] = reply_by_email_address
else

5
spec/jobs/regular/group_smtp_email_spec.rb
View File

@ -5,6 +5,7 @@ require 'rails_helper'
RSpec.describe Jobs::GroupSmtpEmail do
let(:post) { Fabricate(:post) }
let(:group) { Fabricate(:imap_group) }
let!(:recipient_user) { Fabricate(:user, email: "test@test.com") }
let(:args) do
{
group_id: group.id,
@ -23,7 +24,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
it "sends an email using the GroupSmtpMailer and Email::Sender" do
message = Mail::Message.new(body: "hello", to: "myemail@example.invalid")
GroupSmtpMailer.expects(:send_mail).with(group, "test@test.com", post).returns(message)
Email::Sender.expects(:new).with(message, :group_smtp, post.user).returns(stub(send: nil))
Email::Sender.expects(:new).with(message, :group_smtp, recipient_user).returns(stub(send: nil))
subject.execute(args)
end
@ -37,7 +38,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do
subject.execute(args)
incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id)
post_reply_key = PostReplyKey.where(user_id: post.user_id, post_id: post.id).first
post_reply_key = PostReplyKey.where(user_id: recipient_user, post_id: post.id).first
expect(post_reply_key).not_to eq(nil)
expect(incoming.raw).to include("Reply-To: Discourse <test+#{post_reply_key.reply_key}@incoming.com>")
end

84
spec/mailers/group_smtp_mailer_spec.rb
View File

@ -5,18 +5,18 @@ require 'email/receiver'
describe GroupSmtpMailer do
let(:group) {
Fabricate(:group,
name: 'Testers',
title: 'Tester',
full_name: 'Testers Group',
smtp_server: 'smtp.gmail.com',
smtp_port: 587,
smtp_ssl: true,
imap_server: 'imap.gmail.com',
imap_port: 993,
imap_ssl: true,
email_username: 'bugs@gmail.com',
email_password: 'super$secret$password'
)
name: 'Testers',
title: 'Tester',
full_name: 'Testers Group',
smtp_server: 'smtp.gmail.com',
smtp_port: 587,
smtp_ssl: true,
imap_server: 'imap.gmail.com',
imap_port: 993,
imap_ssl: true,
email_username: 'bugs@gmail.com',
email_password: 'super$secret$password'
)
}
let(:user) {
@ -44,33 +44,75 @@ describe GroupSmtpMailer do
let(:receiver) {
receiver = Email::Receiver.new(email,
destinations: [group],
uid_validity: 1,
uid: 10000
)
receiver.process!
receiver
destinations: [group],
uid_validity: 1,
uid: 10000
)
receiver.process!
receiver
}
let(:raw) { 'hello, how are you doing?' }
before do
SiteSetting.enable_smtp = true
SiteSetting.enable_imap = true
Jobs.run_immediately!
end
it 'sends an email as reply' do
post = PostCreator.create(user,
topic_id: receiver.incoming_email.topic.id,
raw: raw
)
topic_id: receiver.incoming_email.topic.id,
raw: raw
)
expect(ActionMailer::Base.deliveries.size).to eq(1)
sent_mail = ActionMailer::Base.deliveries[0]
expect(sent_mail.to).to contain_exactly('john@doe.com')
expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com')
expect(sent_mail.subject).to eq('Re: Hello from John')
expect(sent_mail.to_s).to include(raw)
end
context "when the site has a reply by email address configured" do
before do
SiteSetting.manual_polling_enabled = true
SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com"
SiteSetting.reply_by_email_enabled = true
end
it 'uses the correct IMAP/SMTP reply to address' do
post = PostCreator.create(user,
topic_id: receiver.incoming_email.topic.id,
raw: raw
)
expect(ActionMailer::Base.deliveries.size).to eq(1)
sent_mail = ActionMailer::Base.deliveries[0]
expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com')
end
context "when IMAP is disabled for the group" do
before do
group.update(
imap_server: nil
)
end
it "uses the reply key based reply to address" do
post = PostCreator.create(user,
topic_id: receiver.incoming_email.topic.id,
raw: raw
)
expect(ActionMailer::Base.deliveries.size).to eq(1)
sent_mail = ActionMailer::Base.deliveries[0]
post_reply_key = PostReplyKey.last
expect(sent_mail.reply_to).to contain_exactly("test+#{post_reply_key.reply_key}@test.com")
end
end
end
end

74
spec/models/group_spec.rb
View File

@ -973,7 +973,7 @@ describe Group do
end
end
describe "#imap_mailboxes" do
describe "IMAP" do
let(:group) { Fabricate(:group) }
def mock_imap
@ -1010,39 +1010,55 @@ describe Group do
Discourse.redis.del("group_imap_mailboxes_#{group.id}")
end
it "returns an empty array if group imap is not configured" do
expect(group.imap_mailboxes).to eq([])
describe "#imap_enabled?" do
it "returns true if imap is configured and enabled for the site" do
mock_imap
configure_imap
enable_imap
expect(group.imap_enabled?).to eq(true)
end
it "returns false if imap is configured and not enabled for the site" do
configure_imap
expect(group.imap_enabled?).to eq(false)
end
end
it "returns an empty array and does not contact IMAP server if group imap is configured but the setting is disabled" do
configure_imap
Imap::Providers::Detector.expects(:init_with_detected_provider).never
expect(group.imap_mailboxes).to eq([])
end
describe "#imap_mailboxes" do
it "returns an empty array if group imap is not configured" do
expect(group.imap_mailboxes).to eq([])
end
it "logs the imap error if one occurs" do
configure_imap
mock_imap
SiteSetting.enable_imap = true
@mocked_imap_provider.stubs(:connect!).raises(Net::IMAP::NoResponseError)
group.imap_mailboxes
expect(group.reload.imap_last_error).not_to eq(nil)
end
it "returns an empty array and does not contact IMAP server if group imap is configured but the setting is disabled" do
configure_imap
Imap::Providers::Detector.expects(:init_with_detected_provider).never
expect(group.imap_mailboxes).to eq([])
end
it "returns a list of mailboxes from the IMAP provider" do
configure_imap
mock_imap
enable_imap
expect(group.imap_mailboxes).to eq(["Inbox"])
end
it "logs the imap error if one occurs" do
configure_imap
mock_imap
SiteSetting.enable_imap = true
@mocked_imap_provider.stubs(:connect!).raises(Net::IMAP::NoResponseError)
group.imap_mailboxes
expect(group.reload.imap_last_error).not_to eq(nil)
end
it "caches the login and mailbox fetch" do
configure_imap
mock_imap
enable_imap
group.imap_mailboxes
Imap::Providers::Detector.expects(:init_with_detected_provider).never
group.imap_mailboxes
it "returns a list of mailboxes from the IMAP provider" do
configure_imap
mock_imap
enable_imap
expect(group.imap_mailboxes).to eq(["Inbox"])
end
it "caches the login and mailbox fetch" do
configure_imap
mock_imap
enable_imap
group.imap_mailboxes
Imap::Providers::Detector.expects(:init_with_detected_provider).never
group.imap_mailboxes
end
end
end