From 5cd733a334846669d6435517f7d9913c4a9b1eb1 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Thu, 17 Jan 2019 11:13:40 -0600 Subject: Ensure Action Mailbox processes an email only once when received multiple times This also adds a new column, message_checksum, to the action_mailbox_inbound_emails table for storing SHA1 digest of the email source. Additionally, it makes generating the missing message id deterministic and adds a unique index on message_checksum and message_id to detect duplicate emails. --- .../action_mailbox/inbound_email/message_id.rb | 22 +++++++++++----------- .../20180917164000_create_action_mailbox_tables.rb | 5 ++++- ...20180208205311_create_action_mailroom_tables.rb | 5 ++++- actionmailbox/test/dummy/db/schema.rb | 2 ++ actionmailbox/test/unit/inbound_email_test.rb | 22 ++++++++++++++++++++++ actionmailbox/test/unit/mailbox/routing_test.rb | 3 +-- 6 files changed, 44 insertions(+), 15 deletions(-) (limited to 'actionmailbox') diff --git a/actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb b/actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb index 57b4a2445d..470b93ca20 100644 --- a/actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb +++ b/actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb @@ -9,30 +9,30 @@ module ActionMailbox::InboundEmail::MessageId extend ActiveSupport::Concern - included do - before_save :generate_missing_message_id - end - class_methods do # Create a new +InboundEmail+ from the raw +source+ of the email, which be uploaded as a Active Storage # attachment called +raw_email+. Before the upload, extract the Message-ID from the +source+ and set # it as an attribute on the new +InboundEmail+. def create_and_extract_message_id!(source, **options) - create! options.merge(message_id: extract_message_id(source)) do |inbound_email| + message_checksum = Digest::SHA1.hexdigest(source) + message_id = extract_message_id(source) || generate_missing_message_id(message_checksum) + + create! options.merge(message_id: message_id, message_checksum: message_checksum) do |inbound_email| inbound_email.raw_email.attach io: StringIO.new(source), filename: "message.eml", content_type: "message/rfc822" end + rescue ActiveRecord::RecordNotUnique + nil end private def extract_message_id(source) Mail.from_source(source).message_id rescue nil end - end - private - def generate_missing_message_id - self.message_id ||= Mail::MessageIdField.new.message_id.tap do |message_id| - logger.warn "Message-ID couldn't be parsed or is missing. Generated a new Message-ID: #{message_id}" + def generate_missing_message_id(message_checksum) + Mail::MessageIdField.new("<#{message_checksum}@#{::Socket.gethostname}.mail>").message_id.tap do |message_id| + logger.warn "Message-ID couldn't be parsed or is missing. Generated a new Message-ID: #{message_id}" + end end - end + end end diff --git a/actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb b/actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb index 8cf621d7e3..e697748f82 100644 --- a/actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb +++ b/actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb @@ -2,10 +2,13 @@ class CreateActionMailboxTables < ActiveRecord::Migration[6.0] def change create_table :action_mailbox_inbound_emails do |t| t.integer :status, default: 0, null: false - t.string :message_id + t.string :message_id, null: false + t.string :message_checksum, null: false t.datetime :created_at, precision: 6, null: false t.datetime :updated_at, precision: 6, null: false + + t.index [ :message_id, :message_checksum ], unique: true end end end diff --git a/actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb b/actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb index 0124b4b98f..f587f37095 100644 --- a/actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb +++ b/actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb @@ -2,10 +2,13 @@ class CreateActionMailboxTables < ActiveRecord::Migration[6.0] def change create_table :action_mailbox_inbound_emails do |t| t.integer :status, default: 0, null: false - t.string :message_id + t.string :message_id, null: false + t.string :message_checksum, null: false t.datetime :created_at, precision: 6 t.datetime :updated_at, precision: 6 + + t.index [ :message_id, :message_checksum ], unique: true end end end diff --git a/actionmailbox/test/dummy/db/schema.rb b/actionmailbox/test/dummy/db/schema.rb index 6cfe7de765..4aba92bfc6 100644 --- a/actionmailbox/test/dummy/db/schema.rb +++ b/actionmailbox/test/dummy/db/schema.rb @@ -15,8 +15,10 @@ ActiveRecord::Schema.define(version: 2018_02_12_164506) do create_table "action_mailbox_inbound_emails", force: :cascade do |t| t.integer "status", default: 0, null: false t.string "message_id" + t.string "message_checksum" t.datetime "created_at", precision: 6 t.datetime "updated_at", precision: 6 + t.index ["message_id", "message_checksum"], name: "index_action_mailbox_inbound_emails_uniqueness", unique: true end create_table "active_storage_attachments", force: :cascade do |t| diff --git a/actionmailbox/test/unit/inbound_email_test.rb b/actionmailbox/test/unit/inbound_email_test.rb index 993423406f..76f047eb61 100644 --- a/actionmailbox/test/unit/inbound_email_test.rb +++ b/actionmailbox/test/unit/inbound_email_test.rb @@ -11,5 +11,27 @@ module ActionMailbox test "source returns the contents of the raw email" do assert_equal file_fixture("welcome.eml").read, create_inbound_email_from_fixture("welcome.eml").source end + + test "email with message id is processed only once when received multiple times" do + mail = Mail.from_source(file_fixture("welcome.eml").read) + assert mail.message_id + + inbound_email_1 = create_inbound_email_from_source(mail.to_s) + assert inbound_email_1 + + inbound_email_2 = create_inbound_email_from_source(mail.to_s) + assert_nil inbound_email_2 + end + + test "email with missing message id is processed only once when received multiple times" do + mail = Mail.from_source("Date: Fri, 28 Sep 2018 11:08:55 -0700\r\nTo: a@example.com\r\nMime-Version: 1.0\r\nContent-Type: text/plain\r\nContent-Transfer-Encoding: 7bit\r\n\r\nHello!") + assert_nil mail.message_id + + inbound_email_1 = create_inbound_email_from_source(mail.to_s) + assert inbound_email_1 + + inbound_email_2 = create_inbound_email_from_source(mail.to_s) + assert_nil inbound_email_2 + end end end diff --git a/actionmailbox/test/unit/mailbox/routing_test.rb b/actionmailbox/test/unit/mailbox/routing_test.rb index d4dad7eafb..d4ba702ac5 100644 --- a/actionmailbox/test/unit/mailbox/routing_test.rb +++ b/actionmailbox/test/unit/mailbox/routing_test.rb @@ -15,11 +15,10 @@ end class ActionMailbox::Base::RoutingTest < ActiveSupport::TestCase setup do $processed = false - @inbound_email = create_inbound_email_from_fixture("welcome.eml") end test "string routing" do - ApplicationMailbox.route @inbound_email + ApplicationMailbox.route create_inbound_email_from_fixture("welcome.eml") assert_equal "Discussion: Let's debate these attachments", $processed end -- cgit v1.2.3