improve robustness of incoming message processor

replaced the mailman gem with custom code with more error
handling. this will allow the incoming message processor to
continue processing messages after encountering a message with
an encoding or parsing error. the broken messages will be moved
aside to a separate folder for later inspection.

fixes CNVS-4970

test plan:
 - read up on the new incoming_mail.yml configuration settings.
 - configure incoming_mail.yml with the test imap accounts
   using legacy settings and check for regressions.
 - reconfigure incoming_mail.yml to read from a directory.
 - copy some testing email files into the configured directory.
   test files should be a mix of:
   - emails with encoding errors
   - emails with syntax errors
   - normal emails
 - all of the normal emails should be processed normally
 - all of the error emails should be moved into the error
   subdirectory

Change-Id: I0f946a56b41492f007db2775aa6da3cdfa4fdd3f
Reviewed-on: https://gerrit.instructure.com/19729
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Zach Pendleton <zachp@instructure.com>
Product-Review: Marc LeGendre <marc@instructure.com>
QA-Review: Marc LeGendre <marc@instructure.com>

Gemfile

@@ -50,7 +50,6 @@ if ONE_NINE
 else
   gem 'mail', '2.4.4'
 end
-gem 'mailman',        '0.5.3'
 # using this forked gem until https://github.com/37signals/marginalia/pull/15 is in the source gem
 gem 'instructure-marginalia',     '1.1.3',    :require => false
 gem 'mime-types',     '1.17.2',   :require => 'mime/types'

app/controllers/messages_controller.rb

@@ -36,7 +36,7 @@ class MessagesController < ApplicationController
     message['From']         = params[:from]
     message.body            = params[:message]
 
-    IncomingMessageProcessor.process_single(message, secure_id, message_id)
+    IncomingMail::IncomingMessageProcessor.process_single(message, secure_id, message_id)
     render :nothing => true
   end
 

app/models/conversation_message.rb

@@ -227,7 +227,7 @@ class ConversationMessage < ActiveRecord::Base
   end
 
   def reply_from(opts)
-    raise IncomingMessageProcessor::UnknownAddressError if self.context.try(:root_account).try(:deleted?)
+    raise IncomingMail::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

@@ -121,7 +121,7 @@ class DiscussionEntry < ActiveRecord::Base
   end
 
   def reply_from(opts)
-    raise IncomingMessageProcessor::UnknownAddressError if self.context.root_account.deleted?
+    raise IncomingMail::IncomingMessageProcessor::UnknownAddressError if self.context.root_account.deleted?
     user = opts[:user]
     if opts[:html]
       message = opts[:html].strip
@@ -143,7 +143,7 @@ class DiscussionEntry < ActiveRecord::Base
         entry.save!
         entry
       else
-        raise IncomingMessageProcessor::ReplyToLockedTopicError
+        raise IncomingMail::IncomingMessageProcessor::ReplyToLockedTopicError
       end
     end
   end

app/models/discussion_topic.rb

@@ -395,7 +395,7 @@ class DiscussionTopic < ActiveRecord::Base
   end
 
   def reply_from(opts)
-    raise IncomingMessageProcessor::UnknownAddressError if self.context.root_account.deleted?
+    raise IncomingMail::IncomingMessageProcessor::UnknownAddressError if self.context.root_account.deleted?
     user = opts[:user]
     if opts[:html]
       message = opts[:html].strip
@@ -417,7 +417,7 @@ class DiscussionTopic < ActiveRecord::Base
         :user => user,
       })
       if !entry.grants_right?(user, :create)
-        raise IncomingMessageProcessor::ReplyToLockedTopicError
+        raise IncomingMail::IncomingMessageProcessor::ReplyToLockedTopicError
       else
         entry.save!
         entry

app/models/submission_comment.rb

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

config/incoming_mail.yml.example

@@ -1,48 +1,57 @@
-# See http://rubydoc.info/github/titanous/mailman/master/file/USER_GUIDE.md for
+# Update note: incoming mail processing used to use the mailman gem and accepted
-# available options.  rails_root will be configured automatically.
+# mailman configurations. The implementation has changed to accomodate more
+# robust error handling. Some of the mailman features have been removed (such as
+# stdin and maildir processing). Some mailman configuration settings are still
+# accepted but log a deprecation warning. The configuration settings for imap
+# and pop3 are the same as before.
 #
-# Currently, there are three ways to process incoming mail:
+# The mail processor can run in the following ways:
-#  * Fetch directly from POP3/IMAP, controlled by the Canvas job server
+#  * controlled by the Canvas job server:
-#    * Make sure to configure pop3/imap, ignore stdin, and set the poll_interval to 0
+#     * set run_periodically: true
-#  * Process messages by piping them to script/process_incoming_messages
+#  * by running script/process_incoming_emails periodically
-#    * Make sure to not ignore stdin
+#
-#  * Monitor a maildir
+# Incoming mail can be received in the following ways:
-#    * Make sure to ignore stdin
+#  * imap
-#    * If poll_interval is 0, it will be run periodically by the Canvas job
+#     * requires server, username, password
-#      server
+#     * defaults ssl => true, port => 993, filter => "ALL", error_folder => "errors"
-#    * If poll_interval is non-0, script/process_incoming_messages will never
+#     * filter can be an array
-#      return, continually monitoring the directory
+#  * pop3
+#     * requires server, username, password
+#     * defaults ssl => true, port => 995
+#  * directory (read email files from a directory)
+#     * requires folder (relative to rails root, but an absolute path would be best)
+#     * defaults error_folder => "errors"
+# 
+
 
 development:
-  # defaults are to allow reading from stdin
+  # defaults will not read incoming mail from any source
 
 test:
-  maildir: "maildir"
+  directory: 
-  ignore_stdin: true
+    folder: "/tmp/maildir"
-  poll_interval: 0
 
 production:
-  poll_interval: 0
+  run_periodically: true
-  ignore_stdin: true
   pop3:
     server: "pop.example.com"
-    port: 110
+    port: 995
+    ssl: true
     username: "user"
     password: "password"
-    ssl: true
 
-# To configure multiple pop3/imap accounts, configure a pop3/imap section with 
+# To configure multiple pop3/imap accounts, configure a pop3/imap/directory
-# default values and add an accounts section that contains an array of 
+# section with default values and add an accounts section that contains an
-# override values for each account. In this case, poll_interval must be set to 0.
+# array of override values for each account. 
 # example:
 
 multiple_inboxes:
-  poll_interval: 0
+  run_periodically: true
-  ignore_stdin: true
   imap:
     server: "imap.example.com"
-    port: 143
+    port: 993
     ssl: true
+    filter: ALL
     accounts:
     - username: "user1@example.com"
       password: "pass1"
@@ -51,4 +60,4 @@ multiple_inboxes:
       password: "pass2"
 
     - username: "user3@example.com"
-      password: "pass3"
+      password: "pass3"

config/initializers/incoming_mail.rb

@@ -3,5 +3,5 @@
 config = Setting.from_config("incoming_mail") || {}
 
 Rails.configuration.to_prepare do
-  IncomingMessageProcessor.configure(config)
+  IncomingMail::IncomingMessageProcessor.configure(config)
 end

config/periodic_jobs.rb

@@ -64,9 +64,9 @@ Delayed::Periodic.cron 'StreamItem.destroy_stream_items', '45 11 * * *' do
   end
 end
 
-if Mailman.config.poll_interval == 0 && Mailman.config.ignore_stdin == true
+if IncomingMail::IncomingMessageProcessor.run_periodically?
   Delayed::Periodic.cron 'IncomingMessageProcessor.process', '*/1 * * * *' do
-    IncomingMessageProcessor.process
+    IncomingMail::IncomingMessageProcessor.process
   end
 end
 

lib/incoming_mail/deprecated_settings.rb

