Merge lp:~sinzui/launchpad/moderate-empty-messages into lp:launchpad

[Curtis Hovey]

This is my branch to prevent empty messages from entering the moderation queue.

    lp:~sinzui/launchpad/moderate-empty-messages
    Diff size: 121
    Launchpad bug: https://bugs.launchpad.net/bugs/498741
    Test command: ./bin/test -vv --layer=Mailman -t postings
    Pre-implementation: barry
    Target release: 10.04

Prevent empty messages from entering the moderation queue
----------------------------------------------------------

Launchpad only supports text/plain email messages, but mailman is asking
users to moderate text/html or image/* messages. Moderators see a subject with
no body. Besides the insanity of this case, there are two bugs that directly
relate to these messages:

Bug 498741 [spam offered for moderation on a ML]
    More that 99% of all spam in the moderation queue have no body.
Bug 498740 [timeout trying to moderate a mailing list with many messages]
    The number of spam messages causes timeouts.

Note that no user has ever complained that Launchpad does not support
non-text messages. We should discard any message that has not meaningful
content.

Rules
-----

    * Iterate over all the parts of each message that mailman call hold()
      on. If there are no text parts with 1 or more non-white-space, raise
      Errors.DiscardMessage

QA
--

    * On staging, send an image only or html-only message to haibunku from
      a non-subscribed address.
      * Most spam messages purport to be from the list itself, eg
        "spam" <email address hidden>.
      * Verify the message does not appear in the moderation queue.
    * On staging send a normal email (with a text part) from the same address.
      * Verify that the message is in the moderation queue.
    * On staging, send an image only or html-only message to haibunku from
      your real address (assuming you are a list member)
      * Verify the message arrives to the list members.

Lint
----

Linting changed files:
  lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py
  lib/lp/services/mailman/doc/postings.txt

Test
----

    * lib/lp/services/mailman/doc/postings.txt
      * Revised the large message size test to look like real examples.
        Barry and I felt that these tests looked like spam according to
        the new rule and would probably fail when the code is added.
      * Added tests to verify that list members can send non-text messages,
        but such messages from non-members are discarded.

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

    * lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py
      * Added a guard to the hold() function to ensure that non-text
        messages are discarded.

[Henning Eggers (henninge)]

Hi Curtis
thank you for this branch and making moderators lifes easier!

Pleae find my commengs in-line.

Am 06.04.2010 16:45, schrieb Curtis Hovey:
> Prevent empty messages from entering the moderation queue
> ----------------------------------------------------------
>
> Launchpad only supports text/plain email messages, but mailman is asking
> users to moderate text/html or image/* messages. Moderators see a subject with
> no body. Besides the insanity of this case, there are two bugs that directly
> relate to these messages:
>
> Bug 498741 [spam offered for moderation on a ML]
> More that 99% of all spam in the moderation queue have no body.
> Bug 498740 [timeout trying to moderate a mailing list with many messages]
> The number of spam messages causes timeouts.

Too bugs in one branch? Nice hunting! ;-)

>
> Note that no user has ever complained that Launchpad does not support
> non-text messages. We should discard any message that has not meaningful
> content.
>
>
> Rules
> -----
>
> * Iterate over all the parts of each message that mailman call hold()
> on. If there are no text parts with 1 or more non-white-space, raise
> Errors.DiscardMessage

I do not know the inner workings of the Mailman code but I trust that you and
Barry came up with The Right Thing here ... ;-)

>
>
> QA
> --
>
> * On staging, send an image only or html-only message to haibunku from
> a non-subscribed address.
> * Most spam messages purport to be from the list itself, eg
> "spam"<email address hidden>.
> * Verify the message does not appear in the moderation queue.
> * On staging send a normal email (with a text part) from the same address.
> * Verify that the message is in the moderation queue.
> * On staging, send an image only or html-only message to haibunku from
> your real address (assuming you are a list member)
> * Verify the message arrives to the list members.

Only question I have here is how to send emails on staging?

>
>
> Lint
> ----
>
> Linting changed files:
> lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py
> lib/lp/services/mailman/doc/postings.txt
>
>
>
> Test
> ----
>
> * lib/lp/services/mailman/doc/postings.txt
> * Revised the large message size test to look like real examples.
> Barry and I felt that these tests looked like spam according to
> the new rule and would probably fail when the code is added.
> * Added tests to verify that list members can send non-text messages,
> but such messages from non-members are discarded.
>
>
> Implementation
> --------------
>
> * lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py
> * Added a guard to the hold() function to ensure that non-text
> messages are discarded.
>

I am not familiar with the overall structure of the file but wouldn't it have
been nicer to put that guard into it's own function?

> === modified file 'lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py'
> --- lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py 2009-10-01 15:36:20 +0000
> +++ li...

review: Approve (code)

[Curtis Hovey (sinzui)]

Hi Henning.

Thanks for the review.

On Tue, 2010-04-06 at 15:51 +0000, Henning Eggers wrote:
> Review: Approve code
...

> > Rules
> > -----
> >
> > * Iterate over all the parts of each message that mailman call hold()
> > on. If there are no text parts with 1 or more non-white-space, raise
> > Errors.DiscardMessage
>
> I do not know the inner workings of the Mailman code but I trust that you and
> Barry came up with The Right Thing here ... ;-)

Yes. I was perplexed by how to locate a text/plain part reliably. Barry
pointed me to the iterators.

> >
> >
> > QA
> > --
> >
> > * On staging, send an image only or html-only message to haibunku from
> > a non-subscribed address.
> > * Most spam messages purport to be from the list itself, eg
> > "spam"<email address hidden>.
> > * Verify the message does not appear in the moderation queue.
> > * On staging send a normal email (with a text part) from the same address.
> > * Verify that the message is in the moderation queue.
> > * On staging, send an image only or html-only message to haibunku from
> > your real address (assuming you are a list member)
> > * Verify the message arrives to the list members.
>
> Only question I have here is how to send emails on staging?

Staging has a separate mailman. The team page lists the staging mailman
address. Just send the messages to the listed address. Barry and I can
see the messages that go to haibunku.

...

> > Implementation
> > --------------
> >
> > * lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py
> > * Added a guard to the hold() function to ensure that non-text
> > messages are discarded.
> >
>
> I am not familiar with the overall structure of the file but wouldn't it have
> been nicer to put that guard into it's own function?

I like your suggestion.

> > === modified file 'lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py'
> > --- lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py 2009-10-01 15:36:20 +0000
> > +++ lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py 2010-04-06 14:45:42 +0000
...

> > @@ -73,6 +74,18 @@
> > syslog('vette',
> > 'Discarding duplicate held message-id: %s', message_id)
> > raise Errors.DiscardMessage
> > + # Discard messages without text content since there will be nothing to
> > + # moderate. Most of these messages are spam.
> > + has_content = False
> > + for part in typed_subpart_iterator(msg, 'text'):
> > + if part.get_content_subtype() == 'plain':
> > + if len(part.get_payload().strip()) > 0:
> > + has_content = True
> > + break
>
> Yes, at least this loop could be put in a function:
>
> def is_message_empty(msg):
> for part in typed_subpart_iterator(msg, 'text'):
> if part.get_content_subtype() == 'plain':
> if len(part.get_payload().strip()) > 0:
> return False
> return True
>
> > + if not has_content:
>
> if is_message_empty(msg):

Yes. That is much better.

> > + syslog('vette',
> Not ...

=== modified file 'lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py'
--- lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py	2009-10-01 15:36:20 +0000
+++ lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py	2010-04-06 17:22:43 +0000
@@ -6,6 +6,7 @@
 
 import xmlrpclib
 
+from email.iterators import typed_subpart_iterator
 from email.Utils import formatdate, make_msgid
 
 # pylint: disable-msg=F0401
@@ -40,6 +41,15 @@
     hold(mlist, msg, msgdata, 'Not subscribed')
 
 
+def is_message_empty(msg):
+    """Is the message missing a text/plain part with content?"""
+    for part in typed_subpart_iterator(msg, 'text'):
+        if part.get_content_subtype() == 'plain':
+            if len(part.get_payload().strip()) > 0:
+                return False
+    return True
+
+
 def hold(mlist, msg, msgdata, annotation):
     """Hold the message in both Mailman and Launchpad.
 
@@ -73,6 +83,12 @@
         syslog('vette',
                'Discarding duplicate held message-id: %s', message_id)
         raise Errors.DiscardMessage
+    # Discard messages without text content since there will be nothing to
+    # moderate. Most of these messages are spam.
+    if is_message_empty(msg):
+        syslog('vette',
+               'Discarding text-less message-id: %s', message_id)
+        raise Errors.DiscardMessage
     holds[message_id] = request_id
     # In addition to Message-ID, the librarian requires a Date header.
     if 'date' not in msg:

=== modified file 'lib/lp/services/mailman/doc/postings.txt'
--- lib/lp/services/mailman/doc/postings.txt	2009-10-19 16:21:21 +0000
+++ lib/lp/services/mailman/doc/postings.txt	2010-04-06 17:22:43 +0000
@@ -845,13 +845,19 @@
     >>> config.mailman.soft_max_size
     40000
 
-    >>> from email.MIMENonMultipart import MIMENonMultipart
-    >>> big_message = MIMENonMultipart('application', 'octet-stream')
-    >>> big_message.set_payload('\n'.join(['x' * 50] * 1000))
+    >>> 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')
@@ -888,12 +894,15 @@
     >>> config.mailman.hard_max_size
     1000000
 
-    >>> huge_message = MIMENonMultipart('application', 'octet-stream')
-    >>> huge_message.set_payload('\n'.join(['x' * 50] * 20000))
+    >>> 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')
@@ -920,7 +929,7 @@
     ...
     ... Watch out for badgers! \xa9
     ... """)
-    
+
 However, because Cate is not a member of the mailing list, her message gets
 held for moderator approval.
 
@@ -931,3 +940,37 @@
         ...
         Itest One <[e-mail]>
         New mailing list message requiring approval for Itest One
+
+
+Non-text messages
+=================
+
+List members can send non-text messages to agitate the other list members.
+
+    >>> html_message = MIMEText('<a><img /></a>.', 'html')
+    >>> html_message['From'] = '[e-mail]'
+    >>> html_message['To'] = '[e-mail]'
+    >>> html_message['Subject'] = "It's full of stars"
+    >>> html_message['Message-ID'] = '<loggerhead>'
+    >>> inject('itest-one', html_message.as_string())
+
+    >>> smtpd_watcher.wait_for_mbox_delivery('loggerhead')
+    >>> messages = list(smtpd)
+    >>> for message in messages:
+    ...     print message['Subject']
+    [Itest-one] It's full of stars
+    [Itest-one] It's full of stars
+
+Messages without non-empty text/plain parts from non-list members are rejected
+because launchpad does not store non-text messages for moderation.
+
+    >>> spam_message = MIMEText('<a><img /></a>.', 'html')
+    >>> spam_message['From'] = '[e-mail]'
+    >>> spam_message['To'] = '[e-mail]'
+    >>> spam_message['Subject'] = 'get drugs'
+    >>> spam_message['Message-ID'] = '<ocelot>'
+    >>> inject('itest-one', spam_message.as_string())
+
+    >>> vette_watcher.wait_for_discard('ocelot')
+    >>> print_message_summaries()
+    Number of messages: 0