launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-rework-bug-authentication-tests into launchpad:master.

Commit message:
Rework authentication tests for bug command emails

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/397515

TestSignatureTimestampValidation.test_bad_timestamp_but_no_commands didn't work on Python 3 because it's no longer possible to clobber the SignedMessage.signature property.  However, on inspection, this test was only passing by accident anyway, and substituting an object with a timestamp for SignedMessage.signature couldn't have worked, because SignedMessage.signature is supposed to be a byte string.  It looks as though the author of this test confused SignedMessage.signature with the return value of GPGHandler.getVerifiedSignature.  Furthermore, the test didn't actually check the signature timestamp, because it calls the handler directly rather than first calling authenticateEmail; and a timestamp too far in the future in fact results in an InvalidSignature exception from authenticateEmail rather than successfully processing command-free emails.

Rework this whole test suite so that it tests something meaningful, namely that strong authentication is required to process commands in bug emails.  We now also include a test for a scenario that involves rejecting an insufficiently-authenticated email, making it easier to notice when tests don't really test what they claim to test.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-rework-bug-authentication-tests into launchpad:master.

diff --git a/lib/lp/bugs/mail/tests/test_handler.py b/lib/lp/bugs/mail/tests/test_handler.py
index 1f6df0b..6994404 100644
--- a/lib/lp/bugs/mail/tests/test_handler.py
+++ b/lib/lp/bugs/mail/tests/test_handler.py
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-import time
-
 import transaction
 from zope.component import getUtility
 from zope.security.management import (
@@ -29,9 +27,12 @@ from lp.bugs.mail.handler import (
     )
 from lp.bugs.model.bugnotification import BugNotification
 from lp.registry.enums import BugSharingPolicy
+from lp.registry.interfaces.person import IPersonSet
 from lp.services.config import config
 from lp.services.database.interfaces import IStore
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
+from lp.services.mail.incoming import authenticateEmail
+from lp.services.mail.interfaces import IWeaklyAuthenticatedPrincipal
 from lp.services.webapp.authorization import LaunchpadSecurityPolicy
 from lp.testing import (
     celebrity_logged_in,
@@ -42,7 +43,10 @@ from lp.testing import (
     )
 from lp.testing.dbuser import switch_dbuser
 from lp.testing.factory import GPGSigningContext
-from lp.testing.gpgkeys import import_secret_test_key
+from lp.testing.gpgkeys import (
+    import_public_key,
+    import_secret_test_key,
+    )
 from lp.testing.layers import (
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
@@ -668,47 +672,69 @@ class BugCommandGroupsTestCase(TestCase):
             expected, [str(command) for command in ordered_commands])
 
 
-class FakeSignature:
-
-    def __init__(self, timestamp):
-        self.timestamp = timestamp
-
-
-class TestSignatureTimestampValidation(TestCaseWithFactory):
-    """GPG signature timestamps are checked for emails containing commands."""
+class TestAuthenticationRequirements(TestCaseWithFactory):
+    """Emails containing commands require strong authentication."""
 
     layer = LaunchpadFunctionalLayer
 
-    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.
+    def test_commands_with_strong_authentication(self):
+        # A good GPG signature counts as strong authentication, and allows
+        # command processing.
+        # The test keys belong to test@xxxxxxxxxxxxx.
+        import_public_key('test@xxxxxxxxxxxxx')
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            bug.setSecurityRelated(True, bug.owner)
+        sender = getUtility(IPersonSet).getByEmail('test@xxxxxxxxxxxxx')
         signing_context = GPGSigningContext(
             import_secret_test_key(), password='test')
         msg = self.factory.makeSignedMessage(
-            body=' security no', signing_context=signing_context)
+            body=' security no',
+            email_address=removeSecurityProxy(sender).preferredemail.email,
+            signing_context=signing_context,
+            to_address='%d@xxxxxxxxxxxxxxxxxxx' % bug.id)
+        self.assertIsNotNone(msg.signature)
+        principal = authenticateEmail(msg)
+        self.assertEqual(sender, principal.person)
+        self.assertFalse(IWeaklyAuthenticatedPrincipal.providedBy(principal))
         handler = MaloneHandler()
-        with person_logged_in(self.factory.makePerson()):
-            handler.process(msg, msg['To'])
-        # Since there were no commands in the poorly-timestamped message, no
-        # error emails were generated.
+        handler.process(msg, msg['To'])
+        # Since the message was strongly authenticated, no error emails were
+        # generated, and the command was run.
         self.assertEmailQueueLength(0)
+        self.assertFalse(bug.security_related)
 
-    def test_bad_timestamp_but_no_commands(self):
-        # If an email message's GPG signature's timestamp is too far in the
-        # future or past but it doesn't contain any commands, the email is
-        # processed anyway.
+    def test_commands_with_weak_authentication(self):
+        # An unsigned message does not allow command processing.
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            bug.setSecurityRelated(True, bug.owner)
+        msg = self.factory.makeSignedMessage(
+            body=' security no', to_address='%d@xxxxxxxxxxxxxxxxxxx' % bug.id)
+        self.assertIsNone(msg.signature)
+        principal = authenticateEmail(msg)
+        self.assertTrue(IWeaklyAuthenticatedPrincipal.providedBy(principal))
+        handler = MaloneHandler()
+        handler.process(msg, msg['To'])
+        [notification] = self.assertEmailQueueLength(1)
+        self.assertEqual('Submit Request Failure', notification['Subject'])
+        self.assertTrue(bug.security_related)
 
+    def test_no_commands_with_weak_authentication(self):
+        # A message that does not contain commands is accepted with only
+        # weak authentication.
+        bug = self.factory.makeBug()
         msg = self.factory.makeSignedMessage(
-            body='I really hope this bug gets fixed.')
-        now = time.time()
-        one_week = 60 * 60 * 24 * 7
-        msg.signature = FakeSignature(timestamp=now + one_week)
+            body='I really hope this bug gets fixed.',
+            to_address='%d@xxxxxxxxxxxxxxxxxxx' % bug.id)
+        self.assertIsNone(msg.signature)
+        principal = authenticateEmail(msg)
+        self.assertTrue(IWeaklyAuthenticatedPrincipal.providedBy(principal))
         handler = MaloneHandler()
-        # Clear old emails before potentially generating more.
-        pop_notifications()
-        with person_logged_in(self.factory.makePerson()):
-            handler.process(msg, msg['To'])
-        # Since there were no commands in the poorly-timestamped message, no
-        # error emails were generated.
+        handler.process(msg, msg['To'])
+        # Since there were no commands in the unsigned message, no error
+        # emails were generated, and the message was added.
         self.assertEmailQueueLength(0)
+        self.assertEqual(
+            'I really hope this bug gets fixed.',
+            bug.messages[0].text_contents)