verify incoming mail sender addresses

fixes CNVS-20518

test plan
- receive a notification from canvas that you can reply to
 (conversation, discussion)
- forward the email to a third party with a different email address
- as the third party, send a reply to the canvas incoming email
 address
- ensure that a reply is not posted in canvas

Change-Id: Ibc79c591d2f231bce519dd056d02a516a3d336c7
Reviewed-on: https://gerrit.instructure.com/54628
Tested-by: Jenkins
Reviewed-by: Matthew Wheeler <mwheeler@instructure.com>
QA-Review: Steven Shepherd <sshepherd@instructure.com>
Product-Review: Joel Hough <joel@instructure.com>

app/models/incoming_mail/errors.rb

@@ -3,6 +3,7 @@ module IncomingMail
     class SilentIgnore < StandardError; end
     class ReplyFrom < StandardError; end
     class UnknownAddress < ReplyFrom; end
+    class UnknownSender < ReplyFrom; end
     class ReplyToLockedTopic < ReplyFrom; end
   end
-end
+end

app/models/incoming_mail/message_handler.rb

@@ -28,11 +28,13 @@ module IncomingMail
       raise IncomingMail::Errors::SilentIgnore unless original_message && original_message.notification_id
       raise IncomingMail::Errors::SilentIgnore unless valid_secure_id?(original_message, secure_id)
 
+      from_channel = nil
       original_message.shard.activate do
-
         context = original_message.context
         user = original_message.user
         raise IncomingMail::Errors::UnknownAddress unless valid_user_and_context?(context, user)
+        from_channel = sent_from_channel(user, incoming_message)
+        raise IncomingMail::Errors::UnknownSender unless from_channel
         context.reply_from({
                                :purpose => 'general',
                                :user => user,
@@ -42,15 +44,15 @@ module IncomingMail
                            })
       end
     rescue IncomingMail::Errors::ReplyFrom => error
-      bounce_message(original_message, incoming_message, error, outgoing_from_address)
+      bounce_message(original_message, incoming_message, error, outgoing_from_address, from_channel)
     rescue IncomingMail::Errors::SilentIgnore
       #do nothing
     end
 
     private
 
-    def bounce_message(original_message, incoming_message, error, outgoing_from_address)
+    def bounce_message(original_message, incoming_message, error, outgoing_from_address, from_channel)
-      incoming_from = incoming_message.from.try(:first)
+      incoming_from = from_channel.try(:path) || incoming_message.from.try(:first)
       incoming_subject = incoming_message.subject
       return unless incoming_from
 
@@ -69,7 +71,7 @@ module IncomingMail
       outgoing_message_delivered = false
 
       original_message.shard.activate do
-        comch = CommunicationChannel.active.email.by_path(incoming_from).first
+        comch = from_channel || CommunicationChannel.active.email.by_path(incoming_from).first
         outgoing_message.communication_channel = comch
         outgoing_message.user = comch.try(:user)
         if outgoing_message.communication_channel
@@ -98,6 +100,14 @@ module IncomingMail
           ndr_body = I18n.t('lib.incoming_message_processor.locked_topic.body', <<-BODY, :subject => subject).gsub(/^ +/, '')
           The message titled "%{subject}" could not be delivered because the discussion topic is locked. If you are trying to contact someone through Canvas you can try logging in to your account and sending them a message using the Inbox tool.
 
+          Thank you,
+          Canvas Support
+          BODY
+        when IncomingMail::Errors::UnknownSender
+          ndr_subject = I18n.t("Message Reply Failed: %{subject}", :subject => subject)
+          ndr_body = I18n.t(<<-BODY, :subject => subject).gsub(/^ +/, '')
+          The message titled "%{subject}" could not be delivered.  The message was sent from an address that is not linked with your Canvas account.  If you are trying to contact someone through Canvas you can try logging in to your account and sending them a message using the Inbox tool.
+
           Thank you,
           Canvas Support
           BODY
@@ -122,6 +132,11 @@ module IncomingMail
       user && context && context.respond_to?(:reply_from)
     end
 
+    def sent_from_channel(user, incoming_message)
+      from_addresses = ((incoming_message.from || []) + (incoming_message.reply_to || [])).uniq
+      user && from_addresses.lazy.map {|addr| user.communication_channels.active.email.by_path(addr).first}.first
+    end
+
     # MOVE!
     def utf8ify(string, encoding)
       encoding ||= 'UTF-8'

spec/models/incoming_mail/message_handler_spec.rb

@@ -25,13 +25,18 @@ describe IncomingMail::MessageHandler do
   let(:original_message_id) { 1 }
   let(:secure_id) { "123abc" }
   let(:tag) { "#{secure_id}-#{original_message_id}" }
