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

DEV: Add bounce_error_code to EmailLog (#15948) 

Whenever we got a bounced email in the Email::Receiver we
previously would just set bounced: true on the EmailLog and
discard the status/diagnostic code. This commit changes this
flow to store the bounce error code (defined in the RFC at
https://www.iana.org/assignments/smtp-enhanced-status-codes/smtp-enhanced-status-codes.xhtml)
not just in the Email::Receiver, but also via webhook events
from other mail services and from SNS.

This commit does not surface the bounce error in the UI,
we can do that later if necessary.
This commit is contained in:
Martin Brennan 2022-02-15 14:17:26 +10:00 committed by GitHub
parent a0c65e91d9
commit 4086ee551e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 92 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
app/controllers/webhooks_controller.rb
View File

@ -15,14 +15,15 @@ class WebhooksController < ActionController::Base
events.each do |event|
message_id = Email::MessageIdService.message_id_clean((event["smtp-id"] || ""))
to_address = event["email"]
error_code = event["status"]
if event["event"] == "bounce"
if event["status"]["4."]
process_bounce(message_id, to_address, SiteSetting.soft_bounce_score)
if error_code[Email::SMTP_STATUS_TRANSIENT_FAILURE]
process_bounce(message_id, to_address, SiteSetting.soft_bounce_score, error_code)
else
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score)
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score, error_code)
end
elsif event["event"] == "dropped"
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score)
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score, error_code)
end
end
@ -51,12 +52,13 @@ class WebhooksController < ActionController::Base
events.each do |event|
message_id = event.dig("msg", "metadata", "message_id")
to_address = event.dig("msg", "email")
error_code = event.dig("msg", "diag")
case event["event"]
when "hard_bounce"
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score)
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score, error_code)
when "soft_bounce"
process_bounce(message_id, to_address, SiteSetting.soft_bounce_score)
process_bounce(message_id, to_address, SiteSetting.soft_bounce_score, error_code)
end
end
@ -152,14 +154,15 @@ class WebhooksController < ActionController::Base
event = params["event"]
message_id = Email::MessageIdService.message_id_clean(params["Message-Id"])
to_address = params["recipient"]
error_code = params["code"]
# only handle soft bounces, because hard bounces are also handled
# by the "dropped" event and we don't want to increase bounce score twice
# for the same message
if event == "bounced" && params["error"]["4."]
process_bounce(message_id, to_address, SiteSetting.soft_bounce_score)
if event == "bounced" && params["error"][Email::SMTP_STATUS_TRANSIENT_FAILURE]
process_bounce(message_id, to_address, SiteSetting.soft_bounce_score, error_code)
elsif event == "dropped"
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score)
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score, error_code)
end
success
@ -170,28 +173,29 @@ class WebhooksController < ActionController::Base
return mailgun_failure unless valid_mailgun_signature?(signature["token"], signature["timestamp"], signature["signature"])
data = params["event-data"]
error_code = params.dig("delivery-status", "code")
message_id = data.dig("message", "headers", "message-id")
to_address = data["recipient"]
severity = data["severity"]
if data["event"] == "failed"
if severity == "temporary"
process_bounce(message_id, to_address, SiteSetting.soft_bounce_score)
process_bounce(message_id, to_address, SiteSetting.soft_bounce_score, error_code)
elsif severity == "permanent"
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score)
process_bounce(message_id, to_address, SiteSetting.hard_bounce_score, error_code)
end
end
success
end
def process_bounce(message_id, to_address, bounce_score)
def process_bounce(message_id, to_address, bounce_score, bounce_error_code = nil)
return if message_id.blank? || to_address.blank?
email_log = EmailLog.find_by(message_id: message_id, to_address: to_address)
return if email_log.nil?
email_log.update_columns(bounced: true)
email_log.update_columns(bounced: true, bounce_error_code: bounce_error_code)
return if email_log.user.nil? || email_log.user.email.blank?
Email::Receiver.update_bounce_score(email_log.user.email, bounce_score)

2
app/jobs/regular/process_sns_notification.rb
View File

