Launchpad itself

Merge lp:~thumper/launchpad/strip-email-attachment-path into lp:launchpad

  1. strip-email-attachment-path
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 12563
Proposed branch: lp:~thumper/launchpad/strip-email-attachment-path
Merge into: lp:launchpad
Diff against target: 66 lines (+30/-1)
2 files modified
lib/canonical/launchpad/database/message.py (+3/-1)
lib/canonical/launchpad/database/tests/test_message.py (+27/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/strip-email-attachment-path
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Ian Booth (community) *code Approve
Review via email: mp+52159@code.launchpad.net

Commit message

[r=lifeless,wallyworld][bug=221938] Strip any path information off email attachments when storing in the librarian.

Description of the change

Another relatively trivial email OOPS.

The attachment disposition may specify a path in the filename, but the librarian database table really doesn't like that, so we strip it off.

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

Looks good to me. Nice test.

review: Approve (*code)
Revision history for this message
Robert Collins (lifeless) wrote :

If you use the fake librarian you can do without the commit() and the test will be faster. This is optional but *highly* recommended.

review: Approve
Revision history for this message
Tim Penhey (thumper) wrote :

I got weird failures when attempting to use the fake librarian. Didn't dive too deep as trying to clear current work in progress. Still using the real librarian.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/database/message.py'
2--- lib/canonical/launchpad/database/message.py 2011-02-17 17:02:54 +0000
3+++ lib/canonical/launchpad/database/message.py 2011-03-08 21:52:53 +0000
4@@ -14,7 +14,6 @@
5 'UserToUserEmail',
6 ]
7
8-
9 from cStringIO import StringIO as cStringIO
10 from datetime import datetime
11 import email
12@@ -29,6 +28,7 @@
13 parsedate_tz,
14 )
15 from operator import attrgetter
16+import os.path
17
18 from lazr.config import as_timedelta
19 from lazr.enum import (
20@@ -476,6 +476,8 @@
21 sequence += 1
22 else:
23 filename = part.get_filename() or 'unnamed'
24+ # Strip off any path information.
25+ filename = os.path.basename(filename)
26 # Note we use the Content-Type header instead of
27 # part.get_content_type() here to ensure we keep
28 # parameters as sent. If Content-Type is None we default
29
30=== modified file 'lib/canonical/launchpad/database/tests/test_message.py'
31--- lib/canonical/launchpad/database/tests/test_message.py 2010-11-06 12:50:22 +0000
32+++ lib/canonical/launchpad/database/tests/test_message.py 2011-03-08 21:52:53 +0000
33@@ -114,6 +114,33 @@
34 transaction.commit()
35 self.assertEqual('This is the diff, honest.', diff.blob.read())
36
37+ def test_fromEmail_strips_attachment_paths(self):
38+ # Build a simple multipart message with a plain text first part
39+ # and an text/x-diff attachment.
40+ sender = self.factory.makePerson()
41+ msg = MIMEMultipart()
42+ msg['Message-Id'] = make_msgid('launchpad')
43+ msg['Date'] = formatdate()
44+ msg['To'] = 'to@example.com'
45+ msg['From'] = sender.preferredemail.email
46+ msg['Subject'] = 'Sample'
47+ msg.attach(MIMEText('This is the body of the email.'))
48+ attachment = Message()
49+ attachment.set_payload('This is the diff, honest.')
50+ attachment['Content-Type'] = 'text/x-diff'
51+ attachment['Content-Disposition'] = (
52+ 'attachment; filename="/tmp/foo/review.diff"')
53+ msg.attach(attachment)
54+ # Now create the message from the MessageSet.
55+ message = MessageSet().fromEmail(msg.as_string())
56+ text, diff = message.chunks
57+ self.assertEqual('This is the body of the email.', text.content)
58+ self.assertEqual('review.diff', diff.blob.filename)
59+ self.assertEqual('text/x-diff', diff.blob.mimetype)
60+ # Need to commit in order to read back out of the librarian.
61+ transaction.commit()
62+ self.assertEqual('This is the diff, honest.', diff.blob.read())
63+
64
65 class TestMessageJob(TestCaseWithFactory):
66 """Tests for MessageJob."""