launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/private-bug-attachments into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/private-bug-attachments/+merge/179624

Add a new restricted flag to MessageSet.fromEmail() that will make certain that LFAs that are created from parsing the email are uploaded to the restricted librarian if it's true. Since fromEmail optionally takes an LFA as an filealias argument, if restricted is true, the email is re-uploaded to the restricted librarian, and since the old LFA is not referenced, it should be cleaned up by the librarian gc.

Do a bunch of drive-by cleanup, mostly whitespace and a little bit of refactoring.
-- 
https://code.launchpad.net/~stevenk/launchpad/private-bug-attachments/+merge/179624
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/private-bug-attachments into lp:launchpad.

=== modified file 'lib/lp/bugs/mail/handler.py'
--- lib/lp/bugs/mail/handler.py	2012-09-27 20:38:32 +0000
+++ lib/lp/bugs/mail/handler.py	2013-08-12 04:28:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Handle incoming Bugs email."""
@@ -399,24 +399,19 @@
         """Append the message text to the bug comments."""
         messageset = getUtility(IMessageSet)
         message = messageset.fromEmail(
-            signed_msg.as_string(),
-            owner=getUtility(ILaunchBag).user,
-            filealias=filealias,
-            parsed_message=signed_msg,
-            fallback_parent=bug.initial_message)
-        # If the new message's parent is linked to
-        # a bug watch we also link this message to
-        # that bug watch.
-        bug_message_set = getUtility(IBugMessageSet)
-        parent_bug_message = (
-            bug_message_set.getByBugAndMessage(bug, message.parent))
+            signed_msg.as_string(), owner=getUtility(ILaunchBag).user,
+            filealias=filealias, parsed_message=signed_msg,
+            fallback_parent=bug.initial_message, restricted=bug.private)
+        # If the new message's parent is linked to a bug watch we also link
+        # this message to that bug watch.
+        parent_bug_message = getUtility(IBugMessageSet).getByBugAndMessage(
+            bug, message.parent)
         if (parent_bug_message is not None and
             parent_bug_message.bugwatch):
             bug_watch = parent_bug_message.bugwatch
         else:
             bug_watch = None
-        bugmessage = bug.linkMessage(
-            message, bug_watch)
+        bugmessage = bug.linkMessage(message, bug_watch)
         notify(ObjectCreatedEvent(bugmessage))
         return message
 

=== modified file 'lib/lp/services/mail/helpers.py'
--- lib/lp/services/mail/helpers.py	2012-09-27 20:45:31 +0000
+++ lib/lp/services/mail/helpers.py	2013-08-12 04:28:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -225,7 +225,7 @@
         raise IncomingEmailError(error_message)
 
 
-def save_mail_to_librarian(raw_mail):
+def save_mail_to_librarian(raw_mail, restricted=False):
     """Save the message to the librarian.
 
     It can be referenced from errors, and also accessed by code that needs to
@@ -236,9 +236,8 @@
     # be guessable for example.
     file_name = str(uuid1()) + '.txt'
     file_alias = getUtility(ILibraryFileAliasSet).create(
-            file_name,
-            len(raw_mail),
-            cStringIO(raw_mail), 'message/rfc822')
+        file_name, len(raw_mail), cStringIO(raw_mail), 'message/rfc822',
+        restricted=restricted)
     return file_alias
 
 

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2012-12-05 21:48:09 +0000
+++ lib/lp/services/mail/incoming.py	2013-08-12 04:28:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functions dealing with mails coming into Launchpad."""
@@ -391,8 +391,7 @@
     return report['id']
 
 
-def handleMail(trans=transaction,
-               signature_timestamp_checker=None):
+def handleMail(trans=transaction, signature_timestamp_checker=None):
 
     log = logging.getLogger('process-mail')
     mailbox = getUtility(IMailBox)
@@ -551,5 +550,4 @@
 
     handled = handler.process(mail, email_addr, file_alias)
     if not handled:
-        raise AssertionError(
-            "Handler found, but message was not handled")
+        raise AssertionError("Handler found, but message was not handled")

=== modified file 'lib/lp/services/messages/interfaces/message.py'
--- lib/lp/services/messages/interfaces/message.py	2013-01-07 02:40:55 +0000
+++ lib/lp/services/messages/interfaces/message.py	2013-08-12 04:28:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -134,34 +134,28 @@
         """Construct a Message from a text string and return it."""
 
     def fromEmail(email_message, owner=None, filealias=None,
-            parsed_message=None, fallback_parent=None, date_created=None):
+                  parsed_message=None, fallback_parent=None, date_created=None,
+                  restricted=False):
         """Construct a Message from an email message and return it.
 
