launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/epic-email-bug-description into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #67899 Large emails should tell users that their email was rejected because it's too big.
  https://bugs.launchpad.net/bugs/67899
  #203961 Prohibit epic bug descriptions by email
  https://bugs.launchpad.net/bugs/203961

For more details, see:
https://code.launchpad.net/~thumper/launchpad/epic-email-bug-description/+merge/51977

This branch does the same validation on the email comment
when creating a new branch as the web form does.  Simply
put, it checks the validator of the IBugAddForm comment
field.

The "except ValidationError" part was to just catch other
validation errors that may occur during the checking of
the comment.  I didn't want to create any extra OOPSes by
trying to fix one.

The only really annoying part of the test is the assumption
that the MaloneHandler has that the user has been
authenticated, and as such there is a current interaction
for that user, as handled by the authenticateEmail method.
This however actually does real GPG checking for the signed
email, as opposed to the "fake" signed email that the factory
generates, so I use the login method to set the interaction
for the email user.

-- 
https://code.launchpad.net/~thumper/launchpad/epic-email-bug-description/+merge/51977
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/epic-email-bug-description into lp:launchpad.

=== modified file 'lib/canonical/launchpad/mail/commands.py'
--- lib/canonical/launchpad/mail/commands.py	2011-02-24 15:30:54 +0000
+++ lib/canonical/launchpad/mail/commands.py	2011-03-02 22:59:59 +0000
@@ -23,7 +23,10 @@
     implements,
     providedBy,
     )
-from zope.schema import ValidationError
+from zope.schema.interfaces import (
+    TooLong,
+    ValidationError,
+    )
 
 from canonical.launchpad.interfaces.mail import (
     BugTargetNotFound,
@@ -48,6 +51,7 @@
 from lp.bugs.interfaces.bug import (
     CreateBugParams,
     IBug,
+    IBugAddForm,
     IBugSet,
     )
 from lp.bugs.interfaces.bugtask import (
@@ -154,8 +158,27 @@
         bugid = self.string_args[0]
 
         if bugid == 'new':
+            # Check the message validator.
+            comment = parsed_msg.as_string()
+            validator = IBugAddForm['comment'].validate
+            # The validator is very strict on the unicode aspect.
+            if not isinstance(comment, unicode):
+                comment = unicode(comment)
+            try:
+                validator(comment)
+            except TooLong:
+                raise EmailProcessingError(
+                    'The description is too long. If you have lots '
+                    'text to add, use an attachment instead.',
+                    stop_processing=True)
+            except ValidationError as e:
+                # More a just in case than any real expectation of getting
+                # something.
+                raise EmailProcessingError(
+                    str(e),
+                    stop_processing=True)
             message = getUtility(IMessageSet).fromEmail(
-                parsed_msg.as_string(),
+                comment,
                 owner=getUtility(ILaunchBag).user,
                 filealias=filealias,
                 parsed_message=parsed_msg)

=== modified file 'lib/lp/bugs/mail/tests/test_handler.py'
--- lib/lp/bugs/mail/tests/test_handler.py	2010-12-13 13:40:38 +0000
+++ lib/lp/bugs/mail/tests/test_handler.py	2011-03-02 22:59:59 +0000
@@ -10,23 +10,31 @@
 
 import transaction
 
+from canonical.config import config
 from canonical.database.sqlbase import commit
 from canonical.launchpad.ftests import import_secret_test_key
 from canonical.launchpad.mail.commands import BugEmailCommand
-from canonical.testing.layers import LaunchpadFunctionalLayer
+from canonical.testing.layers import (
+    LaunchpadFunctionalLayer,
+    LaunchpadZopelessLayer,
+    ZopelessAppServerLayer,
+    )
 from lp.bugs.mail.handler import MaloneHandler
 from lp.services.mail import stub
+from lp.services.mail.incoming import authenticateEmail
 from lp.testing import (
+    login,
     person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.factory import GPGSigningContext
+from lp.testing.mail_helpers import pop_notifications
 
 
 class TestMaloneHandler(TestCaseWithFactory):
     """Test that the Malone/bugs handler works."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = ZopelessAppServerLayer
 
     def test_getCommandsEmpty(self):
         """getCommands returns an empty list for messages with no command."""
@@ -104,6 +112,34 @@
         transaction.commit()
         return stub.test_emails[:]
 
+    def switchDbUser(self, user):
+        """Commit the transaction and switch to the new user."""
+        transaction.commit()
+        LaunchpadZopelessLayer.switchDbUser(user)
+
+    def test_new_bug_big_body(self):
+        # If a bug email is sent with an excessively large body, we email the
+        # user back and ask that they use attachments instead.
+        big_body_text = 'This is really big.' * 10000
+        mail = self.factory.makeSignedMessage(body=big_body_text)
+        self.switchDbUser(config.processmail.dbuser)
+        # Rejection email goes to the preferred email of the current user.
+        # The current user is extracted from the current interaction, which is
+        # set up using the authenticateEmail method.  However that expects
+        # real GPG signed emails, which we are faking here.
+        login(mail['from'])
+        handler = MaloneHandler()
+        self.assertTrue(handler.process(mail,
+            'new@xxxxxxxxxxxxxxxxxx', None))
+        notification = pop_notifications()[0]
+        self.assertEqual('Submit Request Failure', notification['subject'])
+        # The returned message is a multipart message, the first part is
+        # the message, and the second is the original message.
+        message, original = notification.get_payload()
+        self.assertIn(
+            "The description is too long.",
+            message.get_payload(decode=True))
+
 
 class FakeSignature: