Merge lp:~sinzui/launchpad/hold-message into lp:launchpad

[Curtis Hovey]

This is my branch to forward small messages to the moderation queue.

    lp:~sinzui/launchpad/hold-message
    Diff size: 311
    Launchpad bug:
          https://bugs.launchpad.net/bugs/645702
    Test command: ./bin/test -vv \
          -t test_lpsize
    Pre-implementation: EdwinGrubbs, jcsacket
    Target release: 10.10

Forward small messages to the moderation queue
-----------------------------------------------

OOPS-1726XMLP285 shows that mailman forwarded a message for moderation that
exceeds the db storage limit.

There are two views on this issue. The first is that this is a copy of a
message mailman wants an admin to decide if the real message should be sent to
the list or discarded--the person only needs to see enough to make a decision.
the second issue is that mailman has rules for handling large messages and
they do not appear to agree with Lp. The soft limit enters moderation (which
is this case), and the hard limit is discarded immediately. I recall the soft
limit was raised at the request of statik.

Rules
-----

    * Update the lpsize tests to be unittests and delete the tests from
      postings.txt (often fails because of timeouts)
    * Add a rule to truncate the message so that it is small enough to
      be moderated.
      * Delete non text/plain parts because Lp only stores text/plain.
      * Truncate large text/parts. 10k will be enough for the moderator
        to make a decision to discard or forward. This message is shown
        in a list of messages; we want to limit the text to ensure the page
        is usable.

QA
--

    * Send an email with a large attachment to a list on staging.
      the attachment must be larger than 11m and not over 20m
    * Verify the message appears in the moderation queue

Lint
----

Linting changed files:
  lib/lp/services/mailman/doc/postings.txt
  lib/lp/services/mailman/monkeypatches/lpsize.py
  lib/lp/services/mailman/testing/__init__.py
  lib/lp/services/mailman/tests/test_lpsize.py

Test
----

    * lib/lp/services/mailman/doc/postings.txt
      * Removed redundant test that were prone to fail
    * lib/lp/services/mailman/testing/__init__.py
      * Fixed the test harness to make a proper email with an attachment.
        These tests were the first to really exercise it.
    * lib/lp/services/mailman/tests/test_lpsize.py
      * Converted the doctests to unittests
      * Added tests for the truncated_message function.

Implementation
--------------

    * lib/lp/services/mailman/monkeypatches/lpsize.py
      * Added truncated_message to create a small message and used it when
        calling hold().

[Edwin Grubbs (edwin-grubbs)]

Hi Curtis,

This is a nice improvement. I have some comments below.

-Edwin

>=== modified file 'lib/lp/services/mailman/monkeypatches/lpsize.py'
>--- lib/lp/services/mailman/monkeypatches/lpsize.py 2010-08-20 20:31:18 +0000
>+++ lib/lp/services/mailman/monkeypatches/lpsize.py 2010-10-06 15:36:24 +0000
>@@ -3,15 +3,43 @@
>
> """A pipeline handler for checking message sizes."""
>
>-# pylint: disable-msg=F0401
>+import email
>+
> from Mailman import (
> Errors,
>+ Message,
> mm_cfg,
> )
> from Mailman.Handlers.LPModerate import hold
> from Mailman.Logging.Syslog import syslog
>
>
>+def truncated_message(original_message, limit=10000):
>+ """Create a smaller version of the message for moderation."""
>+ message = email.message_from_string(
>+ original_message.as_string(), Message.Message)
>+ for part in email.iterators.typed_subpart_iterator(message, 'multipart'):
>+ subparts = part.get_payload()
>+ removeable = []
>+ for subpart in subparts:
>+ if subpart.get_content_maintype() == 'multipart':
>+ # This part is handled in the outer loop.
>+ continue
>+ elif subpart.get_content_type() == 'text/plain':

Shouldn't it truncate text/html or any other "text/" content-type,
since the user's mail client might create an empty text/plain section?

>+ # Truncate the message at 10kb so that there is enough
>+ # information for the moderator to make a decision.
>+ content = subpart.get_payload().strip()
>+ if len(content) > limit:
>+ subpart.set_payload(
>+ content[:limit] + ' [truncated for moderation]',

It seems like [truncated for moderation] will be easier to see if
it is on a new line.

>+ subpart.get_charset())
>+ else:
>+ removeable.append(subpart)
>+ for subpart in removeable:
>+ subparts.remove(subpart)
>+ return message
>+
>+
> def process(mlist, msg, msgdata):
> """Check the message size (approximately) against hard and soft limits.
>
>=== added file 'lib/lp/services/mailman/tests/test_lpsize.py'
>--- lib/lp/services/mailman/tests/test_lpsize.py 1970-01-01 00:00:00 +0000
>+++ lib/lp/services/mailman/tests/test_lpsize.py 2010-10-06 15:36:24 +0000
>@@ -0,0 +1,129 @@
>+# Copyright 20010 Canonical Ltd. This software is licensed under the

I think the copyright will expire by the time Bender and Leela can use
it.

>+# GNU Affero General Public License version 3 (see the file LICENSE).
>+"""Test the lpsize monekypatches"""
>+
>+from __future__ import with_statement
>+
>+__metaclass__ = type
>+__all__ = []
>+
>+from email.mime.application import MIMEApplication
>+
>+from Mailman import Errors
>+from Mailman.Handlers import LPSize
>+
>+from canonical.config import config
>+from canonical.testing import (
>+ DatabaseFunctionalLayer,
>+ LaunchpadFunctionalLayer,
>+ )
>+from lp.services.mailman.testing import MailmanTestCase
>+
>+
>+class TestLPSizeTestCase(MailmanTestCase):
>+ """Test LPSize.
>+
>+ Mailman process() methods quietly return. They may...

review: Approve (code)

[Curtis Hovey (sinzui)]

On Wed, 2010-10-06 at 17:23 +0000, Edwin Grubbs wrote:
> Review: Approve code
> Hi Curtis,
>
> This is a nice improvement. I have some comments below.
>
> -Edwin
>
>
> >=== mo>+# GNU Affero General Public License version 3 (see the file LICENSE).
> >+"""Test the lpsize monekypatches"""
> >+
> >+from __future__ import with_statement
> >+
> >+__metaclass__ = type
> >+__all__ = []
> >+
> >+from email.mime.application import MIMEApplication
> >+
> >+from Mailman import Errors
> >+from Mailman.Handlers import LPSize
> >+
> >+from canonical.config import config
> >+from canonical.testing import (
> >+ DatabaseFunctionalLayer,
> >+ LaunchpadFunctionalLayer,
> >+ )
> >+from lp.services.mailman.testing import MailmanTestCase
> >+
> >+
> dified file 'lib/lp/services/mailman/monkeypatches/lpsize.py'
> >--- lib/lp/services/mailman/monkeypatches/lpsize.py 2010-08-20 20:31:18 +0000
> >+++ lib/lp/services/mailman/monkeypatches/lpsize.py 2010-10-06 15:36:24 +0000
> >...
> >+def truncated_message(original_message, limit=10000):
> >+ """Create a smaller version of the message for moderation."""
> >+ message = email.message_from_string(
> >+ original_message.as_string(), Message.Message)
> >+ for part in email.iterators.typed_subpart_iterator(message, 'multipart'):
> >+ subparts = part.get_payload()
> >+ removeable = []
> >+ for subpart in subparts:
> >+ if subpart.get_content_maintype() == 'multipart':
> >+ # This part is handled in the outer loop.
> >+ continue
> >+ elif subpart.get_content_type() == 'text/plain':
>
>
>
> Shouldn't it truncate text/html or any other "text/" content-type,
> since the user's mail client might create an empty text/plain section?

Launchpad does not support text/html messages. Messages without
text/plain parts are discarded in the hold process. There is an old bug
about empty messages in the moderation queue. It was discovered that all
cases are from spammers. We did not see a need to support text/html
since list subscribers are not using it and getting messages in the
moderation queue.

> >+ # Truncate the message at 10kb so that there is enough
> >+ # information for the moderator to make a decision.
> >+ content = subpart.get_payload().strip()
> >+ if len(content) > limit:
> >+ subpart.set_payload(
> >+ content[:limit] + ' [truncated for moderation]',
>
>
>
> It seems like [truncated for moderation] will be easier to see if
> it is on a new line.

Agreed

> >=== added file 'lib/lp/services/mailman/tests/test_lpsize.py'
> >--- lib/lp/services/mailman/tests/test_lpsize.py 1970-01-01 00:00:00 +0000
> >+++ lib/lp/services/mailman/tests/test_lpsize.py 2010-10-06 15:36:24 +0000
> >@@ -0,0 +1,129 @@
> >+# Copyright 20010 Canonical Ltd. This software is licensed under the
>
>
>
> I think the copyright will expire by the time Bender and Leela can use
> it.

:)