@@ -0,0 +1,21 @@
+#
+# 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/>.
+#
+
+module IncomingMail
+  DeprecatedSettings = Struct.new(:poll_interval, :ignore_stdin, :graceful_death)
+end

lib/incoming_mail/directory_mailbox.rb

@@ -0,0 +1,91 @@
+#
+# 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 'fileutils'
+
+module IncomingMail
+  
+  class DirectoryMailbox
+
+    attr_accessor :folder
+
+    def initialize(options = {})
+      @folder = options.fetch(:folder, "")
+    end
+
+    def connect
+      raise "Folder #{folder} does not exist." unless folder_exists?(folder)
+    end
+
+    def disconnect
+      # nothing to do    
+    end
+
+    def each_message
+      files_in_folder(folder).each do |filename|
+        if file?(folder, filename)
+          body = read_file(folder, filename)
+          yield filename, body
+        end
+      end
+    end
+
+    def delete_message(filename)
+      delete_file(folder, filename)
+    end
+
+    def move_message(filename, target_folder)
+      unless folder_exists?(folder, target_folder)
+        create_folder(folder, target_folder)
+      end
+      move_file(folder, filename, target_folder)
+    end
+
+  private
+    def folder_exists?(folder, subfolder = nil)
+      to_check = subfolder ? File.join(folder, subfolder) : folder
+      File.directory?(to_check)
+    end
+
+    def files_in_folder(folder)
+      Dir.entries(folder)
+    end
+
+    def read_file(folder, filename)
+      File.read(File.join(folder, filename))
+    end
+
+    def file?(folder, filename)
+      File.file?(File.join(folder, filename))
+    end
+
+    def delete_file(folder, filename)
+      File.delete(File.join(folder, filename))
+    end
+
+    def move_file(folder, filename, target_folder)
+      FileUtils.mv(File.join(folder, filename), File.join(folder, target_folder))
+    end
+
+    def create_folder(folder, subfolder)
+      Dir.mkdir(File.join(folder, subfolder))
+    end
+
+  end
+
+end

lib/incoming_mail/imap_mailbox.rb

@@ -0,0 +1,71 @@
+#
+# 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 'net/imap'
+
+module IncomingMail
+
+  class ImapMailbox
+    attr_accessor :server, :port, :ssl, :username, :password, :folder, :filter
+
+    def initialize(options = {})
+      @server   = options.fetch(:server, "")
+      @port     = options.fetch(:port, 993)
+      @ssl      = options.fetch(:ssl, true)
+      @username = options.fetch(:username, "")
+      @password = options.fetch(:password, "")
+      @folder   = options.fetch(:folder, "INBOX")
+      @filter   = Array(options.fetch(:filter, "ALL"))
+    end
+
+    def connect
+      @imap = Net::IMAP.new(@server, :port => @port, :ssl => @ssl)
+      @imap.login(@username, @password)
+    end
+
+    def disconnect
+      @imap.logout
+      @imap.disconnect
+    rescue
+    end
+
+    def each_message
+      @imap.select(@folder)
+      @imap.search(@filter).each do |message_id|
+        body = @imap.fetch(message_id, "RFC822")[0].attr["RFC822"]
+        yield message_id, body
+      end
+      @imap.expunge
+    end
+
+    def delete_message(message_id)
+      @imap.store(message_id, "+FLAGS", Net::IMAP::DELETED)
+    end
+
+    def move_message(message_id, target_folder)
+      existing = @imap.list("", target_folder)
+      if !existing || existing.empty?
+        @imap.create(target_folder)
+      end
+      @imap.copy(message_id, target_folder)
+      delete_message(message_id)
+    end
+
+  end
+
+end

lib/incoming_mail/incoming_message_processor.rb

