Merge lp:~benji/launchpad/bug-580035 into lp:launchpad

[Benji York]

During QA this branch caused exceptions when processing GPG signed
emails.

I found that my understanding of the data structures involved was faulty
so the tests I wrote were not representative. I reworked the tests to
be realistic and added the data I needed to fix the bug to
PymeSignature.

You can run all the email tests that pertain to authentication that I
could find with this command:

    bin/test -ct emailauthentication.txt -t bugs-emailinterface.txt \
        -t signed_messages -t test_handlers -t test_helpers

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/interfaces/gpghandler.py'
--- lib/canonical/launchpad/interfaces/gpghandler.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/interfaces/gpghandler.py	2010-08-27 13:45:57 +0000
@@ -38,7 +38,7 @@
         self.fingerprint = fingerprint
         self.pubkey = pubkey
         super(GPGKeyNotFoundError, self).__init__(
-            "No GPG key found with the given content: %s" % (fingerprint,))
+            "No GPG key found with the given content: %s" % (fingerprint, ))
 
 
 class GPGKeyRevoked(Exception):
@@ -47,7 +47,7 @@
     def __init__(self, key):
         self.key = key
         super(GPGKeyRevoked, self).__init__(
-            "%s has been publicly revoked" % (key.keyid,))
+            "%s has been publicly revoked" % (key.keyid, ))
 
 
 class GPGKeyExpired(Exception):
@@ -55,7 +55,7 @@
 
     def __init__(self, key):
         self.key = key
-        super(GPGKeyExpired, self).__init__("%s has expired" % (key.keyid,))
+        super(GPGKeyExpired, self).__init__("%s has expired" % (key.keyid, ))
 
 
 class SecretGPGKeyImportDetected(Exception):
@@ -275,6 +275,7 @@
 
     fingerprint = Attribute("Signer Fingerprint.")
     plain_data = Attribute("Plain Signed Text.")
+    timestamp = Attribute("The time at which the message was signed.")
 
 
 class IPymeKey(Interface):

=== modified file 'lib/canonical/launchpad/mail/handlers.py'
--- lib/canonical/launchpad/mail/handlers.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/handlers.py	2010-08-27 13:45:57 +0000
@@ -32,6 +32,7 @@
     ISpecificationSet,
     QuestionStatus,
     )
+from canonical.launchpad.interfaces.gpghandler import IGPGHandler
 from canonical.launchpad.mail.commands import (
     BugEmailCommands,
     get_error_message,
@@ -58,6 +59,16 @@
     )
 
 
