Code review comment for lp:~mbp/launchpad/dkim

OK, this is now updated with the following changes based on the full-testsuite failure, and the tests in bugs-emailinterface.txt now pass.

 * There is a lack of coherence between some things checking for a .signature and some checking the current security principal. the email doctests had some trouble with this; specifically they didn't make the principal weaklyauthenticated before testing that condition. This is now fixed.

 * Typically for doctests, state is run together across tests, so I cleared the mail queue so we'd get a more obvious failure.

 * The not-gpg-signed error template was misnamed and will become more so in future, so I renamed it. The error is not so much about being unsigned, but being unauthenticated when trying to create a new bug.

Here's the incremental diff:

=== renamed file 'lib/canonical/launchpad/mail/errortemplates/not-gpg-signed.txt' => 'lib/canonical/launchpad/mail/errortemplates/unauthenticated-bug-creation.txt'
=== modified file 'lib/canonical/launchpad/mail/handlers.py'
--- lib/canonical/launchpad/mail/handlers.py 2010-09-21 05:21:10 +0000
+++ lib/canonical/launchpad/mail/handlers.py 2010-10-01 08:16:02 +0000
@@ -94,12 +94,15 @@
         commands = self.getCommands(signed_msg)
         to_user, to_host = to_addr.split('@')
         add_comment_to_bug = False
+ if to_user.lower() == 'new':
+ # Special failure message for unauthenticated attempts to create
+ # new bugs.
+ ensure_not_weakly_authenticated(signed_msg, CONTEXT,
+ 'unauthenticated-bug-creation.txt')
         if len(commands) > 0:
             ensure_not_weakly_authenticated(signed_msg, CONTEXT)
         if to_user.lower() == 'new':
             # A submit request.
- ensure_not_weakly_authenticated(signed_msg, CONTEXT,
- 'not-gpg-signed.txt')
             commands.insert(0, BugEmailCommands.get('bug', ['new']))
         elif to_user.isdigit():
             # A comment to a bug. We set add_comment_to_bug to True so

=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py 2010-09-20 06:40:41 +0000
+++ lib/canonical/launchpad/mail/helpers.py 2010-10-01 08:16:02 +0000
@@ -211,7 +211,11 @@
 def ensure_not_weakly_authenticated(signed_msg, context,
                                     error_template='not-signed.txt',
                                     no_key_template='key-not-registered.txt'):
- """Make sure that the current principal is not weakly authenticated."""
+ """Make sure that the current principal is not weakly authenticated.
+
+ NB: This mixes checking properties of the message with properties of the
+ current principal; it's assumed that this message was just received.
+ """
     cur_principal = get_current_principal()
     # The security machinery doesn't know about
     # IWeaklyAuthenticatedPrincipal yet, so do a manual

=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt 2010-10-01 07:07:03 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt 2010-10-01 08:16:02 +0000
@@ -339,13 +339,15 @@
     ... IWeaklyAuthenticatedPrincipal)
     >>> from zope.interface import directlyProvides, directlyProvidedBy
     >>> from zope.security.management import queryInteraction
- >>> participations = queryInteraction().participations
- >>> len(participations)
- 1
- >>> current_principal = participations[0].principal
- >>> directlyProvides(
- ... current_principal, directlyProvidedBy(current_principal),
- ... IWeaklyAuthenticatedPrincipal)
+
+ >>> def simulate_receiving_untrusted_mail():
+ ... participations = queryInteraction().participations
+ ... assert len(participations) == 1
+ ... current_principal = participations[0].principal
+ ... directlyProvides(
+ ... current_principal, directlyProvidedBy(current_principal),
+ ... IWeaklyAuthenticatedPrincipal)
+ >>> simulate_receiving_untrusted_mail()

 Now we send a comment containing commands.

@@ -382,6 +384,8 @@

     >>> def print_latest_email():
     ... commit()
+ ... if not stub.test_emails:
+ ... raise AssertionError("No emails queued!")
     ... from_addr, to_addrs, raw_message = stub.test_emails[-1]
     ... sent_msg = email.message_from_string(raw_message)
     ... error_mail, original_mail = sent_msg.get_payload()
@@ -454,12 +458,9 @@
     >>> added_message in bug_one.messages
     True

-Unmark the principal:
+In these tests, every time we log in, we're fully trusted again:

- >>> provided_interfaces = directlyProvidedBy(current_principal)
- >>> directlyProvides(
- ... current_principal,
- ... provided_interfaces - IWeaklyAuthenticatedPrincipal)
+ >>> login('<email address hidden>')

 Commands
@@ -1720,6 +1721,7 @@
 If none of the bug tasks can be chosen, an error message is sent to the
 user, telling him that he has to use the 'affects' command.

+ >>> del stub.test_emails[:]
     >>> login('<email address hidden>')
     >>> submit_commands(
     ... bug_one, 'status new', 'assignee <email address hidden>')
@@ -1750,7 +1752,9 @@
 him about the error. Let's start with trying to submit a bug without
 signing the mail:

+ >>> del stub.test_emails[:]
     >>> login('<email address hidden>')
+ >>> simulate_receiving_untrusted_mail()

     >>> from canonical.launchpad.mail import signed_message_from_string
     >>> msg = signed_message_from_string(submit_mail)
@@ -1771,6 +1775,7 @@

 A submit without specifying on what we want to file the bug on:

+ >>> login('<email address hidden>')
     >>> submit_mail_no_bugtask = """From: <email address hidden>
     ... To: new@malone
     ... Date: Fri Jun 17 10:20:23 BST 2005

=== modified file 'utilities/migrater/file-ownership.txt'
--- utilities/migrater/file-ownership.txt 2009-12-02 15:30:54 +0000
+++ utilities/migrater/file-ownership.txt 2010-10-01 08:16:02 +0000
@@ -1053,7 +1053,7 @@
     ./mail/errortemplates/num-arguments-mismatch.txt
     ./mail/errortemplates/no-such-bug.txt
     ./mail/errortemplates/security-parameter-mismatch.txt
- ./mail/errortemplates/not-gpg-signed.txt
+ ./mail/errortemplates/unauthenticated-bug-creation.txt
     ./mail/errortemplates/key-not-registered.txt
     ./mail/errortemplates/oops.txt
     ./mail/errortemplates/branchmergeproposal-exists.txt