diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index f8510deae34..44edec917c3 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 bfbce4e4aba..acd10a0451c 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 c7313fa2c7d..4e95974feac 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 5a07e58ea91..32c004ed0ad 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 2958cea2986..86cf77f8291 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| @@ -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) diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index b580e7ea408..5ec587ed377 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -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 diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index dec90fd99b8..636cc73cad5 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -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) diff --git a/spec/components/imap/sync_spec.rb b/spec/components/imap/sync_spec.rb index 7eac7ac6a33..04ee4eb3cde 100644 --- a/spec/components/imap/sync_spec.rb +++ b/spec/components/imap/sync_spec.rb @@ -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 diff --git a/spec/fixtures/emails/email_reply_to_group_email_username.eml b/spec/fixtures/emails/email_reply_to_group_email_username.eml new file mode 100644 index 00000000000..f66aa37eea9 --- /dev/null +++ b/spec/fixtures/emails/email_reply_to_group_email_username.eml @@ -0,0 +1,12 @@ +Return-Path: +From: Two +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**. diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 3edb1f3b331..344cc55a8ff 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 75630e524e3..a70bae19109 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -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