...

> >+class TestLPSizeTestCase(MailmanTestCase):
> >+ """Test LPSize.
> >+
> >+ Mailman process() methods quietly return. They may set msg_data key-values
> ...

=== modified file 'lib/lp/services/mailman/doc/postings.txt'
--- lib/lp/services/mailman/doc/postings.txt	2010-10-03 15:30:06 +0000
+++ lib/lp/services/mailman/doc/postings.txt	2010-10-06 18:06:45 +0000
@@ -832,79 +832,3 @@
     >>> vette_watcher.wait_for_discard('narwhale')
     >>> len(list(smtpd))
     0
-
-
-Large messages
-==============
-
-Only messages which are less than about 40k in size are allowed straight
-through on the mailing list.  A message bigger than that will be held for
-moderator approval.
-
-    >>> from canonical.config import config
-    >>> config.mailman.soft_max_size
-    40000
-
-    >>> from email.mime.multipart import MIMEMultipart
-    >>> from email.mime.application import MIMEApplication
-    >>> from email.mime.text import MIMEText
-
-    >>> big_message = MIMEMultipart()
-    >>> big_message['From'] = '[e-mail]'
-    >>> big_message['To'] = '[e-mail]'
-    >>> big_message['Subject'] = 'A huge message'
-    >>> big_message['Message-ID'] = '<puma>'
-    >>> big_message.attach(
-    ...     MIMEText('look at this pdf.', 'plain'))
-    >>> big_message.attach(
-    ...     MIMEApplication('\n'.join(['x' * 50] * 1000), 'octet-stream'))
-    >>> inject('itest-one', big_message.as_string())
-
-    >>> vette_watcher.wait_for_hold('itest-one', 'puma')
-    >>> print_message_summaries()
-    Number of messages: 1
-    [e-mail]
-        ...
-        Itest One <[e-mail]>
-        New mailing list message requiring approval for Itest One
-
-Once this message is approved, it is posted through to the mailing list.
-
-    >>> browser.open(
-    ...     'http://launchpad.dev:8085/~itest-one/+mailinglist-moderate')
-    >>> browser.getControl(name='field.%3Cpuma%3E').value = ['approve']
-    >>> browser.getControl('Moderate').click()
-    >>> smtpd_watcher.wait_for_mbox_delivery('puma')
-    >>> smtpd_watcher.wait_for_mbox_delivery('puma')
-
-    >>> print_message_summaries()
-    Number of messages: 2
-    itest-one-bounces+anne.person=[e-mail]
-        <puma>
-        [e-mail]
-        [Itest-one] A huge message
-    itest-one-bounces+archive=[e-mail]
-        <puma>
-        [e-mail]
-        [Itest-one] A huge message
-
-There is a hard limit of 1MB, over which the message is summarily logged and
-discarded.
-
-    >>> config.mailman.hard_max_size
-    1000000
-
-    >>> huge_message = MIMEMultipart()
-    >>> huge_message['From'] = '[e-mail]'
-    >>> huge_message['To'] = '[e-mail]'
-    >>> huge_message['Subject'] = 'A huge message'
-    >>> huge_message['Message-ID'] = '<quahog>'
-    >>> huge_message.attach(
-    ...     MIMEText('look at this huge pdf.', 'plain'))
-    >>> huge_message.attach(
-    ...     MIMEApplication('\n'.join(['x' * 50] * 20000), 'octet-stream'))
-    >>> inject('itest-one', huge_message.as_string())
-
-    >>> vette_watcher.wait_for_discard('quahog')
-    >>> print_message_summaries()
-    Number of messages: 0
 
=== modified file 'lib/lp/services/mailman/monkeypatches/lpsize.py'
--- lib/lp/services/mailman/monkeypatches/lpsize.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/mailman/monkeypatches/lpsize.py	2010-10-06 18:06:45 +0000
@@ -1,17 +1,45 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """A pipeline handler for checking message sizes."""
 
-# pylint: disable-msg=F0401
+import email
+
 from Mailman import (
     Errors,
+    Message,
     mm_cfg,
     )
 from Mailman.Handlers.LPModerate import hold
 from Mailman.Logging.Syslog import syslog
 
 
+def truncated_message(original_message, limit=10000):
+    """Create a smaller version of the message for moderation."""
+    message = email.message_from_string(
+        original_message.as_string(), Message.Message)
+    for part in email.iterators.typed_subpart_iterator(message, 'multipart'):
+        subparts = part.get_payload()
+        removeable = []
+        for subpart in subparts:
+            if subpart.get_content_maintype() == 'multipart':
+                # This part is handled in the outer loop.
+                continue
+            elif subpart.get_content_type() == 'text/plain':
+                # Truncate the message at 10kb so that there is enough
+                # information for the moderator to make a decision.
+                content = subpart.get_payload().strip()
+                if len(content) > limit:
+                    subpart.set_payload(
+                        content[:limit] + '\n[truncated for moderation]',
+                        subpart.get_charset())
+            else:
+                removeable.append(subpart)
+        for subpart in removeable:
+            subparts.remove(subpart)
+    return message
+
+
 def process(mlist, msg, msgdata):
     """Check the message size (approximately) against hard and soft limits.
 
