diff --git a/app/controllers/webhooks_controller.rb b/app/controllers/webhooks_controller.rb index befa6ea22e5..81f4a5d092e 100644 --- a/app/controllers/webhooks_controller.rb +++ b/app/controllers/webhooks_controller.rb @@ -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) diff --git a/app/jobs/regular/process_sns_notification.rb b/app/jobs/regular/process_sns_notification.rb index ff25dc83feb..67e9b0e3273 100644 --- a/app/jobs/regular/process_sns_notification.rb +++ b/app/jobs/regular/process_sns_notification.rb @@ -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" diff --git a/app/models/email_log.rb b/app/models/email_log.rb index e8dd29a5dfa..153cb0b142b 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -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 # diff --git a/db/migrate/20220214233625_add_bounce_error_code_to_email_log.rb b/db/migrate/20220214233625_add_bounce_error_code_to_email_log.rb new file mode 100644 index 00000000000..205eaafed34 --- /dev/null +++ b/db/migrate/20220214233625_add_bounce_error_code_to_email_log.rb @@ -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 diff --git a/lib/email.rb b/lib/email.rb index 91c48dda43f..8b7058b28f3 100644 --- a/lib/email.rb +++ b/lib/email.rb @@ -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) diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 1d181d66a2c..c0ce08eac61 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -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) diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index bf741cf76a8..10112f4ed35 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -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 diff --git a/spec/requests/webhooks_controller_spec.rb b/spec/requests/webhooks_controller_spec.rb index 580c0c72f05..41e1373b8b2 100644 --- a/spec/requests/webhooks_controller_spec.rb +++ b/spec/requests/webhooks_controller_spec.rb @@ -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