Log info messages when MailHandler ignored a reply to a nonexistent issue, journal, or message (#31946).

Patch by Go MAEDA.


git-svn-id: http://svn.redmine.org/redmine/trunk@18480 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/models/mail_handler.rb

@@ -217,8 +217,12 @@ class MailHandler < ActionMailer::Base
 
   # Adds a note to an existing issue
   def receive_issue_reply(issue_id, from_journal=nil)
-    issue = Issue.find_by_id(issue_id)
-    return unless issue
+    issue = Issue.find_by(:id => issue_id)
+    if issue.nil?
+      logger&.info "MailHandler: ignoring reply from [#{email.from.first}] to a nonexistent issue"
+      return nil
+    end
+
     # check permission
     unless handler_options[:no_permission_check]
       unless user.allowed_to?(:add_issue_notes, issue.project) ||
@@ -249,33 +253,42 @@ class MailHandler < ActionMailer::Base
 
   # Reply will be added to the issue
   def receive_journal_reply(journal_id)
-    journal = Journal.find_by_id(journal_id)
-    if journal && journal.journalized_type == 'Issue'
+    journal = Journal.find_by(:id => journal_id)
+    if journal.nil?
+      logger&.info "MailHandler: ignoring reply from [#{email.from.first}] to a nonexistent journal"
+      return nil
+    end
+
+    if journal.journalized_type == 'Issue'
       receive_issue_reply(journal.journalized_id, journal)
+    else
+      logger&.info "MailHandler: ignoring reply from [#{email.from.first}] to a journal whose journalized_type is not Issue"
+      return nil
     end
   end
 
   # Receives a reply to a forum message
   def receive_message_reply(message_id)
-    message = Message.find_by_id(message_id)
-    if message
-      message = message.root
+    message = Message.find_by(:id => message_id)&.root
+    if message.nil?
+      logger&.info "MailHandler: ignoring reply from [#{email.from.first}] to a nonexistent message"
+      return nil
+    end
 
-      unless handler_options[:no_permission_check]
-        raise UnauthorizedAction, "not allowed to add messages to project [#{project.name}]" unless user.allowed_to?(:add_messages, message.project)
-      end
+    unless handler_options[:no_permission_check]
+      raise UnauthorizedAction, "not allowed to add messages to project [#{project.name}]" unless user.allowed_to?(:add_messages, message.project)
+    end
 
-      if !message.locked?
-        reply = Message.new(:subject => cleaned_up_subject.gsub(%r{^.*msg\d+\]}, '').strip,
-                            :content => cleaned_up_text_body)
-        reply.author = user
-        reply.board = message.board
-        message.children << reply
-        add_attachments(reply)
-        reply
-      else
-        logger&.info "MailHandler: ignoring reply from [#{email.from.first}] to a locked topic"
-      end
+    if !message.locked?
+      reply = Message.new(:subject => cleaned_up_subject.gsub(%r{^.*msg\d+\]}, '').strip,
+                          :content => cleaned_up_text_body)
+      reply.author = user
+      reply.board = message.board
+      message.children << reply
+      add_attachments(reply)
+      reply
+    else
+      logger&.info "MailHandler: ignoring reply from [#{email.from.first}] to a locked topic"
     end
   end
 

test/unit/mail_handler_test.rb

@@ -985,6 +985,29 @@ class MailHandlerTest < ActiveSupport::TestCase
     end
   end
 
+  def test_reply_to_a_nonexistent_issue
+    Issue.find(2).destroy
+    assert_no_difference 'Issue.count' do
+      assert_no_difference 'Journal.count' do
+        journal = submit_email('ticket_reply_with_status.eml')
+        assert_nil journal
+      end
+    end
+  end
+
+  def test_reply_to_a_nonexitent_journal
+    journal_id = Issue.find(2).journals.last.id
+    Journal.destroy(journal_id)
+    assert_no_difference 'Issue.count' do
+      assert_no_difference 'Journal.count' do
+        journal = submit_email('ticket_reply.eml') do |email|
+          email.sub! %r{^In-Reply-To:.*$}, "In-Reply-To: <redmine.journal-#{journal_id}.20060719210421@osiris>"
+        end
+        assert_nil journal
+      end
+    end
+  end
+
   def test_reply_to_a_message
     m = submit_email('message_reply.eml')
     assert m.is_a?(Message)
@@ -1015,6 +1038,14 @@ class MailHandlerTest < ActiveSupport::TestCase
     end
   end
 
+  def test_reply_to_a_nonexistent_topic
+    Message.find(2).destroy
+    assert_no_difference('Message.count') do
+      m = submit_email('message_reply_by_subject.eml')
+      assert_nil m
+    end
+  end
+
   def test_should_convert_tags_of_html_only_emails
     with_settings :text_formatting => 'textile' do
       issue = submit_email('ticket_html_only.eml', :issue => {:project => 'ecookbook'})