@@ -0,0 +1,325 @@
+#
+# Copyright (C) 2011-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 'iconv'
+
+module IncomingMail
+
+  class IncomingMessageProcessor
+
+    class SilentIgnoreError < StandardError; end
+    class ReplyFromError < StandardError; end
+    class UnknownAddressError < ReplyFromError; end
+    class ReplyToLockedTopicError < ReplyFromError; end
+
+    MailboxClasses = {
+      :imap => IncomingMail::ImapMailbox,
+      :directory => IncomingMail::DirectoryMailbox,
+      :pop3 => IncomingMail::Pop3Mailbox,
+    }.freeze
+
+    class << self
+      attr_accessor :mailbox_accounts, :settings, :deprecated_settings
+    end
+
+    # See config/incoming_mail.yml.example for documentation on how to configure incoming mail
+    def self.configure(config)
+      configure_settings(config.except(*mailbox_keys))
+      configure_accounts(config.slice(*mailbox_keys))
+    end
+
+    def self.run_periodically?
+      if settings.run_periodically.nil?
+        # check backwards compatibility settings
+        deprecated_settings.poll_interval == 0 && deprecated_settings.ignore_stdin == true
+      else
+        !!settings.run_periodically
+      end
+    end
+
+    def self.process
+      self.mailbox_accounts.each do |account|
+        mailbox = self.create_mailbox(account)
+        process_mailbox(mailbox, account)
+      end
+    end
+
+    def self.process_single(incoming_message, secure_id, message_id, account = IncomingMail::MailboxAccount.new)
+      return if IncomingMessageProcessor.bounce_message?(incoming_message)
+
+      if incoming_message.multipart? && part = incoming_message.parts.find { |p| p.content_type.try(:match, %r{^text/html(;|$)}) }
+        html_body = utf8ify(part.body.decoded, part.charset)
+      end
+      html_body = utf8ify(incoming_message.body.decoded, incoming_message.charset) if !incoming_message.multipart? && incoming_message.content_type.try(:match, %r{^text/html(;|$)})
+      if incoming_message.multipart? && part = incoming_message.parts.find { |p| p.content_type.try(:match, %r{^text/plain(;|$)}) }
+        body = utf8ify(part.body.decoded, part.charset)
+      end
+      body ||= utf8ify(incoming_message.body.decoded, incoming_message.charset)
+      if !html_body
+        self.extend TextHelper
+        html_body = format_message(body).first
+      end
+
+      begin
+        original_message = Message.find_by_id(message_id)
+        # 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 == ReplyToAddress.new(original_message).secure_id
+
+        original_message.shard.activate do
+          context = original_message.context
+          user = original_message.user
+          raise IncomingMessageProcessor::UnknownAddressError unless user && context && context.respond_to?(:reply_from)
+          context.reply_from({
+            :purpose => 'general',
+            :user => user,
+            :subject => utf8ify(incoming_message.subject, incoming_message.header[:subject].try(:charset)),
+            :html => html_body,
+            :text => body
+          })
+        end
+      rescue IncomingMessageProcessor::ReplyFromError => error
+        IncomingMessageProcessor.ndr(original_message, incoming_message, error, account)
+      rescue IncomingMessageProcessor::SilentIgnoreError
+        # ignore it
+      end
+    end
+
+    private
+
+    def self.mailbox_keys
+      MailboxClasses.keys.map(&:to_s)
+    end
+
+    def self.create_mailbox(account)
+      mailbox_class = MailboxClasses.fetch(account.protocol)
+      mailbox_class.new(account.config)
+    end
+
+    def self.configure_settings(config)
+      @settings = IncomingMail::Settings.new
+      @deprecated_settings = IncomingMail::DeprecatedSettings.new
+
+      config.symbolize_keys.each do |key, value|
+        if IncomingMail::Settings.members.map(&:to_sym).include?(key)
+          self.settings.send("#{key}=", value)
+        elsif IncomingMail::DeprecatedSettings.members.map(&:to_sym).include?(key)
+          Rails.logger.warn("deprecated setting sent to IncomingMessageProcessor: #{key}")
+          self.deprecated_settings.send("#{key}=", value)
+        else 
+          raise "unrecognized setting sent to IncomingMessageProcessor: #{key}"
+        end
+      end
+    end
+
+    def self.configure_accounts(account_configs)
+      flat_account_configs = flatten_account_configs(account_configs)
+      self.mailbox_accounts = flat_account_configs.map do |mailbox_protocol, mailbox_config|
+        error_folder = mailbox_config.delete(:error_folder)
+        address = mailbox_config[:username]
+        IncomingMail::MailboxAccount.new({
+          :protocol => mailbox_protocol.to_sym,
+          :config => mailbox_config,
+          :address => address,
+          :error_folder => error_folder,
+        })
+      end
+    end
+
+    def self.flatten_account_configs(account_configs)
+      account_configs.reduce([]) do |flat_account_configs, (mailbox_protocol, mailbox_config)|
+        flat_mailbox_configs = flatten_mailbox_overrides(mailbox_config)
+        flat_mailbox_configs.each do |single_mailbox_config|
+          flat_account_configs << [mailbox_protocol, single_mailbox_config]
+        end
+
+        flat_account_configs
+      end
+    end
+
+    def self.flatten_mailbox_overrides(mailbox_config)
+      mailbox_defaults = mailbox_config.except('accounts')
+      mailbox_overrides = mailbox_config['accounts'] || [{}]
+      mailbox_overrides.map do |override_config|
+        mailbox_defaults.merge(override_config).symbolize_keys
+      end
+    end
+
+    def self.error_report_category
+      "incoming_message_processor"
+    end
+
+    def self.bounce_message?(mail)
+      mail.header.fields.any? do |field|
+        case field.name
+
+        # RFC-3834
+        when 'Auto-Submitted' then field.value != 'no'
+
+        # old klugey stuff uses this
+        when 'Precedence' then ['bulk', 'list', 'junk'].include?(field.value)
+
+        # Exchange sets this        
+        when 'X-Auto-Response-Suppress' then true 
+
+        # some other random headers I found that are easy to check
+        when 'X-Autoreply', 'X-Autorespond', 'X-Autoresponder' then true
+
+        # not a bounce header we care about
+        else false
+        end
+      end
+
+    end
+
+    def self.utf8ify(string, encoding)
+      encoding ||= 'UTF-8'
+      encoding = encoding.upcase
+      # change encoding; if it throws an exception (i.e. unrecognized encoding), just strip invalid UTF-8
+      Iconv.conv('UTF-8//TRANSLIT//IGNORE', encoding, string) rescue TextHelper.strip_invalid_utf8(string)
+    end
+
+
+    def self.process_mailbox(mailbox, account)
+      addr, domain = account.address.split(/@/)
+      error_folder = account.error_folder
+      mailbox.connect
+      mailbox.each_message do |message_id, raw_contents|
+        message = parse_message(raw_contents)
+        if message && !message.errors.present?
+          process_message(message, account)
+          mailbox.delete_message(message_id)
+        else
+          mailbox.move_message(message_id, error_folder)
+          if message
+            ErrorReport.log_error(error_report_category, {
+              :message => "Error parsing email",
+              :backtrace => message.errors.flatten.map(&:to_s).join("\n"),
+              :from => message.from.try(:first),
+              :to => message.to.to_s,
+            })
+          end
+        end
+      end
+      mailbox.disconnect
+    rescue => e
+      # any exception that makes it here probably means the connection is broken
+      # skip this account, but the rest of the accounts should still be tried
+      ErrorReport.log_exception(error_report_category, e)
+    end
+
+    def self.parse_message(raw_contents)
+        message = Mail.new(raw_contents)
+        # access some of the fields to make sure they don't raise errors when accessed
+        message.subject
+        message
+    rescue => e
+      ErrorReport.log_exception(error_report_category, e)
+      nil
+    end
+
+    def self.process_message(message, account)
+      secure_id, outgoing_message_id = find_matching_to_address(message, account)
+      # TODO: Add bounce processing and handling of other email to the default notification address.
+      return unless secure_id && outgoing_message_id
+      self.process_single(message, secure_id, outgoing_message_id, account)
+    rescue => e
+      ErrorReport.log_exception(error_report_category, e, 
+        :from => message.from.try(:first),
+        :to => message.to.to_s)
+    end
+
+    def self.find_matching_to_address(message, account)
+      addr, domain = account.address.split(/@/)
+      regex = Regexp.new("#{Regexp.escape(addr)}\\+([0-9a-f]+)-(\\d+)@#{Regexp.escape(domain)}")
+      message.to.each do |address|
+        if match = regex.match(address)
+          return [match[1], match[2].to_i]
+        end
+      end
+    end
+
+    def self.ndr(original_message, incoming_message, error, account)
+      incoming_from = incoming_message.from.try(:first)
+      incoming_subject = incoming_message.subject
+      return unless incoming_from
+
+      ndr_subject, ndr_body = IncomingMessageProcessor.ndr_strings(incoming_subject, error)
+      outgoing_message = Message.new({
+        :to => incoming_from,
+        :from => account.address,
+        :subject => ndr_subject,
+        :body => ndr_body,
+        :delay_for => 0,
+        :context => nil,
+        :path_type => 'email',
+        :from_name => "Instructure",
+      })
+
+      outgoing_message_delivered = false
+      if original_message
+        original_message.shard.activate do
+          comch = CommunicationChannel.active.find_by_path_and_path_type(incoming_from, 'email')
+          outgoing_message.communication_channel = comch
+          outgoing_message.user = comch.try(:user)
+          if outgoing_message.communication_channel && outgoing_message.user
+            outgoing_message.save
+            outgoing_message.deliver
+            outgoing_message_delivered = true
+          end
+        end
+      end
+
+      unless outgoing_message_delivered
+        # Can't use our usual mechanisms, so just try to send it once now
+        begin 
+          res = Mailer.deliver_message(outgoing_message)
+        rescue => e
+          # TODO: put some kind of error logging here?
+        end
+      end
+    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
+
+end

lib/incoming_mail/mailbox_account.rb

@@ -0,0 +1,36 @@
+#
+# 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/>.
+#
+
+module IncomingMail
+  class MailboxAccount
+    attr_accessor :protocol, :config, :address, :error_folder
+
+    def initialize(options = {})
+      self.protocol     = options[:protocol]
+      self.config       = options[:config] || {}
+      self.address      = options[:address] 
+      self.error_folder = options[:error_folder] || "errors"
+    end
+
+    def address
+      # have to delay checking HostUrl until after rails initialization
+      @address ||= HostUrl.outgoing_email_address
+    end
+
+  end
+end

lib/incoming_mail/pop3_mailbox.rb

@@ -0,0 +1,61 @@
+#
+# 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 'net/pop'
+
+module IncomingMail
+
+  class Pop3Mailbox
+    attr_accessor :server, :port, :ssl, :username, :password
+
+    def initialize(options = {})
+      @server   = options.fetch(:server, "")
+      @port     = options.fetch(:port, 995)
+      @ssl      = options.fetch(:ssl, true)
+      @username = options.fetch(:username, "")
+      @password = options.fetch(:password, "")
+    end
+
+    def connect
+      @pop = Net::POP3.new(server, port)
+      @pop.enable_ssl(OpenSSL::SSL::VERIFY_NONE) if ssl
+      @pop.start(username, password)
+    end
+
+    def disconnect
+      @pop.finish
+    rescue
+    end
+
+    def each_message
+      @pop.each_mail do |message|
+        yield message, message.pop      
+      end
+    end
+
+    def delete_message(pop_message)
+      pop_message.delete
+    end
+
+    def move_message(pop_message, target_folder)
+      # pop can't do this -- just delete the message
+      pop_message.delete
+    end
+  end
+
+end

lib/incoming_mail/settings.rb

@@ -0,0 +1,21 @@
+#
+# 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/>.
+#
+
+module IncomingMail
+  Settings = Struct.new(:run_periodically)
+end

lib/incoming_message_processor.rb

