Merge lp:~sinzui/launchpad/unique-openid-identifiers-0 into lp:launchpad

[Curtis Hovey]

This is my branch to ensure openid identifiers are unique after a merge.

    lp:~sinzui/launchpad/unique-openid-identifiers-0
    Diff size: 48
    Launchpad bug:
          https://bugs.launchpad.net/bugs/595164
    Test command: ./bin/test -vv \
          -t person-merge.txt
    Pre-implementation: no one
    Target release: 10.06

Ensure openid identifiers are unique after a merge
---------------------------------------------------

As seen on OOPS-1626B636 an IntegrityError: duplicate key value violates
unique constraint "account_openid_identifier_key" was raised while the user
was trying to merge accounts

...

It is possible that the account identifier was once used, someone merge the
account, This user got the same identifier, then tried a merge. This is a
legitimate scenario, though I would have expected such an event to be unlikely
to happen in 10 years--it happened in 2 weeks.

...

I have looked at the data synced on staging and I can confirm that the openid
identifier was indeed reused after it was freed and renamed to 'merged-*'. The
code needs a mechanism to create a unique merged openid identifier. I propose
we use the datetime to ensure no collision.

Rules
-----

    * Update Person.merge() to add a sequence to the renamed identifier
      to guarantee that if a reused identifier is merged, it will not
      collide with a previously merged identifier:
      merge_identifier = 'merged-%s-%' % (identifier, unique_sequence)
    * Use seconds from the epoch for the sequence because we do not need
      to guess a sequence number...we know the identifier was unique when
      the account was created.

QA
--

    * Ask Oleg Trifonov to join the beta team to gain access to edge.
    * Ask Oleg to merge his two accounts.

Lint
----

Linting changed files:
  lib/lp/registry/doc/person-merge.txt
  lib/lp/registry/model/person.py

Test
----

    * lib/lp/registry/doc/person-merge.txt
      * Updated the test to verify the epoch seconds are in the merged
        openid identifier string.

Implementation
--------------

    * lib/lp/registry/model/person.py
      * Created epoch seconds from the accounts date_created.

[Aaron Bentley (abentley)]

This looks good so far, but it needs a unit test.

Also, I think "dash which does not occur in SSO identifier." should be "dash, which does not occur in SSO identifiers."

[Curtis Hovey (sinzui)]

Hi Aaron.

Thanks for the review.

On Mon, 2010-06-21 at 19:26 +0000, Aaron Bentley wrote:
> This looks good so far, but it needs a unit test.
>
> Also, I think "dash which does not occur in SSO identifier." should be
> "dash, which does not occur in SSO identifiers."

Incremental diff:

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-06-21 16:47:10 +0000
+++ lib/lp/registry/model/person.py 2010-06-21 20:24:02 +0000
@@ -3519,7 +3519,7 @@
         # Change the merged Account's OpenID identifier so that it cannot be
         # used to lookup the merged Person; Launchpad will instead select the
         # remaining Person based on the email address. The rename uses a
- # dash which does not occur in SSO identifier. The epoch from
+ # dash, which does not occur in SSO identifiers. The epoch from
         # the account date_created is used to ensure it is unique if the
         # original identifier is reused and merged.
         if from_person.account is not None:

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2010-06-07 18:09:00 +0000
+++ lib/lp/registry/tests/test_person.py 2010-06-21 20:58:28 +0000
@@ -6,6 +6,7 @@
 import unittest
 from datetime import datetime
 import pytz
+import time

 import transaction

@@ -14,6 +15,7 @@
 from zope.security.proxy import removeSecurityProxy

 from canonical.database.sqlbase import cursor
+from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
 from canonical.launchpad.ftests import ANONYMOUS, login
 from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
 from lp.bugs.interfaces.bug import CreateBugParams, IBugSet
@@ -37,7 +39,7 @@
 from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
 from lp.answers.model.answercontact import AnswerContact
 from lp.blueprints.model.specification import Specification
-from lp.testing import TestCaseWithFactory
+from lp.testing import login_person, logout, TestCaseWithFactory
 from lp.testing.views import create_initialized_view
 from lp.registry.interfaces.mailinglist import MailingListStatus
 from lp.registry.interfaces.person import PrivatePersonLinkageError
@@ -479,6 +481,63 @@
         self.assertEqual(person1, person2)

