Email replies to locked topics should be ignored.

fixes #9594

test plan:
 - Configure Canvas to send emails to you.
 - Create a student account and set notifications for annoucements and discussions to "ASAP."
 - Create an annoucement and a discussion and make sure you get the notification emails.
 - Reply to the notification emails and see that the replies get posted as normal.
 - Lock the announcement and discussion.
 - Reply to the emails again and see the replies don't get posted.
 - See that you get an appropriate bounce message.

Change-Id: I4436c61a202d3285ee35a9f9002cefa0f18954fd
Reviewed-on: https://gerrit.instructure.com/13912
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>

app/models/conversation_message.rb

@@ -219,7 +219,7 @@ class ConversationMessage < ActiveRecord::Base
   end
 
   def reply_from(opts)
-    return if self.context.try(:root_account).try(:deleted?)
+    raise IncomingMessageProcessor::UnknownAddressError if self.context.try(:root_account).try(:deleted?)
     conversation.reply_from(opts.merge(:root_account_id => self.root_account_id))
   end
 

app/models/discussion_entry.rb

@@ -119,9 +119,14 @@ class DiscussionEntry < ActiveRecord::Base
   end
 
   def reply_from(opts)
-    return if self.context.root_account.deleted?
+    raise IncomingMessageProcessor::UnknownAddressError if self.context.root_account.deleted?
     user = opts[:user]
-    message = opts[:html].strip
+    if opts[:html]
+      message = opts[:html].strip
+    else
+      message = opts[:text].strip
+      message = format_message(message).first
+    end
     user = nil unless user && self.context.users.include?(user)
     if !user
       raise "Only context participants may reply to messages"
@@ -132,8 +137,12 @@ class DiscussionEntry < ActiveRecord::Base
       entry.discussion_topic_id = self.discussion_topic_id
       entry.parent_entry = self
       entry.user = user
-      entry.save!
-      entry
+      if entry.grants_right?(user, :create)
+        entry.save!
+        entry
+      else
+        raise IncomingMessageProcessor::ReplyToLockedTopicError
+      end
     end
   end
 

app/models/discussion_topic.rb

@@ -407,25 +407,33 @@ class DiscussionTopic < ActiveRecord::Base
   end
 
   def reply_from(opts)
-    return if self.context.root_account.deleted?
+    raise IncomingMessageProcessor::UnknownAddressError if self.context.root_account.deleted?
     user = opts[:user]
-    if opts[:text]
+    if opts[:html]
+      message = opts[:html].strip
+    else
       message = opts[:text].strip
       message = format_message(message).first
-    else
-      message = opts[:html].strip
     end
     user = nil unless user && self.context.users.include?(user)
     if !user
       raise "Only context participants may reply to messages"
     elsif !message || message.empty?
       raise "Message body cannot be blank"
+    elsif !self.grants_right?(user, :read)
+      nil
     else
