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

On 8 October 2010 12:19, Martin Pool <email address hidden> wrote:
> 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:

>>> === 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...?

I think it's a bit simpler now. You could tear it apart entirely
which could be nice but this branch is old...

>>> @@ -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.

Cleaner like this:

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.

    NB: While handling an email, the authentication state is stored partly in
    properties of the message object, and partly in the current security
    principal. As a consequence this function will only work correctly if the
    message has just been passed through authenticateEmail -- you can't give
    it an arbitrary message object.
    """

>> 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

I've removed all the occurrences of it as a standalone quoted string
from that test. Where it occurs within doctest output, I'm not sure
what would be best. (I didn't add any of them iirc.) Perhaps it
should wait for this to be rewritten away from doctest?

--
Martin