launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01120
[Merge] lp:~sinzui/launchpad/discard-list-spam-1 into lp:launchpad/devel
Curtis Hovey has proposed merging lp:~sinzui/launchpad/discard-list-spam-1 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#640696 Messages from the list in the moderation queue are spam
https://bugs.launchpad.net/bugs/640696
This is my branch to discard messages the claim to be from the list.
This work is very simple, less than 20 lines of code and test, but we need
a reliable test harness first.
lp:~sinzui/launchpad/discard-list-spam-1
Diff size: 459
Launchpad bug:
https://bugs.launchpad.net/bugs/640696
Test command: ./bin/test -vv \
-t test_lpmoderate -t lp/registry/.*mailinglist.*xmlrpc
Pre-implementation: barry
Target release: 10.10
Discard messages the claim to be from the list
-----------------------------------------------
Mailman accepts messages claiming to be from the list. Launchpad monkey-patch
rules place non-subscriber emails with Lp registered address in the moderation
queue. However, the list email address is not a user, and it is controlled by
Launchpad--no user should ever use this address to send a message to the list.
Thus any message that enters lpmoderation.hold() that has the same email
address as the list (eg mlist.address in msg['From'] ) should raise
Errors.DiscardMessage.
Rules
-----
* Add a rule to lpmoderate to discard email that claim to be from
the list's address.
* Introduce a unittest for the lpmoderate module to isolate the rules
under test because the smtp -> mailman -> monkeypatch -> xmlrpc -> Lp
integration tests timeout most of the time. The MailmanLayer is not
run by the testrunner unless it is specified by the engineer. We need
an alternate test mechanism that avoids the latency of smtp, mailman,
and xmlrpc.
QA
--
* Send a message a list on staging using the team's list address as your
from address.
* Verify the message does not arrive in the moderation queue
Lint
----
Linting changed files:
lib/lp/registry/tests/mailinglists_helper.py
lib/lp/registry/tests/test_doc.py
lib/lp/services/mailman/doc/postings.txt
lib/lp/services/mailman/monkeypatches/lpmoderate.py
lib/lp/services/mailman/monkeypatches/xmlrpcrunner.py
lib/lp/services/mailman/testing/__init__.py
lib/lp/services/mailman/tests/test_lpmoderate.py
Test
----
Moved MailingListXMLRPCTestProxy from test_doc.py to mailinglists_helper.py
so that the test class could be reused
* lib/lp/registry/tests/mailinglists_helper.py
* lib/lp/registry/tests/test_doc.py
Created MailmanTestCase that can create mailman counterpart objects for
Launchpad. This test harness also replaces the mailman api proxy server
with MailingListXMLRPCTestProxy which is a wrapper arround the real view.
These test can run on the DatabaseFunctionalLayer, and they run very quickly.
* lib/lp/services/mailman/testing/__init__.py
Wrote a unittest for lpmoderate that replaces the moderate tests from
postings.txt. Added a test to verify that messages that claim to be from
the list are discarded.
* lib/lp/services/mailman/tests/test_lpmoderate.py
Removed redundant moderation tests.
* lib/lp/services/mailman/doc/postings.txt
Implementation
--------------
* lib/lp/services/mailman/monkeypatches/lpmoderate.py
* Added a rule to discard messages that claim to be from the list.
* Refactored the code to get a proxy server so that the method could
be tested.
* lib/lp/services/mailman/monkeypatches/xmlrpcrunner.py
* Added a function to get the proxy server to make testing easy.
--
https://code.launchpad.net/~sinzui/launchpad/discard-list-spam-1/+merge/36016
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/discard-list-spam-1 into lp:launchpad/devel.
=== modified file 'lib/lp/registry/tests/mailinglists_helper.py'
--- lib/lp/registry/tests/mailinglists_helper.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/mailinglists_helper.py 2010-09-20 14:21:08 +0000
@@ -25,6 +25,7 @@
from canonical.config import config
from canonical.database.sqlbase import flush_database_updates
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.xmlrpc import MailingListAPIView
from lp.registry.interfaces.mailinglist import (
IMailingListSet,
IMessageApprovalSet,
@@ -274,3 +275,29 @@
mailman = MailmanStub()
+
+
+class MailingListXMLRPCTestProxy(MailingListAPIView):
+ """A low impedance test proxy for code that uses MailingListAPIView."""
+
+ @fault_catcher
+ def getPendingActions(self):
+ return super(MailingListXMLRPCTestProxy, self).getPendingActions()
+
+ @fault_catcher
+ def reportStatus(self, statuses):
+ return super(MailingListXMLRPCTestProxy, self).reportStatus(statuses)
+
+ @fault_catcher
+ def getMembershipInformation(self, teams):
+ return super(
+ MailingListXMLRPCTestProxy, self).getMembershipInformation(teams)
+
+ @fault_catcher
+ def isLaunchpadMember(self, address):
+ return super(MailingListXMLRPCTestProxy, self).isLaunchpadMember(
+ address)
+
+ @fault_catcher
+ def isTeamPublic(self, team_name):
+ return super(MailingListXMLRPCTestProxy, self).isTeamPublic(team_name)
=== modified file 'lib/lp/registry/tests/test_doc.py'
--- lib/lp/registry/tests/test_doc.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_doc.py 2010-09-20 14:21:08 +0000
@@ -40,32 +40,8 @@
def mailingListXMLRPCInternalSetUp(test):
setUp(test)
- # Use the direct API view instance, not retrieved through the component
- # architecture. Don't use ServerProxy. We do this because it's easier to
- # debug because when things go horribly wrong, you see the errors on
- # stdout instead of in an OOPS report living in some log file somewhere.
- from canonical.launchpad.xmlrpc import MailingListAPIView
- class ImpedenceMatchingView(MailingListAPIView):
- @mailinglists_helper.fault_catcher
- def getPendingActions(self):
- return super(ImpedenceMatchingView, self).getPendingActions()
- @mailinglists_helper.fault_catcher
- def reportStatus(self, statuses):
- return super(ImpedenceMatchingView, self).reportStatus(statuses)
- @mailinglists_helper.fault_catcher
- def getMembershipInformation(self, teams):
- return super(
- ImpedenceMatchingView, self).getMembershipInformation(teams)
- @mailinglists_helper.fault_catcher
- def isLaunchpadMember(self, address):
- return super(ImpedenceMatchingView, self).isLaunchpadMember(
- address)
- @mailinglists_helper.fault_catcher
- def isTeamPublic(self, team_name):
- return super(ImpedenceMatchingView, self).isTeamPublic(team_name)
- # Expose in the doctest's globals, the view as the thing with the
- # IMailingListAPI interface. Also expose the helper functions.
- mailinglist_api = ImpedenceMatchingView(context=None, request=None)
+ mailinglist_api = mailinglists_helper.MailingListXMLRPCTestProxy(
+ context=None, request=None)
test.globs['mailinglist_api'] = mailinglist_api
test.globs['commit'] = transaction.commit
@@ -189,7 +165,7 @@
def test_suite():
suite = build_test_suite(here, special, layer=DatabaseFunctionalLayer)
- launchpadlib_path = os.path.join(os.path.pardir, 'doc', 'launchpadlib')
+ launchpadlib_path = os.path.join(os.path.pardir, 'doc', 'launchpadlib')
lplib_suite = build_doctest_suite(here, launchpadlib_path,
layer=DatabaseFunctionalLayer)
suite.addTest(lplib_suite)
=== modified file 'lib/lp/services/mailman/doc/postings.txt'
--- lib/lp/services/mailman/doc/postings.txt 2010-04-06 17:19:55 +0000
+++ lib/lp/services/mailman/doc/postings.txt 2010-09-20 14:21:08 +0000
@@ -908,69 +908,3 @@
>>> vette_watcher.wait_for_discard('quahog')
>>> print_message_summaries()
Number of messages: 0
-
-
-Non-ascii messages
-==================
-
-Cate joins Launchpad and posts a message containing non-ascii characters to
-the mailing list.
-
- >>> login('admin@xxxxxxxxxxxxx')
- >>> cate = factory.makePersonByName('Cate')
- >>> logout()
- >>> transaction.commit()
-
- >>> inject('itest-one', """\
- ... From: Cate \xa9 Person <cperson@xxxxxxxxxxx>
- ... To: team-one@xxxxxxxxxxxxxxxxxxx
- ... Subject: \xa9 Badgers!
- ... Message-ID: <racoon>
- ...
- ... Watch out for badgers! \xa9
- ... """)
-
-However, because Cate is not a member of the mailing list, her message gets
-held for moderator approval.
-
- >>> vette_watcher.wait_for_hold('itest-one', 'racoon')
- >>> print_message_summaries()
- Number of messages: 1
- bounces@xxxxxxxxxxxxx
- ...
- Itest One <noreply@xxxxxxxxxxxxx>
- 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'] = 'anne.person@xxxxxxxxxxx'
- >>> html_message['To'] = 'itest-one@xxxxxxxxxxxxxxxxxxx'
- >>> 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'] = 'itest-one@xxxxxxxxxxxxxxxxxxx'
- >>> spam_message['To'] = 'itest-one@xxxxxxxxxxxxxxxxxxx'
- >>> 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
=== modified file 'lib/lp/services/mailman/monkeypatches/lpmoderate.py'
--- lib/lp/services/mailman/monkeypatches/lpmoderate.py 2010-08-20 20:31:18 +0000
+++ lib/lp/services/mailman/monkeypatches/lpmoderate.py 2010-09-20 14:21:08 +0000
@@ -11,11 +11,8 @@
)
import xmlrpclib
-# pylint: disable-msg=F0401
-from Mailman import (
- Errors,
- mm_cfg,
- )
+from Mailman import Errors
+from Mailman.Queue import XMLRPCRunner
from Mailman.Logging.Syslog import syslog
@@ -87,6 +84,15 @@
syslog('vette',
'Discarding duplicate held message-id: %s', message_id)
raise Errors.DiscardMessage
+ # Discard messages that claim to be from the list itself because Mailman's
+ # internal handlers did not approve the message before it arrived at this
+ # step--these messages are forgeries.
+ list_address = mlist.getListAddress()
+ for sender in msg.get_senders():
+ if list_address == sender:
+ syslog('vette',
+ 'Discarding forged 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):
@@ -98,7 +104,7 @@
if 'date' not in msg:
msg['Date'] = formatdate()
# Store the message in the librarian.
- proxy = xmlrpclib.ServerProxy(mm_cfg.XMLRPC_URL)
+ proxy = XMLRPCRunner.get_mailing_list_api_proxy()
# This will fail if we can't talk to Launchpad. That's okay though
# because Mailman's IncomingRunner will re-queue the message and re-start
# processing at this handler.
=== modified file 'lib/lp/services/mailman/monkeypatches/xmlrpcrunner.py'
--- lib/lp/services/mailman/monkeypatches/xmlrpcrunner.py 2010-08-20 20:31:18 +0000
+++ lib/lp/services/mailman/monkeypatches/xmlrpcrunner.py 2010-09-20 14:21:08 +0000
@@ -4,6 +4,7 @@
"""XMLRPC runner for querying Launchpad."""
__all__ = [
+ 'get_mailing_list_api_proxy',
'XMLRPCRunner',
]
@@ -40,10 +41,14 @@
# Mapping from modifiable attributes as they are named by the xmlrpc
# interface, to the attribute names on the MailList instances.
attrmap = {
- 'welcome_message' : 'welcome_msg',
+ 'welcome_message': 'welcome_msg',
}
+def get_mailing_list_api_proxy():
+ return xmlrpclib.ServerProxy(mm_cfg.XMLRPC_URL)
+
+
class MailmanErrorUtility(ErrorReportingUtility):
"""An error utility that for the MailMan xmlrpc process."""
@@ -96,7 +101,7 @@
# is designed for.
self._kids = {}
self._stop = False
- self._proxy = xmlrpclib.ServerProxy(mm_cfg.XMLRPC_URL)
+ self._proxy = get_mailing_list_api_proxy()
# Ensure that the log file exists, mostly for the test suite.
syslog('xmlrpc', 'XMLRPC runner starting')
=== modified file 'lib/lp/services/mailman/testing/__init__.py'
--- lib/lp/services/mailman/testing/__init__.py 2008-07-10 19:46:37 +0000
+++ lib/lp/services/mailman/testing/__init__.py 2010-09-20 14:21:08 +0000
@@ -0,0 +1,92 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+"""Test helpers for mailman integration."""
+
+__metaclass__ = type
+__all__ = []
+
+import email
+from email.mime.text import MIMEText
+import os
+import shutil
+
+from Mailman import (
+ MailList,
+ Message,
+ mm_cfg,
+ )
+from Mailman.Queue import XMLRPCRunner
+
+from canonical.testing import DatabaseFunctionalLayer
+
+from lp.registry.tests.mailinglists_helper import MailingListXMLRPCTestProxy
+from lp.testing import TestCaseWithFactory
+
+
+def get_mailing_list_api_test_proxy():
+ return MailingListXMLRPCTestProxy(context=None, request=None)
+
+
+class MailmanTestCase(TestCaseWithFactory):
+ """TestCase with factory and mailman support."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(MailmanTestCase, self).setUp()
+ # Replace the xmlrpc proxy with a fast wrapper of the real view.
+ self.original_get_proxy = XMLRPCRunner.get_mailing_list_api_proxy
+ XMLRPCRunner.get_mailing_list_api_proxy = (
+ get_mailing_list_api_test_proxy)
+
+ def tearDown(self):
+ super(MailmanTestCase, self).tearDown()
+ # Restore the xmlrpc proxy.
+ XMLRPCRunner.get_mailing_list_api_proxy = self.original_get_proxy
+ self.cleanMailmanList(self.mm_list)
+
+ def makeMailmanList(self, lp_mailing_list):
+ # This utility is based on mailman/tests/TestBase.py.
+ mlist = MailList.MailList()
+ team = lp_mailing_list.team
+ owner_email = team.teamowner.preferredemail.email
+ mlist.Create(team.name, owner_email, 'password')
+ mlist.host_name = 'lists.launchpad.dev'
+ mlist.web_page_url = 'http://lists.launchpad.dev/mailman/'
+ mlist.Save()
+ mlist.addNewMember(owner_email)
+ return mlist
+
+ def cleanMailmanList(self, mlist):
+ # This utility is based on mailman/tests/TestBase.py.
+ mlist.Unlock()
+ listname = mlist.internal_name()
+ paths = [
+ 'lists/%s',
+ 'archives/private/%s',
+ 'archives/private/%s.mbox',
+ 'archives/public/%s',
+ 'archives/public/%s.mbox',
+ ]
+ for dirtmpl in paths:
+ list_dir = os.path.join(mm_cfg.VAR_PREFIX, dirtmpl % listname)
+ if os.path.islink(list_dir):
+ os.unlink(list_dir)
+ elif os.path.isdir(list_dir):
+ shutil.rmtree(list_dir)
+
+ def makeMailmanMessage(self, mm_list, sender, subject, content,
+ mime_type='plain', attachment=None):
+ # Make a Mailman Message.Message.
+ if isinstance(sender, (list, tuple)):
+ sender = ', '.join(sender)
+ message = MIMEText(content, mime_type)
+ message['from'] = sender
+ message['to'] = mm_list.getListAddress()
+ message['subject'] = subject
+ message['message-id'] = self.getUniqueString()
+ 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
=== added file 'lib/lp/services/mailman/tests/test_lpmoderate.py'
--- lib/lp/services/mailman/tests/test_lpmoderate.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/mailman/tests/test_lpmoderate.py 2010-09-20 14:21:08 +0000
@@ -0,0 +1,113 @@
+# Copyright 20010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+"""Test the lpmoderate monekypatches"""
+
+from __future__ import with_statement
+
+__metaclass__ = type
+__all__ = []
+
+from Mailman import Errors
+from Mailman.Handlers import LPModerate
+
+from canonical.testing import LaunchpadFunctionalLayer
+from lp.services.mailman.testing import MailmanTestCase
+
+
+class TestLPModerateTestCase(MailmanTestCase):
+ """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.
+ """
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestLPModerateTestCase, self).setUp()
+ self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
+ 'team-1', 'team-1-owner')
+ self.mm_list = self.makeMailmanList(self.mailing_list)
+ self.lp_user = self.factory.makePerson()
+ self.lp_user_email = self.lp_user.preferredemail.email
+
+ def tearDown(self):
+ super(TestLPModerateTestCase, self).tearDown()
+ self.cleanMailmanList(self.mm_list)
+
+ def test_process_message_from_preapproved(self):
+ # Any message mark with approval will silently complete the process.
+ message = self.makeMailmanMessage(
+ self.mm_list, self.lp_user_email, 'subject', 'any content.')
+ msg_data = dict(approved=True)
+ silence = LPModerate.process(self.mm_list, message, msg_data)
+ self.assertEqual(None, silence)
+
+ def test_process_message_from_subscriber(self):
+ # Messages from subscribers silently complete the process.
+ subscriber_email = self.team.teamowner.preferredemail.email
+ message = self.makeMailmanMessage(
+ self.mm_list, subscriber_email, 'subject', 'any content.')
+ msg_data = {}
+ silence = LPModerate.process(self.mm_list, message, msg_data)
+ self.assertEqual(None, silence)
+
+ def test_process_message_from_lp_user_held_for_moderation(self):
+ # Messages from Launchpad users are held for moderation.
+ message = self.makeMailmanMessage(
+ self.mm_list, self.lp_user_email, 'subject', 'content')
+ msg_data = {}
+ args = (self.mm_list, message, msg_data)
+ self.assertRaises(
+ Errors.HoldMessage, LPModerate.process, *args)
+ self.assertEqual(1, self.mailing_list.getReviewableMessages().count())
+
+ def test_process_message_with_non_ascii_from_lp_user_held(self):
+ # Non-ascii messages can be held for moderation.
+ non_ascii_email = 'I \xa9 M <%s>' % self.lp_user_email.encode('ascii')
+ message = self.makeMailmanMessage(
+ self.mm_list, non_ascii_email, 'subject \xa9', 'content \xa9')
+ msg_data = {}
+ args = (self.mm_list, message, msg_data)
+ self.assertRaises(
+ Errors.HoldMessage, LPModerate.process, *args)
+ self.assertEqual(1, self.mailing_list.getReviewableMessages().count())
+
+ def test_process_duplicate_message_discarded(self):
+ # Messages are discarded is they are already held for moderation.
+ message = self.makeMailmanMessage(
+ self.mm_list, self.lp_user_email, 'subject', 'content')
+ self.mm_list.held_message_ids = {message['message-id']: message}
+ msg_data = {}
+ args = (self.mm_list, message, msg_data)
+ self.assertRaises(
+ Errors.DiscardMessage, LPModerate.process, *args)
+ self.assertEqual(0, self.mailing_list.getReviewableMessages().count())
+
+ def test_process_empty_mesage_from_nonsubcriber_discarded(self):
+ # Messages from Launchpad users without text content are discarded.
+ spam_message = self.makeMailmanMessage(
+ self.mm_list, self.lp_user_email,
+ 'get drugs', '<a><img /></a>.', mime_type='html')
+ msg_data = dict(approved=False)
+ args = (self.mm_list, spam_message, msg_data)
+ self.assertRaises(
+ Errors.DiscardMessage, LPModerate.process, *args)
+ self.assertEqual(0, self.mailing_list.getReviewableMessages().count())
+
+ def test_process_message_from_list_discarded(self):
+ # Messages that claim to be from the list itself (not a subcriber) are
+ # discarded because Mailman's internal handlers did not set 'approve'
+ # in msg_data.
+ list_email = 'spammer <%s>' % self.mailing_list.address
+ message = self.makeMailmanMessage(
+ self.mm_list, list_email, 'subject', 'any content.')
+ msg_data = {}
+ args = (self.mm_list, message, msg_data)
+ self.assertRaises(
+ Errors.DiscardMessage, LPModerate.process, *args)
+ self.assertEqual(0, self.mailing_list.getReviewableMessages().count())