-      DiscussionEntry.create!({
+      entry = DiscussionEntry.new({
         :message => message,
         :discussion_topic => self,
         :user => user,
       })
+      if !entry.grants_right?(user, :create)
+        raise IncomingMessageProcessor::ReplyToLockedTopicError
+      else
+        entry.save!
+        entry
+      end
     end
   end
 

app/models/submission_comment.rb

@@ -123,7 +123,7 @@ class SubmissionComment < ActiveRecord::Base
   end
 
   def reply_from(opts)
-    return if self.context.root_account.deleted?
+    raise IncomingMessageProcessor::UnknownAddressError if self.context.root_account.deleted?
     user = opts[:user]
     message = opts[:text].strip
     user = nil unless user && self.context.users.include?(user)

lib/ext/string.rb

@@ -25,4 +25,12 @@ class String # :nodoc:
     self.gsub(/[^\w]+/, " ").gsub(/\b('?[a-z])/){$1.capitalize}.strip
   end
 
+  # Backporting this from rails3 because I think it's nice
+  unless method_defined?(:strip_heredoc)
+    def strip_heredoc
+      indent = scan(/^[ \t]*(?=\S)/).min.try(:size) || 0
+      gsub(/^[ \t]{#{indent}}/, '')
+    end
+  end
+
 end

lib/incoming_message_processor.rb

@@ -18,6 +18,11 @@
 require 'iconv'
 
 class IncomingMessageProcessor
+  
+  class ReplyFromError < StandardError; end
+  class UnknownAddressError < ReplyFromError; end
+  class ReplyToLockedTopicError < ReplyFromError; end
+  
   def self.utf8ify(string, encoding)
     encoding ||= 'UTF-8'
     encoding = encoding.upcase
@@ -39,25 +44,23 @@ class IncomingMessageProcessor
       html_body = format_message(body).first
     end
 
-    msg = Message.find_by_id(message_id)
-    if msg
+    begin
+      msg = Message.find_by_id(message_id)
+      raise IncomingMessageProcessor::UnknownAddressError unless msg
       msg.shard.activate do
         context = msg.context if secure_id == msg.reply_to_secure_id
         user = msg.user
-        if user && context && context.respond_to?(:reply_from)
-          context.reply_from({
-            :purpose => 'general',
-            :user => user,
-            :subject => utf8ify(message.subject, message.header[:subject].try(:charset)),
-            :html => html_body,
-            :text => body
-          })
-        else
-          IncomingMessageProcessor.ndr(message.from.first, message.subject) if message.from.try(:first)
-        end
+        raise IncomingMessageProcessor::UnknownAddressError unless user && context && context.respond_to?(:reply_from)
+        context.reply_from({
+          :purpose => 'general',
+          :user => user,
+          :subject => utf8ify(message.subject, message.header[:subject].try(:charset)),
+          :html => html_body,
+          :text => body
+        })
       end
-    else
-      IncomingMessageProcessor.ndr(message.from.first, message.subject) if message.from.try(:first)
+    rescue IncomingMessageProcessor::ReplyFromError => error
+      IncomingMessageProcessor.ndr(message.from.first, message.subject, error) if message.from.try(:first)
     end
   end
 
@@ -79,17 +82,14 @@ class IncomingMessageProcessor
     end
   end
 
-  def self.ndr(from, subject)
+  def self.ndr(from, subject, error = IncomingMessageProcessor::UnknownAddressError)
+    ndr_subject, ndr_body = IncomingMessageProcessor.ndr_strings(subject, error)
+
     message = Message.create!(
       :to => from,
       :from => HostUrl.outgoing_email_address,
-      :subject => I18n.t('lib.incoming_message_processor.failure_message.subject', "Message Reply Failed: %{subject}", :subject => subject),
-      :body => I18n.t('lib.incoming_message_processor.failure_message.body', <<-BODY, :subject => subject),
-The message titled "%{subject}" could not be delivered.  The message was sent to an unknown mailbox address.  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
+      :subject => ndr_subject,
+      :body => ndr_body,
       :delay_for => 0,
       :context => nil,
       :path_type => 'email',
@@ -98,4 +98,29 @@ Canvas Support
     message.deliver
   end
 
+  def self.ndr_strings(subject, error)
+    ndr_subject = ""
+    ndr_body = ""
+    case error
+    when IncomingMessageProcessor::ReplyToLockedTopicError
+      ndr_subject = I18n.t('lib.incoming_message_processor.locked_topic.subject', "Message Reply Failed: %{subject}", :subject => subject)
+      ndr_body = I18n.t('lib.incoming_message_processor.locked_topic.body', <<-BODY, :subject => subject).strip_heredoc
+        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
+    else # including IncomingMessageProcessor::UnknownAddressError
+      ndr_subject = I18n.t('lib.incoming_message_processor.failure_message.subject', "Message Reply Failed: %{subject}", :subject => subject)
+      ndr_body = I18n.t('lib.incoming_message_processor.failure_message.body', <<-BODY, :subject => subject).strip_heredoc
+        The message titled "%{subject}" could not be delivered.  The message was sent to an unknown mailbox address.  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
+    end
+
+    [ndr_subject, ndr_body]
+  end
+
 end

spec/lib/incoming_message_processor_spec.rb

@@ -25,6 +25,19 @@ describe IncomingMessageProcessor do
       def reply_from(opts)
         @@incoming_replies ||= []
         @@incoming_replies << opts
+        if @@reply_from_result.kind_of?(Class) && @@reply_from_result.ancestors.include?(Exception)
+          raise @@reply_from_result
+        else
+          @@reply_from_result
+        end
+      end
+
+      def self.reply_from_result
+        @@reply_from_result
+      end
+
+      def self.reply_from_result=(value)
+        @@reply_from_result = value
       end
 
       def self.incoming_replies
@@ -39,9 +52,12 @@ describe IncomingMessageProcessor do
 
   before(:each) do
     DiscussionTopic.incoming_replies = []
+    DiscussionTopic.reply_from_result = DiscussionEntry.new
 
     discussion_topic_model
     @message = Message.create(:context => @topic, :user => @user)
+    @previous_message_count = Message.count
+    @previous_message_count.should == 1
   end
 
   after(:all) do
@@ -80,6 +96,61 @@ describe IncomingMessageProcessor do
     DiscussionTopic.incoming_replies[0][:html].should == '<h1>This is HTML</h1>'
   end
 
+  describe "when data is not found" do
+    it "should send a bounce reply when sent a bogus secure_id" do
+      IncomingMessageProcessor.process_single(Mail.new { body "body"; from "a@b.c" }, 
+        "deadbeef", @message.id)
+      Message.count.should == @previous_message_count + 1
+      # new_message = Message.find_by_id(@message.id + 1)
+      new_message = Message.find(:first, :order => 'created_at DESC')
+      new_message.subject.should match(/Reply Failed/)
+      new_message.body.should match(/unknown mailbox/)
+    end
+
+    it "should send a bounce reply when sent a bogus message id" do
+      IncomingMessageProcessor.process_single(Mail.new { body "body"; from "a@b.c" },
+        @message.reply_to_secure_id, -1)
+      Message.count.should == @previous_message_count + 1
+      # new_message = Message.find_by_id(@message.id + 1)
+      new_message = Message.find(:first, :order => 'created_at DESC')
+      new_message.subject.should match(/Reply Failed/)
+      new_message.body.should match(/unknown mailbox/)
+    end
+
+    it "should send a bounce reply when user is not found" do
+      @message.user = nil
+      @message.save!
+      IncomingMessageProcessor.process_single(Mail.new { body "body"; from "a@b.c" }, 
+        @message.reply_to_secure_id, @message.id)
+      Message.count.should == @previous_message_count + 1
+      # new_message = Message.find_by_id(@message.id + 1)
+      new_message = Message.find(:first, :order => 'created_at DESC')
+      new_message.subject.should match(/Reply Failed/)
+      new_message.body.should match(/unknown mailbox/)
+    end
+
+    it "should send a bounce reply when context is not found" do
+      @message.context = nil
+      @message.save!
+      IncomingMessageProcessor.process_single(Mail.new { body "body"; from "a@b.c" }, 
+        @message.reply_to_secure_id, @message.id)
+      Message.count.should == @previous_message_count + 1
+      # new_message = Message.find_by_id(@message.id + 1)
+      new_message = Message.find(:first, :order => 'created_at DESC')
+      new_message.subject.should match(/Reply Failed/)
+      new_message.body.should match(/unknown mailbox/)
+    end
+  end
+
+  it "should send a bounce reply when reply_from raises ReplyToLockedTopicError" do
+    DiscussionTopic.reply_from_result = IncomingMessageProcessor::ReplyToLockedTopicError
+    test_mail = Mail.new { body "reply body"; from "test@example.a" }
+    IncomingMessageProcessor.process_single(test_mail, @message.reply_to_secure_id, @message.id)
+    Message.count.should == @previous_message_count + 1
+    new_message = Message.find(:first, :order => 'created_at DESC')
+    new_message.body.should match(/topic is locked/)
+  end
+
   it "should process emails from mailman" do
     Dir.mktmpdir do |tmpdir|
       newdir = tmpdir + "/new"

spec/models/conversation_message_spec.rb

@@ -270,14 +270,13 @@ describe ConversationMessage do
       Account.default.destroy
       cm.reload
 
-      cm2 = cm.reply_from({
+      lambda { cm.reply_from({
         :purpose => 'general',
         :user => @teacher,
         :subject => "an email reply",
         :html => "body",
         :text => "body"
-      })
-      cm2.should be_nil
+      }) }.should raise_error(IncomingMessageProcessor::UnknownAddressError)
     end
   end
 end

spec/models/discussion_entry_spec.rb

@@ -390,7 +390,24 @@ describe DiscussionEntry do
       root = @topic.reply_from(:user => @teacher, :text => "root entry")
       Account.default.destroy
       root.reload
-      root.reply_from(:user => @teacher, :text => "sub entry").should be_nil
+      lambda { root.reply_from(:user => @teacher, :text => "sub entry") }.should raise_error(IncomingMessageProcessor::UnknownAddressError)
+    end
+
+    it "should prefer html to text" do
+      course_with_teacher
+      discussion_topic_model
+      @entry = @topic.reply_from(:user => @teacher, :text => "topic")
+      msg = @entry.reply_from(:user => @teacher, :text => "text body", :html => "<p>html body</p>")
+      msg.should_not be_nil
+      msg.message.should == "<p>html body</p>"
+    end
+
+    it "should not allow replies to locked topics" do
+      course_with_teacher
+      discussion_topic_model
+      @entry = @topic.reply_from(:user => @teacher, :text => "topic")
+      @topic.lock!
+      lambda { @entry.reply_from(:user => @teacher, :text => "reply") }.should raise_error(IncomingMessageProcessor::ReplyToLockedTopicError)
     end
   end
 end

spec/models/discussion_topic_spec.rb

@@ -755,7 +755,23 @@ describe DiscussionTopic do
       @context = @course
       discussion_topic_model(:user => @teacher)
       account.destroy
-      @topic.reply_from(:user => @teacher, :text => "entry").should be_nil
+      lambda { @topic.reply_from(:user => @teacher, :text => "entry") }.should raise_error(IncomingMessageProcessor::UnknownAddressError)
     end
+
+    it "should prefer html to text" do
+      course_with_teacher
+      discussion_topic_model
+      msg = @topic.reply_from(:user => @teacher, :text => "text body", :html => "<p>html body</p>")
+      msg.should_not be_nil
+      msg.message.should == "<p>html body</p>"
+    end
+
+    it "should not allow replies to locked topics" do
+      course_with_teacher
+      discussion_topic_model
+      @topic.lock!
+      lambda { @topic.reply_from(:user => @teacher, :text => "reply") }.should raise_error(IncomingMessageProcessor::ReplyToLockedTopicError)
+    end
+
   end
 end

spec/models/submission_comment_spec.rb

@@ -606,4 +606,16 @@ This text has a http://www.google.com link in it...
     @reviewer_comment.grants_right?(@student1, :read).should be_true
     @my_comment.grants_right?(@student1, :read).should be_true
   end
+
+  describe "reply_from" do
+    it "should ignore replies on deleted accounts" do
+      comment = @submission.add_comment(:user => @teacher, :comment => "some comment")
+      Account.default.destroy
+      comment.reload
+      lambda { 
+        comment.reply_from(:user => @student, :text => "some reply") 
+      }.should raise_error(IncomingMessageProcessor::UnknownAddressError)
+    end
+  end
+
 end