@ -25,7 +25,7 @@ module Jobs
message.dig("bounce", "bouncedRecipients").each do |r|
if email_log = EmailLog.order("created_at DESC").where(to_address: r["emailAddress"]).first
email_log.update_columns(bounced: true)
email_log.update_columns(bounced: true, bounce_error_code: r["status"])
if email_log.user&.email.present?
if email_log.user.user_stat.bounce_score.to_s.start_with?("4.") || bounce_type == "Transient"

31
app/models/email_log.rb
View File

@ -117,21 +117,22 @@ end
#
# Table name: email_logs
#
# id :integer not null, primary key
# to_address :string not null
# email_type :string not null
# user_id :integer
# created_at :datetime not null
# updated_at :datetime not null
# post_id :integer
# bounce_key :uuid
# bounced :boolean default(FALSE), not null
# message_id :string
# smtp_group_id :integer
# cc_addresses :text
# cc_user_ids :integer is an Array
# raw :text
# topic_id :integer
# id :integer not null, primary key
# to_address :string not null
# email_type :string not null
# user_id :integer
# created_at :datetime not null
# updated_at :datetime not null
# post_id :integer
# bounce_key :uuid
# bounced :boolean default(FALSE), not null
# message_id :string
# smtp_group_id :integer
# cc_addresses :text
# cc_user_ids :integer is an Array
# raw :text
# topic_id :integer
# bounce_error_code :string
#
# Indexes
#

7
db/migrate/20220214233625_add_bounce_error_code_to_email_log.rb Normal file
View File

@ -0,0 +1,7 @@
# frozen_string_literal: true
class AddBounceErrorCodeToEmailLog < ActiveRecord::Migration[6.1]
def change
add_column :email_logs, :bounce_error_code, :string, null: true
end
end

5
lib/email.rb
View File

@ -3,6 +3,11 @@
require 'mail'
module Email
# See https://www.iana.org/assignments/smtp-enhanced-status-codes/smtp-enhanced-status-codes.xhtml#smtp-enhanced-status-codes-1
SMTP_STATUS_SUCCESS = "2."
SMTP_STATUS_TRANSIENT_FAILURE = "4."
SMTP_STATUS_PERMANENT_FAILURE = "5."
def self.is_valid?(email)
return false unless String === email
!!(EmailValidator.email_regex =~ email)

8
lib/email/receiver.rb
View File

@ -238,16 +238,20 @@ module Email
def handle_bounce
@incoming_email.update_columns(is_bounce: true)
mail_error_statuses = Array.wrap(@mail.error_status)
if email_log.present?
email_log.update_columns(bounced: true)
email_log.update_columns(
bounced: true,
bounce_error_code: mail_error_statuses.first
)
post = email_log.post
topic = email_log.topic
end
DiscourseEvent.trigger(:email_bounce, @mail, @incoming_email, @email_log)
if @mail.error_status.present? && Array.wrap(@mail.error_status).any? { |s| s.start_with?("4.") }
if mail_error_statuses.any? { |s| s.start_with?(Email::SMTP_STATUS_TRANSIENT_FAILURE) }
Email::Receiver.update_bounce_score(@from_email, SiteSetting.soft_bounce_score)
else
Email::Receiver.update_bounce_score(@from_email, SiteSetting.hard_bounce_score)

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

