Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FEATURE: Use group SMTP settings for sending user notification emails (initial) (#13220) 

This PR changes the `UserNotification` class to send outbound `user_private_message` using the group's SMTP settings, but only if:

* The first allowed_group on the topic has SMTP configured and enabled
* SiteSetting.enable_smtp is true
* The group does not have IMAP enabled, if this is enabled the `GroupSMTPMailer` handles things

The email is sent using the group's `email_username` as both the `from` and `reply-to` address, so when the user replies from their email it will go through the group's SMTP inbox, which needs to have email forwarding set up to send the message on to a location (such as a hosted site email address like meta@discoursemail.com) where it can be POSTed into discourse's handle_mail route.

Also includes a fix to `EmailReceiver#group_incoming_emails_regex` to include the `group.email_username` so the group does not get a staged user created and invited to the topic (which was a problem for IMAP), as well as updating `Group.find_by_email` to find using the `email_username` as well for inbound emails with that as the TO address.

#### Note

This is safe to merge without impacting anyone seriously. If people had SMTP enabled for a group they would have IMAP enabled too currently, and that is a very small amount of users because IMAP is an alpha product, and also because the UserNotification change has a guard to make sure it is not used if IMAP is enabled for the group. The existing IMAP tests work, and I tested this functionality by manually POSTing replies to the SMTP address into my local discourse.

There will probably be more work needed on this, but it needs to be tested further in a real hosted environment to continue.
This commit is contained in:
Martin Brennan 2021-06-03 14:47:32 +10:00 committed by GitHub
parent 3249312c81
commit eb2c399445
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 219 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/mailers/group_smtp_mailer.rb
View File

@ -26,7 +26,7 @@ class GroupSmtpMailer < ActionMailer::Base
delivery_options = {
address: from_group.smtp_server,
port: from_group.smtp_port,
domain: from_group.email_username.split('@').last,
domain: from_group.email_username_domain,
user_name: from_group.email_username,
password: from_group.email_password,
authentication: GlobalSetting.smtp_authentication,

54
app/mailers/user_notifications.rb
View File

@ -327,6 +327,7 @@ class UserNotifications < ActionMailer::Base
opts[:show_category_in_subject] = false
opts[:show_tags_in_subject] = false
opts[:show_group_in_subject] = true if SiteSetting.group_in_subject
opts[:use_group_smtp_if_configured] = true
# We use the 'user_posted' event when you are emailed a post in a PM.
opts[:notification_type] = 'posted'
@ -460,6 +461,7 @@ class UserNotifications < ActionMailer::Base
notification_type: notification_type,
use_invite_template: opts[:use_invite_template],
use_topic_title_subject: use_topic_title_subject,
use_group_smtp_if_configured: opts[:use_group_smtp_if_configured],
user: user
}
@ -485,6 +487,12 @@ class UserNotifications < ActionMailer::Base
group_name = opts[:group_name]
locale = user_locale(user)
# this gets set in MessageBuilder if it is nil here, we just want to be
# able to override it if the group has SMTP enabled
from_address = nil
delivery_method_options = nil
use_from_address_for_reply_to = false
template = +"user_notifications.user_#{notification_type}"
if post.topic.private_message?
template << "_pm"
@ -520,9 +528,48 @@ class UserNotifications < ActionMailer::Base
show_tags_in_subject = tags.any? ? tags.join(" ") : nil
end
group = post.topic.allowed_groups&.first
# If the group has IMAP enabled, then this will be handled by
# the Jobs::GroupSmtpEmail which is enqueued from the PostAlerter
#
# use_group_smtp_if_configured is used to ensure that no notifications
# expect for specific ones that we bless (such as user_private_message)
# accidentally get sent with the group SMTP settings.
if group.present? &&
group.smtp_enabled &&
!group.imap_enabled &&
SiteSetting.enable_smtp &&
opts[:use_group_smtp_if_configured]
port, enable_tls, enable_starttls_auto = EmailSettingsValidator.provider_specific_ssl_overrides(
group.smtp_server, group.smtp_port, group.smtp_ssl, group.smtp_ssl
)
# TODO (martin): Remove this once testing is over and this is more stable.
Rails.logger.warn("Using SMTP settings from group #{group.name} (#{group.id}) to send user notification for topic #{post.topic.id} and user #{user.id} (#{user.email})")
delivery_method_options = {
address: group.smtp_server,
port: port,
domain: group.email_username_domain,
user_name: group.email_username,
password: group.email_password,
authentication: GlobalSetting.smtp_authentication,
enable_starttls_auto: enable_starttls_auto
}
# We want from to be the same as the group's email_username, so if
# someone emails support@discourse.org they will get a reply from
# support@discourse.org and be able to email the SMTP email, which
# will forward the email back into Discourse and process/link it correctly.
use_from_address_for_reply_to = true
from_address = group.email_username
end
if post.topic.private_message?
subject_pm =
if opts[:show_group_in_subject] && group = post.topic.allowed_groups&.first
if opts[:show_group_in_subject] && group.present?
if group.full_name
"[#{group.full_name}] "
else
@ -664,7 +711,10 @@ class UserNotifications < ActionMailer::Base
site_description: SiteSetting.site_description,
site_title: SiteSetting.title,
site_title_url_encoded: UrlHelper.encode_component(SiteSetting.title),
locale: locale
locale: locale,
delivery_method_options: delivery_method_options,
use_from_address_for_reply_to: use_from_address_for_reply_to,
from: from_address
}
unless translation_override_exists

