extract secure-id handling from incoming message processor

fixes CNVS-12147

test plan
- regression test incoming mail

Change-Id: Id3d8a95dda3566bae8d395d1b759c7897ab3c1ee
Reviewed-on: https://gerrit.instructure.com/32671
Reviewed-by: Braden Anderson <banderson@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Trevor deHaan <tdehaan@instructure.com>
Product-Review: Joel Hough <joel@instructure.com>

app/controllers/messages_controller.rb

@@ -36,7 +36,7 @@ class MessagesController < ApplicationController
     message['From']         = params[:from]
     message.body            = params[:message]
 
-    IncomingMail::IncomingMessageProcessor.new(IncomingMail::MessageHandler.new).process_single(message, secure_id, message_id)
+    IncomingMail::IncomingMessageProcessor.new(IncomingMail::MessageHandler.new).process_single(message, "#{secure_id}-#{message_id}")
     render :nothing => true
   end
 

app/models/incoming_mail/message_handler.rb

@@ -18,8 +18,11 @@
 
 module IncomingMail
   class MessageHandler
-    def handle(outgoing_from_address, body, html_body, incoming_message, message_id, secure_id)
-      original_message = Message.find_by_id(message_id)
+    def handle(outgoing_from_address, body, html_body, incoming_message, tag)
+      secure_id, original_message_id = parse_tag(tag)
+      raise IncomingMail::IncomingMessageProcessor::SilentIgnoreError unless original_message_id
+
+      original_message = Message.find_by_id(original_message_id)
       # This prevents us from rebouncing users that have auto-replies setup -- only bounce something
       # that was sent out because of a notification.
       raise IncomingMail::IncomingMessageProcessor::SilentIgnoreError unless original_message && original_message.notification_id
@@ -126,5 +129,10 @@ module IncomingMail
       # change encoding; if it throws an exception (i.e. unrecognized encoding), just strip invalid UTF-8
       Iconv.conv('UTF-8//TRANSLIT//IGNORE', encoding, string) rescue TextHelper.strip_invalid_utf8(string)
     end
+
+    def parse_tag(tag)
+      match = tag.match /^(\h+)-(\d+)$/
+      return match[1], match[2].to_i if match
+    end
   end
-end
+end

config/initializers/outgoing_mail.rb

@@ -32,4 +32,3 @@ when :smtp
 when :sendmail
   ActionMailer::Base.sendmail_settings.merge!(config)
 end
-

lib/incoming_mail/incoming_message_processor.rb

@@ -66,12 +66,12 @@ module IncomingMail
       end
     end
 
-    def process_single(incoming_message, secure_id, message_id, mailbox_account = IncomingMail::MailboxAccount.new)
+    def process_single(incoming_message, tag, mailbox_account = IncomingMail::MailboxAccount.new)
       return if self.class.bounce_message?(incoming_message)
 
       body, html_body = extract_body(incoming_message)
 
-      @message_handler.handle(mailbox_account.address, body, html_body, incoming_message, message_id, secure_id)
+      @message_handler.handle(mailbox_account.address, body, html_body, incoming_message, tag)
     end
 
     private
@@ -246,28 +246,28 @@ module IncomingMail
     end
 
     def process_message(message, account)
-      secure_id, outgoing_message_id = self.class.find_matching_to_address(message, account)
+      tag = self.class.extract_address_tag(message, account)
       # TODO: Add bounce processing and handling of other email to the default notification address.
-      return unless secure_id && outgoing_message_id
-      process_single(message, secure_id, outgoing_message_id, account)
+      return unless tag
+      process_single(message, tag, account)
     rescue => e
       ErrorReport.log_exception(self.class.error_report_category, e,
         :from => message.from.try(:first),
         :to => message.to.to_s)
     end
 
-    def self.find_matching_to_address(message, account)
+    def self.extract_address_tag(message, account)
       addr, domain = account.address.split(/@/)
-      regex = Regexp.new("#{Regexp.escape(addr)}\\+([0-9a-f]+)-(\\d+)@#{Regexp.escape(domain)}")
+      regex = Regexp.new("#{Regexp.escape(addr)}\\+([^@]+)@#{Regexp.escape(domain)}")
       message.to.each do |address|
         if match = regex.match(address)
-          return [match[1], match[2].to_i]
+          return match[1]
         end
       end
 
-      # if no match is found, return false secure_id and outgoing_message_id
+      # if no match is found, return false
       # so that self.process message stops processing.
-      [false, false]
+      false
     end
   end
 end

lib/reply_to_address.rb

@@ -16,13 +16,15 @@
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 #
 
-# Public: Error thrown when ReplyToAddress is used with an empty address pool.
-class EmptyReplyAddressPool < StandardError; end
 
 # Public: Represents a reply-to address for a message.
 class ReplyToAddress
   attr_reader :message
 
