launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/catch-incomingemailerror into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1011980 in Launchpad itself: "IncomingEmailError raised when signature timestamp is too far in the past or the future"
  https://bugs.launchpad.net/launchpad/+bug/1011980

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/catch-incomingemailerror/+merge/125416

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.
-- 
https://code.launchpad.net/~stevenk/launchpad/catch-incomingemailerror/+merge/125416
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/catch-incomingemailerror into lp:launchpad.

=== 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 05:53: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.
@@ -521,7 +519,7 @@
     try:
         principal = authenticateEmail(
             mail, signature_timestamp_checker)
-    except InvalidSignature as error:
+    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 05:53: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 = 'eric@xxxxxxxxxxxxxxxxxxx'
         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 = 'eric@xxxxxxxxxxxxxxxxxxxxxxxx'