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

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.

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

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

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

Again here please.

jml