@@ -42,7 +70,10 @@
     if message_size < mm_cfg.LAUNCHPAD_HARD_MAX_SIZE:
         # Hold the message in Mailman.  See lpmoderate.py for similar
         # algorithm.
-        hold(mlist, msg, msgdata, 'Too big')
+        # There is a limit to the size that can be stored in Lp. Send
+        # a trucated copy of the message that has enough information for
+        # the moderator to make a decision.
+        hold(mlist, truncated_message(msg), msgdata, 'Too big')
     # The message is larger than the hard limit, so log and discard.
     syslog('vette', 'Discarding message w/size > hard limit: %s',
            msg.get('message-id', 'n/a'))
 
=== modified file 'lib/lp/services/mailman/testing/__init__.py'
--- lib/lp/services/mailman/testing/__init__.py	2010-10-04 19:50:45 +0000
+++ lib/lp/services/mailman/testing/__init__.py	2010-10-06 18:06:45 +0000
@@ -7,6 +7,7 @@
 
 from contextlib import contextmanager
 import email
+from email.mime.multipart import MIMEMultipart
 from email.mime.text import MIMEText
 import os
 import shutil
@@ -87,13 +88,16 @@
         # Make a Mailman Message.Message.
         if isinstance(sender, (list, tuple)):
             sender = ', '.join(sender)
-        message = MIMEText(content, mime_type)
+        message = MIMEMultipart()
         message['from'] = sender
         message['to'] = mm_list.getListAddress()
         message['subject'] = subject
         message['message-id'] = self.getUniqueString()
+        message.attach(MIMEText(content, mime_type))
+        if attachment is not None:
+            # Rewrap the text message in a multipart message and add the
+            # attachment.
+            message.attach(attachment)
         mm_message = email.message_from_string(
             message.as_string(), Message.Message)
-        if attachment is not None:
-            mm_message.attach(attachment, 'octet-stream')
         return mm_message
 
=== modified file 'lib/lp/services/mailman/tests/test_lpmoderate.py'
--- lib/lp/services/mailman/tests/test_lpmoderate.py	2010-10-04 19:50:45 +0000
+++ lib/lp/services/mailman/tests/test_lpmoderate.py	2010-10-06 18:06:45 +0000
@@ -18,11 +18,11 @@
     """Test lpmoderate.
 
     Mailman process() methods quietly return. They may set msg_data key-values
-    or raise an error to end processing. This group of tests tests often check
-    for errors, but that does not mean there is an error condition, it only
-    means message processing has reached a final decision. Messages that do
-    not cause a final decision pass-through and the process() methods ends
-    without a return.
+    or raise an error to end processing. These tests often check for errors,
+    but that does not mean there is an error condition, it only means message
+    processing has reached a final decision. Messages that do not cause a
+    final decision pass through, and the process() methods ends without a
+    return.
     """
 
     layer = LaunchpadFunctionalLayer
 