@@ -1,232 +0,0 @@
-#
-# Copyright (C) 2011 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 'iconv'
-
-class IncomingMessageProcessor
-  
-  class SilentIgnoreError < StandardError; end
-  class ReplyFromError < StandardError; end
-  class UnknownAddressError < ReplyFromError; end
-  class ReplyToLockedTopicError < ReplyFromError; end
-  
-  class << self
-    attr_accessor :mailman_method, :mailman_accounts
-  end
-
-  # See config/incoming_mail.yml.example for documentation on how to configure incoming mail
-  def self.configure(config)
-    configure_mailman(config.except(*account_keys))
-    configure_accounts(config.slice(*account_keys))
-  end
-
-  def self.account_keys
-    %w(imap pop3)
-  end
-
-  def self.configure_mailman(mailman_config)
-    mailman_config.each do |key, value|
-      Mailman.config.send(key + '=', value)
-    end
-    # yes, this is lame, but setting this to real nil makes mailman assume '.',
-    # which then reloads the rails configuration (and gets an error because we
-    # try to remove a method that's already there)
-    Mailman.config.rails_root = 'nil'
-    Mailman.config.logger = Rails.logger
-  end
-
-  def self.configure_accounts(account_config)
-    raise "Only one of [#{account_keys.join(', ')}] can be specified in incoming_mail" if account_config.size > 1
-    self.mailman_method, account_defaults = account_config.first || [nil, {}]
-    accounts = account_defaults['accounts'] || [{}]
-    account_defaults = account_defaults.except('accounts')
-
-    raise "poll_interval must be 0 if multiple accounts are specified" if accounts.size > 1 && Mailman.config.poll_interval != 0
-
-    self.mailman_accounts = accounts.map { |account| account_defaults.merge(account) }.map(&:symbolize_keys)
-  end
-
-
-  def self.bounce_message?(mail)
-    mail.header.fields.any? do |field|
-      case field.name
-
-      # RFC-3834
-      when 'Auto-Submitted' then field.value != 'no'
-
-      # old klugey stuff uses this
-      when 'Precedence' then ['bulk', 'list', 'junk'].include?(field.value)
-
-      # Exchange sets this        
-      when 'X-Auto-Response-Suppress' then true 
-
-      # some other random headers I found that are easy to check
-      when 'X-Autoreply', 'X-Autorespond', 'X-Autoresponder' then true
-
-      # not a bounce header we care about
-      else false
-      end
-    end
-
-  end
-
-  def self.utf8ify(string, encoding)
-    encoding ||= 'UTF-8'
-    encoding = encoding.upcase
-    # change encoding; if it throws an exception (i.e. unrecognized encoding), just strip invalid UTF-8
-    Iconv.conv('UTF-8//TRANSLIT//IGNORE', encoding, string) rescue TextHelper.strip_invalid_utf8(string)
-  end
-
-  def self.process_single(incoming_message, secure_id, message_id, inbox_address = default_inbox_address)
-    return if IncomingMessageProcessor.bounce_message?(incoming_message)
-
-    if incoming_message.multipart? && part = incoming_message.parts.find { |p| p.content_type.try(:match, %r{^text/html(;|$)}) }
-      html_body = utf8ify(part.body.decoded, part.charset)
-    end
-    html_body = utf8ify(incoming_message.body.decoded, incoming_message.charset) if !incoming_message.multipart? && incoming_message.content_type.try(:match, %r{^text/html(;|$)})
-    if incoming_message.multipart? && part = incoming_message.parts.find { |p| p.content_type.try(:match, %r{^text/plain(;|$)}) }
-      body = utf8ify(part.body.decoded, part.charset)
-    end
-    body ||= utf8ify(incoming_message.body.decoded, incoming_message.charset)
-    if !html_body
-      self.extend TextHelper
-      html_body = format_message(body).first
-    end
-
-    begin
-      original_message = Message.find_by_id(message_id)
-      # 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 == ReplyToAddress.new(original_message).secure_id
-
-      original_message.shard.activate do
-        context = original_message.context
-        user = original_message.user
-        raise IncomingMessageProcessor::UnknownAddressError unless user && context && context.respond_to?(:reply_from)
-        context.reply_from({
-          :purpose => 'general',
-          :user => user,
-          :subject => utf8ify(incoming_message.subject, incoming_message.header[:subject].try(:charset)),
-          :html => html_body,
-          :text => body
-        })
-      end
-    rescue IncomingMessageProcessor::ReplyFromError => error
-      IncomingMessageProcessor.ndr(original_message, incoming_message, error, inbox_address)
-    rescue IncomingMessageProcessor::SilentIgnoreError
-      # ignore it
-    end
-  end
-
-  def self.process
-    mailman_accounts.each do |account|
-      Mailman.config.send(mailman_method + '=', account) if mailman_method
-      inbox_address = account[:username] || default_inbox_address
-      process_account(inbox_address)
-    end
-  end
-
-  def self.default_inbox_address
-    HostUrl.outgoing_email_address
-  end
-
-  def self.process_account(inbox_address)
-    addr, domain = inbox_address.split(/@/)
-    regex = Regexp.new("#{Regexp.escape(addr)}\\+([0-9a-f]+)-(\\d+)@#{Regexp.escape(domain)}")
-    Mailman::Application.run do
-      to regex do
-        begin
-          IncomingMessageProcessor.process_single(message, params['captures'][0], params['captures'][1].to_i, inbox_address)
-        rescue => e
-          ErrorReport.log_exception(:default, e, :from => message.from.try(:first),
-                                                 :to => message.to.to_s)
-        end
-      end
-      default do
-        # TODO: Add bounce processing and handling of other email to the default notification address.
-      end
-    end
-  end
-
-  def self.ndr(original_message, incoming_message, error, inbox_address)
-    incoming_from = incoming_message.from.try(:first)
-    incoming_subject = incoming_message.subject
-    return unless incoming_from
-
-    ndr_subject, ndr_body = IncomingMessageProcessor.ndr_strings(incoming_subject, error)
-    outgoing_message = Message.new({
-      :to => incoming_from,
-      :from => inbox_address,
-      :subject => ndr_subject,
-      :body => ndr_body,
-      :delay_for => 0,
-      :context => nil,
-      :path_type => 'email',
-      :from_name => "Instructure",
-    })
-
-    outgoing_message_delivered = false
-    if original_message
-      original_message.shard.activate do
-        comch = CommunicationChannel.active.find_by_path_and_path_type(incoming_from, 'email')
-        outgoing_message.communication_channel = comch
-        outgoing_message.user = comch.try(:user)
-        if outgoing_message.communication_channel && outgoing_message.user
-          outgoing_message.save
-          outgoing_message.deliver
-          outgoing_message_delivered = true
-        end
-      end
-    end
-
-    unless outgoing_message_delivered
-      # Can't use our usual mechanisms, so just try to send it once now
-      begin 
-        res = Mailer.deliver_message(outgoing_message)
-      rescue => e
-        # TODO: put some kind of error logging here?
-      end
-    end
-  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

script/process_incoming_emails

@@ -1,6 +1,6 @@
 #!/usr/bin/env ruby
 
 require File.expand_path(File.join(File.dirname(__FILE__), '..', 'config', 'environment'))
-require 'incoming_message_processor'
+require 'incoming_mail/incoming_message_processor'
 
-IncomingMessageProcessor::process
+IncomingMail::IncomingMessageProcessor.process

spec/controllers/messages_controller_spec.rb

@@ -33,7 +33,7 @@ describe MessagesController do
 
       it "should be able to send messages" do
         secure_id, message_id = ['secure_id', 42]
-        IncomingMessageProcessor.expects(:process_single).with(anything, secure_id, message_id)
+        IncomingMail::IncomingMessageProcessor.expects(:process_single).with(anything, secure_id, message_id)
         post 'create', :secure_id  => secure_id,
                        :message_id => message_id,
                        :subject    => 'subject',

