Code review comment for lp:~benji/launchpad/bug-612754-2

Hi Benji,

I think the regex and the test need some work to handle trailing whitespace on any line.

-Edwin

>=== modified file 'lib/canonical/launchpad/mail/incoming.py'
>--- lib/canonical/launchpad/mail/incoming.py 2010-10-03 15:30:06 +0000
>+++ lib/canonical/launchpad/mail/incoming.py 2010-10-06 22:26:05 +0000
>@@ -55,9 +55,11 @@
> from lp.services.mail.signedmessage import signed_message_from_string
>
> # Match '\n' and '\r' line endings. That is, all '\r' that are not
>-# followed by a # '\n', and all '\n' that are not preceded by a '\r'.
>+# followed by a '\n', and all '\n' that are not preceded by a '\r'.
> non_canonicalised_line_endings = re.compile('((?<!\r)\n)|(\r(?!\n))')
>
>+# Match trailing whitespace.
>+trailing_whitespace = re.compile(' *$')

Since you are not using rstrip(), I assume you want the regex to
strip the whitespace at the end of each line. In that case, you need
to tell re.compile() that you want to use a multiline expression.
Also, " *$" will not match " \r\n".

> def canonicalise_line_endings(text):
> r"""Canonicalise the line endings to '\r\n'.
>@@ -73,6 +75,8 @@
> """
> if non_canonicalised_line_endings.search(text):
> text = non_canonicalised_line_endings.sub('\r\n', text)
>+ if trailing_whitespace.search(text):
>+ text = trailing_whitespace.sub('', text)
> return text
>
>
>
>=== modified file 'lib/lp/services/mail/tests/test_signedmessage.py'
>--- lib/lp/services/mail/tests/test_signedmessage.py 2010-08-20 20:31:18 +0000
>+++ lib/lp/services/mail/tests/test_signedmessage.py 2010-10-06 22:26:05 +0000
>@@ -59,17 +59,18 @@
> IWeaklyAuthenticatedPrincipal.providedBy(principle))
> self.assertIs(None, msg.signature)
>
>- def _get_clearsigned_for_person(self, sender):
>+ def _get_clearsigned_for_person(self, sender, body=None):
> # Create a signed message for the sender specified with the test
> # secret key.
> key = import_secret_test_key()
> signing_context = GPGSigningContext(key.fingerprint, password='test')
>- body = dedent("""\
>- This is a multi-line body.
>+ if body is None:
>+ body = dedent("""\
>+ This is a multi-line body.
>
>- Sincerely,
>- Your friendly tester.
>- """)
>+ Sincerely,
>+ Your friendly tester.
>+ """)
> msg = self.factory.makeSignedMessage(
> email_address=sender.preferredemail.email,
> body=body, signing_context=signing_context)
>@@ -97,6 +98,18 @@
> self.assertFalse(
> IWeaklyAuthenticatedPrincipal.providedBy(principle))
>
>+ def test_trailing_whitespace(self):
>+ # Trailing whitespace should be ignored when verifying a message's
>+ # signature.
>+ sender = getUtility(IPersonSet).getByEmail('<email address hidden>')
>+ body = 'A message with trailing whitespace. '

A multiline body with trailing whitespace on each line will fail.

>+ msg = self._get_clearsigned_for_person(sender, body)
>+ principle = authenticateEmail(msg)
>+ self.assertIsNot(None, msg.signature)
>+ self.assertEqual(sender, principle.person)
>+ self.assertFalse(
>+ IWeaklyAuthenticatedPrincipal.providedBy(principle))
>+
> def _get_detached_message_for_person(self, sender):
> # Return a signed message that contains a detached signature.
> body = dedent("""\
>