16
app/models/group.rb
View File

@ -707,7 +707,10 @@ class Group < ActiveRecord::Base
end
def self.find_by_email(email)
self.where("string_to_array(incoming_email, '|') @> ARRAY[?]", Email.downcase(email)).first
self.where(
"email_username = :email OR string_to_array(incoming_email, '|') @> ARRAY[:email]",
email: Email.downcase(email)
).first
end
def bulk_add(user_ids)
@ -870,8 +873,17 @@ class Group < ActiveRecord::Base
}
end
def email_username_domain
email_username.split('@').last
end
def email_username_user
email_username.split('@').first
end
def email_username_regex
user, domain = email_username.split('@')
user = email_username_user
domain = email_username_domain
if user.present? && domain.present?
/^#{Regexp.escape(user)}(\+[^@]*)?@#{Regexp.escape(domain)}$/i
end

9
app/models/topic.rb
View File

@ -1674,6 +1674,15 @@ class Topic < ActiveRecord::Base
# group if the group is present. If combined addresses is empty we do
# not need to do this check, and instead can proceed on to adding the
# from address.
#
# Will not include test1@gmail.com if the only IncomingEmail
# is:
#
# from: test1@gmail.com
# to: test+support@discoursemail.com
#
# Because we don't care about the from addresses and also the to address
# is not the email_username, which will be something like test1@gmail.com.
if group.present? && combined_addresses.any?
next if combined_addresses.none? { |address| address =~ group.email_username_regex }
end

16
app/services/post_alerter.rb
View File

@ -588,13 +588,19 @@ class PostAlerter
warn_if_not_sidekiq
# Users who interacted with the post by _directly_ emailing the group
# via the group's email_username. This excludes people who replied via
# email to a user_private_message notification email. These people should
# via the group's email_username which is configured via SMTP/IMAP.
#
# This excludes people who replied via email to a user_private_message
# notification email which will have a PostReplyKey. These people should
# not be emailed again by the user_private_message notifications below.
#
# This also excludes people who emailed the group by one of its incoming_email
# addresses, e.g. somegroup+support@discoursemail.com, which is part of the
# normal group email flow and has nothing to do with SMTP/IMAP.
emails_to_skip_send = notify_group_direct_emailers(post)
# Users that aren't part of any mentioned groups and who did not email
# the group directly.
# the group directly at the group's email_username.
users = directly_targeted_users(post).reject { |u| notified.include?(u) }
DiscourseEvent.trigger(:before_create_notifications_for_users, users, post)
users.each do |user|
@ -604,7 +610,7 @@ class PostAlerter
end
end
# Users that are part of all mentioned groups
# Users that are part of all mentioned groups.
users = indirectly_targeted_users(post).reject { |u| notified.include?(u) }
DiscourseEvent.trigger(:before_create_notifications_for_users, users, post)
users.each do |user|
@ -636,7 +642,7 @@ class PostAlerter
end
end
# Add the group's email into the array, because it is used for
# Add the group's email_username into the array, because it is used for
# skip_send_email_to in the case of user private message notifications
# (we do not want the group to be sent any emails from here because it
# will make another email for IMAP to pick up in the group's mailbox)

8
lib/email/receiver.rb
View File

@ -952,7 +952,13 @@ module Email
end
def group_incoming_emails_regex
@group_incoming_emails_regex ||= Regexp.union Group.pluck(:incoming_email).select(&:present?).map { |e| e.split("|") }.flatten.uniq
@group_incoming_emails_regex = Regexp.union(
DB.query_single(<<~SQL).map { |e| e.split("|") }.flatten.compact_blank.uniq
SELECT CONCAT(incoming_email, '|', email_username)
FROM groups
WHERE incoming_email IS NOT NULL OR email_username IS NOT NULL
SQL
)
end
def category_email_in_regex

17
spec/components/email/receiver_spec.rb
View File