spec/lib/incoming_mail/directory_mailbox_spec.rb

@@ -0,0 +1,106 @@
+#
+# 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 '../../../../lib/incoming_mail/directory_mailbox', __FILE__
+require File.expand_path '../../../mocha_rspec_adapter', __FILE__
+require File.expand_path '../mailbox_spec_helper', __FILE__
+
+describe IncomingMail::DirectoryMailbox do
+  it_should_behave_like 'Mailbox'
+
+  def default_config
+    {
+      :folder => "/tmp/directory_mailbox",
+    }
+  end
+
+  before do
+    @mailbox = IncomingMail::DirectoryMailbox.new(default_config)
+  end
+
+  it "should connect if folder exists" do
+    @mailbox.expects(:folder_exists?).with(default_config[:folder]).returns(true)
+    expect { @mailbox.connect}.to_not raise_error
+  end
+
+  it "should raise on connect if folder does not exist" do
+    @mailbox.expects(:folder_exists?).with(default_config[:folder]).returns(false)
+    expect { @mailbox.connect }.to raise_error
+  end
+
+  it "should iterate through and yield files in a directory" do
+    folder = default_config[:folder]
+    folder_entries = %w(. .. foo bar baz)
+    @mailbox.expects(:files_in_folder).with(folder).returns(folder_entries)
+    folder_entries.each do |entry|
+      @mailbox.expects(:file?).with(folder, entry).returns(!entry.include?('.'))
+    end
+
+    @mailbox.expects(:read_file).with(folder, "foo").returns("foo body")
+    @mailbox.expects(:read_file).with(folder, "bar").returns("bar body")
+    @mailbox.expects(:read_file).with(folder, "baz").returns("baz body")
+
+    yielded_values = []
+    @mailbox.each_message do |*values|
+      yielded_values << values
+    end
+    yielded_values.should eql [["foo", "foo body"], ["bar", "bar body"], ["baz", "baz body"], ]
+  end
+
+  context "with simple foo file" do
+
+    before do
+      @mailbox.expects({
+        :file? => true,
+        :read_file => "foo body",
+        :files_in_folder => ["foo"],
+      })
+      @mailbox.expects(:folder_exists?).with(default_config[:folder]).returns(true)
+      @mailbox.connect
+    end
+
+    it "should delete files" do
+      @mailbox.expects(:delete_file).with(default_config[:folder], "foo")
+      @mailbox.each_message do |id, body|
+        @mailbox.delete_message(id)
+      end
+    end
+
+    it "should move files" do
+      folder = default_config[:folder]
+      @mailbox.expects(:move_file).with(folder, "foo", "aside")
+      @mailbox.expects(:folder_exists?).with(folder, "aside").returns(true)
+      @mailbox.expects(:create_folder).never
+      @mailbox.each_message do |id, body|
+        @mailbox.move_message(id, "aside")
+      end
+    end
+
+    it "should create target folder when moving file if target folder doesn't exist" do
+      folder = default_config[:folder]
+      @mailbox.expects(:move_file).with(folder, "foo", "aside")
+      @mailbox.expects(:folder_exists?).with(folder, "aside").returns(false)
+      @mailbox.expects(:create_folder).with(default_config[:folder], "aside")
+      @mailbox.each_message do |id, body|
+        @mailbox.move_message(id, "aside")
+      end
+    end
+
+  end
+end
+

spec/lib/incoming_mail/imap_mailbox_spec.rb

@@ -0,0 +1,169 @@
+#
+# 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('../../../../lib/incoming_mail/imap_mailbox', __FILE__)
+require File.expand_path('../../../mocha_rspec_adapter', __FILE__)
+require File.expand_path('../mailbox_spec_helper', __FILE__)
+
+describe IncomingMail::ImapMailbox do
+  it_should_behave_like 'Mailbox'
+
+  def default_config
+    {
+      :server => "mail.example.com",
+      :username => "user",
+      :password => "password",
+    }
+  end
+
+  def mock_net_imap
+    @imap_mock = stub_everything('Net::IMAP instance')
+
+    Net::IMAP.expects(:new).
+      with("mail.example.com", {:port => 993, :ssl => true}).
+      times(0..1). # allow simple tests to not call #connect
+      returns(@imap_mock)
+  end
+
+  before do
+    mock_net_imap
+    @mailbox = IncomingMail::ImapMailbox.new(default_config)
+  end
+
+  describe "#initialize" do
+    it "should accept existing mailman imap configuration" do
+      @mailbox = IncomingMail::ImapMailbox.new({
+        :server => "imap.server.com",
+        :port => 1234,
+        :ssl => "truthy-value",
+        :filter => ["ALL"],
+        :username => "user@server.com",
+        :password => "secret-user-password",
+      })
+
+      @mailbox.server.should eql "imap.server.com"
+      @mailbox.port.should eql 1234
+      @mailbox.ssl.should eql "truthy-value"
+      @mailbox.filter.should eql ["ALL"]
+      @mailbox.username.should eql "user@server.com"
+      @mailbox.password.should eql "secret-user-password"
+    end
+
+    it "should accept non-array filter" do
+      @mailbox = IncomingMail::ImapMailbox.new(:filter => "BLAH")
+      @mailbox.filter.should eql ["BLAH"]
+    end
+
+    it "should accept folder parameter" do
+      # this isn't necessary for gmail, but just in case
+      @mailbox = IncomingMail::ImapMailbox.new(:folder => "inbox")
+      @mailbox.folder.should eql "inbox"
+    end
+
+  end
+
+  it "should connect to the server" do
+    @imap_mock.expects(:login).with("user", "password").once
+    @mailbox.connect
+  end
+
+  context "connected" do
+    before do
+      @mailbox.connect
+    end
+
+    def mock_fetch_response(body)
+      result = mock()
+      result.expects(:attr).returns({"RFC822" => body})
+      [result]
+    end
+
+    it "should retrieve and yield messages" do
+      @mailbox.folder = "message_folder"
+      @imap_mock.expects(:select).with("message_folder").once
+      @imap_mock.expects(:search).with(["ALL"]).once.returns([1, 2, 3])
+
+      fetch = sequence('fetch')
+      @imap_mock.expects(:fetch).in_sequence(fetch).with(1, "RFC822").returns(mock_fetch_response("foo"))
+      @imap_mock.expects(:fetch).in_sequence(fetch).with(2, "RFC822").returns(mock_fetch_response("bar"))
+      @imap_mock.expects(:fetch).in_sequence(fetch).with(3, "RFC822").returns(mock_fetch_response("baz"))
+      @imap_mock.expects(:expunge).with().once
+
+      yielded_values = []
+      @mailbox.each_message do |message_id, message_body|
+        yielded_values << [message_id, message_body]
+      end
+      yielded_values.should eql [[1, "foo"], [2, "bar"], [3, "baz"]]
+    end
+
+    it "should delete a retrieved message" do
+      @imap_mock.expects(:search).returns([42])
+      @imap_mock.expects(:fetch).returns(mock_fetch_response("body"))
+      @imap_mock.expects(:store).with(42, "+FLAGS", Net::IMAP::DELETED)
+      @mailbox.each_message do |id, body|
+        @mailbox.delete_message(id)
+      end
+    end
+
+    it "should move a retrieved message" do
+      @imap_mock.expects(:search).returns([42])
+      @imap_mock.expects(:fetch).returns(mock_fetch_response("body"))
+      @imap_mock.expects(:list).returns([stub_everything])
+      @imap_mock.expects(:copy).with(42, "other_folder")
+      @imap_mock.expects(:store).with(42, "+FLAGS", Net::IMAP::DELETED)
+      @mailbox.each_message do |id, body|
+        @mailbox.move_message(id, "other_folder")
+      end
+    end
+
+    it "should create the folder if necessary when moving a message (imap list returns empty)" do
+      @imap_mock.expects(:search).returns([42])
+      @imap_mock.expects(:fetch).returns(mock_fetch_response("body"))
+      @imap_mock.expects(:list).with("", "other_folder").returns([])
+      @imap_mock.expects(:create).with("other_folder")
+      @imap_mock.expects(:copy).with(42, "other_folder")
+      @imap_mock.expects(:store).with(42, "+FLAGS", Net::IMAP::DELETED)
+      @mailbox.each_message do |id, body|
+        @mailbox.move_message(id, "other_folder")
+      end
+    end
+
+    it "should create the folder if necessary when moving a message (imap list returns nil)" do
+      @imap_mock.expects(:search).returns([42])
+      @imap_mock.expects(:fetch).returns(mock_fetch_response("body"))
+      @imap_mock.expects(:list).with("", "other_folder").returns(nil)
+      @imap_mock.expects(:create).with("other_folder")
+      @imap_mock.expects(:copy).with(42, "other_folder")
+      @imap_mock.expects(:store).with(42, "+FLAGS", Net::IMAP::DELETED)
+      @mailbox.each_message do |id, body|
+        @mailbox.move_message(id, "other_folder")
+      end
+    end
+      
+  end
+
+  describe "#disconnect" do
+    it "should disconnect" do
+      @mailbox.connect
+      @imap_mock.expects(:logout)
+      @imap_mock.expects(:disconnect)
+      @mailbox.disconnect
+    end
+  end
+
+end

