Merge lp:~benji/launchpad/bug-612754-2 into lp:launchpad

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

On Wed, Oct 6, 2010 at 6:54 PM, Edwin Grubbs <email address hidden> wrote:
> Review: Needs Fixing code
> Hi Benji,
>
> I think the regex and the test need some work to handle trailing whitespace
> on any line.

That's odd. I had made a change along those lines before even
submitting the branch for review but the diff on the MP doesn't reflect
the change.

Darn! I forgot to push after commit. The more appropriate regex and
test should be visible after the diff updates.
--
Benji York

Hi Benji,

It looks so much better since I have all your revisions.

-Edwin