add multiple reply-to support to outgoing mail.

fixes CNVS-2514

allow multiple reply-to addresses that will be rotated
across when sending mail. this commit, in conjunction with
CNVS-3781, allows canvas to receive mail from multiple
inboxes and distribute its incoming mail across them.

test plan:
  * configure config/outgoing_mail.yml with multiple
    reply_to_addresses as explained in
    config/outgoing_mail.yml.example;
  * trigger at least n notifications for a user in canvas,
    where n is the number of reply_to_addresses configured;
  * visit /user/:id/messages and verify that each message
    is sent from a different reply_to address.

Change-Id: I53c01c7c355dcbf7c16595070f41eee721814849
Reviewed-on: https://gerrit.instructure.com/17870
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Zach Pendleton <zachp@instructure.com>
QA-Review: Zach Pendleton <zachp@instructure.com>

app/models/mailer.rb

@@ -25,7 +25,7 @@ class Mailer < ActionMailer::Base
     bcc m.bcc if m.bcc
     cc m.cc if m.cc
     from ("#{m.from_name || HostUrl.outgoing_email_default_name} <" + HostUrl.outgoing_email_address + ">")
-    reply_to m.reply_to_address
+    reply_to ReplyToAddress.new(m).address
     subject m.subject
     if m.html_body
       content_type 'multipart/alternative'

app/models/message.rb

@@ -435,31 +435,6 @@ class Message < ActiveRecord::Base
     @i18n_scope = nil
   end
 
-  # Public: Construct a unique reply_to string for this message. This allows
-  # us to associate any email responses to this message.
-  #
-  # Returns a secure ID string.
-  def reply_to_secure_id
-    Canvas::Security.hmac_sha1(global_id.to_s)
-  end
-
-  # Public: Construct a reply_to address for this message.
-  #
-  # Returns an email address string.
-  def reply_to_address
-    # Not sure this first line needs to be a thing.
-    reply_address = (forced_reply_to || nil)  rescue nil
-    reply_address = nil if path_type == 'sms' rescue false
-    reply_address = from if context_type == 'ErrorReport'
-
-    unless reply_address
-      address, domain = HostUrl.outgoing_email_address.split('@')
-      reply_address = "#{address}+#{reply_to_secure_id}-#{global_id}@#{domain}"
-    end
-
-    reply_address
-  end
-
   # Public: Deliver this message.
   #
   # Returns nothing.

app/views/messages/_message.html.erb

@@ -1,6 +1,6 @@
 <li class="message list-view border border-trbl"
-    data-message-id="<%= message.id %>"
-    data-secure-id="<%= message.reply_to_secure_id %>">
+  data-message-id="<%= message.id %>"
+  data-secure-id="<%= ReplyToAddress.new(message).secure_id %>">
   <header>
     <span class="h6"><%= message.subject %></span>
 
@@ -18,7 +18,7 @@
 
     <dt><%= t('titles.reply_to', 'Reply to') %></dt>
     <dd>
-      <span class="message-reply"><%= message.reply_to_address %></span>
+      <span class="message-reply"><%= ReplyToAddress.new(message).address %></span>
       (<a class="reply-button" href="#" role="button"><%= t('actions.reply', 'Reply') %></a>)
     </dd>
 

config/initializers/outgoing_mail.rb

@@ -17,6 +17,9 @@ Rails.configuration.to_prepare do
   HostUrl.outgoing_email_address = config[:outgoing_address]
   HostUrl.outgoing_email_domain = config[:domain]
   HostUrl.outgoing_email_default_name = config[:default_name]
+
+  ReplyToAddress.address_pool = config[:reply_to_addresses] ||
+    Array(HostUrl.outgoing_email_address)
 end
 
 # delivery_method can be :smtp, :sendmail or :test
@@ -29,3 +32,4 @@ when :smtp
 when :sendmail
   ActionMailer::Base.sendmail_settings.merge!(config)
 end
+

config/outgoing_mail.yml.example

@@ -25,3 +25,23 @@ production:
   domain: "example.com"
   outgoing_address: "canvas@example.com"
   default_name: "Instructure Canvas"
+
+# If receiving mail from multiple inboxes (see incoming_mail.yml.example),
+# you'll want to include those addresses in a reply_to_addresses array so
+# Canvas will select the Reply-To field of outgoing messages from all of the
+# incoming mailboxes.
+
+multiple_inboxes:
+  address: "smtp.example.com"
+  port: "25"
+  user_name: "user"
+  password: "password"
+  authentication: "plain" # plain, login, or cram_md5
+  domain: "example.com"
+  outgoing_address: "canvas@example.com"
+  default_name: "Instructure Canvas"
+  reply_to_addresses:
+  - "canvas1@example.com"
+  - "canvas2@example.com"
+  - "canvas3@example.com"
+  - "canvas4@example.com"

lib/incoming_message_processor.rb