-        `email_message` should be the original email as a string.
-
-        `owner` specifies the owner of the new Message. The default
-        is calculated using the From: or Reply-To: headers, and will raise
-        a UnknownSender error if they cannot be found.
-
-        `filealias` is the LibraryFileAlias of the raw email if it has
-        already been stuffed into the Librarian. Default is for this
-        method to stuff it into the Librarian for you. It should be an
-        ILibraryFileAlias.
-
-        `parsed_message` may be an email.Message.Message instance. If given,
-        it is used internally instead of building one from the raw
-        email_message. This is purely an optimization step, significant
-        in many places because the emails we are handling may contain huge
-        attachments and we should avoid reparsing them if possible.
-
-        'fallback_parent' can be specified if you want a parent to be
-        set, if no parent could be identified.
-
-        `date_created` may be a datetime, and can be specified if you
-        wish to force the created date for a message. This is
-        particularly useful when the email_message being passed might
-        not contain a Date field. Any Date field in the passed message
-        will be ignored in favour of the value of `date_created`.
+        :param email_message: The original email as a string.
+        :param owner: Specifies the owner of the new Message. The default
+            is calculated using the From: or Reply-To: headers, and will raise
+            a UnknownSender error if they cannot be found.
+        :param filealias: The `LibraryFileAlias` of the raw email if it has
+            already been uploaded to the Librarian.
+        :param parsed_message: An email.Message.Message instance. If given,
+            it is used internally instead of building one from the raw
+            email_message. This is purely an optimization step, significant
+            in many places because the emails we are handling may contain huge
+            attachments and we should avoid reparsing them if possible.
+        :param fallback_parent: The parent message if it could not be
+            identified.
+        :param date_created: Force a created date for the message. If
+            specified, the value in the Date field in the passed message will
+            be ignored.
+        :param restricted: If set, the `LibraryFileAlias` will be uploaded to
+            the restricted librarian.
 
         Callers may want to explicitly handle the following exceptions:
             * UnknownSender

=== modified file 'lib/lp/services/messages/model/message.py'
--- lib/lp/services/messages/model/message.py	2013-01-07 02:40:55 +0000
+++ lib/lp/services/messages/model/message.py	2013-08-12 04:28:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -274,7 +274,7 @@
 
     def fromEmail(self, email_message, owner=None, filealias=None,
                   parsed_message=None, create_missing_persons=False,
-                  fallback_parent=None, date_created=None):
+                  fallback_parent=None, date_created=None, restricted=False):
         """See IMessageSet.fromEmail."""
         # It does not make sense to handle Unicode strings, as email
         # messages may contain chunks encoded in differing character sets.
@@ -300,15 +300,12 @@
 
         # Over-long messages are checked for at the handle_on_message level.
 
-        # Stuff a copy of the raw email into the Librarian, if it isn't
-        # already in there.
-        file_alias_set = getUtility(ILibraryFileAliasSet)  # Reused later
-        if filealias is None:
-            # Avoid circular import.
-            from lp.services.mail.helpers import (
-                save_mail_to_librarian,
-                )
-            raw_email_message = save_mail_to_librarian(email_message)
+        # If it's a restricted mail (IE: for a private bug), or it hasn't been
+        # uploaded, do so now.
+        from lp.services.mail.helpers import save_mail_to_librarian
+        if restricted or filealias is None:
+            raw_email_message = save_mail_to_librarian(
+                email_message, restricted=restricted)
         else:
             raw_email_message = filealias
 
