4 files changed, 260 insertions, 109 deletions

diff --git a/src/mailman/app/bounces.py b/src/mailman/app/bounces.py
index 46c4515ca..50a9f54d8 100644
--- a/src/mailman/app/bounces.py
+++ b/src/mailman/app/bounces.py
@@ -45,7 +45,7 @@ from mailman.config import config
 from mailman.core.i18n import _
 from mailman.email.message import OwnerNotification, UserNotification
 from mailman.interfaces.bounce import (
-    IBounceDetector, UnrecognizedBounceDisposition)
+    IBounceDetector, Stop, UnrecognizedBounceDisposition)
 from mailman.interfaces.listmanager import IListManager
 from mailman.interfaces.membership import ISubscriptionService
 from mailman.interfaces.pending import IPendable, IPendings
@@ -107,13 +107,15 @@ def scan_message(mlist, msg):
         set will be empty if no addresses were found.
     :rtype: set
     """
+    fatal_addresses = set()
     for detector_class in find_components('mailman.bouncers', IBounceDetector):
         addresses = detector_class().process(msg)
-        # Detectors may return None or an empty sequence to signify that no
-        # addresses have been found.
-        if addresses:
-            return set(addresses)
-    return set()
+        # Detectors may return Stop to signify that no fatal errors were
+        # found, or a sequence of addresses.
+        if addresses is Stop:
+            return Stop
+        fatal_addresses.update(addresses)
+    return fatal_addresses
 
 
 
diff --git a/src/mailman/bouncers/dsn.py b/src/mailman/bouncers/dsn.py
index 49803ef88..c9d7803ee 100644
--- a/src/mailman/bouncers/dsn.py
+++ b/src/mailman/bouncers/dsn.py
@@ -37,84 +37,66 @@ from mailman.interfaces.bounce import IBounceDetector, Stop
 
 
 
-def check(msg):
-    # Iterate over each message/delivery-status subpart.
-    failed_addresses = []
-    delayed_addresses = []
-    for part in typed_subpart_iterator(msg, 'message', 'delivery-status'):
-        if not part.is_multipart():
-            # Huh?
-            continue
-        # Each message/delivery-status contains a list of Message objects
-        # which are the header blocks.  Iterate over those too.
-        for msgblock in part.get_payload():
-            address_set = None
-            # We try to dig out the Original-Recipient (which is optional) and
-            # Final-Recipient (which is mandatory, but may not exactly match
-            # an address on our list).  Some MTA's also use X-Actual-Recipient
-            # as a synonym for Original-Recipient, but some apparently use
-            # that for other purposes :(
-            #
-            # Also grok out Action so we can do something with that too.
-            action = msgblock.get('action', '').lower()
-            # Some MTAs have been observed that put comments on the action.
-            if action.startswith('delayed'):
-                address_set = delayed_addresses
-            elif action.startswith('fail'):
-                address_set = failed_addresses
-            else:
-                # Some non-permanent failure, so ignore this block.
-                continue
-            params = []
-            foundp = False
-            for header in ('original-recipient', 'final-recipient'):
-                for k, v in msgblock.get_params([], header):
-                    if k.lower() == 'rfc822':
-                        foundp = True
-                    else:
-                        params.append(k)
-                if foundp:
-                    # Note that params should already be unquoted.
-                    address_set.extend(params)
-                    break
-                else:
-                    # MAS: This is a kludge, but SMTP-GATEWAY01.intra.home.dk
-                    # has a final-recipient with an angle-addr and no
-                    # address-type parameter at all. Non-compliant, but ...
-                    for param in params:
-                        if param.startswith('<') and param.endswith('>'):
-                            address_set.append(param[1:-1])
-    # There may be both delayed and failed addresses.  If there are any failed
-    # addresses, return those, otherwise just stop processing.
-    if len(failed_addresses) == 0:
-        if len(delayed_addresses) == 0:
-            return set()
-        else:
-            return Stop
-    return set(parseaddr(address)[1] for address in failed_addresses
-               if address is not None)
-
-
-
 class DSN:
     """Parse RFC 3464 (i.e. DSN) bounce formats."""
 
     implements(IBounceDetector)
 
     def process(self, msg):
-        return check(msg)
-        ## # A DSN has been seen wrapped with a "legal disclaimer" by an outgoing
-        ## # MTA in a multipart/mixed outer part.
-        ## if msg.is_multipart() and msg.get_content_subtype() == 'mixed':
-        ##     msg = msg.get_payload()[0]
-        ## # The above will suffice if the original message 'parts' were wrapped
-        ## # with the disclaimer added, but the original DSN can be wrapped as a
-        ## # message/rfc822 part.  We need to test that too.
-        ## if msg.is_multipart() and msg.get_content_type() == 'message/rfc822':
-        ##     msg = msg.get_payload()[0]
-        ## # The report-type parameter should be "delivery-status", but it seems
-        ## # that some DSN generating MTAs don't include this on the
-        ## # Content-Type: header, so let's relax the test a bit.
-        ## if not msg.is_multipart() or msg.get_content_subtype() <> 'report':
-        ##     return set()
-        ## return check(msg)
+        """See `IBounceDetector`."""
+        # Iterate over each message/delivery-status subpart.
+        failed_addresses = []
+        delayed_addresses = []
+        for part in typed_subpart_iterator(msg, 'message', 'delivery-status'):
+            if not part.is_multipart():
+                # Huh?
+                continue
+            # Each message/delivery-status contains a list of Message objects
+            # which are the header blocks.  Iterate over those too.
+            for msgblock in part.get_payload():
+                address_set = None
+                # We try to dig out the Original-Recipient (which is optional)
+                # and Final-Recipient (which is mandatory, but may not exactly
+                # match an address on our list).  Some MTA's also use
+                # X-Actual-Recipient as a synonym for Original-Recipient, but
+                # some apparently use that for other purposes :(
+                #
+                # Also grok out Action so we can do something with that too.
+                action = msgblock.get('action', '').lower()
+                # Some MTAs have been observed that put comments on the action.
+                if action.startswith('delayed'):
+                    address_set = delayed_addresses
+                elif action.startswith('fail'):
+                    address_set = failed_addresses
+                else:
+                    # Some non-permanent failure, so ignore this block.
+                    continue
+                params = []
+                foundp = False
+                for header in ('original-recipient', 'final-recipient'):
+                    for k, v in msgblock.get_params([], header):
+                        if k.lower() == 'rfc822':
+                            foundp = True
+                        else:
+                            params.append(k)
+                    if foundp:
+                        # Note that params should already be unquoted.
+                        address_set.extend(params)
+                        break
+                    else:
+                        # MAS: This is a kludge, but
+                        # SMTP-GATEWAY01.intra.home.dk has a final-recipient
+                        # with an angle-addr and no address-type parameter at
+                        # all. Non-compliant, but ...
+                        for param in params:
+                            if param.startswith('<') and param.endswith('>'):
+                                address_set.append(param[1:-1])
+        # There may be both delayed and failed addresses.  If there are any
+        # failed addresses, return those, otherwise just stop processing.
+        if len(failed_addresses) == 0:
+            if len(delayed_addresses) == 0:
+                return set()
+            else:
+                return Stop
+        return set(parseaddr(address)[1] for address in failed_addresses
+                   if address is not None)
diff --git a/src/mailman/queue/bounce.py b/src/mailman/queue/bounce.py
index 819f0e368..a714f2669 100644
--- a/src/mailman/queue/bounce.py
+++ b/src/mailman/queue/bounce.py
@@ -19,9 +19,11 @@
 
 import logging
 
-from mailman.app.bounces import StandardVERP
-from mailman.config import config
-from mailman.interfaces.bounce import Stop
+from zope.component import getUtility
+
+from mailman.app.bounces import (
+    ProbeVERP, StandardVERP, maybe_forward, scan_message)
+from mailman.interfaces.bounce import BounceContext, IBounceProcessor, Stop
 from mailman.queue import Runner
 
 
@@ -35,37 +37,44 @@ elog = logging.getLogger('mailman.error')
 class BounceRunner(Runner):
     """The bounce runner."""
 
+    def __init__(self, name, slice=None):
+        super(BounceRunner, self).__init__(name, slice)
+        self._processor = getUtility(IBounceProcessor)
+
     def _dispose(self, mlist, msg, msgdata):
         # List isn't doing bounce processing?
         if not mlist.bounce_processing:
             return False
         # Try VERP detection first, since it's quick and easy
-        addrs = StandardVERP().get_verp(mlist, msg)
-        if addrs:
-            # We have an address, but check if the message is non-fatal.
-            if scan_messages(mlist, msg) is Stop:
-                return
+        context = BounceContext.normal
+        addresses = StandardVERP().get_verp(mlist, msg)
+        if addresses:
+            # We have an address, but check if the message is non-fatal.  It
+            # will be non-fatal if the bounce scanner returns Stop.  It will
+            # return a set of addresses when the bounce is fatal, but we don't
+            # care about those addresses, since we got it out of the VERP.
+            if scan_message(mlist, msg) is Stop:
+                return False
         else:
             # See if this was a probe message.
-            token = verp_probe(mlist, msg)
-            if token:
-                self._probe_bounce(mlist, token)
-                return
-            # That didn't give us anything useful, so try the old fashion
-            # bounce matching modules.
-            addrs = scan_messages(mlist, msg)
-            if addrs is Stop:
-                # This is a recognized, non-fatal notice. Ignore it.
-                return
+            addresses = ProbeVERP().get_verp(mlist, msg)
+            if addresses:
+                context = BounceContext.probe
+            else:
+                # That didn't give us anything useful, so try the old fashion
+                # bounce matching modules.
+                addresses = scan_message(mlist, msg)
+                if addresses is Stop:
+                    # This is a recognized, non-fatal notice. Ignore it.
+                    return False
         # If that still didn't return us any useful addresses, then send it on
         # or discard it.
-        if not addrs:
-            log.info('bounce message w/no discernable addresses: %s',
-                     msg.get('message-id'))
+        if len(addresses) > 0:
+            for address in addresses:
+                self._processor.register(mlist, address, msg, context)
+        else:
+            log.info('Bounce message w/no discernable addresses: %s',
+                     msg.get('message-id', 'n/a'))
             maybe_forward(mlist, msg)
-            return
-        # BAW: It's possible that there are None's in the list of addresses,
-        # although I'm unsure how that could happen.  Possibly scan_messages()
-        # can let None's sneak through.  In any event, this will kill them.
-        addrs = filter(None, addrs)
-        self._queue_bounces(mlist.fqdn_listname, addrs, msg)
+        # Dequeue this message.
+        return False
diff --git a/src/mailman/queue/tests/test_bounce.py b/src/mailman/queue/tests/test_bounce.py
index ba2476e79..1946df50c 100644
--- a/src/mailman/queue/tests/test_bounce.py
+++ b/src/mailman/queue/tests/test_bounce.py
@@ -29,12 +29,16 @@ import unittest
 
 from zope.component import getUtility
 
+from mailman.app.bounces import send_probe
 from mailman.app.lifecycle import create_list
 from mailman.config import config
+from mailman.interfaces.bounce import (
+    BounceContext, IBounceProcessor, UnrecognizedBounceDisposition)
 from mailman.interfaces.member import MemberRole
 from mailman.interfaces.usermanager import IUserManager
 from mailman.queue.bounce import BounceRunner
 from mailman.testing.helpers import (
+    LogFileMark,
     get_queue_messages,
     make_testable_runner,
     specialized_message_from_string as message_from_string)
@@ -56,11 +60,19 @@ class TestBounceQueue(unittest.TestCase):
         self._member = self._mlist.subscribe(self._anne, MemberRole.member)
         self._msg = message_from_string("""\
 From: mail-daemon@example.com
