Merge lp:~stevenk/launchpad/catch-incomingemailerror into lp:launchpad

[Steve Kowalik]

Also catch IncomingEmailError as well as InvalidSignature in handle_one_mail so the sender gets a nice mail back, rather than an OOPS.

In the incoming mail handler, handle_one_mail, we call a function called authenticateEmail which tries to work out based on DKIM or GPG signature who the principal is. This is called inside a try block and the two exceptions InvalidSignature or InactiveAccount are handled and do not cause an OOPS. The problem is, authenticateEmail can also raise an IncomingEmailError, which isn't caught, and so will oops.

I have changed the except block to treat InvalidSignature or IncomingEmailError as the same. I can not see a way to test this, since it would require sending a mail with a plausible out of date timestamp. The pieces themselves are tested, there just isn't a way to test everything together.

Perform some cleanup, mostly whitespace.

[Ian Booth (wallyworld)]

review: Approve

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2012-06-29 08:40:05 +0000
+++ lib/lp/services/mail/incoming.py	2012-09-20 06:02:20 +0000
@@ -3,8 +3,6 @@
 
 """Functions dealing with mails coming into Launchpad."""
 
-# pylint: disable-msg=W0631
-
 __metaclass__ = type
 
 from cStringIO import StringIO as cStringIO
@@ -42,6 +40,7 @@
 from lp.services.mail.helpers import (
     ensure_sane_signature_timestamp,
     get_error_message,
+    IncomingEmailError,
     save_mail_to_librarian,
     )
 from lp.services.mail.interfaces import IWeaklyAuthenticatedPrincipal
@@ -238,8 +237,7 @@
     return (dkim_principal, dkim_trusted_address)
 
 
-def authenticateEmail(mail,
-    signature_timestamp_checker=None):
+def authenticateEmail(mail, signature_timestamp_checker=None):
     """Authenticates an email by verifying the PGP signature.
 
     The mail is expected to be an ISignedMessage.
@@ -511,17 +509,13 @@
         # It's probably big and it's probably mostly binary, so trim it pretty
         # aggressively.
         send_process_error_notification(
-            mail['From'],
-            'Mail to Launchpad was too large',
-            complaint,
-            mail,
-            max_return_size=8192)
+            mail['From'], 'Mail to Launchpad was too large', complaint,
+            mail, max_return_size=8192)
         return
 
     try:
-        principal = authenticateEmail(
-            mail, signature_timestamp_checker)
-    except InvalidSignature as error:
+        principal = authenticateEmail(mail, signature_timestamp_checker)
+    except (InvalidSignature, IncomingEmailError) as error:
         send_process_error_notification(
             mail['From'], 'Submit Request Failure', str(error), mail)
         return

=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py	2012-04-16 00:07:11 +0000
+++ lib/lp/services/mail/tests/test_incoming.py	2012-09-20 06:02:20 +0000
@@ -88,12 +88,8 @@
         self.assertEqual([], self.oopses)
         [notification] = pop_notifications()
         body = notification.get_payload()[0].get_payload(decode=True)
-        self.assertIn(
-            "The mail you sent to Launchpad is too long.",
-            body)
-        self.assertIn(
-            "was 55 MB and the limit is 10 MB.",
-            body)
+        self.assertIn("The mail you sent to Launchpad is too long.", body)
+        self.assertIn("was 55 MB and the limit is 10 MB.", body)
 
     def test_invalid_to_addresses(self):
         """Invalid To: header should not be handled as an OOPS."""
@@ -121,9 +117,8 @@
         def fail_all_timestamps(timestamp, context):
             raise helpers.IncomingEmailError("fail!")
         self.assertRaises(
-            helpers.IncomingEmailError,
-            authenticateEmail,
-            msg, fail_all_timestamps)
+            helpers.IncomingEmailError, authenticateEmail, msg,
+            fail_all_timestamps)
 
     def test_unknown_email(self):
         # An unknown email address returns no principal.
@@ -141,8 +136,7 @@
         original_to = '[e-mail]'
         mail[ORIGINAL_TO_HEADER] = original_to
         self.assertThat(
-            extract_addresses(mail, None, None),
-            Equals([original_to]))
+            extract_addresses(mail, None, None), Equals([original_to]))
 
     def test_original_to_in_body(self):
         header_to = '[e-mail]'