@@ -363,20 +360,19 @@
         else:
             parent = fallback_parent
 
-        # figure out the date of the message
+        # Figure out the date of the message.
         if date_created is not None:
             datecreated = date_created
         else:
             datecreated = utcdatetime_from_field(parsed_message['date'])
 
-        # make sure we don't create an email with a datecreated in the
-        # future. also make sure we don't create an ancient one
+        # Make sure we don't create an email with a datecreated in the
+        # distant past or future.
         now = datetime.now(pytz.timezone('UTC'))
         thedistantpast = datetime(1990, 1, 1, tzinfo=pytz.timezone('UTC'))
         if datecreated < thedistantpast or datecreated > now:
             datecreated = UTC_NOW
 
-        # DOIT
         message = Message(subject=subject, owner=owner,
             rfc822msgid=rfc822msgid, parent=parent,
             raw=raw_email_message, datecreated=datecreated)
@@ -446,8 +442,7 @@
 
                 if content.strip():
                     MessageChunk(
-                        message=message, sequence=sequence,
-                        content=content)
+                        message=message, sequence=sequence, content=content)
                     sequence += 1
             else:
                 filename = part.get_filename() or 'unnamed'
@@ -463,13 +458,11 @@
                     content_type = part['content-type']
 
                 if len(content) > 0:
-                    blob = file_alias_set.create(
-                        name=filename,
-                        size=len(content),
-                        file=cStringIO(content),
-                        contentType=content_type)
-                    MessageChunk(message=message, sequence=sequence,
-                                 blob=blob)
+                    blob = getUtility(ILibraryFileAliasSet).create(
+                        name=filename, size=len(content),
+                        file=cStringIO(content), contentType=content_type,
+                        restricted=restricted)
+                    MessageChunk(message=message, sequence=sequence, blob=blob)
                     sequence += 1
 
         # Don't store the epilogue

=== modified file 'lib/lp/services/messages/tests/test_message.py'
--- lib/lp/services/messages/tests/test_message.py	2012-06-14 05:18:22 +0000
+++ lib/lp/services/messages/tests/test_message.py	2013-08-12 04:28:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -17,13 +17,12 @@
 from lp.services.messages.model.message import MessageSet
 from lp.testing import (
     login,
-    TestCase,
+    TestCaseWithFactory,
     )
-from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.layers import LaunchpadFunctionalLayer
 
 
-class TestMessageSet(TestCase):
+class TestMessageSet(TestCaseWithFactory):
     """Test the methods of `MessageSet`."""
 
     layer = LaunchpadFunctionalLayer
@@ -34,7 +33,6 @@
         super(TestMessageSet, self).setUp()
         # Testing behavior, not permissions here.
         login('foo.bar@xxxxxxxxxxxxx')
-        self.factory = LaunchpadObjectFactory()
 
     def createTestMessages(self):
         """Create some test messages."""
@@ -71,30 +69,31 @@
         message1, message2, message3, message4 = messages
         threads = MessageSet.threadMessages(messages)
         flattened = list(MessageSet.flattenThreads(threads))
-        expected = [(0, message1),
-                    (1, message2),
-                    (2, message4),
-                    (1, message3)]
+        expected = [(0, message1), (1, message2), (2, message4), (1, message3)]
         self.assertEqual(expected, flattened)
 
