launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/process-email-oops-on-person-adaptation into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #728132 OOPS in process-mail adapting principle to person
  https://bugs.launchpad.net/bugs/728132

For more details, see:
https://code.launchpad.net/~thumper/launchpad/process-email-oops-on-person-adaptation/+merge/51992

I found this while looking through the process mail log file.
-- 
https://code.launchpad.net/~thumper/launchpad/process-email-oops-on-person-adaptation/+merge/51992
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/process-email-oops-on-person-adaptation into lp:launchpad.

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2010-12-14 03:48:06 +0000
+++ lib/lp/services/mail/incoming.py	2011-03-03 01:02:11 +0000
@@ -186,9 +186,14 @@
     # Check that sender is registered in Launchpad and the email is signed.
     if principal is None:
         setupInteraction(authutil.unauthenticatedPrincipal())
-        return
+        return None
 
-    person = IPerson(principal)
+    # People with accounts but no related person will have a principle, but
+    # the person adaptation will fail.
+    person = IPerson(principal, None)
+    if person is None:
+        setupInteraction(authutil.unauthenticatedPrincipal())
+        return None
 
     if person.account_status != AccountStatus.ACTIVE:
         raise InactiveAccount(

=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py	2010-12-01 06:15:29 +0000
+++ lib/lp/services/mail/tests/test_incoming.py	2011-03-03 01:02:11 +0000
@@ -6,6 +6,7 @@
 import os
 import unittest
 
+from testtools.matchers import Is
 import transaction
 from zope.security.management import setSecurityPolicy
 
@@ -104,6 +105,19 @@
             authenticateEmail,
             msg, fail_all_timestamps)
 
+    def test_unknown_email(self):
+        # An unknown email address returns no principle.
+        unknown = 'random-unknown@xxxxxxxxxxx'
+        mail = self.factory.makeSignedMessage(email_address=unknown)
+        self.assertThat(authenticateEmail(mail), Is(None))
+
+    def test_accounts_without_person(self):
+        # An account without a person should be the same as an unknown email.
+        email = 'non-person@xxxxxxxxxxx'
+        self.factory.makeAccount(email=email)
+        mail = self.factory.makeSignedMessage(email_address=email)
+        self.assertThat(authenticateEmail(mail), Is(None))
+
 
 def setUp(test):
     test._old_policy = setSecurityPolicy(LaunchpadSecurityPolicy)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-03-02 17:48:28 +0000
+++ lib/lp/testing/factory.py	2011-03-03 01:02:11 +0000
@@ -502,10 +502,12 @@
             pocket)
         return ProxyFactory(location)
 
-    def makeAccount(self, displayname, email=None, password=None,
+    def makeAccount(self, displayname=None, email=None, password=None,
                     status=AccountStatus.ACTIVE,
                     rationale=AccountCreationRationale.UNKNOWN):
         """Create and return a new Account."""
+        if displayname is None:
+            displayname = self.getUniqueString('displayname')
         account = getUtility(IAccountSet).new(
             rationale, displayname, password=password)
         removeSecurityProxy(account).status = status