diff --git a/app/models/incoming_mail/errors.rb b/app/models/incoming_mail/errors.rb index 13523d8e96b..e80d36f797d 100644 --- a/app/models/incoming_mail/errors.rb +++ b/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 \ No newline at end of file +end diff --git a/app/models/incoming_mail/message_handler.rb b/app/models/incoming_mail/message_handler.rb index 2f59e2dae13..adb750c484f 100644 --- a/app/models/incoming_mail/message_handler.rb +++ b/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) - incoming_from = incoming_message.from.try(:first) + def bounce_message(original_message, incoming_message, error, outgoing_from_address, from_channel) + 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' diff --git a/spec/models/incoming_mail/message_handler_spec.rb b/spec/models/incoming_mail/message_handler_spec.rb index 42e3ecb8d94..fdf0964ffa3 100644 --- a/spec/models/incoming_mail/message_handler_spec.rb +++ b/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(:context) { stub("context", reply_from: "reply-from@example.com") } + let_once(:user) do + 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).never - Mailer.expects(:create_message) - + Message.any_instance.expects(:deliver) 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 - Mailer.expects(:create_message) + Message.any_instance.expects(:save) + 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