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

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.

diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb
index f8510de..44edec9 100644
--- a/app/mailers/group_smtp_mailer.rb
+++ b/app/mailers/group_smtp_mailer.rb
@@ -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,
diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb
index bfbce4e..acd10a0 100644
--- a/app/mailers/user_notifications.rb
+++ b/app/mailers/user_notifications.rb
@@ -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
diff --git a/app/models/group.rb b/app/models/group.rb
index c7313fa..4e95974 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -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
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 5a07e58..32c004e 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -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
diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index 2958cea..86cf77f 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -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|

[... diff too long, it was truncated ...]

GitHub sha: eb2c3994

This commit appears in #13220 which was approved by SamSaffron. It was merged by martin.