15 files changed, 91 insertions, 25 deletions

diff --git a/src/mailman/chains/hold.py b/src/mailman/chains/hold.py
index f111aee3c..42fed66a6 100644
--- a/src/mailman/chains/hold.py
+++ b/src/mailman/chains/hold.py
@@ -142,8 +142,9 @@ class HoldChain(TerminalChainBase):
         rule_misses = msgdata.get('rule_misses')
         if rule_misses:
             msg['X-Mailman-Rule-Misses'] = SEMISPACE.join(rule_misses)
+        reasons = msgdata.get('moderation_reasons', ['n/a'])
         # Hold the message by adding it to the list's request database.
-        request_id = hold_message(mlist, msg, msgdata, None)
+        request_id = hold_message(mlist, msg, msgdata, SEMISPACE.join(reasons))
         # Calculate a confirmation token to send to the author of the
         # message.
         pendable = HeldMessagePendable(id=request_id)
diff --git a/src/mailman/chains/tests/test_hold.py b/src/mailman/chains/tests/test_hold.py
index 13dd1b40e..6686c4777 100644
--- a/src/mailman/chains/tests/test_hold.py
+++ b/src/mailman/chains/tests/test_hold.py
@@ -26,6 +26,7 @@ from mailman.core.chains import process as process_chain
 from mailman.interfaces.autorespond import IAutoResponseSet, Response
 from mailman.interfaces.member import MemberRole
 from mailman.interfaces.messages import IMessageStore