+def extract_signature_timestamp(signed_msg):
+    # break import cycle
+    from canonical.launchpad.mail.incoming import (
+        canonicalise_line_endings)
+    signature = getUtility(IGPGHandler).getVerifiedSignature(
+        canonicalise_line_endings(signed_msg.signedContent),
+        signed_msg.signature)
+    return signature.timestamp
+
+
 class MaloneHandler:
     """Handles emails sent to Malone.
 
@@ -77,7 +88,8 @@
                 name, args in parse_commands(content,
                                              BugEmailCommands.names())]
 
-    def process(self, signed_msg, to_addr, filealias=None, log=None):
+    def process(self, signed_msg, to_addr, filealias=None, log=None,
+                extract_signature_timestamp=extract_signature_timestamp):
         """See IMailHandler."""
         commands = self.getCommands(signed_msg)
         user, host = to_addr.split('@')
@@ -89,7 +101,8 @@
                 CONTEXT = 'bug report'
                 ensure_not_weakly_authenticated(signed_msg, CONTEXT)
                 if signature is not None:
-                    ensure_sane_signature_timestamp(signature, CONTEXT)
+                    ensure_sane_signature_timestamp(
+                        extract_signature_timestamp(signed_msg), CONTEXT)
 
             if user.lower() == 'new':
                 # A submit request.

=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/helpers.py	2010-08-27 13:45:57 +0000
@@ -230,7 +230,7 @@
         raise IncomingEmailError(error_message)
 
 
-def ensure_sane_signature_timestamp(signature, context,
+def ensure_sane_signature_timestamp(timestamp, context,
                                     error_template='old-signature.txt'):
     """Ensure the signature was generated recently but not in the future."""
     fourty_eight_hours = 48 * 60 * 60
@@ -239,7 +239,7 @@
     fourty_eight_hours_ago = now - fourty_eight_hours
     ten_minutes_in_the_future = now + ten_minutes
 
-    if (signature.timestamp < fourty_eight_hours_ago
-            or signature.timestamp > ten_minutes_in_the_future):
+    if (timestamp < fourty_eight_hours_ago
+            or timestamp > ten_minutes_in_the_future):
         error_message = get_error_message(error_template, context=context)
         raise IncomingEmailError(error_message)

=== modified file 'lib/canonical/launchpad/mail/tests/test_handlers.py'
--- lib/canonical/launchpad/mail/tests/test_handlers.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/tests/test_handlers.py	2010-08-27 13:45:57 +0000
@@ -9,6 +9,7 @@
 import unittest
 
 from canonical.database.sqlbase import commit
+from canonical.launchpad.ftests import import_secret_test_key
 from canonical.launchpad.mail.commands import BugEmailCommand
 from canonical.launchpad.mail.handlers import MaloneHandler
 from canonical.testing import LaunchpadFunctionalLayer
@@ -17,6 +18,7 @@
     person_logged_in,
     TestCaseWithFactory,
     )
+from lp.testing.factory import GPGSigningContext
 
 
 class TestMaloneHandler(TestCaseWithFactory):
@@ -68,39 +70,21 @@
 
     layer = LaunchpadFunctionalLayer
 
-    def test_far_past_gpg_signature_timestamp(self):
-        # If an email message's GPG signature's timestamp is too far in the
-        # past and the message contains a command, the command will fail and
-        # send the user an email telling them what happened.
-
-        msg = self.factory.makeSignedMessage(body=' security no')
-        now = time.time()
-        one_week = 60 * 60 * 24 * 7
-        msg.signature = FakeSignature(timestamp=now-one_week)
-        handler = MaloneHandler()
-        with person_logged_in(self.factory.makePerson()):
-            success = handler.process(msg, msg['To'])
-        commit()
-        email = get_last_email()
-        self.assertEqual(email['subject'], 'Submit Request Failure')
-        self.assertIn(BAD_SIGNATURE_TIMESTAMP_MESSAGE, email['body'])
-
-    def test_far_future_gpg_signature_timestamp(self):
-        # If an email message's GPG signature's timestamp is too far in the
-        # future and the message contains a command, the command will fail and
-        # send the user an email telling them what happened.
-
-        msg = self.factory.makeSignedMessage(body=' security no')
-        now = time.time()
-        one_week = 60 * 60 * 24 * 7
-        msg.signature = FakeSignature(timestamp=now+one_week)
-        handler = MaloneHandler()
-        with person_logged_in(self.factory.makePerson()):
-            success = handler.process(msg, msg['To'])
-        commit()
-        email = get_last_email()
-        self.assertEqual(email['subject'], 'Submit Request Failure')
-        self.assertIn(BAD_SIGNATURE_TIMESTAMP_MESSAGE, email['body'])
+    def test_good_signature_timestamp(self):
+        # An email message's GPG signature's timestamp checked to be sure it
+        # isn't too far in the future or past.  This test shows that a
+        # signature with a timestamp of appxoimately now will be accepted.
+        signing_context = GPGSigningContext(
+            import_secret_test_key().fingerprint, password='test')
+        msg = self.factory.makeSignedMessage(
+            body=' security no', signing_context=signing_context)
+        handler = MaloneHandler()
+        with person_logged_in(self.factory.makePerson()):
+            success = handler.process(msg, msg['To'])
+        commit()
+        # Since there were no commands in the poorly-timestamped message, no
+        # error emails were generated.
+        self.assertEqual(stub.test_emails, [])
 
     def test_bad_timestamp_but_no_commands(self):
         # If an email message's GPG signature's timestamp is too far in the

=== modified file 'lib/canonical/launchpad/mail/tests/test_helpers.py'
--- lib/canonical/launchpad/mail/tests/test_helpers.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/tests/test_helpers.py	2010-08-27 13:45:57 +0000
@@ -91,12 +91,6 @@
             parse_commands(' command:', ['command']))
 
 
-class FakeSignature:
-
-    def __init__(self, timestamp):
-        self.timestamp = timestamp
-
-
 class TestEnsureSaneSignatureTimestamp(unittest.TestCase):
     """Tests for ensure_sane_signature_timestamp"""
 
@@ -104,35 +98,31 @@
         # signature timestamps shouldn't be too old
         now = time.time()
         one_week = 60 * 60 * 24 * 7
-        signature = FakeSignature(timestamp=now-one_week)
         self.assertRaises(
             IncomingEmailError, ensure_sane_signature_timestamp,
-            signature, 'bug report')
+            now-one_week, 'bug report')
 
     def test_future_timestamp(self):
         # signature timestamps shouldn't be (far) in the future
         now = time.time()
         one_week = 60 * 60 * 24 * 7
-        signature = FakeSignature(timestamp=now+one_week)
         self.assertRaises(
             IncomingEmailError, ensure_sane_signature_timestamp,
-            signature, 'bug report')
+            now+one_week, 'bug report')
 
     def test_near_future_timestamp(self):
         # signature timestamps in the near future are OK
         now = time.time()
         one_minute = 60
-        signature = FakeSignature(timestamp=now+one_minute)
         # this should not raise an exception
-        ensure_sane_signature_timestamp(signature, 'bug report')
+        ensure_sane_signature_timestamp(now+one_minute, 'bug report')
 
     def test_recent_timestamp(self):
         # signature timestamps in the recent past are OK
         now = time.time()
         one_hour = 60 * 60
-        signature = FakeSignature(timestamp=now-one_hour)
         # this should not raise an exception
-        ensure_sane_signature_timestamp(signature, 'bug report')
+        ensure_sane_signature_timestamp(now-one_hour, 'bug report')
 
 
 class TestEnsureNotWeaklyAuthenticated(TestCaseWithFactory):

=== modified file 'lib/canonical/launchpad/utilities/gpghandler.py'
--- lib/canonical/launchpad/utilities/gpghandler.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/utilities/gpghandler.py	2010-08-27 13:45:57 +0000
@@ -218,8 +218,10 @@
                 "Unable to map subkey: %s" % signature.fpr)
 
         # return the signature container
-        return PymeSignature(fingerprint=key.fingerprint,
-                             plain_data=plain.getvalue())
+        return PymeSignature(
+            fingerprint=key.fingerprint,
+            plain_data=plain.getvalue(),
+            timestamp=signature.timestamp)
 
     def importPublicKey(self, content):
         """See IGPGHandler."""
@@ -550,10 +552,11 @@
     """See IPymeSignature."""
     implements(IPymeSignature)
 
-    def __init__(self, fingerprint=None, plain_data=None):
+    def __init__(self, fingerprint=None, plain_data=None, timestamp=None):
         """Initialized a signature container."""
         self.fingerprint = fingerprint
         self.plain_data = plain_data
+        self.timestamp = timestamp
 
 
 class PymeKey:

=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt	2010-08-18 18:23:37 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt	2010-08-27 13:45:57 +0000
@@ -50,16 +50,11 @@
 signed, so that the system can verify the sender. But to avoid having
 to sign each email, we'll create a class which fakes a signed email:
 
-    >>> import time
-    >>> class MockSignature(object):
-    ...     def __init__(self):
-    ...         self.timestamp = time.time()
-
     >>> import email.Message
     >>> class MockSignedMessage(email.Message.Message):
     ...     def __init__(self, *args, **kws):
     ...         email.Message.Message.__init__(self, *args, **kws)
-    ...         self.signature = MockSignature()
+    ...         self.signature = 'fake'
     ...     @property
     ...     def signedMessage(self):
     ...         return self
@@ -82,9 +77,14 @@
     ...         msg['Message-Id'] = factory.makeUniqueRFC822MsgId()
     ...     return msg
 
+    >>> import time
+    >>> def fake_extract_signature_timestamp(signed_msg):
+    ...     return time.time()
+
     >>> def process_email(raw_mail):
     ...     msg = construct_email(raw_mail)
-    ...     handler.process(msg, msg['To'])
+    ...     handler.process(msg, msg['To'],
+    ...         extract_signature_timestamp=fake_extract_signature_timestamp)
 
     >>> process_email(submit_mail)
 
@@ -410,7 +410,9 @@
     ...     signature = None
     >>> msg = email.message_from_string(
     ...     comment_mail, _class=MockUnsignedMessage)
-    >>> handler.process(msg, msg['To'])
+    >>> handler.process(
+    ...     msg, msg['To'],
+    ...     extract_signature_timestamp=fake_extract_signature_timestamp)
     True
     >>> commit()
 
@@ -482,7 +484,9 @@
     ...     return construct_email(edit_mail)
 
     >>> def submit_command_email(msg):
-    ...     handler.process(msg, msg['To'])
+    ...     handler.process(
+    ...         msg, msg['To'],
+    ...         extract_signature_timestamp=fake_extract_signature_timestamp)
     ...     commit()
     ...     sync(bug)
 
@@ -1756,7 +1760,9 @@
     >>> msg = signed_message_from_string(submit_mail)
     >>> import email.Utils
     >>> msg['Message-Id'] = email.Utils.make_msgid()
-    >>> handler.process(msg, msg['To'])
+    >>> handler.process(
+    ...     msg, msg['To'],
+    ...     extract_signature_timestamp=fake_extract_signature_timestamp)
     True
     >>> print_latest_email()
     Subject: Submit Request Failure