=== added file 'lib/lp/services/mailman/tests/test_lpsize.py'
--- lib/lp/services/mailman/tests/test_lpsize.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/mailman/tests/test_lpsize.py	2010-10-06 18:06:45 +0000
@@ -0,0 +1,129 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+"""Test the lpsize monekypatches"""
+
+from __future__ import with_statement
+
+__metaclass__ = type
+__all__ = []
+
+from email.mime.application import MIMEApplication
+
+from Mailman import Errors
+from Mailman.Handlers import LPSize
+
+from canonical.config import config
+from canonical.testing import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
+from lp.services.mailman.testing import MailmanTestCase
+
+
+class TestLPSizeTestCase(MailmanTestCase):
+    """Test LPSize.
+
+    Mailman process() methods quietly return. They may set msg_data key-values
+    or raise an error to end processing. These tests often check for errors,
+    but that does not mean there is an error condition, it only means message
+    processing has reached a final decision. Messages that do not cause a
+    final decision pass through, and the process() methods ends without a
+    return.
+    """
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestLPSizeTestCase, self).setUp()
+        self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
+            'team-1', 'team-1-owner')
+        self.mm_list = self.makeMailmanList(self.mailing_list)
+        self.subscriber_email = self.team.teamowner.preferredemail.email
+
+    def tearDown(self):
+        super(TestLPSizeTestCase, self).tearDown()
+        self.cleanMailmanList(self.mm_list)
+
+    def test_process_size_under_soft_limit(self):
+        # Any message under 40kb is sent to the list.
+        attachment = MIMEApplication(
+            '\n'.join(['x' * 20] * 1000), 'octet-stream')
+        message = self.makeMailmanMessage(
+            self.mm_list, self.subscriber_email, 'subject', 'content',
+            attachment=attachment)
+        msg_data = {}
+        silence = LPSize.process(self.mm_list, message, msg_data)
+        self.assertEqual(None, silence)
+
+    def test_process_size_over_soft_limit_held(self):
+        # Messages over 40kb held for moderation.
+        self.assertEqual(40000, config.mailman.soft_max_size)
+        attachment = MIMEApplication(
+            '\n'.join(['x' * 40] * 1000), 'octet-stream')
+        message = self.makeMailmanMessage(
+            self.mm_list, self.subscriber_email, 'subject', 'content',
+            attachment=attachment)
+        msg_data = {}
+        args = (self.mm_list, message, msg_data)
+        self.assertRaises(
+            Errors.HoldMessage, LPSize.process, *args)
+        self.assertEqual(1, self.mailing_list.getReviewableMessages().count())
+
+    def test_process_size_over_hard_limit_discarded(self):
+        # Messages over 1MB are discarded.
+        self.assertEqual(1000000, config.mailman.hard_max_size)
+        attachment = MIMEApplication(
+            '\n'.join(['x' * 1000] * 1000), 'octet-stream')
+        message = self.makeMailmanMessage(
+            self.mm_list, self.subscriber_email, 'subject', 'content',
+            attachment=attachment)
+        msg_data = {}
+        args = (self.mm_list, message, msg_data)
+        self.assertRaises(
+            Errors.DiscardMessage, LPSize.process, *args)
+        self.assertEqual(0, self.mailing_list.getReviewableMessages().count())
+
+
+class TestTruncatedMessage(MailmanTestCase):
+    """Test truncated_message helper."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestTruncatedMessage, self).setUp()
+        self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
+            'team-1', 'team-1-owner')
+        self.mm_list = self.makeMailmanList(self.mailing_list)
+        self.subscriber_email = self.team.teamowner.preferredemail.email
+
+    def test_attchments_are_removed(self):
+        # Plain-text and multipart are preserved, everything else is removed.
+        attachment = MIMEApplication('binary gibberish', 'octet-stream')
+        message = self.makeMailmanMessage(
+            self.mm_list, self.subscriber_email, 'subject', 'content',
+            attachment=attachment)
+        moderated_message = LPSize.truncated_message(message)
+        parts = [part for part in moderated_message.walk()]
+        types = [part.get_content_type() for part in parts]
+        self.assertEqual(['multipart/mixed', 'text/plain'], types)
+
+    def test_small_text_is_preserved(self):
+        # Text parts below the limit are unchanged.
+        message = self.makeMailmanMessage(
+            self.mm_list, self.subscriber_email, 'subject', 'content')
+        moderated_message = LPSize.truncated_message(message, limit=1000)
+        parts = [part for part in moderated_message.walk()]
+        types = [part.get_content_type() for part in parts]
+        self.assertEqual(['multipart/mixed', 'text/plain'], types)
+        self.assertEqual('content', parts[1].get_payload())
+
+    def test_large_text_is_truncated(self):
+        # Text parts above the limit are truncated.
+        message = self.makeMailmanMessage(
+            self.mm_list, self.subscriber_email, 'subject', 'content excess')
+        moderated_message = LPSize.truncated_message(message, limit=7)
+        parts = [part for part in moderated_message.walk()]
+        types = [part.get_content_type() for part in parts]
+        self.assertEqual(['multipart/mixed', 'text/plain'], types)
+        self.assertEqual(
+            'content\n[truncated for moderation]', parts[1].get_payload())