Merge lp:~abentley/launchpad/invalid-sig into lp:launchpad

[Aaron Bentley]

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

= Summary =
Stop handling InvalidSignature as an OOPS, instead handle as user error.

== Proposed fix ==
Implement a new _handle_user_error method, and use it.

== Pre-implementation notes ==
None

== Implementation details ==
Until now, handleIncoming has only had doctests, not unittests.

== Tests ==
bin/test test_incoming -t test_invalid_signature

== Demo and Q/A ==
Send a message with an invalid signature. The resulting error message
should explain what the problem was.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/mail/incoming.py
  lib/canonical/launchpad/mail/tests/test_incoming.py
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkp4np4ACgkQ0F+nu1YWqI3JLwCfQvI5Hd/gM/FTz7a7I5yHrDFn
J54AmgLOpZ1v+7r+HQ6ZBQuVRMFktEpJ
=pIUT
-----END PGP SIGNATURE-----

[Gavin Panella (allenap)]

Hi Aaron,

It's a shame the error message isn't even more informative, but I
guess this is outside of your responsibility here, and this branch is
a great improvement over the current situation as it is.

One minor/trivial comment.

Gavin.

> === modified file 'lib/canonical/launchpad/mail/incoming.py'
> --- lib/canonical/launchpad/mail/incoming.py 2009-06-25 05:30:52 +0000
> +++ lib/canonical/launchpad/mail/incoming.py 2009-08-04 20:43:38 +0000
> @@ -177,6 +177,12 @@
> msg)
> trans.commit()
>
> + def _handle_user_error(error, mail):
> + mailbox.delete(mail_id)
> + send_process_error_notification(
> + mail['From'], 'Submit Request Failure', str(error), mail)
> + trans.commit()
> +
> log = getLogger('process-mail')
> mailbox = getUtility(IMailBox)
> log.info("Opening the mail box.")
> @@ -250,10 +256,7 @@
> try:
> principal = authenticateEmail(mail)
> except InvalidSignature, error:
> - _handle_error(
> - "Invalid signature for %s:\n %s" % (mail['From'],
> - str(error)),
> - file_alias_url)
> + _handle_user_error(error, mail)
> continue
> except InactiveAccount:
> _handle_error(
>
> === modified file 'lib/canonical/launchpad/mail/tests/test_incoming.py'
> --- lib/canonical/launchpad/mail/tests/test_incoming.py 2009-06-25 05:30:52 +0000
> +++ lib/canonical/launchpad/mail/tests/test_incoming.py 2009-08-04 20:43:38 +0000
> @@ -5,9 +5,55 @@
>
> from zope.testing.doctest import DocTestSuite
>
> +from canonical.launchpad.mail.incoming import handleMail, MailErrorUtility
> +from canonical.testing import LaunchpadZopelessLayer
> +from lp.testing import TestCaseWithFactory
> +from lp.testing.mail_helpers import pop_notifications
> +from lp.services.mail.sendmail import MailController
> +
> +
> +class TestIncoming(TestCaseWithFactory):
> +
> + layer = LaunchpadZopelessLayer
> +
> + def test_invalid_signature(self):
> + """Invalid signature should not be handled as an OOPs.
> +
> + It should produce a message explaining to the user what went wrong.
> + """
> + person = self.factory.makePerson()
> + email_address = person.preferredemail.email
> + invalid_body = (
> + '-----BEGIN PGP SIGNED MESSAGE-----\n'
> + 'Hash: SHA1\n\n'
> + 'Body\n'
> + '-----BEGIN PGP SIGNATURE-----\n'
> + 'Not a signature.\n'
> + '-----END PGP SIGNATURE-----\n')
> + ctrl = MailController(
> + email_address, '<email address hidden>', 'subject', invalid_body,
> + bulk=False)
> + ctrl.send()
> + error_utility = MailErrorUtility()
> + old_oops = error_utility.getLastOopsReport()
> + handleMail()
> + current_oops = error_utility.getLastOopsReport()
> + if old_oops is None:
> + self.assertIs(None, current_oops)
> + else:
> + self.assertEqual(old_oops...

review: Approve

=== modified file 'lib/canonical/launchpad/mail/incoming.py'
--- lib/canonical/launchpad/mail/incoming.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/mail/incoming.py	2009-08-04 20:43:38 +0000
@@ -177,6 +177,12 @@
                 msg)
         trans.commit()
 
+    def _handle_user_error(error, mail):
+        mailbox.delete(mail_id)
+        send_process_error_notification(
+            mail['From'], 'Submit Request Failure', str(error), mail)
+        trans.commit()
+
     log = getLogger('process-mail')
     mailbox = getUtility(IMailBox)
     log.info("Opening the mail box.")
@@ -250,10 +256,7 @@
                 try:
                     principal = authenticateEmail(mail)
                 except InvalidSignature, error:
-                    _handle_error(
-                        "Invalid signature for %s:\n    %s" % (mail['From'],
-                                                               str(error)),
-                        file_alias_url)
+                    _handle_user_error(error, mail)
                     continue
                 except InactiveAccount:
                     _handle_error(

=== modified file 'lib/canonical/launchpad/mail/tests/test_incoming.py'
--- lib/canonical/launchpad/mail/tests/test_incoming.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/mail/tests/test_incoming.py	2009-08-04 20:43:38 +0000
@@ -5,9 +5,55 @@
 
 from zope.testing.doctest import DocTestSuite
 
+from canonical.launchpad.mail.incoming import handleMail, MailErrorUtility
+from canonical.testing import LaunchpadZopelessLayer
+from lp.testing import TestCaseWithFactory
+from lp.testing.mail_helpers import pop_notifications
+from lp.services.mail.sendmail import MailController
+
+
+class TestIncoming(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_invalid_signature(self):
+        """Invalid signature should not be handled as an OOPs.
+
+        It should produce a message explaining to the user what went wrong.
+        """
+        person = self.factory.makePerson()
+        email_address = person.preferredemail.email
+        invalid_body = (
+            '-----BEGIN PGP SIGNED MESSAGE-----\n'
+            'Hash: SHA1\n\n'
+            'Body\n'
+            '-----BEGIN PGP SIGNATURE-----\n'
+            'Not a signature.\n'
+            '-----END PGP SIGNATURE-----\n')
+        ctrl = MailController(
+            email_address, '[e-mail]', 'subject', invalid_body,
+            bulk=False)
+        ctrl.send()
+        error_utility = MailErrorUtility()
+        old_oops = error_utility.getLastOopsReport()
+        handleMail()
+        current_oops = error_utility.getLastOopsReport()
+        if old_oops is None:
+            self.assertIs(None, current_oops)
+        else:
+            self.assertEqual(old_oops.id, current_oops.id)
+        (notification,) = pop_notifications()
+        body = notification.get_payload()[0].get_payload(decode=True)
+        self.assertIn(
+            "An error occurred while processing a mail you sent to "
+            "Launchpad's email\ninterface.\n\n\n"
+            "Error message:\n\nSignature couldn't be verified: No data",
+            body)
+
 
 def test_suite():
     suite = unittest.TestSuite()
+    suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
     suite.addTest(DocTestSuite('canonical.launchpad.mail.incoming'))
     return suite