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

On 1 October 2010 23:33, Jonathan Lange <email address hidden> wrote:
> On Fri, Oct 1, 2010 at 9:20 AM, Martin Pool <email address hidden> wrote:
>> 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:
>>
>
> Thanks for persisting with this. There are a couple of points where
> I'm not clear on what's going on and a couple of minor tweaks.
>
>> === 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')
>
> Why do you need to two clauses for "to_user.lower() == 'new'"? I can't
> figure it out from the code. Perhaps there should be a comment.

I'll add a comment. It's because we need to check for authentication
on mails that either attempt to create a new bug or that have commands
that modify an existing bug, and we give a different error in either
case. After doing this, we must check the recipient address is
recognized, and that again includes the case of 'new@'.

Perhaps I should try harder to refactor this...?

>>         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.
>> +    """
>
> I can't say the expansion clears things up much for me.

Well, thanks for the feedback.

The problem I'm trying to warn people of is this: when we're running
code to handle an incoming message, we have some state about who
claimed to send the message, whether they were strongly or weakly
authenticated, and the mechanism of authentication. Some of these are
stored on the thread-local state of the current security principal and
some are attributes of the message; the tests had a latent bug where
during the test the two would not be synchronized in the same way
there were in real use.

>
>> === 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
> ...
>> @@ -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>')
>>
>
> Could you please use lp.testing.sampledata.USER_EMAIL instead of
> '<email address hidden>'? We're trying to kill off some of our more
> gratuitous magic literals.

sure, thanks

>
> ...
>> @@ -1771,6 +1775,7 @@
>>
>>  A submit without specifying on what we want to file the bug on:
>>
>> +    >>> login('<email address hidden>')
>
> Again here please.

Thanks for the review.

--
Martin