+  # Public: Error thrown when ReplyToAddress is used with an empty address pool.
+  class EmptyReplyAddressPool < StandardError
+  end
+
   # Public: Create a new ReplyToAddress.
   #
   # message - A Message object.
@@ -37,7 +39,7 @@ class ReplyToAddress
     return nil if message.path_type == 'sms'
     return message.from if message.context_type == 'ErrorReport'
 
-    address, domain = ReplyToAddress.address_from_pool(message).split('@')
+    address, domain = self.class.address_from_pool(message).split('@')
     "#{address}+#{secure_id}-#{message.global_id}@#{domain}"
   end
 

spec/controllers/messages_controller_spec.rb

@@ -33,7 +33,7 @@ describe MessagesController do
 
       it "should be able to send messages" do
         secure_id, message_id = ['secure_id', 42]
-        IncomingMail::IncomingMessageProcessor.any_instance.expects(:process_single).with(anything, secure_id, message_id)
+        IncomingMail::IncomingMessageProcessor.any_instance.expects(:process_single).with(anything, "#{secure_id}-#{message_id}")
         post 'create', :secure_id  => secure_id,
                        :message_id => message_id,
                        :subject    => 'subject',
@@ -56,4 +56,3 @@ describe MessagesController do
     end
   end
 end
-

spec/lib/incoming_mail/incoming_message_processor_spec.rb

@@ -34,7 +34,7 @@ describe IncomingMail::IncomingMessageProcessor do
     ActionMailer::Base.delivery_method = @original_delivery_method if @original_delivery_method
     ActionMailer::Base.perform_deliveries = @original_perform_deliveries if @original_perform_deliveries
   end
-  
+
   def simple_mail_from_user
     Mail.new(:body => "body", :from => @user.email_channel.path)
   end
@@ -56,18 +56,19 @@ describe IncomingMail::IncomingMessageProcessor do
   let(:message_handler) { MockMessageHandler.new }
 
   class MockMessageHandler
-    attr_reader :account, :body, :html_body, :incoming_message, :message_id, :secure_id
+    attr_reader :account, :body, :html_body, :incoming_message, :tag
 
-    def handle(account, body, html_body, incoming_message, message_id, secure_id)
+    def handle(account, body, html_body, incoming_message, tag)
       @account = account
       @body = body
       @html_body = html_body
       @incoming_message = incoming_message
-      @message_id = message_id
-      @secure_id = secure_id
+      @tag = tag
     end
   end
 
+  let(:tag) { '123abc' }
+
   before(:all) do
     setup_test_outgoing_mail
 
@@ -150,7 +151,7 @@ describe IncomingMail::IncomingMessageProcessor do
     end
 
     it "should not choke on invalid UTF-8" do
-      IncomingMessageProcessor.new(message_handler).process_single(Mail.new { body "he\xffllo" }, ReplyToAddress.new(@message).secure_id, @message.id)
+      IncomingMessageProcessor.new(message_handler).process_single(Mail.new { body "he\xffllo" }, tag)
 
       message_handler.body.should == "hello"
       message_handler.html_body.should == "hello"
@@ -160,7 +161,7 @@ describe IncomingMail::IncomingMessageProcessor do
       IncomingMessageProcessor.new(message_handler).process_single(Mail.new {
           content_type 'text/plain; charset=Shift-JIS'
           body "\x83\x40"
-        }, ReplyToAddress.new(@message).secure_id, @message.id)
+        }, tag)
 
       comparison_string = "\xe3\x82\xa1"
       comparison_string.force_encoding("UTF-8")
@@ -177,7 +178,7 @@ describe IncomingMail::IncomingMessageProcessor do
             content_type 'text/html; charset=UTF-8'
             body '<h1>This is HTML</h1>'
           end
-        }, ReplyToAddress.new(@message).secure_id, @message.id)
+        }, tag)
       message_handler.body.should == 'This is plain text'
       message_handler.html_body.should == '<h1>This is HTML</h1>'
     end
@@ -195,7 +196,7 @@ describe IncomingMail::IncomingMessageProcessor do
       message_handler.expects(:handle).never
 
       IncomingMessageProcessor.new(message_handler).process_single(incoming_bounce_mail,
-        ReplyToAddress.new(@message).secure_id, @message.id)
+        tag)
     end
   end
 
@@ -311,9 +312,9 @@ describe IncomingMail::IncomingMessageProcessor do
         @mock_mailbox.expects(:each_message).multiple_yields([:foo, foo], [:bar, bar], [:baz, baz])
         @mock_mailbox.expects(:disconnect)
         imp = IncomingMessageProcessor.new(message_handler)