-To: test-bounce+anne=example.com@example.com
+To: test-bounces+anne=example.com@example.com
 Message-Id: <first>
 
 """)
         self._msgdata = dict(listname='test@example.com')
+        self._processor = getUtility(IBounceProcessor)
+        config.push('site owner', """
+        [mailman]
+        site_owner: postmaster@example.com
+        """)
+
+    def tearDown(self):
+        config.pop('site owner')
 
     def test_does_no_processing(self):
         # If the mailing list does no bounce processing, the messages are
@@ -69,6 +81,152 @@ Message-Id: <first>
         self._bounceq.enqueue(self._msg, self._msgdata)
         self._runner.run()
         self.assertEqual(len(get_queue_messages('bounces')), 0)
+        self.assertEqual(len(list(self._processor.events)), 0)
+
+    def test_verp_detection(self):
+        # When we get a VERPd bounce, and we're doing processing, a bounce
+        # event will be registered.
+        self._bounceq.enqueue(self._msg, self._msgdata)
+        self._runner.run()
+        self.assertEqual(len(get_queue_messages('bounces')), 0)
+        events = list(self._processor.events)
+        self.assertEqual(len(events), 1)
+        self.assertEqual(events[0].email, 'anne@example.com')
+        self.assertEqual(events[0].list_name, 'test@example.com')
+        self.assertEqual(events[0].message_id, '<first>')
+        self.assertEqual(events[0].context, BounceContext.normal)
+        self.assertEqual(events[0].processed, False)
+
+    def test_nonfatal_verp_detection(self):
+        # A VERPd bounce was received, but the error was nonfatal.
+        nonfatal = message_from_string("""\
+From: mail-daemon@example.com
+To: test-bounces+anne=example.com@example.com
+Message-Id: <first>
+Content-Type: multipart/report; report-type=delivery-status; boundary=AAA
+MIME-Version: 1.0
+
+--AAA
+Content-Type: message/delivery-status
+
+Action: delayed
+Original-Recipient: rfc822; somebody@example.com
+
+--AAA--
+""")
+        self._bounceq.enqueue(nonfatal, self._msgdata)
+        self._runner.run()
+        self.assertEqual(len(get_queue_messages('bounces')), 0)
+        events = list(self._processor.events)
+        self.assertEqual(len(events), 0)
+
+    def test_verp_probe_bounce(self):
+        # A VERP probe bounced.  The primary difference here is that the
+        # registered bounce event will have a different context.  The
+        # Message-Id will be different too, because of the way we're
+        # simulating the probe bounce.
+        #
+        # Start be simulating a probe bounce.
+        send_probe(self._member, self._msg)
+        message = get_queue_messages('virgin')[0].msg
+        bounce = message_from_string("""\
+To: {0}
+From: mail-daemon@example.com
+Message-Id: <second>
+
+""".format(message['From']))
+        self._bounceq.enqueue(bounce, self._msgdata)
+        self._runner.run()
+        self.assertEqual(len(get_queue_messages('bounces')), 0)
+        events = list(self._processor.events)
+        self.assertEqual(len(events), 1)
+        self.assertEqual(events[0].email, 'anne@example.com')
+        self.assertEqual(events[0].list_name, 'test@example.com')
+        self.assertEqual(events[0].message_id, '<second>')
+        self.assertEqual(events[0].context, BounceContext.probe)
+        self.assertEqual(events[0].processed, False)
+
+    def test_nonverp_detectable_fatal_bounce(self):
+        # Here's a bounce that is not VERPd, but which has a bouncing address
+        # that can be parsed from a known bounce format.  DSN is as good as
+        # any, but we'll make the parsed address different for the fun of it.
+        dsn = message_from_string("""\
+From: mail-daemon@example.com
+To: test-bounces@example.com
+Message-Id: <first>
+Content-Type: multipart/report; report-type=delivery-status; boundary=AAA
+MIME-Version: 1.0
+
+--AAA
+Content-Type: message/delivery-status
+
+Action: fail
+Original-Recipient: rfc822; bart@example.com
+
+--AAA--
+""")
+        self._bounceq.enqueue(dsn, self._msgdata)
+        self._runner.run()
+        self.assertEqual(len(get_queue_messages('bounces')), 0)
+        events = list(self._processor.events)
+        self.assertEqual(len(events), 1)
+        self.assertEqual(events[0].email, 'bart@example.com')
+        self.assertEqual(events[0].list_name, 'test@example.com')
+        self.assertEqual(events[0].message_id, '<first>')
+        self.assertEqual(events[0].context, BounceContext.normal)
+        self.assertEqual(events[0].processed, False)
+
+    def test_nonverp_detectable_nonfatal_bounce(self):
+        # Here's a bounce that is not VERPd, but which has a bouncing address
+        # that can be parsed from a known bounce format.  The bounce is
+        # non-fatal so no bounce event is registered.
+        dsn = message_from_string("""\
+From: mail-daemon@example.com
+To: test-bounces@example.com
+Message-Id: <first>
+Content-Type: multipart/report; report-type=delivery-status; boundary=AAA
+MIME-Version: 1.0
+
+--AAA
+Content-Type: message/delivery-status
+
+Action: delayed
+Original-Recipient: rfc822; bart@example.com
+
+--AAA--
+""")
+        self._bounceq.enqueue(dsn, self._msgdata)
+        self._runner.run()
+        self.assertEqual(len(get_queue_messages('bounces')), 0)
+        events = list(self._processor.events)
+        self.assertEqual(len(events), 0)
+
+    def test_no_detectable_bounce_addresses(self):
+        # A bounce message was received, but no addresses could be detected.
+        # A message will be logged in the bounce log though, and the message
+        # can be forwarded to someone who can do something about it.
+        self._mlist.forward_unrecognized_bounces_to = (
+            UnrecognizedBounceDisposition.site_owner)
+        bogus = message_from_string("""\
+From: mail-daemon@example.com
+To: test-bounces@example.com
+Message-Id: <third>
+
+""")
+        self._bounceq.enqueue(bogus, self._msgdata)
+        mark = LogFileMark('mailman.bounce')
+        self._runner.run()
+        self.assertEqual(len(get_queue_messages('bounces')), 0)
+        events = list(self._processor.events)
+        self.assertEqual(len(events), 0)
+        line = mark.readline()
+        self.assertEqual(
+            line[-51:-1],
+            'Bounce message w/no discernable addresses: <third>')
+        # Here's the forwarded message to the site owners.
+        forwards = get_queue_messages('virgin')
+        self.assertEqual(len(forwards), 1)
+        self.assertEqual(forwards[0].msg['to'], 'postmaster@example.com')