-    let(:shard) do
+  let(:shard) do
     stub("shard").tap do |shard|
       shard.stubs(:activate).yields
     end
   end
-  let(:user) { stub("user") }
+  let_once(:user) do
-  let(:context) { stub("context", reply_from: "reply-from@example.com") }
+    user_model
+    channel = @user.communication_channels.create!(:path => "lucy@example.com", :path_type => "email")
+    channel.confirm!
+    @user
+  end
+  let(:context) { stub("context") }
 
   let(:original_message_attributes) {
     {
@@ -39,7 +44,8 @@ describe IncomingMail::MessageHandler do
         :shard => shard,
         :context => context,
         :user => user,
-        :global_id => 1
+        :global_id => 1,
+        :to => "lucy@example.com"
     }
   }
 
@@ -49,7 +55,8 @@ describe IncomingMail::MessageHandler do
         header: {
             :subject => stub("subject", :charset => "utf8")
         },
-        from: ["lucy@example.com"]
+        from: ["lucy@example.com"],
+        reply_to: ["lucy@example.com"]
     }
   }
 
@@ -89,6 +96,7 @@ describe IncomingMail::MessageHandler do
         end
 
         it "silently fails on invalid secure id" do
+          Message.stubs(:where).with(id: original_message_id).returns(stub(first: original_message))
           Canvas::Security.stubs(:verify_hmac_sha1).returns(false)
 
           Mailer.expects(:create_message).never
@@ -109,16 +117,23 @@ describe IncomingMail::MessageHandler do
           Message.any_instance.expects(:deliver).never
           subject.handle(outgoing_from_address, body, html_body, incoming_message, "#{secure_id}-not-an-id")
         end
+
+        it "silently fails if the message is not from one of the original recipient's email addresses" do
+          Message.stubs(:where).with(id: original_message_id).returns(stub(first: original_message))
+          Message.any_instance.expects(:deliver).never
+          original_message.context.expects(:reply_from).never
+          message = stub("incoming message with bad from",
+                         incoming_message_attributes.merge(:from => ['not_lucy@example.com'],
+                                                           :reply_to => ['also_not_lucy@example.com']))
+          subject.handle(outgoing_from_address, body, html_body, message, tag)
+        end
       end
 
       context "bounced messages" do
-        it "bounces the message if user is missing" do
+        it "bounces if user is missing" do
           message = stub("original message without user", original_message_attributes.merge(:user => nil))
           Message.stubs(:where).with(id: original_message_id).returns(stub(first: message))
-
+          Message.any_instance.expects(:deliver)
-          Message.any_instance.expects(:deliver).never
-          Mailer.expects(:create_message)
-
           subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
         end
 
@@ -126,16 +141,13 @@ describe IncomingMail::MessageHandler do
           message = stub("original message with invalid context", original_message_attributes.merge({context: stub("context")}))
           Message.stubs(:where).with(id: original_message_id).returns(stub(first: message))
 
-          Message.any_instance.expects(:deliver).never
+          Message.any_instance.expects(:save)
-          Mailer.expects(:create_message)
+          Message.any_instance.expects(:deliver)
 
           subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
         end
 
         it "saves and delivers the message with proper input" do
-          user_model
-          channel = @user.communication_channels.create!(:path => "lucy@example.com", :path_type => "email")
-          channel.confirm!
           message = stub("original message with invalid context", original_message_attributes.merge({context: stub("context")}))
           Message.stubs(:where).with(id: original_message_id).returns(stub(first: message))
 
@@ -156,7 +168,7 @@ describe IncomingMail::MessageHandler do
 
         context "with a generic generic_error" do
           it "constructs the message correctly" do
-            message = stub("original message without user", original_message_attributes.merge(:user => nil))
+            message = stub("original message without user", original_message_attributes.merge(:context => nil))
             Message.stubs(:where).with(id: original_message_id).returns(stub(first: message))
 
             email_subject = "Message Reply Failed: some subject"
@@ -247,12 +259,14 @@ describe IncomingMail::MessageHandler do
         context "when there is no communication channel" do
           it "bounces the message back to the incoming from address" do
             Message.stubs(:where).with(id: original_message_id).returns(stub(first: original_message))
-            context.expects(:reply_from).raises(IncomingMail::Errors::ReplyToLockedTopic.new)
 
             Message.any_instance.expects(:deliver).never
             Mailer.expects(:create_message)
 
-            subject.handle(outgoing_from_address, body, html_body, incoming_message, tag)
+            message = stub("incoming message with bad from",
+                           incoming_message_attributes.merge(:from => ['not_lucy@example.com'],
+                                                             :reply_to => ['also_not_lucy@example.com']))
+            subject.handle(outgoing_from_address, body, html_body, message, tag)
           end
         end
       end