Launchpad itself

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

  1. unique-openid-identifiers-0
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Approved by: Aaron Bentley
Approved revision: no longer in the source branch.
Merged at revision: 11043
Proposed branch: lp:~sinzui/launchpad/unique-openid-identifiers-0
Merge into: lp:launchpad
Diff against target: 140 lines (+71/-7)
3 files modified
lib/lp/registry/doc/person-merge.txt (+1/-1)
lib/lp/registry/model/person.py (+10/-5)
lib/lp/registry/tests/test_person.py (+60/-1)
To merge this branch: bzr merge lp:~sinzui/launchpad/unique-openid-identifiers-0
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve
Review via email: mp+28081@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) 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."

Revision history for this message
Curtis Hovey (sinzui) wrote :
Download full text (4.9 KiB)

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

Read more...

Revision history for this message
Aaron Bentley (abentley) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/doc/person-merge.txt'
2--- lib/lp/registry/doc/person-merge.txt 2010-05-07 15:44:19 +0000
3+++ lib/lp/registry/doc/person-merge.txt 2010-06-21 21:03:31 +0000
4@@ -205,7 +205,7 @@
5 if needed.
6
7 >>> print marilize.account.openid_identifier
8- merged-marilize_oid
9+ merged-marilize_oid-1118028591.0
10
11 An email is sent to the user informing him that he should review his email
12 and mailing list subscription settings.
13
14=== modified file 'lib/lp/registry/model/person.py'
15--- lib/lp/registry/model/person.py 2010-06-02 15:59:47 +0000
16+++ lib/lp/registry/model/person.py 2010-06-21 21:03:31 +0000
17@@ -30,6 +30,7 @@
18 import pytz
19 import random
20 import re
21+import time
22 import weakref
23
24 from bzrlib.plugins.builder import RecipeParser
25@@ -3517,13 +3518,17 @@
26
27 # Change the merged Account's OpenID identifier so that it cannot be
28 # used to lookup the merged Person; Launchpad will instead select the
29- # remaining Person based on the email address.
30+ # remaining Person based on the email address. The rename uses a
31+ # dash, which does not occur in SSO identifiers. The epoch from
32+ # the account date_created is used to ensure it is unique if the
33+ # original identifier is reused and merged.
34 if from_person.account is not None:
35 account = IMasterObject(from_person.account)
36- # This works because dashes do not occur in SSO identifiers.
37- merge_identifier = 'merged-%s' % removeSecurityProxy(
38- account).openid_identifier
39- removeSecurityProxy(account).openid_identifier = merge_identifier
40+ naked_account = removeSecurityProxy(account)
41+ unique_part = time.mktime(naked_account.date_created.timetuple())
42+ merge_identifier = 'merged-%s-%s' % (
43+ naked_account.openid_identifier, unique_part)
44+ naked_account.openid_identifier = merge_identifier
45
46 # Inform the user of the merge changes.
47 if not to_person.isTeam():
48
49=== modified file 'lib/lp/registry/tests/test_person.py'
50--- lib/lp/registry/tests/test_person.py 2010-06-07 18:09:00 +0000
51+++ lib/lp/registry/tests/test_person.py 2010-06-21 21:03:31 +0000
52@@ -6,6 +6,7 @@
53 import unittest
54 from datetime import datetime
55 import pytz
56+import time
57
58 import transaction
59
60@@ -14,6 +15,7 @@
61 from zope.security.proxy import removeSecurityProxy
62
63 from canonical.database.sqlbase import cursor
64+from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
65 from canonical.launchpad.ftests import ANONYMOUS, login
66 from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
67 from lp.bugs.interfaces.bug import CreateBugParams, IBugSet
68@@ -37,7 +39,7 @@
69 from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
70 from lp.answers.model.answercontact import AnswerContact
71 from lp.blueprints.model.specification import Specification
72-from lp.testing import TestCaseWithFactory
73+from lp.testing import login_person, logout, TestCaseWithFactory
74 from lp.testing.views import create_initialized_view
75 from lp.registry.interfaces.mailinglist import MailingListStatus
76 from lp.registry.interfaces.person import PrivatePersonLinkageError
77@@ -479,6 +481,63 @@
78 self.assertEqual(person1, person2)
79
80
81+class TestPersonSetMerge(TestCaseWithFactory):
82+ """Test cases for PersonSet merge."""
83+
84+ layer = DatabaseFunctionalLayer
85+
86+ def setUp(self):
87+ super(TestPersonSetMerge, self).setUp()
88+ self.person_set = getUtility(IPersonSet)
89+
90+ def _do_premerge(self, from_person, to_person):
91+ # Do the pre merge work performed by the LoginToken.
92+ login('admin@canonical.com')
93+ email = from_person.preferredemail
94+ email.status = EmailAddressStatus.NEW
95+ email.person = to_person
96+ email.account = to_person.account
97+ transaction.commit()
98+ logout()
99+
100+ def _get_testible_account(self, person, date_created, openid_identifier):
101+ # Return a naked account with predicable attributes.
102+ account = removeSecurityProxy(person.account)
103+ account.date_created = date_created
104+ account.openid_identifier = openid_identifier
105+ return account
106+
107+ def test_reused_openid_identifier(self):
108+ # Verify that an account can be merged when it has a reused OpenID
109+ # identifier. eg. The identifier was freed by a previous merge.
110+ test_identifier = 'Z1Y2X3W4'
111+ test_date = datetime(
112+ 2010, 04, 01, 0, 0, 0, 0, tzinfo=pytz.timezone('UTC'))
113+ # Free an OpenID identifier using merge.
114+ first_duplicate = self.factory.makePerson()
115+ first_account = self._get_testible_account(
116+ first_duplicate, test_date, test_identifier)
117+ first_person = self.factory.makePerson()
118+ self._do_premerge(first_duplicate, first_person)
119+ login_person(first_person)
120+ self.person_set.merge(first_duplicate, first_person)
121+ expected = 'merged-%s-%s' % (
122+ test_identifier, time.mktime(test_date.timetuple()))
123+ self.assertEqual(expected, first_account.openid_identifier)
124+ # Create an account that reuses the freed OpenID_identifier.
125+ test_date = test_date.replace(2010, 05)
126+ second_duplicate = self.factory.makePerson()
127+ second_account = self._get_testible_account(
128+ second_duplicate, test_date, test_identifier)
129+ second_person = self.factory.makePerson()
130+ self._do_premerge(second_duplicate, second_person)
131+ login_person(second_person)
132+ self.person_set.merge(second_duplicate, second_person)
133+ expected = 'merged-%s-%s' % (
134+ test_identifier, time.mktime(test_date.timetuple()))
135+ self.assertEqual(expected, second_account.openid_identifier)
136+
137+
138 class TestCreatePersonAndEmail(unittest.TestCase):
139 """Test `IPersonSet`.createPersonAndEmail()."""
140 layer = DatabaseFunctionalLayer