spec/lib/incoming_mail/incoming_message_processor_spec.rb

@@ -16,9 +16,13 @@
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 #
 
-require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
+require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
+
+describe IncomingMail::IncomingMessageProcessor do
+
+  # Import this one constant
+  IncomingMessageProcessor = IncomingMail::IncomingMessageProcessor
 
-describe IncomingMessageProcessor do
   def setup_test_outgoing_mail
     @original_delivery_method = ActionMailer::Base.delivery_method
     @original_perform_deliveries = ActionMailer::Base.perform_deliveries
@@ -88,7 +92,38 @@ describe IncomingMessageProcessor do
     DiscussionTopic.class_eval { alias_method :reply_from, :old_reply_from }
   end
 
-  describe "IncomingMessageProcessor.process_single" do
+  describe ".configure" do
+    it "should raise on invalid configuration settings" do
+      expect { IncomingMessageProcessor.configure('bogus_setting' => 42) }.to raise_error(StandardError)
+    end
+
+    it "should accept legacy mailman configurations" do
+      IncomingMessageProcessor.configure('poll_interval' => 42, 'ignore_stdin' => true)
+    end
+  end
+
+  describe ".run_periodically?" do
+    it "should consult .poll_interval and .ignore_stdin for backwards compatibility" do
+      IncomingMessageProcessor.configure('poll_interval' => 0, 'ignore_stdin' => true)
+      IncomingMessageProcessor.run_periodically?.should be_true
+
+      IncomingMessageProcessor.configure('poll_interval' => 0, 'ignore_stdin' => false)
+      IncomingMessageProcessor.run_periodically?.should be_false
+
+      IncomingMessageProcessor.configure('poll_interval' => 42, 'ignore_stdin' => true)
+      IncomingMessageProcessor.run_periodically?.should be_false
+    end
+
+    it "should use 'run_periodically' configuration setting" do
+      IncomingMessageProcessor.configure({})
+      IncomingMessageProcessor.run_periodically?.should be_false
+
+      IncomingMessageProcessor.configure('run_periodically' => true)
+      IncomingMessageProcessor.run_periodically?.should be_true
+    end
+  end
+
+  describe ".process_single" do
     before(:each) do
       IncomingMessageProcessor.configure({})
 
@@ -213,45 +248,12 @@ describe IncomingMessageProcessor do
       check_new_message(:locked)
     end
 
-    it "should process emails from mailman" do
-      Dir.mktmpdir do |tmpdir|
-        newdir = tmpdir + "/new"
-        Dir.mkdir(newdir)
-        
-        addr, domain = HostUrl.outgoing_email_address.split(/@/)
-        to_address = "#{addr}+#{ReplyToAddress.new(@message).secure_id}-#{@message.id}@#{domain}"
-        mail = Mail.new do
-          from 'test@example.com'
-          to to_address
-          subject 'subject of test message'
-          body 'body of test message'
-        end
-        
-        Mailman.config.maildir = nil
-        Mailman.config.ignore_stdin = false
-        Mailman.config.poll_interval = 0
-        Mailman.config.pop3 = nil
-        
-        # If we try to use maildir with mailman, it will just poll the maildir forever.
-        # Using stdin is the safest, but we have to do this little dance for it to work.
-        read, write = IO.pipe
-        saved_stdin = STDIN.dup
-        write.puts mail.to_s
-        write.close
-        STDIN.reopen(read)
-        IncomingMessageProcessor.process
-        STDIN.reopen(saved_stdin)
-        
-        DiscussionTopic.incoming_replies.length.should == 1
-        DiscussionTopic.incoming_replies[0][:text].should == 'body of test message'
-      end
-    end
   end
 
-  describe "IncomingMessageProcessor.process" do
+  describe ".process" do
     before(:each) do
-      @configured = OpenObject.new
+      @mock_mailbox = mock
-      Mailman.stubs(:config).returns(@configured)
+      IncomingMessageProcessor.stubs(:create_mailbox => @mock_mailbox)
     end
 
     it "should support original incoming_mail configuration format for a single inbox" do
@@ -268,15 +270,16 @@ describe IncomingMessageProcessor do
         }
       }
 
-      Mailman::Application.expects(:run).once
+      IncomingMessageProcessor.expects(:create_mailbox).returns(@mock_mailbox).with do |account|
+        account.protocol == :imap &&
+        account.config == config['imap'].symbolize_keys
+      end
       IncomingMessageProcessor.configure(config)
-      @configured.poll_interval.should eql 42
+
-      @configured.ignore_stdin.should be_true
+      @mock_mailbox.expects(:connect)
-      @configured.rails_root.should eql 'nil'
+      @mock_mailbox.expects(:each_message)
-      @configured.logger.should eql Rails.logger
+      @mock_mailbox.expects(:disconnect)
-      @configured.imap.should be_nil
       IncomingMessageProcessor.process
-      @configured.imap.should eql config['imap'].symbolize_keys
     end
 
     it "should process incoming_mail configuration with multiple accounts" do
@@ -294,63 +297,154 @@ describe IncomingMessageProcessor do
         },
       }
 
-      Mailman::Application.expects(:run).times(3)
+      seq = sequence('create_mailbox')
-      @configured.expects(:imap=).with({:server => 'fake', :username => 'user1@fake.fake', :password => 'pass1'})
+      imp = IncomingMessageProcessor
-      @configured.expects(:imap=).with({:server => 'fake', :username => 'user2@fake.fake', :password => 'pass2'})
+      imp.expects(:create_mailbox).in_sequence(seq).returns(@mock_mailbox).with do |account| 
-      @configured.expects(:imap=).with({:server => 'fake', :username => 'user3@fake.fake', :password => 'pass3'})
+        account.protocol == :imap &&
+        account.config == { :server => 'fake', :username => 'user1@fake.fake', :password => 'pass1'}
+      end
+      imp.expects(:create_mailbox).in_sequence(seq).returns(@mock_mailbox).with do |account|
+        account.protocol == :imap &&
+        account.config == { :server => 'fake', :username => 'user2@fake.fake', :password => 'pass2'}
+      end
+      imp.expects(:create_mailbox).in_sequence(seq).returns(@mock_mailbox).with do |account|
+        account.protocol == :imap &&
+        account.config == { :server => 'fake', :username => 'user3@fake.fake', :password => 'pass3'}
+      end
+
+      @mock_mailbox.expects(:connect).times(3)
+      @mock_mailbox.expects(:each_message).times(3)
+      @mock_mailbox.expects(:disconnect).times(3)
 
       IncomingMessageProcessor.configure(config)
       IncomingMessageProcessor.process
     end
 
