launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/email-authentication-type-error into lp:launchpad.

Commit message:
Updates authenticateEmail to gracefully handle TypeErrors from badly formed email addresses.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #924292 in Launchpad itself: "TypeError raised during email authentication when the email contains non-ascii characters"
  https://bugs.launchpad.net/launchpad/+bug/924292

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/email-authentication-type-error/+merge/131282

Summary
=======
authenticateEmail crashes on bad email address (e.g. ones that are not pure
unicode but also not US-ASCII). While we can't process them, we shouldn't
crash either.

The better approach is to simply proceed without having been able to
authenticate, and set an anauthenticated principal as we do with unknown
emails.

Implementation
==============
The call in `authenticateEmail` to `getPrincipalByLogin` is now in a
try/except block, which handles `TypeError` by setting principal to None. A
principal of None is a case which the rest of the code already handles
gracefully.

Tests
=====
bin/test -vvct test_badly_formed_email

QA
==
I'm actually unsure of how to QA this.

My belief is, as it requires generating a message with a bad mail and sending
it into the system, that this is not testable.

Suggestions are welcome.

LoC
===
I have approx 400 LoC credit.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/mail/incoming.py
  lib/lp/services/mail/tests/test_incoming.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/email-authentication-type-error/+merge/131282
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2012-09-20 05:59:58 +0000
+++ lib/lp/services/mail/incoming.py	2012-10-24 20:40:30 +0000
@@ -258,7 +258,11 @@
     principal, dkim_trusted_address = _getPrincipalByDkim(mail)
     if dkim_trusted_address is None:
         from_addr = parseaddr(mail['From'])[1]
-        principal = authutil.getPrincipalByLogin(from_addr)
+        try:
+            principal = authutil.getPrincipalByLogin(from_addr)
+        except TypeError:
+            # The email isn't valid, so don't authenticate
+            principal = None
 
     if principal is None:
         setupInteraction(authutil.unauthenticatedPrincipal())

=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py	2012-09-20 05:44:34 +0000
+++ lib/lp/services/mail/tests/test_incoming.py	2012-10-24 20:40:30 +0000
@@ -126,6 +126,12 @@
         mail = self.factory.makeSignedMessage(email_address=unknown)
         self.assertThat(authenticateEmail(mail), Is(None))
 
+    def test_badly_formed_email(self):
+        # A badly formed email address returns no principal.
+        bad = '\xed@xxxxxxxxxxx'
+        mail = self.factory.makeSignedMessage(email_address=bad)
+        self.assertThat(authenticateEmail(mail), Is(None))
+
 
 class TestExtractAddresses(TestCaseWithFactory):