@ -968,6 +968,23 @@ describe Email::Receiver do
include_examples "creates topic with forwarded message as quote", :group, "team@bar.com|meat@bar.com"
end
context "when a reply is sent to a group's email_username" do
let!(:topic) do
group.update(email_username: "team@somesmtpaddress.com")
process(:email_reply_1)
Topic.last
end
it "does not invite the group email_username as a staged user" do
process(:email_reply_to_group_email_username)
expect(User.find_by_email("team@somesmtpaddress.com")).to eq(nil)
end
it "creates the reply when the sender and referenced messsage id are known" do
expect { process(:email_reply_to_group_email_username) }.to change { topic.posts.count }.by(1).and change { Topic.count }.by(0)
end
end
context "when message sent to a group has no key and find_related_post_with_key is enabled" do
let!(:topic) do
process(:email_reply_1)

4
spec/components/imap/sync_spec.rb
View File

@ -20,8 +20,8 @@ describe Imap::Sync do
:group,
imap_server: 'imap.gmail.com',
imap_port: 993,
email_username: 'discourse@example.com',
email_password: 'discourse@example.com',
email_username: 'groupemailusername@example.com',
email_password: 'password',
imap_mailbox_name: '[Gmail]/All Mail'
)
end

12
spec/fixtures/emails/email_reply_to_group_email_username.eml vendored Normal file
View File

@ -0,0 +1,12 @@
Return-Path: <two@foo.com>
From: Two <two@foo.com>
To: team@somesmtpaddress.com
Subject: RE: Testing email threading
Date: Fri, 15 Jan 2016 00:12:43 +0100
Message-ID: <44@foo.bar.mail>
In-Reply-To: <34@foo.bar.mail>
Mime-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
This is email reply **1**.

78
spec/mailers/user_notifications_spec.rb
View File

@ -611,6 +611,84 @@ describe UserNotifications do
expect(mail.body).to include("[group1 (2)](http://test.localhost/groups/group1), [group2 (1)](http://test.localhost/groups/group2), [one](http://test.localhost/u/one), [two](http://test.localhost/u/two)")
end
context "when group smtp is configured and SiteSetting.enable_smtp" do
let!(:group1) do
Fabricate(
:group,
name: "group1",
smtp_enabled: true,
smtp_port: 587,
smtp_ssl: true,
smtp_server: "smtp.test.com",
email_username: "user@test.com",
email_password: "password"
)
end
before do
SiteSetting.enable_smtp = true
topic.allowed_groups = [group1]
end
it "uses the from address, which is the group's email_username, for reply-to" do
mail = UserNotifications.user_private_message(
user,
post: response,
notification_type: notification.notification_type,
notification_data_hash: notification.data_hash
)
expect(mail.from).to eq([group1.email_username])
expect(mail.reply_to).to eq([group1.email_username])
end
it "uses the SMTP settings from the group for delivery" do
mail = UserNotifications.user_private_message(
user,
post: response,
notification_type: notification.notification_type,
notification_data_hash: notification.data_hash
)
delivery_method = mail.delivery_method.settings
expect(delivery_method[:port]).to eq(group1.smtp_port)
expect(delivery_method[:address]).to eq(group1.smtp_server)
expect(delivery_method[:domain]).to eq("test.com")
expect(delivery_method[:password]).to eq("password")
expect(delivery_method[:user_name]).to eq("user@test.com")
end
context "when imap is configured for the group" do
before do
group1.update(
imap_server: "imap.test.com",
imap_port: 993,
imap_ssl: true,
imap_enabled: true,
imap_mailbox_name: "All Mail"
)
end
it "does not use group SMTP settings for delivery, this is handled by Jobs::GroupSmtpEmail" do
mail = UserNotifications.user_private_message(
user,
post: response,
notification_type: notification.notification_type,
notification_data_hash: notification.data_hash
)
expect(mail.from).to eq([SiteSetting.notification_email])
expect(mail.reply_to).to eq([SiteSetting.notification_email])
delivery_method = mail.delivery_method.settings
expect(delivery_method[:port]).not_to eq(group1.smtp_port)
expect(delivery_method[:address]).not_to eq(group1.smtp_server)
expect(delivery_method[:domain]).not_to eq("test.com")
expect(delivery_method[:password]).not_to eq("password")
expect(delivery_method[:user_name]).not_to eq("user@test.com")
end
end
end
context "when SiteSetting.group_name_in_subject is true" do
before do
SiteSetting.group_in_subject = true

16
spec/models/group_spec.rb
View File

@ -1291,4 +1291,20 @@ describe Group do
expect(group.imap_updated_by).to eq(user)
end
end
describe "#find_by_email" do
it "finds the group by any of its incoming emails" do
group.update!(incoming_email: "abc@test.com|support@test.com")
expect(Group.find_by_email("abc@test.com")).to eq(group)
expect(Group.find_by_email("support@test.com")).to eq(group)
expect(Group.find_by_email("nope@test.com")).to eq(nil)
end
it "finds the group by its email_username" do
group.update!(email_username: "abc@test.com", incoming_email: "support@test.com")
expect(Group.find_by_email("abc@test.com")).to eq(group)
expect(Group.find_by_email("support@test.com")).to eq(group)
expect(Group.find_by_email("nope@test.com")).to eq(nil)
end
end
end