+from mailman.interfaces.requests import IListRequests, RequestType
 from mailman.interfaces.usermanager import IUserManager
 from mailman.testing.helpers import (
     LogFileMark, configuration, get_queue_messages, set_preferred,
@@ -130,6 +131,31 @@ A message body.
         logged = logfile.read()
         self.assertIn('TEST-REASON-1', logged)
         self.assertIn('TEST-REASON-2', logged)
+        # Check the reason passed to hold_message().
+        requests = IListRequests(self._mlist)
+        self.assertEqual(requests.count_of(RequestType.held_message), 1)
+        request = requests.of_type(RequestType.held_message)[0]
+        key, data = requests.get_request(request.id)
+        self.assertEqual(
+            data.get('_mod_reason'), 'TEST-REASON-1; TEST-REASON-2')
+
+    def test_hold_chain_no_reasons_given(self):
+        msg = mfs("""\
+From: anne@example.com
+To: test@example.com
+Subject: A message
+Message-ID: <ant>
+MIME-Version: 1.0
+
+A message body.
+""")
+        process_chain(self._mlist, msg, {}, start_chain='hold')
+        # No reason was given, so a default is used.
+        requests = IListRequests(self._mlist)
+        self.assertEqual(requests.count_of(RequestType.held_message), 1)
+        request = requests.of_type(RequestType.held_message)[0]
+        key, data = requests.get_request(request.id)
+        self.assertEqual(data.get('_mod_reason'), 'n/a')
 
     def test_hold_chain_charset(self):
         # Issue #144 - UnicodeEncodeError in the hold chain.
diff --git a/src/mailman/config/tests/test_archivers.py b/src/mailman/config/tests/test_archivers.py
index 322a5040d..b09b89273 100644
--- a/src/mailman/config/tests/test_archivers.py
+++ b/src/mailman/config/tests/test_archivers.py
@@ -28,11 +28,11 @@ class TestArchivers(unittest.TestCase):
     layer = ConfigLayer
 
     def test_enabled(self):
-        # By default, the testing configuration enables the archivers.
+        # By default, the testing configuration enables some archivers.
         archivers = {}
         for archiver in config.archivers:
             archivers[archiver.name] = archiver
-        self.assertTrue(archivers['prototype'].is_enabled)
+        self.assertFalse(archivers['prototype'].is_enabled)
         self.assertTrue(archivers['mail-archive'].is_enabled)
         self.assertTrue(archivers['mhonarc'].is_enabled)
 
@@ -42,6 +42,6 @@ class TestArchivers(unittest.TestCase):
         archivers = {}
         for archiver in config.archivers:
             archivers[archiver.name] = archiver
-        self.assertTrue(archivers['prototype'].is_enabled)
+        self.assertFalse(archivers['prototype'].is_enabled)
         self.assertTrue(archivers['mail-archive'].is_enabled)
         self.assertFalse(archivers['mhonarc'].is_enabled)
diff --git a/src/mailman/docs/NEWS.rst b/src/mailman/docs/NEWS.rst
index 80cadd1b7..26d7433c3 100644
--- a/src/mailman/docs/NEWS.rst
+++ b/src/mailman/docs/NEWS.rst
@@ -99,6 +99,10 @@ Bugs
    Bompard.  (Closes: #259)
  * Messages sent to the list's moderators now include the actual recipient
    addresses.  Given by Tom Briles.  (Closes: #68)
+ * Transmit the moderation reason and expose it in the REST API as the
+   ``reason`` attribute.  Given by Aurélien Bompard.
+ * Don't return a 500 error from the REST API when trying to handle a held
+   message with defective content.  Given by Abhilash Raj.  (Closes: #256)
 
 Configuration
 -------------
@@ -241,6 +245,8 @@ REST
    Aurélien Bompard.  (Closes #284)
  * Query parameters now allow you to filter mailing lists by the
    ``advertised`` boolean parameter.  Given by Aurélien Bompard.
+ * Only the system-enabled archivers are returned in the REST API.  Given by
+   Aurélien Bompard.
 
 Other
 -----
diff --git a/src/mailman/model/tests/test_mailinglist.py b/src/mailman/model/tests/test_mailinglist.py
index 0a5c8a5e9..658ffc7d1 100644
--- a/src/mailman/model/tests/test_mailinglist.py
+++ b/src/mailman/model/tests/test_mailinglist.py
@@ -121,11 +121,11 @@ class TestListArchiver(unittest.TestCase):
 
     def test_get_archiver(self):
         # Use .get() to see if a mailing list has an archiver.
-        archiver = self._set.get('prototype')
-        self.assertEqual(archiver.name, 'prototype')
+        archiver = self._set.get('mhonarc')
+        self.assertEqual(archiver.name, 'mhonarc')
         self.assertTrue(archiver.is_enabled)
         self.assertEqual(archiver.mailing_list, self._mlist)
-        self.assertEqual(archiver.system_archiver.name, 'prototype')
+        self.assertEqual(archiver.system_archiver.name, 'mhonarc')
 
     def test_get_archiver_no_such(self):
         # Using .get() on a non-existing name returns None.
@@ -137,15 +137,15 @@ class TestListArchiver(unittest.TestCase):
         # then the site-wide archiver gets disabled, so the list specific
         # archiver will also be disabled.
         archiver_set = IListArchiverSet(self._mlist)
-        archiver = archiver_set.get('prototype')
+        archiver = archiver_set.get('mhonarc')
         self.assertTrue(archiver.is_enabled)
         # Disable the site-wide archiver.
-        config.push('enable prototype', """\
-        [archiver.prototype]
+        config.push('enable mhonarc', """\
+        [archiver.mhonarc]
         enable: no
         """)
         self.assertFalse(archiver.is_enabled)
-        config.pop('enable prototype')
+        config.pop('enable mhonarc')
 
 
 class TestDisabledListArchiver(unittest.TestCase):
diff --git a/src/mailman/rest/docs/lists.rst b/src/mailman/rest/docs/lists.rst
index 247b2f4e7..6a034df94 100644
--- a/src/mailman/rest/docs/lists.rst
+++ b/src/mailman/rest/docs/lists.rst
@@ -275,7 +275,6 @@ archivers are available, and whether they are enabled for this mailing list.
     http_etag: "..."
     mail-archive: True
     mhonarc: True
-    prototype: True
 
 You can set all the archiver states by putting new state flags on the
 resource.
@@ -285,7 +284,6 @@ resource.
     ...     'http://localhost:9001/3.0/lists/dog@example.com/archivers', {
     ...         'mail-archive': False,
     ...         'mhonarc': True,
-    ...         'prototype': False,
     ...         }, method='PUT')
     content-length: 0
     date: ...
@@ -296,7 +294,6 @@ resource.
     http_etag: "..."
     mail-archive: False
     mhonarc: True
-    prototype: False
 
 You can change the state of a subset of the list archivers.
 ::
@@ -314,7 +311,6 @@ You can change the state of a subset of the list archivers.
     http_etag: "..."
     mail-archive: False
     mhonarc: False
-    prototype: False
 
 
 List digests
diff --git a/src/mailman/rest/lists.py b/src/mailman/rest/lists.py
index 8952f1156..1fdf595fb 100644
--- a/src/mailman/rest/lists.py
+++ b/src/mailman/rest/lists.py
@@ -337,13 +337,15 @@ class ListArchivers:
         """Get all the archiver statuses."""
         archiver_set = IListArchiverSet(self._mlist)
         resource = {archiver.name: archiver.is_enabled
-                    for archiver in archiver_set.archivers}
+                    for archiver in archiver_set.archivers
+                    if archiver.system_archiver.is_enabled}
         okay(response, etag(resource))
 
     def patch_put(self, request, response, is_optional):
         archiver_set = IListArchiverSet(self._mlist)
         kws = {archiver.name: ArchiverGetterSetter(self._mlist)
-               for archiver in archiver_set.archivers}
+               for archiver in archiver_set.archivers
+               if archiver.system_archiver.is_enabled}
         if is_optional:
             # For a PATCH, all attributes are optional.
             kws['_optional'] = kws.keys()
diff --git a/src/mailman/rest/post_moderation.py b/src/mailman/rest/post_moderation.py
index fc38af359..33a32de20 100644
--- a/src/mailman/rest/post_moderation.py
+++ b/src/mailman/rest/post_moderation.py
@@ -71,7 +71,14 @@ class _HeldMessageBase(_ModerationBase):
         # resource.  XXX See LP: #967954
         key = resource.pop('key')
         msg = getUtility(IMessageStore).get_message_by_id(key)
-        resource['msg'] = msg.as_string()
+        try:
+            resource['msg'] = msg.as_string()
+        except KeyError:
+            # If the message can't be parsed, return a generic message instead
+            # of raising an error.
+            #
+            # See http://bugs.python.org/issue27321 and GL#256
+            resource['msg'] = 'This message is defective'
         # Some of the _mod_* keys we want to rename and place into the JSON
         # resource.  Others we can drop.  Since we're mutating the dictionary,
         # we need to make a copy of the keys.  When you port this to Python 3,
diff --git a/src/mailman/rest/tests/data/__init__.py b/src/mailman/rest/tests/data/__init__.py
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/src/mailman/rest/tests/data/__init__.py
diff --git a/src/mailman/rest/tests/data/bad_email.eml b/src/mailman/rest/tests/data/bad_email.eml
new file mode 100644
index 000000000..83f9337dd
--- /dev/null
+++ b/src/mailman/rest/tests/data/bad_email.eml
@@ -0,0 +1,8 @@
+To: <test@example.com>
+Subject: =?koi8-r?B?UF9AX/NfQ1/5X+xfS1/p?=
+From: =?koi8-r?B?8sXL0sXB1MnXzs/FIMHHxc7U09TXzw==?=
+Content-Type: text/plain; charset=koi8-r
+Message-Id: <20160614102505.9OFQ19L1C>
+
+þôï ôáëïå òåëìáíîáñ òáóóùìëá?
+ëÁËÏÊ ÏÔËÌÉË ÖÄÁÔØ ÏÔ ÜÔÏÇÏ ÍÅÔÏÄÁ ÐÏÉÓËÁ ËÌÉÅÎÔÏ×?
diff --git a/src/mailman/rest/tests/test_lists.py b/src/mailman/rest/tests/test_lists.py
index da6e95f3b..49764f86f 100644
--- a/src/mailman/rest/tests/test_lists.py
+++ b/src/mailman/rest/tests/test_lists.py
@@ -339,7 +339,6 @@ class TestListArchivers(unittest.TestCase):
         self.assertEqual(resource, {
             'mail-archive': True,
             'mhonarc': True,
-            'prototype': True,
             })
 
     def test_archiver_statuses_on_missing_lists(self):
@@ -375,7 +374,7 @@ class TestListArchivers(unittest.TestCase):
 
     def test_put_incomplete_statuses(self):
         # PUT requires the full resource representation.  This one forgets to
-        # specify the prototype and mhonarc archiver.
+        # specify the mhonarc archiver.
         with self.assertRaises(HTTPError) as cm:
             call_api(
                 'http://localhost:9001/3.0/lists/ant.example.com/archivers', {
@@ -384,7 +383,7 @@ class TestListArchivers(unittest.TestCase):
                 method='PUT')
         self.assertEqual(cm.exception.code, 400)
         self.assertEqual(cm.exception.reason,
-                         b'Missing parameters: mhonarc, prototype')
+                         b'Missing parameters: mhonarc')
 
     def test_patch_bogus_status(self):
         # Archiver statuses must be interpretable as booleans.
@@ -392,8 +391,7 @@ class TestListArchivers(unittest.TestCase):
             call_api(
                 'http://localhost:9001/3.0/lists/ant.example.com/archivers', {
                     'mail-archive': 'sure',
-                    'mhonarc': False,
-                    'prototype': 'no'
+                    'mhonarc': 'no'
                     },
                 method='PATCH')
         self.assertEqual(cm.exception.code, 400)
diff --git a/src/mailman/rest/tests/test_moderation.py b/src/mailman/rest/tests/test_moderation.py
index e0c3f1ccf..21129207c 100644
--- a/src/mailman/rest/tests/test_moderation.py
+++ b/src/mailman/rest/tests/test_moderation.py
@@ -19,6 +19,7 @@
 
 import unittest
 
+from email import message_from_binary_file
 from mailman.app.lifecycle import create_list
 from mailman.app.moderator import hold_message
 from mailman.database.transaction import transaction
@@ -31,6 +32,7 @@ from mailman.testing.helpers import (
     call_api, get_queue_messages, set_preferred,
     specialized_message_from_string as mfs)
 from mailman.testing.layers import RESTLayer
+from pkg_resources import resource_filename
 from urllib.error import HTTPError
 from zope.component import getUtility
 
@@ -206,6 +208,24 @@ class TestSubscriptionModeration(unittest.TestCase):
         emails = set(json['email'] for json in content['entries'])
         self.assertEqual(emails, {'anne@example.com', 'bart@example.com'})
 
+    def test_view_malformed_held_message(self):
+        # Opening a bad (i.e. bad structure) email and holding it.
+        email_path = resource_filename(
+            'mailman.rest.tests.data', 'bad_email.eml')
+        with open(email_path, 'rb') as fp:
+            msg = message_from_binary_file(fp)
+        msg.sender = 'aperson@example.com'
+        with transaction():
+            hold_message(self._mlist, msg)
+        # Now trying to access held messages from REST API should not give
+        # 500 server error if one of the messages can't be parsed properly.
+        content, response = call_api(
+            'http://localhost:9001/3.0/lists/ant@example.com/held')
+        self.assertEqual(response.status, 200)
+        self.assertEqual(len(content['entries']), 1)
+        self.assertEqual(content['entries'][0]['msg'],
+                         'This message is defective')
+
     def test_individual_request(self):
         # We can view an individual request.
         with transaction():
diff --git a/src/mailman/testing/testing.cfg b/src/mailman/testing/testing.cfg
index 022f9f289..8b6a48c36 100644
--- a/src/mailman/testing/testing.cfg
+++ b/src/mailman/testing/testing.cfg
@@ -65,7 +65,7 @@ max_restarts: 1
 max_restarts: 1
 
 [archiver.prototype]
-enable: yes
+enable: no
 
 [archiver.mail_archive]
 enable: yes
diff --git a/src/mailman/utilities/importer.py b/src/mailman/utilities/importer.py
index 00c30c9f0..cf14fa73a 100644
--- a/src/mailman/utilities/importer.py
+++ b/src/mailman/utilities/importer.py
@@ -336,7 +336,7 @@ def import_config_pck(mlist, config_dict):
         for line_pattern in line_patterns.splitlines():
             if len(line_pattern.strip()) == 0:
                 continue
-            for sep in (': ', ':.', ':'):
+            for sep in (': ', ':.*', ':.', ':'):
                 header, sep, pattern = line_pattern.partition(sep)
                 if sep:
                     # We found it.
diff --git a/src/mailman/utilities/tests/test_import.py b/src/mailman/utilities/tests/test_import.py
index 5a226ba26..62985545f 100644
--- a/src/mailman/utilities/tests/test_import.py
+++ b/src/mailman/utilities/tests/test_import.py
@@ -337,6 +337,7 @@ class TestBasicImport(unittest.TestCase):
             ('^Subject: dev-\r\n^Subject: staging-', 3, False),
             ('from: .*info@aolanchem.com\r\nfrom: .*@jw-express.com',
              2, False),
+            ('^Subject:.*\\Wwas:\\W', 3, False),
             ('^Received: from smtp-.*\\.fedoraproject\\.org\r\n'
              '^Received: from mx.*\\.redhat.com\r\n'
              '^Resent-date:\r\n'
@@ -373,6 +374,7 @@ class TestBasicImport(unittest.TestCase):
                 ('subject', 'staging-', 'discard'),
                 ('from', '.*info@aolanchem.com', 'reject'),
                 ('from', '.*@jw-express.com', 'reject'),
+                ('subject', '\\Wwas:\\W', 'discard'),
                 ('received', 'from smtp-.*\\.fedoraproject\\.org', 'hold'),
                 ('received', 'from mx.*\\.redhat.com', 'hold'),
                 ('resent-date', '.*', 'hold'),