@@ -112,7 +112,7 @@ class IncomingMessageProcessor
       # This prevents us from rebouncing users that have auto-replies setup -- only bounce something
       # that was sent out because of a notification.
       raise IncomingMessageProcessor::SilentIgnoreError unless original_message && original_message.notification_id
-      raise IncomingMessageProcessor::SilentIgnoreError unless secure_id == original_message.reply_to_secure_id
+      raise IncomingMessageProcessor::SilentIgnoreError unless secure_id == ReplyToAddress.new(original_message).secure_id
 
       original_message.shard.activate do
         context = original_message.context

lib/reply_to_address.rb

@@ -0,0 +1,79 @@
+#
+# Copyright (C) 2013 Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# 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: Create a new ReplyToAddress.
+  #
+  # message - A Message object.
+  def initialize(message)
+    @message = message
+  end
+
+  # Public: Construct a reply-to address.
+  #
+  # Returns an email address string.
+  def address
+    return nil if message.path_type == 'sms'
+    return message.from if message.context_type == 'ErrorReport'
+
+    address, domain = ReplyToAddress.address_from_pool(message).split('@')
+    "#{address}+#{secure_id}-#{message.global_id}@#{domain}"
+  end
+
+  alias :to_s :address
+
+  # Public: Generate the unique, secure ID for this address' message.
+  #
+  # Returns a secure ID string.
+  def secure_id
+    Canvas::Security.hmac_sha1(message.global_id.to_s)
+  end
+
+  class << self
+    # Internal: An array of email addresses to be used in the reply-to field.
+    attr_writer :address_pool
+
+    # Public: Return a reply-to address from the class' address pool. Use a
+    # modulo operation w/ the message ID to ensure that the same Reply-To is
+    # always used for a given message.
+    #
+    # message - A message object to construct a reply-to address for.
+    #
+    # Returns an email address string.
+    def address_from_pool(message)
+      raise EmptyReplyAddressPool unless address_pool.present?
+      index = if message.id.present?
+                message.id % address_pool.length
+              else
+                rand(address_pool.length)
+              end
+
+      address_pool[index]
+    end
+
+    private
+    # Internal: Array of email addresses to use as Reply-To addresses.
+    attr_reader :address_pool
+  end
+end

spec/lib/incoming_message_processor_spec.rb

@@ -106,7 +106,7 @@ describe IncomingMessageProcessor do
     end
 
     it "should not choke on invalid UTF-8" do
-      IncomingMessageProcessor.process_single(Mail.new { body "he\xffllo" }, @message.reply_to_secure_id, @message.id)
+      IncomingMessageProcessor.process_single(Mail.new { body "he\xffllo" }, ReplyToAddress.new(@message).secure_id, @message.id)
       DiscussionTopic.incoming_replies.length.should == 1
       DiscussionTopic.incoming_replies[0][:text].should == 'hello'
       DiscussionTopic.incoming_replies[0][:html].should == 'hello'
@@ -116,7 +116,7 @@ describe IncomingMessageProcessor do
       IncomingMessageProcessor.process_single(Mail.new {
           content_type 'text/plain; charset=Shift-JIS'
           body "\x83\x40"
-        }, @message.reply_to_secure_id, @message.id)
+        }, ReplyToAddress.new(@message).secure_id, @message.id)
       DiscussionTopic.incoming_replies.length.should == 1
       comparison_string = "\xe3\x82\xa1"
       comparison_string.force_encoding("UTF-8") if RUBY_VERSION >= '1.9'
@@ -133,7 +133,7 @@ describe IncomingMessageProcessor do
             content_type 'text/html; charset=UTF-8'
             body '<h1>This is HTML</h1>'
           end
-        }, @message.reply_to_secure_id, @message.id)
+        }, ReplyToAddress.new(@message).secure_id, @message.id)
       DiscussionTopic.incoming_replies.length.should == 1
       DiscussionTopic.incoming_replies[0][:text].should == 'This is plain text'
       DiscussionTopic.incoming_replies[0][:html].should == '<h1>This is HTML</h1>'
@@ -143,7 +143,7 @@ describe IncomingMessageProcessor do
       it "should not send a bounce reply sent a bogus message id" do
         expect {
           IncomingMessageProcessor.process_single(simple_mail_from_user,
-            @message.reply_to_secure_id, -1)
+            ReplyToAddress.new(@message).secure_id, -1)
         }.to change { ActionMailer::Base.deliveries.size }.by(0)
         Message.count.should eql @previous_message_count
       end
@@ -162,7 +162,7 @@ describe IncomingMessageProcessor do
         @message.save!
         expect {
           IncomingMessageProcessor.process_single(simple_mail_from_user,
-            @message.reply_to_secure_id, @message.id)
+            ReplyToAddress.new(@message).secure_id, @message.id)
         }.to change { ActionMailer::Base.deliveries.size }.by(0)
         Message.count.should eql @previous_message_count
       end