@ -147,17 +147,29 @@ describe Email::Receiver do
expect(IncomingEmail.last.is_bounce).to eq(true)
end
it "when bounce with verp" do
SiteSetting.reply_by_email_address = "foo+%{reply_key}@discourse.org"
bounce_key = "14b08c855160d67f2e0c2f8ef36e251e"
create_post_reply_key(bounce_key)
Fabricate(:email_log, to_address: email_address, user: user2, bounce_key: bounce_key, post: post)
context "when bounce with verp" do
let(:bounce_key) { "14b08c855160d67f2e0c2f8ef36e251e" }
expect { process(:hard_bounce_via_verp) }.to raise_error(Email::Receiver::BouncedEmailError)
post = Post.last
expect(post.whisper?).to eq(true)
expect(post.raw).to eq(I18n.t("system_messages.email_bounced", email: email_address, raw: "Your email bounced").strip)
expect(IncomingEmail.last.is_bounce).to eq(true)
before do
SiteSetting.reply_by_email_address = "foo+%{reply_key}@discourse.org"
create_post_reply_key(bounce_key)
Fabricate(:email_log, to_address: email_address, user: user2, bounce_key: bounce_key, post: post)
end
it "creates a post with the bounce error" do
expect { process(:hard_bounce_via_verp) }.to raise_error(Email::Receiver::BouncedEmailError)
post = Post.last
expect(post.whisper?).to eq(true)
expect(post.raw).to eq(I18n.t("system_messages.email_bounced", email: email_address, raw: "Your email bounced").strip)
expect(IncomingEmail.last.is_bounce).to eq(true)
end
it "updates the email log with the bounce error message" do
expect { process(:hard_bounce_via_verp) }.to raise_error(Email::Receiver::BouncedEmailError)
email_log = EmailLog.find_by(bounce_key: bounce_key)
expect(email_log.bounced).to eq(true)
expect(email_log.bounce_error_code).to eq("5.1.1")
end
end
end
end

19
spec/requests/webhooks_controller_spec.rb
View File

@ -29,13 +29,16 @@ describe WebhooksController do
"event" => "dropped",
"recipient" => email,
"Message-Id" => "<#{message_id}>",
"signature" => signature
"signature" => signature,
"error" => "smtp; 550-5.1.1 The email account that you tried to reach does not exist.",
"code" => "5.1.1"
}
expect(response.status).to eq(200)
email_log.reload
expect(email_log.bounced).to eq(true)
expect(email_log.bounce_error_code).to eq("5.1.1")
expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.hard_bounce_score)
end
@ -58,6 +61,11 @@ describe WebhooksController do
"message-id" => message_id,
}
}
},
"delivery-status" => {
"message" => "smtp; 550-5.1.1 The email account that you tried to reach does not exist.",
"code" => "5.1.1",
"description" => ""
}
}
@ -65,6 +73,7 @@ describe WebhooksController do
email_log.reload
expect(email_log.bounced).to eq(true)
expect(email_log.bounce_error_code).to eq("5.1.1")
expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.soft_bounce_score)
end
end
@ -89,6 +98,7 @@ describe WebhooksController do
email_log.reload
expect(email_log.bounced).to eq(true)
expect(email_log.bounce_error_code).to eq("5.0.0")
expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.hard_bounce_score)
end
end
@ -109,6 +119,7 @@ describe WebhooksController do
email_log.reload
expect(email_log.bounced).to eq(true)
expect(email_log.bounce_error_code).to eq(nil) # mailjet doesn't give us this
expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.hard_bounce_score)
end
end
@ -123,6 +134,8 @@ describe WebhooksController do
"event" => "hard_bounce",
"msg" => {
"email" => email,
"diag" => "5.1.1",
"bounce_description": "smtp; 550-5.1.1 The email account that you tried to reach does not exist.",
"metadata" => {
"message_id" => message_id
}
@ -134,6 +147,7 @@ describe WebhooksController do
email_log.reload
expect(email_log.bounced).to eq(true)
expect(email_log.bounce_error_code).to eq("5.1.1")
expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.hard_bounce_score)
end
end
@ -152,6 +166,7 @@ describe WebhooksController do
email_log.reload
expect(email_log.bounced).to eq(true)
expect(email_log.bounce_error_code).to eq(nil) # postmark doesn't give us this
expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.hard_bounce_score)
end
it "soft bounces" do
@ -167,6 +182,7 @@ describe WebhooksController do
email_log.reload
expect(email_log.bounced).to eq(true)
expect(email_log.bounce_error_code).to eq(nil) # postmark doesn't give us this
expect(email_log.user.user_stat.bounce_score).to eq(SiteSetting.soft_bounce_score)
end
end
@ -181,6 +197,7 @@ describe WebhooksController do
"msys" => {
"message_event" => {
"bounce_class" => 10,
"error_code" => "554",
"rcpt_to" => email,
"rcpt_meta" => {
"message_id" => message_id