+    def _makeMessageWithAttachment(self, filename='review.diff'):
+        sender = self.factory.makePerson()
+        msg = MIMEMultipart()
+        msg['Message-Id'] = make_msgid('launchpad')
+        msg['Date'] = formatdate()
+        msg['To'] = 'to@xxxxxxxxxxx'
+        msg['From'] = sender.preferredemail.email
+        msg['Subject'] = 'Sample'
+        msg.attach(MIMEText('This is the body of the email.'))
+        attachment = Message()
+        attachment.set_payload('This is the diff, honest.')
+        attachment['Content-Type'] = 'text/x-diff'
+        attachment['Content-Disposition'] = (
+            'attachment; filename="%s"' % filename)
+        msg.attach(attachment)
+        return msg
+
     def test_fromEmail_keeps_attachments(self):
         """Test that the parsing of the email keeps the attachments."""
         # Build a simple multipart message with a plain text first part
         # and an text/x-diff attachment.
-        sender = self.factory.makePerson()
-        msg = MIMEMultipart()
-        msg['Message-Id'] = make_msgid('launchpad')
-        msg['Date'] = formatdate()
-        msg['To'] = 'to@xxxxxxxxxxx'
-        msg['From'] = sender.preferredemail.email
-        msg['Subject'] = 'Sample'
-        msg.attach(MIMEText('This is the body of the email.'))
-        attachment = Message()
-        attachment.set_payload('This is the diff, honest.')
-        attachment['Content-Type'] = 'text/x-diff'
-        attachment['Content-Disposition'] = (
-            'attachment; filename="review.diff"')
-        msg.attach(attachment)
+        msg = self._makeMessageWithAttachment()
         # Now create the message from the MessageSet.
         message = MessageSet().fromEmail(msg.as_string())
         text, diff = message.chunks
@@ -108,20 +107,7 @@
     def test_fromEmail_strips_attachment_paths(self):
         # Build a simple multipart message with a plain text first part
         # and an text/x-diff attachment.
-        sender = self.factory.makePerson()
-        msg = MIMEMultipart()
-        msg['Message-Id'] = make_msgid('launchpad')
-        msg['Date'] = formatdate()
-        msg['To'] = 'to@xxxxxxxxxxx'
-        msg['From'] = sender.preferredemail.email
-        msg['Subject'] = 'Sample'
-        msg.attach(MIMEText('This is the body of the email.'))
-        attachment = Message()
-        attachment.set_payload('This is the diff, honest.')
-        attachment['Content-Type'] = 'text/x-diff'
-        attachment['Content-Disposition'] = (
-            'attachment; filename="/tmp/foo/review.diff"')
-        msg.attach(attachment)
+        msg = self._makeMessageWithAttachment(filename='/tmp/foo/review.diff')
         # Now create the message from the MessageSet.
         message = MessageSet().fromEmail(msg.as_string())
         text, diff = message.chunks
@@ -136,11 +122,29 @@
         """Even when messages are identical, fromEmail creates a new one."""
         email = self.factory.makeEmailMessage()
         orig_message = MessageSet().fromEmail(email.as_string())
-        # update librarian
         transaction.commit()
         dupe_message = MessageSet().fromEmail(email.as_string())
         self.assertNotEqual(orig_message.id, dupe_message.id)
 
+    def test_fromEmail_restricted_reuploads(self):
+        """fromEmail will re-upload the email to the restricted librarian if
+        restricted is True."""
+        filealias = self.factory.makeLibraryFileAlias()
+        transaction.commit()
+        email = self.factory.makeEmailMessage()
+        message = MessageSet().fromEmail(
+            email.as_string(), filealias=filealias, restricted=True)
+        self.assertTrue(message.raw.restricted)
+        self.assertNotEqual(message.raw.id, filealias.id)
+
+    def test_fromEmail_restricted_attachments(self):
+        """fromEmail creates restricted attachments correctly."""
+        msg = self._makeMessageWithAttachment()
+        message = MessageSet().fromEmail(msg.as_string(), restricted=True)
+        text, diff = message.chunks
+        self.assertEqual('review.diff', diff.blob.filename)
+        self.assertTrue('review.diff', diff.blob.restricted)
+
     def makeEncodedEmail(self, encoding_name, actual_encoding):
         email = self.factory.makeEmailMessage(body=self.high_characters)
         email.set_type('text/plain')