Launchpad itself

Merge lp:~barry/launchpad/403606-expat into lp:launchpad

  1. 403606-expat
  2. Merge into devel
Proposed by Barry Warsaw
Status: Merged
Merged at revision: not available
Proposed branch: lp:~barry/launchpad/403606-expat
Merge into: lp:launchpad
Diff against target: 278 lines
9 files modified
lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py (+2/-1)
lib/canonical/launchpad/webapp/publication.py (+1/-1)
lib/canonical/launchpad/webapp/servers.py (+1/-0)
lib/canonical/launchpad/xmlrpc/mailinglist.py (+11/-2)
lib/lp/registry/doc/mailinglist-xmlrpc.txt (+7/-6)
lib/lp/registry/doc/message-holds-xmlrpc.txt (+50/-1)
lib/lp/registry/interfaces/mailinglist.py (+2/-2)
lib/lp/services/mailman/doc/create-lists.txt (+2/-2)
lib/lp/services/mailman/doc/postings.txt (+32/-0)
To merge this branch: bzr merge lp:~barry/launchpad/403606-expat
Reviewer Review Type Date Requested Status
Paul Hummer (community) code Approve
Canonical Launchpad Engineering Pending
Review via email: mp+12734@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Barry Warsaw (barry) wrote :
Download full text (6.5 KiB)

= Summary =

Bug 403606 is a fun one. For a long while now, we've been getting mysterious
OOPSes in the private XMLRPC server that Mailman talks too. Unfortunately,
Zope swallows all the information that would be useful to diagnose the
problem. OTOH, it never seemed to degrade the service, but it annoyed the
LOSAs and helping them out is always a good thing!

Now that 3.0 is out, it was time to look into this problem, so with the LOSAs
help, I instrumented the private server to gather the additional information.
Specifically, I added some code so that when the XMLRPC request failed, I
logged the entire request body. We ran that code overnight and it was obvious
what the problem was.

It turns out that IMailingListAPIView.holdMessage() is the culprit, and
specifically it's when Mailman tries to hold messages with non-ascii
characters in its headers or body. This makes sense as non-ascii must be
encoded over the wire for XMLRPC.

The downside is that all the messages in the overnight log that I could see
that triggered this were spam. That jives with my experience in Mailman (why
spammers would send at worst illegal or at best often incompatible messages is
beyond me, but there ya go). So the good news is that this should eliminate
or at least largely reduce the OOPSes we're seeing, but the bad news is that
it will probably increase the amount of spam list owners have to moderate.
That is a separate problem though, out of scope for this branch (and we have
ways of mitigating that).

== Proposed fix ==

On the XMLRPC client side in Mailman, wrap the message body texts in an
xmlrpc.Binary for transport over the wire. On the private XMLRPC server side,
unwrap the Binary to get the text bytes of the original message.

== Pre-implementation notes ==

I didn't have any discussions specifically about this fix, but had several
discussions with the LOSAs, Brad, and Francis about how to diagnose the
problem.

== Implementation details ==

A few things to note. On the server side, I do a type-test of the 'bytes'
argument to see whether to unwrap the Binary or not. The reason I don't do
this unconditionally is that it would require a rewrite of all the existing
tests, where most of the message bodies are pure-ascii. So while the client
always sends a Binary, most of the tests will still send strings.

I paid some tech debt along the way. Apologies for cluttering up the meat of
this change.

== Tests ==

% bin/test -vv -t mailinglist-xmlrpc.txt
% bin/test -vv --layer=MailmanLayer -t create-list -t postings

Note that the MailmanLayer tests are sometimes flaky and are not run by 'make
check' by default. Still, you should be able to get through them, though ping
me if you can't.

== Demo and Q/A ==

There are no user visible changes on this branch, but we'll QA it by watching
the daily OOPS reports.

= Launchpad lint =

These lint warnings are bogus.

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

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

Read more...