-    it "should raise if both imap and pop3 are specified" do
+    it "should extract special values from account settings" do
       config = {
-        'poll_interval' => 0,
-        'ignore_stdin' => true,
         'imap' => {
           'server' => 'fake',
-        },
+          'username' => 'user@fake.fake',
-        'pop3' => {
+          'password' => 'fake',
-          'server' => 'fake',
+          'error_folder' => 'broken',
         },
       }
-      lambda { IncomingMessageProcessor.configure(config) }.should raise_error
+
+      IncomingMail::MailboxAccount.expects(:new).with({
+        :protocol => :imap,
+        :address => 'user@fake.fake',
+        :error_folder => 'broken',
+        :config => config['imap'].except('error_folder').symbolize_keys,
+      })
+      IncomingMessageProcessor.configure(config)
     end
 
-    it "should raise if multiple accounts are specified and poll_interval is not 0" do
+    describe "message processing" do
+      before do
+        IncomingMessageProcessor.configure({
+          'imap' => {
+            'username' => 'me@fake.fake',
+            'error_folder' => 'errors_go_here',
+          }
+        })
+      end
+
+      it "should perform normal message processing of messages retrieved from mailbox" do
+        foo = "To: me+123-1@fake.fake\r\n\r\nfoo body"
+        bar = "To: me+456-2@fake.fake\r\n\r\nbar body"
+        baz = "To: me+abc-3@fake.fake\r\n\r\nbaz body"
+
+        @mock_mailbox.expects(:connect)
+        @mock_mailbox.expects(:move_message).never
+        @mock_mailbox.expects(:delete_message).with(:foo)
+        @mock_mailbox.expects(:delete_message).with(:bar)
+        @mock_mailbox.expects(:delete_message).with(:baz)
+        @mock_mailbox.expects(:each_message).multiple_yields([:foo, foo], [:bar, bar], [:baz, baz])
+        @mock_mailbox.expects(:disconnect)
+        imp = IncomingMessageProcessor
+        imp.expects(:process_single).with(kind_of(Mail::Message), "123", 1, anything)
+        imp.expects(:process_single).with(kind_of(Mail::Message), "456", 2, anything)
+        imp.expects(:process_single).with(kind_of(Mail::Message), "abc", 3, anything)
+
+        IncomingMessageProcessor.process
+      end
+
+      it "should move aside messages that have parsing errors" do
+        foo = "To: me+123-1@fake.f\n ake\r\n\r\nfoo body" # illegal folding of "to" header
+
+        @mock_mailbox.expects(:connect)
+        @mock_mailbox.expects(:delete_message).never
+        @mock_mailbox.expects(:move_message).with(:foo, 'errors_go_here')
+        @mock_mailbox.expects(:each_message).yields(:foo, foo)
+        @mock_mailbox.expects(:disconnect)
+        ErrorReport.expects(:log_error).
+          with(IncomingMessageProcessor.error_report_category, kind_of(Hash))
+
+        IncomingMessageProcessor.process
+      end
+
+      it "should move aside messages that raise errors" do
+        foo = "To: me+123-1@fake.fake\r\n\r\nfoo body"
+
+        Mail.stubs(:new).raises(StandardError)
+
+        @mock_mailbox.expects(:connect)
+        @mock_mailbox.expects(:delete_message).never
+        @mock_mailbox.expects(:move_message).with(:foo, 'errors_go_here')
+        @mock_mailbox.expects(:each_message).yields(:foo, foo)
+        @mock_mailbox.expects(:disconnect)
+        ErrorReport.expects(:log_exception).
+          with(IncomingMessageProcessor.error_report_category, kind_of(StandardError), anything)
+
+        IncomingMessageProcessor.process
+      end
+
+      it "should abort account processing on exception, but continue processing other accounts" do
+        IncomingMessageProcessor.configure({
+          'imap' => {
+            'error_folder' => 'errors_go_here',
+            'accounts' => [
+              {'username' => 'first'},
+              {'username' => 'second'},
+            ]
+          }
+        })
+
+        seq = sequence('connect')
+        @mock_mailbox.expects(:connect).in_sequence(seq).raises(StandardError)
+        @mock_mailbox.expects(:connect).in_sequence(seq)
+        @mock_mailbox.expects(:disconnect).in_sequence(seq)
+        @mock_mailbox.expects(:each_message).once
+        ErrorReport.expects(:log_exception).
+          with(IncomingMessageProcessor.error_report_category, kind_of(StandardError), anything)
+
+        IncomingMessageProcessor.process
+      end
+    end
+
+    it "should accept multiple account types with overrides" do
       config = {
-        'poll_interval' => 42,
-        'ignore_stdin' => true,
         'imap' => {
-          'server' => 'fake',
+          'server' => 'fake', 
           'accounts' => [
-            { 'username' => 'user1@fake.fake', 'password' => 'pass1' },
+            {'username' => 'foo'},
-            { 'username' => 'user2@fake.fake', 'password' => 'pass2' },
+            {'username' => 'bar'},
-          ],
+          ]
         },
-      }
-      lambda { IncomingMessageProcessor.configure(config) }.should raise_error
-    end
 
-    it "should succeed if poll_interval is non-0 and only one account is specified" do
+        'directory' => {'folder' => '/tmp/mail'},
-      config = {
-        'poll_interval' => 42,
-        'ignore_stdin' => true,
-        'imap' => {
-          'server' => 'fake',
-          'username' => 'user',
-          'password' => 'pass',
-        },
       }
-      lambda { IncomingMessageProcessor.configure(config) }.should_not raise_error
+      IncomingMessageProcessor.configure(config)
-    end
+      accounts = IncomingMessageProcessor.mailbox_accounts
-
+      accounts.size.should eql 3
-    it "should succeed if poll_interval is non-0 and imap and pop3 are not specified" do
+      protocols = accounts.map(&:protocol)
-      config = {
+      protocols.count.should eql 3
-        'poll_interval' => 42,
+      protocols.count(:imap).should eql 2
-        'ignore_stdin' => false,
+      protocols.count(:directory).should eql 1
-      }
+      usernames = accounts.map(&:config).map{ |c| c[:username] }
-      lambda { IncomingMessageProcessor.configure(config) }.should_not raise_error
+      usernames.count('foo').should eql 1
+      usernames.count('bar').should eql 1
+      usernames.count(nil).should eql 1
     end
 
   end

spec/lib/incoming_mail/mailbox_spec_helper.rb

@@ -0,0 +1,28 @@
+#
+# 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/>.
+#
+
+shared_examples_for 'Mailbox' do
+
+  describe "Mailbox interface" do
+    it { should respond_to :connect }
+    it { should respond_to :each_message }
+    it { should respond_to :delete_message }
+    it { should respond_to :move_message }
+    it { should respond_to :disconnect }
+  end
+end

spec/lib/incoming_mail/pop3_mailbox_spec.rb