+class TestPersonSetMerge(TestCaseWithFactory):
+ """Test cases for PersonSet merge."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestPersonSetMerge, self).setUp()
+ self.person_set = getUtility(IPersonSet)
+
+ def _do_premerge(self, from_person, to_person):
+ # Do the pre merge work performed by the LoginToken.
+ login('<email address hidden>')
+ email = from_person.preferredemail
+ email.status = EmailAddressStatus.NEW
+ email.person = to_person
+ email.account = to_person.account
+ transaction.commit()
+ logout()
+
+ def _get_testible_account(self, person, date_created, openid_identifier):
+ # Return a naked account with predicable attributes.
+ account = removeSecurityProxy(person.account)
+ account.date_created = date_created
+ account.openid_identifier ...

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'lib/lp/registry/doc/person-merge.txt'
--- lib/lp/registry/doc/person-merge.txt	2010-05-07 15:44:19 +0000
+++ lib/lp/registry/doc/person-merge.txt	2010-06-21 21:03:31 +0000
@@ -205,7 +205,7 @@
 if needed.
 
     >>> print marilize.account.openid_identifier
-    merged-marilize_oid
+    merged-marilize_oid-1118028591.0
 
 An email is sent to the user informing him that he should review his email
 and mailing list subscription settings.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-06-02 15:59:47 +0000
+++ lib/lp/registry/model/person.py	2010-06-21 21:03:31 +0000
@@ -30,6 +30,7 @@
 import pytz
 import random
 import re
+import time
 import weakref
 
 from bzrlib.plugins.builder import RecipeParser
@@ -3517,13 +3518,17 @@
 
         # Change the merged Account's OpenID identifier so that it cannot be
         # used to lookup the merged Person; Launchpad will instead select the
-        # remaining Person based on the email address.
+        # remaining Person based on the email address. The rename uses a
+        # dash, which does not occur in SSO identifiers. The epoch from
+        # the account date_created is used to ensure it is unique if the
+        # original identifier is reused and merged.
         if from_person.account is not None:
             account = IMasterObject(from_person.account)
-            # This works because dashes do not occur in SSO identifiers.
-            merge_identifier = 'merged-%s' % removeSecurityProxy(
-                account).openid_identifier
-            removeSecurityProxy(account).openid_identifier = merge_identifier
+            naked_account = removeSecurityProxy(account)
+            unique_part = time.mktime(naked_account.date_created.timetuple())
+            merge_identifier = 'merged-%s-%s' % (
+                naked_account.openid_identifier, unique_part)
+            naked_account.openid_identifier = merge_identifier
 
         # Inform the user of the merge changes.
         if not to_person.isTeam():

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-06-07 18:09:00 +0000
+++ lib/lp/registry/tests/test_person.py	2010-06-21 21:03:31 +0000
@@ -6,6 +6,7 @@
 import unittest
 from datetime import datetime
 import pytz
+import time
 
 import transaction
 
@@ -14,6 +15,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.database.sqlbase import cursor
+from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
 from canonical.launchpad.ftests import ANONYMOUS, login
 from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
 from lp.bugs.interfaces.bug import CreateBugParams, IBugSet
@@ -37,7 +39,7 @@
 from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
 from lp.answers.model.answercontact import AnswerContact
 from lp.blueprints.model.specification import Specification
-from lp.testing import TestCaseWithFactory
+from lp.testing import login_person, logout, TestCaseWithFactory
 from lp.testing.views import create_initialized_view
 from lp.registry.interfaces.mailinglist import MailingListStatus
 from lp.registry.interfaces.person import PrivatePersonLinkageError
@@ -479,6 +481,63 @@
         self.assertEqual(person1, person2)
 
 
+class TestPersonSetMerge(TestCaseWithFactory):
+    """Test cases for PersonSet merge."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonSetMerge, self).setUp()
+        self.person_set = getUtility(IPersonSet)
+
+    def _do_premerge(self, from_person, to_person):
+        # Do the pre merge work performed by the LoginToken.
+        login('[e-mail]')
+        email = from_person.preferredemail
+        email.status = EmailAddressStatus.NEW
+        email.person = to_person
+        email.account = to_person.account
+        transaction.commit()
+        logout()
+
+    def _get_testible_account(self, person, date_created, openid_identifier):
+        # Return a naked account with predicable attributes.
+        account = removeSecurityProxy(person.account)
+        account.date_created = date_created
+        account.openid_identifier = openid_identifier
+        return account
+
+    def test_reused_openid_identifier(self):
+        # Verify that an account can be merged when it has a reused OpenID
+        # identifier. eg. The identifier was freed by a previous merge.
+        test_identifier = 'Z1Y2X3W4'
+        test_date = datetime(
+            2010, 04, 01, 0, 0, 0, 0, tzinfo=pytz.timezone('UTC'))
+        # Free an OpenID identifier using merge.
+        first_duplicate = self.factory.makePerson()
+        first_account = self._get_testible_account(
+            first_duplicate, test_date, test_identifier)
+        first_person = self.factory.makePerson()
+        self._do_premerge(first_duplicate, first_person)
+        login_person(first_person)
+        self.person_set.merge(first_duplicate, first_person)
+        expected = 'merged-%s-%s' % (
+            test_identifier, time.mktime(test_date.timetuple()))
+        self.assertEqual(expected, first_account.openid_identifier)
+        # Create an account that reuses the freed OpenID_identifier.
+        test_date = test_date.replace(2010, 05)
+        second_duplicate = self.factory.makePerson()
+        second_account = self._get_testible_account(
+            second_duplicate, test_date, test_identifier)
+        second_person = self.factory.makePerson()
+        self._do_premerge(second_duplicate, second_person)
+        login_person(second_person)
+        self.person_set.merge(second_duplicate, second_person)
+        expected = 'merged-%s-%s' % (
+            test_identifier, time.mktime(test_date.timetuple()))
+        self.assertEqual(expected, second_account.openid_identifier)
+
+
 class TestCreatePersonAndEmail(unittest.TestCase):
     """Test `IPersonSet`.createPersonAndEmail()."""
     layer = DatabaseFunctionalLayer