Revision history for this message
Paul Hummer (rockstar) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py'
2--- lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py 2009-06-25 05:30:52 +0000
3+++ lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py 2009-10-01 16:59:15 +0000
4@@ -82,7 +82,8 @@
5 # This will fail if we can't talk to Launchpad. That's okay though
6 # because Mailman's IncomingRunner will re-queue the message and re-start
7 # processing at this handler.
8- proxy.holdMessage(mlist.internal_name(), msg.as_string())
9+ proxy.holdMessage(mlist.internal_name(),
10+ xmlrpclib.Binary(msg.as_string()))
11 syslog('vette', 'Holding message for LP approval: %s', message_id)
12 # Raise this exception, signaling to the incoming queue runner that it is
13 # done processing this message, and should not send it through any further
14
15=== modified file 'lib/canonical/launchpad/webapp/publication.py'
16--- lib/canonical/launchpad/webapp/publication.py 2009-09-17 14:29:22 +0000
17+++ lib/canonical/launchpad/webapp/publication.py 2009-10-01 16:59:15 +0000
18@@ -482,7 +482,7 @@
19 pass
20
21 # Log a soft OOPS for DisconnectionErrors as per Bug #373837.
22- # We need to do this before we re-raise the excaptionsas a Retry.
23+ # We need to do this before we re-raise the exception as a Retry.
24 if isinstance(exc_info[1], DisconnectionError):
25 getUtility(IErrorReportingUtility).raising(exc_info, request)
26
27
28=== modified file 'lib/canonical/launchpad/webapp/servers.py'
29--- lib/canonical/launchpad/webapp/servers.py 2009-07-28 22:35:01 +0000
30+++ lib/canonical/launchpad/webapp/servers.py 2009-10-01 16:59:15 +0000
31@@ -1301,6 +1301,7 @@
32 """Request type for doing private XML-RPC in Launchpad."""
33 # For now, the same as public requests.
34
35+
36 # ---- Protocol errors
37
38 class ProtocolErrorRequest(LaunchpadBrowserRequest):
39
40=== modified file 'lib/canonical/launchpad/xmlrpc/mailinglist.py'
41--- lib/canonical/launchpad/xmlrpc/mailinglist.py 2009-06-25 05:30:52 +0000
42+++ lib/canonical/launchpad/xmlrpc/mailinglist.py 2009-10-01 16:59:15 +0000
43@@ -9,6 +9,8 @@
44 ]
45
46
47+import xmlrpclib
48+
49 from zope.component import getUtility
50 from zope.interface import implements
51 from zope.security.proxy import removeSecurityProxy
52@@ -222,10 +224,17 @@
53 return person.personal_standing in (PersonalStanding.GOOD,
54 PersonalStanding.EXCELLENT)
55
56- def holdMessage(self, team_name, text):
57+ def holdMessage(self, team_name, bytes):
58 """See `IMailingListAPIView`."""
59+ # For testing purposes, accept both strings and Binary instances. In
60+ # production, and as tested by 'bin/test--layer=MailmanLayer' bytes
61+ # will always be a Binary so that unencoded non-ascii characters in
62+ # the message can be safely passed across XMLRPC. For most tests
63+ # though it's much more convenient to just pass 8-bit strings.
64+ if isinstance(bytes, xmlrpclib.Binary):
65+ bytes = bytes.data
66 mailing_list = getUtility(IMailingListSet).get(team_name)
67- message = getUtility(IMessageSet).fromEmail(text)
68+ message = getUtility(IMessageSet).fromEmail(bytes)
69 mailing_list.holdMessage(message)
70 return True
71
72
73=== modified file 'lib/lp/registry/doc/mailinglist-xmlrpc.txt'
74--- lib/lp/registry/doc/mailinglist-xmlrpc.txt 2009-05-06 15:13:39 +0000
75+++ lib/lp/registry/doc/mailinglist-xmlrpc.txt 2009-10-01 16:59:15 +0000
76@@ -1,3 +1,4 @@
77+=====================================
78 XML-RPC integration for mailing lists
79 =====================================
80
81@@ -11,7 +12,7 @@
82
83
84 Registered lists
85-----------------
86+================
87
88 When there's nothing for Mailman to do, we'll just get an empty dictionary
89 back from the XMLRPC call.
90@@ -93,7 +94,7 @@
91
92
93 Creating the mailing lists
94---------------------------
95+==========================
96
97 Mailman will attempt to satisfy the requested creation. This will either
98 succeed or fail, and this status is communicated back to Launchpad. Let's say
99@@ -158,7 +159,7 @@
100
101
102 Deactivating mailing lists
103---------------------------
104+==========================
105
106 The other action that Mailman needs to take is to deactivate a list when the
107 team owner requests it. These also show up as pending actions when Mailman
108@@ -220,7 +221,7 @@
109
110
111 Modifying mailing lists
112------------------------
113+=======================
114
115 Activate mailing lists can also be modified. Currently the only modification
116 supported is changing the list's welcome message. Because deactivated and
117@@ -289,7 +290,7 @@
118
119
120 Error conditions
121-----------------
122+================
123
124 Although we control both ends of this XMLRPC interface, it might still be
125 possible to pass bad input, due to bugs and such. The XMLRPC interface is
126@@ -391,7 +392,7 @@
127
128
129 Public teams
130-------------
131+============
132
133 We provide a method to query whether or not a given team is public. That's
134 useful when deciding whether or not anonymous access to the team's mailing
135
136=== modified file 'lib/lp/registry/doc/message-holds-xmlrpc.txt'
137--- lib/lp/registry/doc/message-holds-xmlrpc.txt 2009-05-12 21:39:35 +0000
138+++ lib/lp/registry/doc/message-holds-xmlrpc.txt 2009-10-01 16:59:15 +0000
139@@ -1,3 +1,4 @@
140+=====================================
141 XMLRPC interface for holding messages
142 =====================================
143
144@@ -176,7 +177,7 @@
145
146
147 Private membership teams
148-------------------------
149+========================
150
151 When a private membership team has a mailing list, its messages can also be
152 held for moderator approval.
153@@ -220,3 +221,51 @@
154 >>> dispositions = mailinglist_api.getMessageDispositions()
155 >>> print_dispositions(dispositions)
156 <fourth-post> team-private accept
157+
158+
159+Non-ascii messages
160+==================
161+
162+Messages with non-ascii in their headers or bodies are not exactly legal (they
163+should be encoded) but do occur especially in spam. These messages can be
164+held for moderator approval too.
165+
166+ >>> spam_message = message_from_string("""\
167+ ... From: Anne \xa9 Person <anne.person@example.com>
168+ ... To: team-one@lists.launchpad.dev
169+ ... Subject: \xa9 Badgers!
170+ ... Message-ID: <fifth-post>
171+ ... Date: Fri, 01 Aug 2000 01:08:59 -0000
172+ ...
173+ ... Watch out for badgers! \xa9
174+ ... """)
175+
176+ >>> import xmlrpclib
177+ >>> mailinglist_api.holdMessage(
178+ ... 'team-one',
179+ ... xmlrpclib.Binary(spam_message.as_string()))
180+ True
181+ >>> commit()
182+
183+ >>> held_message_spam = message_set.getMessageByMessageID('<fifth-post>')
184+ >>> print held_message_spam.message_id
185+ <fifth-post>
186+ >>> print held_message_spam.posted_by.displayname
187+ Anne Person
188+
189+ >>> held_message_spam.posted_message.open()
190+ >>> try:
191+ ... print held_message_spam.posted_message.read()
192+ ... finally:
193+ ... held_message_spam.posted_message.close()
194+ From: Anne © Person <anne.person@example.com>
195+ To: team-one@lists.launchpad.dev
196+ Subject: © Badgers!
197+ Message-ID: <fifth-post>
198+ Date: Fri, 01 Aug 2000 01:08:59 -0000
199+ <BLANKLINE>
200+ Watch out for badgers! ©
201+ <BLANKLINE>
202+
203+ >>> held_message_spam.status
204+ <DBItem PostedMessageStatus.NEW, (0) New status>
205
206=== modified file 'lib/lp/registry/interfaces/mailinglist.py'
207--- lib/lp/registry/interfaces/mailinglist.py 2009-06-25 04:06:00 +0000
208+++ lib/lp/registry/interfaces/mailinglist.py 2009-10-01 16:59:15 +0000
209@@ -651,12 +651,12 @@
210 team.
211 """
212
213- def holdMessage(team_name, text):
214+ def holdMessage(team_name, bytes):
215 """Hold the message for approval though the Launchpad u/i.
216
217 :param team_name: The name of the team/mailing list that this message
218 was posted to.
219- :param text: The original text of the message.
220+ :param bytes: The original text of the message as bytes.
221 :return: True
222 """
223
224
225=== modified file 'lib/lp/services/mailman/doc/create-lists.txt'
226--- lib/lp/services/mailman/doc/create-lists.txt 2009-09-24 18:51:29 +0000
227+++ lib/lp/services/mailman/doc/create-lists.txt 2009-10-01 16:59:15 +0000
228@@ -15,8 +15,8 @@
229 >>> browser.getControl(name='field.subscriptionpolicy').displayValue = [
230 ... 'Open Team']
231 >>> browser.getControl('Create').click()
232- >>> browser.title
233- 'ITest One in Launchpad'
234+ >>> print browser.title
235+ ITest One in Launchpad
236
237 To request a mailing list, No Privileges Person navigates to the 'Configure
238 mailing list' page and registers their application for a mailing list.
239
240=== modified file 'lib/lp/services/mailman/doc/postings.txt'
241--- lib/lp/services/mailman/doc/postings.txt 2009-09-24 20:10:01 +0000
242+++ lib/lp/services/mailman/doc/postings.txt 2009-10-01 16:59:15 +0000
243@@ -899,3 +899,35 @@
244 >>> vette_watcher.wait_for_discard('quahog')
245 >>> print_message_summaries()
246 Number of messages: 0
247+
248+
249+Non-ascii messages
250+==================
251+
252+Cate joins Launchpad and posts a message containing non-ascii characters to
253+the mailing list.
254+
255+ >>> login('admin@canonical.com')
256+ >>> cate = factory.makePersonByName('Cate')
257+ >>> logout()
258+ >>> transaction.commit()
259+
260+ >>> inject('itest-one', """\
261+ ... From: Cate \xa9 Person <cperson@example.org>
262+ ... To: team-one@lists.launchpad.dev
263+ ... Subject: \xa9 Badgers!
264+ ... Message-ID: <racoon>
265+ ...
266+ ... Watch out for badgers! \xa9
267+ ... """)
268+
269+However, because Cate is not a member of the mailing list, her message gets
270+held for moderator approval.
271+
272+ >>> vette_watcher.wait_for_hold('itest-one', 'racoon')
273+ >>> print_message_summaries()
274+ Number of messages: 1
275+ bounces@canonical.com
276+ ...
277+ Itest One <noreply@launchpad.net>
278+ New mailing list message requiring approval for Itest One