@@ -0,0 +1,148 @@
+#
+# 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 '../../../../lib/incoming_mail/pop3_mailbox', __FILE__
+require File.expand_path '../../../mocha_rspec_adapter', __FILE__
+require File.expand_path '../mailbox_spec_helper', __FILE__
+
+describe IncomingMail::Pop3Mailbox do
+  it_should_behave_like 'Mailbox'
+
+  def default_config
+    {
+      :server => "mail.example.com",
+      :ssl => false,
+      :port => 2345,
+      :username => "user",
+      :password => "password",
+    }
+  end
+
+  def mock_net_pop
+    @pop_mock = mock
+    Net::POP3.stubs(:new).returns(@pop_mock)
+    @pop_mock.stubs(:start)
+  end
+
+  describe "#initialize" do
+    it "should accept existing mailman pop3 configuration" do
+      @mailbox = IncomingMail::Pop3Mailbox.new({
+        :server => "pop3.server.com",
+        :port => 1234,
+        :ssl => "truthy-value",
+        :username => "user@server.com",
+        :password => "secret-user-password",
+      })
+
+      @mailbox.server.should eql "pop3.server.com"
+      @mailbox.port.should eql 1234
+      @mailbox.ssl.should eql "truthy-value"
+      @mailbox.username.should eql "user@server.com"
+      @mailbox.password.should eql "secret-user-password"
+    end
+  end
+
+  describe "#connect" do
+    before do
+      @pop_mock = mock
+    end
+
+    it "should connect to the server" do
+      config = default_config.merge(:ssl => false, :port => 110)
+      Net::POP3.expects(:new).with(config[:server], config[:port]).returns(@pop_mock)
+      @pop_mock.expects(:start).with(config[:username], config[:password])
+
+      @mailbox = IncomingMail::Pop3Mailbox.new(config)
+      @mailbox.connect
+    end
+
+    it "should use ssl if configured" do
+      config = default_config.merge(:ssl => true, :port => 995)
+
+      Net::POP3.expects(:new).with(config[:server], config[:port]).returns(@pop_mock)
+      @pop_mock.expects(:enable_ssl).with(OpenSSL::SSL::VERIFY_NONE)
+      @pop_mock.expects(:start).with(config[:username], config[:password])
+
+      @mailbox = IncomingMail::Pop3Mailbox.new(config)
+      @mailbox.connect
+    end
+  end
+
+  describe "#disconnect" do
+    it "should disconnect" do
+      mock_net_pop
+      @pop_mock.expects(:finish)
+
+      @mailbox = IncomingMail::Pop3Mailbox.new(default_config)
+      @mailbox.connect
+      @mailbox.disconnect
+    end
+  end
+
+  describe "#each_message" do
+    before do
+      mock_net_pop
+      @mailbox = IncomingMail::Pop3Mailbox.new(default_config)
+      @mailbox.connect
+    end
+
+    def mock_pop_mail(body)
+      result = mock(:pop => body)
+    end
+
+    it "should retrieve and yield messages" do
+      foo = mock_pop_mail("foo body")
+      bar = mock_pop_mail("bar body")
+      baz = mock_pop_mail("baz body")
+      @pop_mock.expects(:each_mail).multiple_yields(foo, bar, baz)
+
+      yielded_values = []
+      @mailbox.each_message do |message_id, body|
+        yielded_values << [message_id, body]
+      end
+
+      yielded_values.should eql [[foo, "foo body"], [bar, "bar body"], [baz, "baz body"]]
+    end
+
+    context "with simple foo message" do
+      before do
+        @foo = mock_pop_mail("foo body")
+        @pop_mock.expects(:each_mail).yields(@foo)
+      end
+
+      it "should delete when asked" do
+        @foo.expects(:delete)
+        @mailbox.each_message do |message_id, body|
+          @mailbox.delete_message(message_id)
+        end
+      end
+
+      it "should delete when asked to move" do
+        @foo.expects(:delete)
+        @mailbox.each_message do |message_id, body|
+          @mailbox.move_message(message_id, "anything")
+        end
+      end
+
+    end
+
+  end
+
+
+
+end

spec/mocha_rspec_adapter.rb

@@ -0,0 +1,18 @@
+require 'mocha/api'
+
+module MochaRspecAdapter
+  include Mocha::API
+  def setup_mocks_for_rspec
+    mocha_setup
+  end
+  def verify_mocks_for_rspec
+    mocha_verify
+  end
+  def teardown_mocks_for_rspec
+    mocha_teardown
+  end
+end
+
+Spec::Runner.configure do |config|
+  config.mock_with MochaRspecAdapter
+end

spec/models/conversation_message_spec.rb

@@ -269,7 +269,7 @@ describe ConversationMessage do
         :subject => "an email reply",
         :html => "body",
         :text => "body"
-      }) }.should raise_error(IncomingMessageProcessor::UnknownAddressError)
+      }) }.should raise_error(IncomingMail::IncomingMessageProcessor::UnknownAddressError)
     end
   end
 end

spec/models/discussion_entry_spec.rb

@@ -390,7 +390,7 @@ describe DiscussionEntry do
       root = @topic.reply_from(:user => @teacher, :text => "root entry")
       Account.default.destroy
       root.reload
-      lambda { root.reply_from(:user => @teacher, :text => "sub entry") }.should raise_error(IncomingMessageProcessor::UnknownAddressError)
+      lambda { root.reply_from(:user => @teacher, :text => "sub entry") }.should raise_error(IncomingMail::IncomingMessageProcessor::UnknownAddressError)
     end
 
     it "should prefer html to text" do
@@ -407,7 +407,7 @@ describe DiscussionEntry do
       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)
+      lambda { @entry.reply_from(:user => @teacher, :text => "reply") }.should raise_error(IncomingMail::IncomingMessageProcessor::ReplyToLockedTopicError)
     end
   end
 end

spec/models/discussion_topic_spec.rb

@@ -820,7 +820,7 @@ describe DiscussionTopic do
       @context = @course
       discussion_topic_model(:user => @teacher)
       account.destroy
-      lambda { @topic.reply_from(:user => @teacher, :text => "entry") }.should raise_error(IncomingMessageProcessor::UnknownAddressError)
+      lambda { @topic.reply_from(:user => @teacher, :text => "entry") }.should raise_error(IncomingMail::IncomingMessageProcessor::UnknownAddressError)
     end
 
     it "should prefer html to text" do
@@ -835,7 +835,7 @@ describe DiscussionTopic do
       course_with_teacher
       discussion_topic_model
       @topic.lock!
-      lambda { @topic.reply_from(:user => @teacher, :text => "reply") }.should raise_error(IncomingMessageProcessor::ReplyToLockedTopicError)
+      lambda { @topic.reply_from(:user => @teacher, :text => "reply") }.should raise_error(IncomingMail::IncomingMessageProcessor::ReplyToLockedTopicError)
     end
 
   end

spec/models/submission_comment_spec.rb

@@ -659,7 +659,7 @@ This text has a http://www.google.com link in it...
       comment.reload
       lambda { 
         comment.reply_from(:user => @student, :text => "some reply") 
-      }.should raise_error(IncomingMessageProcessor::UnknownAddressError)
+      }.should raise_error(IncomingMail::IncomingMessageProcessor::UnknownAddressError)
     end
   end
 

spec/spec_helper.rb

@@ -28,6 +28,7 @@ else
 end
 require 'webrat'
 require 'mocha/api'
+require File.expand_path(File.dirname(__FILE__) + '/mocha_rspec_adapter')
 require File.expand_path(File.dirname(__FILE__) + '/mocha_extensions')
 
 Dir.glob("#{File.dirname(__FILE__).gsub(/\\/, "/")}/factories/*.rb").each { |file| require file }
@@ -121,19 +122,6 @@ Spec::Matchers.define :match_ignoring_whitespace do |expected|
   end
 end
 
-module MochaRspecAdapter
-  include Mocha::API
-  def setup_mocks_for_rspec
-    mocha_setup
-  end
-  def verify_mocks_for_rspec
-    mocha_verify
-  end
-  def teardown_mocks_for_rspec
-    mocha_teardown
-  end
-end
-
 Spec::Runner.configure do |config|
   # If you're not using ActiveRecord you should remove these
   # lines, delete config/database.yml and disable :active_record
@@ -141,7 +129,6 @@ Spec::Runner.configure do |config|
   config.use_transactional_fixtures = true
   config.use_instantiated_fixtures  = false
   config.fixture_path = Rails.root+'spec/fixtures/'
-  config.mock_with MochaRspecAdapter
 
   config.include Webrat::Matchers, :type => :views