@@ -174,7 +174,7 @@ describe IncomingMessageProcessor do
         incoming_bounce_mail['Auto-Submitted'] = 'auto-generated' # but don't bounce with this header
         expect {
           IncomingMessageProcessor.process_single(incoming_bounce_mail,
-            @message.reply_to_secure_id, @message.id)
+            ReplyToAddress.new(@message).secure_id, @message.id)
         }.to change { ActionMailer::Base.deliveries.size }.by(0)
         Message.count.should eql @previous_message_count
       end
@@ -183,7 +183,7 @@ describe IncomingMessageProcessor do
         @message.user = nil
         @message.save!
         IncomingMessageProcessor.process_single(simple_mail_from_user, 
-          @message.reply_to_secure_id, @message.id)
+          ReplyToAddress.new(@message).secure_id, @message.id)
         check_new_message(:unknown)
       end
 
@@ -191,7 +191,7 @@ describe IncomingMessageProcessor do
         @message.context = nil
         @message.save!
         IncomingMessageProcessor.process_single(simple_mail_from_user, 
-          @message.reply_to_secure_id, @message.id)
+          ReplyToAddress.new(@message).secure_id, @message.id)
         check_new_message(:unknown)
       end
 
@@ -200,7 +200,7 @@ describe IncomingMessageProcessor do
         @message.save!
         expect {
           IncomingMessageProcessor.process_single(Mail.new(:body => "body", :from => "bogus_email@example.com"),
-          @message.reply_to_secure_id, @message.id)
+          ReplyToAddress.new(@message).secure_id, @message.id)
         }.to change { ActionMailer::Base.deliveries.size }.by(1)
         Message.count.should eql @previous_message_count
       end
@@ -209,7 +209,7 @@ describe IncomingMessageProcessor do
     it "should send a bounce reply when reply_from raises ReplyToLockedTopicError" do
       DiscussionTopic.reply_from_result = IncomingMessageProcessor::ReplyToLockedTopicError
       IncomingMessageProcessor.process_single(simple_mail_from_user, 
-        @message.reply_to_secure_id, @message.id)
+        ReplyToAddress.new(@message).secure_id, @message.id)
       check_new_message(:locked)
     end
 
@@ -219,7 +219,7 @@ describe IncomingMessageProcessor do
         Dir.mkdir(newdir)
         
         addr, domain = HostUrl.outgoing_email_address.split(/@/)
-        to_address = "#{addr}+#{@message.reply_to_secure_id}-#{@message.id}@#{domain}"
+        to_address = "#{addr}+#{ReplyToAddress.new(@message).secure_id}-#{@message.id}@#{domain}"
         mail = Mail.new do
           from 'test@example.com'
           to to_address

spec/lib/reply_to_address_spec.rb

@@ -0,0 +1,113 @@
+#
+# Copyright (C) 2013 Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+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
+
+    it 'should persist the message argument' do
+      message = Message.new
+      ReplyToAddress.new(message).message.should == message
+    end
+  end
+
+  describe 'address' do
+    it 'should return nil for SMS messages' do
+      message = mock()
+      message.expects(:path_type).returns('sms')
+
+      ReplyToAddress.new(message).address.should be_nil
+    end
+
+    it 'should return the message from address for error reports' do
+      message = mock()
+      message.expects(:path_type).returns('email')
+      message.expects(:context_type).returns('ErrorReport')
+      message.expects(:from).returns('user@example.com')
+
+      ReplyToAddress.new(message).address.should == 'user@example.com'
+    end
+
+    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')
+      message.expects(:id).twice.returns(1)
+      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"
+    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
+    end
+  end
+
+  describe 'self.address_pool=' do
+    it 'should persist an address pool' do
+      pool = %w{canvas@example.com canvas2@example.com}
+      ReplyToAddress.address_pool = pool
+
+      ReplyToAddress.instance_variable_get(:@address_pool).should == pool
+    end
+  end
+
+  describe 'self.address_from_pool' do
+    it 'should return an address from the pool in a deterministic way' do
+      message, message2 = [mock(), mock()]
+
+      message.expects(:id).twice.returns(14)
+      message2.expects(:id).twice.returns(15)
+      ReplyToAddress.address_pool = %w{canvas@example.com canvas2@example.com}
+
+      ReplyToAddress.address_from_pool(message).should  == 'canvas@example.com'
+      ReplyToAddress.address_from_pool(message2).should == 'canvas2@example.com'
+    end
+
+    it 'should raise EmptyReplyAddressPool if pool is empty' do
+      message = mock()
+      ReplyToAddress.address_pool = []
+
+      lambda {
+        ReplyToAddress.address_from_pool(message)
+      }.should raise_error(EmptyReplyAddressPool)
+    end
+
+    it 'should randomly select a pool address if the message has no id' do
+      message = mock()
+
+      message.expects(:id).returns(nil)
+      ReplyToAddress.address_pool = %w{canvas@example.com}
+
+      ReplyToAddress.address_from_pool(message).should  == 'canvas@example.com'
+    end
+  end
+end