-        imp.expects(:process_single).with(kind_of(Mail::Message), "123", 1, anything)
-        imp.expects(:process_single).with(kind_of(Mail::Message), "456", 2, anything)
-        imp.expects(:process_single).with(kind_of(Mail::Message), "abc", 3, anything)
+        imp.expects(:process_single).with(kind_of(Mail::Message), "123-1", anything)
+        imp.expects(:process_single).with(kind_of(Mail::Message), "456-2", anything)
+        imp.expects(:process_single).with(kind_of(Mail::Message), "abc-3", anything)
 
         imp.process
       end
@@ -328,7 +329,7 @@ describe IncomingMail::IncomingMessageProcessor do
         @mock_mailbox.expects(:each_message).yields(:foo, foo)
         @mock_mailbox.expects(:disconnect)
         imp = IncomingMessageProcessor.new(message_handler)
-        imp.expects(:process_single).with(kind_of(Mail::Message), "123", 1, anything)
+        imp.expects(:process_single).with(kind_of(Mail::Message), "123-1", anything)
 
         imp.process
       end
@@ -411,14 +412,14 @@ describe IncomingMail::IncomingMessageProcessor do
       usernames.count(nil).should eql 1
     end
 
-    it "should not try to load messages with invalid IDs" do
+    it "should not try to load messages with invalid address tags" do
       # this should be tested through the public "process" method
-      # rather than calling the private "find_matching_to_address" directly
+      # rather than calling the private "extract_address_tag" directly
       account, message = [mock, mock]
       account.expects(:address).returns('user@example.com')
       message.expects(:to).returns(['user@example.com'])
-      result = IncomingMessageProcessor.find_matching_to_address(message, account)
-      result.should == [false, false]
+      result = IncomingMessageProcessor.extract_address_tag(message, account)
+      result.should == false
     end
   end
 

spec/lib/reply_to_address_spec.rb

@@ -19,14 +19,11 @@
 require File.expand_path('../spec_helper.rb', File.dirname(__FILE__))
 
 describe ReplyToAddress do
-  describe 'initialize' do
-    it 'should require a message argument' do
-      lambda { ReplyToAddress.new }.should raise_error(ArgumentError)
-    end
+  expect_secure_id = Canvas::Security.hmac_sha1("1000001")
 
+  describe 'initialize' do
     it 'should persist the message argument' do
-      message = Message.new
-      ReplyToAddress.new(message).message.should == message
+      ReplyToAddress.new("some message").message.should == "some message"
     end
   end
 
@@ -49,7 +46,6 @@ describe ReplyToAddress do
 
     it 'should generate a reply-to address for email messages' do
       message = mock()
-      secure_id = Canvas::Security.hmac_sha1(1000001.to_s)
 
       message.expects(:path_type).returns('email')
       message.expects(:context_type).returns('Course')
@@ -57,17 +53,16 @@ describe ReplyToAddress do
       message.expects(:global_id).twice.returns(1000001)
       ReplyToAddress.address_pool = %w{canvas@example.com}
 
-      ReplyToAddress.new(message).address.should == "canvas+#{secure_id}-1000001@example.com"
+      ReplyToAddress.new(message).address.should == "canvas+#{expect_secure_id}-1000001@example.com"
     end
   end
 
   describe 'secure_id' do
     it 'should generate a unique hash for the message' do
       message       = mock()
-      expected_hash = Canvas::Security.hmac_sha1(1000001.to_s)
       message.expects(:global_id).returns(1000001)
 
-      ReplyToAddress.new(message).secure_id.should == expected_hash
+      ReplyToAddress.new(message).secure_id.should == expect_secure_id
     end
   end
 
@@ -98,7 +93,7 @@ describe ReplyToAddress do
 
       lambda {
         ReplyToAddress.address_from_pool(message)
-      }.should raise_error(EmptyReplyAddressPool)
+      }.should raise_error(ReplyToAddress::EmptyReplyAddressPool)
     end
 
     it 'should randomly select a pool address if the message has no id' do

spec/models/incoming_mail/message_handler_spec.rb

@@ -24,6 +24,7 @@ describe IncomingMail::MessageHandler do
   let(:html_body) { "Hello" }
   let(:message_id) { 1 }
   let(:secure_id) { "123abc" }
+  let(:tag) { "#{secure_id}-#{message_id}" }
   let(:shard) do
     stub("shard").tap do |shard|
       shard.stubs(:activate).yields
@@ -72,7 +73,7 @@ describe IncomingMail::MessageHandler do
       ReplyToAddress.any_instance.stubs(:secure_id).returns(secure_id)
       shard.expects(:activate)
 
