Merge lp:~canonical-isd-hackers/canonical-identity-provider/862944-preferred-email-constraint into lp:canonical-identity-provider/release

[Simon Davy]

Fixes 862944 by also looking for old LP emails to use for the preferred email

Fix for bug https://bugs.launchpad.net/canonical-identity-provider/+bug/862944

Problem arising from SSO model identity crisis:
  - user has account '1' with no PREFERRED emails
  - account '1' is linked to person 'A'
  - person 'A' also has links to account '2'
  - there is a preferred email for set for person 'A' for account '2'
  - when attempting to log in to account '1', code attempts to set the latest account '1' email to PREFERRED
  - but there is a constraint that a Person can only have one preferred email - see bug

This fixes the issue by checking both Account and Person for preferred emails.

The only question I have is whether it is valid to use a Person-based email (from another account) as the preferred email? Should we also be changing/removing the additional constraint?

[Simon Davy (bloodearnest)]

hmm - I don't think this is ready yet. Need to add some tests and think through implications of using an email that is the preferred email for to a different account for the same person

[Simon Davy (bloodearnest)]

> hmm - I don't think this is ready yet. Need to add some tests and think
> through implications of using an email that is the preferred email for to a
> different account for the same person

Done!

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

=== modified file 'identityprovider/models/account.py'
--- identityprovider/models/account.py	2011-08-25 14:10:04 +0000
+++ identityprovider/models/account.py	2011-11-24 13:59:26 +0000
@@ -1,18 +1,21 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
-
+import logging
 from datetime import date, datetime, timedelta
 
 from django.conf import settings
 from django.contrib.auth import get_backends
 from django.contrib.auth.models import User
 from django.db import models
+from django.db.models import Q
 from django.utils.translation import ugettext_lazy as _
 
 from oauth_backend.models import Consumer
 
 from identityprovider.models.const import (AccountCreationRationale,
                                            AccountStatus, EmailStatus)
+
+
 from identityprovider.utils import (
     encrypt_launchpad_password,
     generate_openid_identifier,
@@ -130,22 +133,35 @@
         return sites.order_by('-date_last_used')
 
     def _get_preferredemail(self):
+        # import here to avoid circular dependancy issue
+        from identityprovider.models.emailaddress import EmailAddress
         if not hasattr(self, '_preferredemail'):
             try:
-                self._preferredemail = self.emailaddress_set.filter(
-                    status=EmailStatus.PREFERRED)[0]
-            except IndexError:
-                # Try to determine a suitable address, and mark it as preferred
-                try:
+                # note emails coming via self.person are legacy pre-sso
+                # launchpad emails
+                account_emails = self.emailaddress_set.filter(
+                    status=EmailStatus.PREFERRED)
+                if account_emails.count() > 0:
+                    self._preferredemail = account_emails[0]
+                elif self.person:
+                    self._preferredemail = self.person.emailaddress_set.filter(
+                        status=EmailStatus.PREFERRED)[0]
+                    logging.warn("Old LP email used as preferred email, " 
+                                 "needs migrating. Account id: %s" % self.id)
+                else:
+                    # Try to determine a suitable address, and mark it as preferred
+                    # note: we don't try to make a person-based email preferred
                     emails = self.emailaddress_set.filter(
                         status=EmailStatus.VALIDATED)
                     email = emails.order_by('date_created')[0]
                     email.status = EmailStatus.PREFERRED
                     email.save()
+                    logging.info(
+                        "Updating preferred email for account %s" % self.id)
                     self._preferredemail = email
-                except IndexError:
-                    # Now we really don't have anything
-                    self._preferredemail = None
+            except IndexError:
+                # Now we really don't have anything
+                self._preferredemail = None
         return self._preferredemail
 
     def _set_preferredemail(self, email):

=== modified file 'identityprovider/tests/test_models_account.py'
--- identityprovider/tests/test_models_account.py	2011-07-27 13:00:22 +0000
+++ identityprovider/tests/test_models_account.py	2011-11-24 13:59:26 +0000
@@ -11,9 +11,10 @@
 from oauth_backend.models import Consumer
 
 from identityprovider.models.account import (Account, AccountPassword,
-    AccountStatus)
+    AccountStatus, LPOpenIdIdentifier)
 from identityprovider.models import account as a
 from identityprovider.models.emailaddress import EmailAddress
+from identityprovider.models.person import Person
 from identityprovider.models.const import AccountCreationRationale, EmailStatus
 from identityprovider.readonly import ReadOnlyManager
 from identityprovider.utils import encrypt_launchpad_password, generate_salt
@@ -194,8 +195,6 @@
 
     def test_validated_email_address_is_preferred_email_by_default(self):
         account = self.create_account()
-        EmailAddress.objects.filter(
-            email__iexact='[e-mail]').delete()
         email_address = account.emailaddress_set.create(
             email='[e-mail]',
             status=EmailStatus.VALIDATED)
@@ -204,6 +203,34 @@
         email_address = EmailAddress.objects.get(pk=email_address.id)
         self.assertEqual(email_address.status, EmailStatus.PREFERRED)
 
+    def test_person_email_address_is_preferred_email(self):
+        account = self.create_account()
+        account.emailaddress_set.create(
+            email='[e-mail]',
+            status=EmailStatus.VALIDATED)
+        # account is set up with validated email
+        
+	# now set up person with preferred email, but no account
+        person = Person.objects.create(
+            name="Sample", 
+            lp_account=100)
+
+        # link person to account
+        LPOpenIdIdentifier.objects.create(
+            identifier=account.openid_identifier,
+            lp_account=person.lp_account)
+
+	# add an address to the person, but NOT the account
+        EmailAddress.objects.create(
+            email='[e-mail]',
+            account=None,
+            lp_person=person,
+            status=EmailStatus.PREFERRED
+        )
+
+        self.assertEqual(account.preferredemail.email, '[e-mail]')
+
+
     def test_verified_emails_has_preferred_first(self):
         account = Account.objects.get_by_email('[e-mail]')
         emails = account.verified_emails()