-      subject.handle(outgoing_from_address, body, html_body, incoming_message, message_id, secure_id)
+      subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
     end
 
     it "calls reply from on the message's context" do
@@ -87,7 +88,7 @@ describe IncomingMail::MessageHandler do
                                             :text => body
                                         })
 
-      subject.handle(outgoing_from_address, body, html_body, incoming_message, message_id, secure_id)
+      subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
     end
 
     context "when a reply from error occurs" do
@@ -107,24 +108,24 @@ describe IncomingMail::MessageHandler do
           Mailer.expects(:create_message).never
           message.context.expects(:reply_from).never
 
-          subject.handle(outgoing_from_address, body, html_body, message, message_id, secure_id)
+          subject.handle(outgoing_from_address, body, html_body, message, tag)
         end
 
         it "silenty fails on invalid secure id" do
           message = stub("message", :notification_id => 1, :global_id => 2, :context => context)
+          ReplyToAddress.any_instance.stubs(:secure_id).returns("deadbeef") # non-matching secure-id
           Message.stubs(:find_by_id).with(message_id).returns(message)
-          ReplyToAddress.any_instance.stubs(:secure_id).returns("non-matching-secure-id")
 
           Mailer.expects(:create_message).never
           message.context.expects(:reply_from).never
-          subject.handle(outgoing_from_address, body, html_body, incoming_message, message_id, secure_id)
+          subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
         end
 
         it "silenty fails if the original message is missing" do
-          Message.expects(:find_by_id).with("unknown-message-id").returns(nil)
+          Message.expects(:find_by_id).with(42).returns(nil)
           Message.any_instance.expects(:deliver).never
 
-          subject.handle(outgoing_from_address, body, html_body, incoming_message, "unknown-message-id", secure_id)
+          subject.handle(outgoing_from_address, body, html_body, incoming_message, "#{secure_id}-42")
         end
       end
 
@@ -138,7 +139,7 @@ describe IncomingMail::MessageHandler do
           Message.any_instance.expects(:deliver).never
           Mailer.expects(:create_message)
 
-          subject.handle(outgoing_from_address, body, html_body, incoming_message, message_id, secure_id)
+          subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
         end
 
         it "bounces the message on invalid context" do
@@ -148,7 +149,7 @@ describe IncomingMail::MessageHandler do
           Message.any_instance.expects(:deliver).never
           Mailer.expects(:create_message)
 
-          subject.handle(outgoing_from_address, body, html_body, invalid_context_message, message_id, secure_id)
+          subject.handle(outgoing_from_address, body, html_body, invalid_context_message, tag)
         end
 
         it "saves and delivers the message with proper input" do
@@ -161,7 +162,7 @@ describe IncomingMail::MessageHandler do
           Message.any_instance.expects(:save)
           Message.any_instance.expects(:deliver)
 
-          subject.handle(outgoing_from_address, body, html_body, incoming_message, message_id, secure_id)
+          subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
         end
 
         it "does not send a message if the incoming message has no from" do
@@ -169,12 +170,12 @@ describe IncomingMail::MessageHandler do
 
           Message.any_instance.expects(:deliver).never
 
-          subject.handle(outgoing_from_address, body, html_body, invalid_incoming_message, message_id, secure_id)
+          subject.handle(outgoing_from_address, body, html_body, invalid_incoming_message, tag)
         end
 
         context "with a generic generic_error" do
           it "constructs the message correctly" do
-            Message.expects(:find_by_id).with("bad-user-message").returns(invalid_user_message)
+            Message.expects(:find_by_id).with(message_id).returns(invalid_user_message)
 
             email_subject = "Message Reply Failed: sorry"
             body = <<-BODY.strip_heredoc
@@ -197,7 +198,7 @@ describe IncomingMail::MessageHandler do
             message = Message.new(message_attributes)
             Message.expects(:new).with(message_attributes).returns(message)
 
-            subject.handle(outgoing_from_address, body, html_body, incoming_message, "bad-user-message", secure_id)
+            subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
           end
         end
 
@@ -226,7 +227,7 @@ describe IncomingMail::MessageHandler do
             message = Message.new(message_attributes)
             Message.expects(:new).with(message_attributes).returns(message)
 
-            subject.handle(outgoing_from_address, body, html_body, incoming_message, message_id, secure_id)
+            subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
           end
         end
 
@@ -235,10 +236,10 @@ describe IncomingMail::MessageHandler do
             Message.any_instance.expects(:deliver).never
             Mailer.expects(:create_message)
 
-            subject.handle(outgoing_from_address, body, html_body, valid_message, message_id, secure_id)
+            subject.handle(outgoing_from_address, body, html_body, valid_message, tag)
           end
         end
       end
     end
   end
-end
+end