Merge lp:~michael.nelson/launchpad/db-598464-get-or-create-from-identity into lp:launchpad/db-devel

[Michael Nelson]

Overview
=======
This branch is part of the fix for bug 598464 - enabling the software center agent to get-or-create an LP person based on an openid identifier.

After a pre- (or mid-)implementation call with Francis, I've reworked the branch to re-use the existing code from webapp.login.processPositiveAssertion() which was doing nearly exactly what we needed.

Details
======
I pulled out the relevant code from processPositiveAssertion that is actually doing the getOrCreate... and put it on IPersonSet.

Based on the comment in the code:
{{{
      # The two lines below are duplicated a few more lines down,
      # but to avoid this duplication we'd have to refactor most of
      # our tests to provide an SREG response, which would be rather
      # painful.
}}}
I updated the login_test tests to always provide the SREG response, as initially I was always calling self._getEmailAddressAndFullName() in processPositiveAssertion() before passing the results into the IPersonSet.getOrCreateByOpenIDIdentifier() method.

Issues
=====
Always calling _getEmailAddressAndFullName() was fine for TestOpenIDCallbackView tests, but other tests failed, and when I checked, it seemed to be because when logging in, the simple registration (SREG) response is not always returned (verified this locally using the devserver). I had assumed it was always included when logging in.

For this reason, I had to make the changes in r9514 which I'm not particularly happy with, and I'd like to better understand why we need to do this (hence the request Guilherme :) ).

To test:
bin/test -vvm test_personset -m test_login

[Guilherme Salgado (salgado)]

Your general approach looks sane to me. I might be able to do a proper review today or tomorrow if you'd like.

[Michael Nelson (michael.nelson)]

{{{
13:55 < noodles775> Hi salgado, if you've got a spare few mins, can you take a look at the issue I've identified at: https://code.edge.launchpad.net/~michael
.nelson/launchpad/db-598464-get-or-create-from-identity/+merge/28885
13:55 < noodles775> Let me know if I'm not making sense on the MP.
13:56 < salgado> noodles775, sure, I have it in my inbox and will get to it shortly
13:56 < noodles775> salgado: thanks.
14:06 < salgado> noodles775, in which case is the sreg extension not included in the response?
14:07 < noodles775> salgado: normal login with existing account. ie. 1) start dev server, 2) click login 3) enter <email address hidden>:test 4) click login. In
 this case calling _getEmailAddressAndFullName raises the assert.
14:08 < noodles775> It's not seen in the current code as _getEmailAddressAndFullName is only called if the account doesn't exist.
14:09 -!- abentley [~<email address hidden>] has quit [Ping timeout: 260 seconds]
14:10 < salgado> noodles775, that's weird because the code that generates the positive openid response is including the sreg extension unconditionally
14:10 * salgado tries to dupe
14:11 < salgado> indeed, duped
14:11 -!- abentley [~<email address hidden>] has joined #launchpad-reviews
14:12 < noodles775> Yeah, I'd assumed the sreg response should always be there, and was surprised too.
14:14 < salgado> noodles775, but this is a bug in the testopenid implementation
14:14 < noodles775> Where should I look?
14:14 < salgado> testopenid == the OpenID provider we use when developing/testing
14:14 < salgado> I'm checking it now
14:14 < salgado> noodles775, lib/lp/testopenid/browser/server.py
14:15 < noodles775> OK, so we can be confident that the staging/production SSO always include the sreg.
14:15 < noodles775> Thanks.
14:15 < salgado> createPositiveResponse() adds the sreg extension, but it's empty
14:15 -!- jtv-afk [~jtv@canonical/launchpad/jtv] has quit [Quit: Leaving.]
14:16 < salgado> noodles775, I think I've found the problem
14:17 < salgado> noodles775, OpenIDLogin.render() asks for 'email' and 'fullname' in the sreg response
14:17 < salgado> bug testopenid ignores that and only includes the nickname
14:17 -!- rinze is now known as jelmer-lunch
14:17 < salgado> s/bug/but
14:18 < noodles775> Right... I see.
14:18 < noodles775> So should it instead include *only* the requested items (email/fullname) or in addition to the nickname?
14:19 -!- abentley [~<email address hidden>] has quit [Quit: Ex-Chat]
14:19 < salgado> I think it's fine to include the 3 of them, but the nickname shouldn't be used anywhere
14:20 < noodles775> OK, I'll update it... thanks salgado !
14:22 < noodles775> salgado: also, I assume you don't have time to review the actual branch, but if you could have a brief look and make sure you think it's a sane change, that would be great.
14:24 < salgado> noodles775, sure, I can do that
14:25 < noodles775> Ta.
14:27 < salgado> noodles775, out of curiosity, why somebody needs a launchpad account to buy stuff from the software center? wouldn't a Ubuntu SSO account be more app...

[Abel Deuring (adeuring)]

(12:04:05) adeuring: noodles775: overall, your code looks good. Just one question: SHouldn't the "with MasterDBPolicy():" be moved from processPositiveAssrtion() to getOrCreateByOpenidIdentifier()?
(12:06:19) noodles775: adeuring: yes, you're right.

And a minor nitpick:

> @@ -1754,6 +1761,33 @@
> on the displayname or other arguments.
> """
>
> + def getOrCreateByOpenIDIdentifier(openid_identifier, email,
> + full_name, creation_rationale, comment):
> + """Get or create a person for a given OpenID identifier.
> +
> + This is used when users login. We get the account with the given
> + OpenID identifier (creating one if it doesn't already exist) and
> + act according to the account's state:
> + - If the account is suspended, we stop and raise an error.
> + - If the account is deactivated, we reactivate it and proceed;
> + - If the account is active, we just proceed.
> +
> + If there is no existing Launchpad person for the account, we
> + create it.
> +
> + :param openid_identifier: representing the authenticated user.
> + :param email_address: the email address of the user.
> + :param full_name: the full name of the user.
> + :param creataion_rationale: When an account or person needs to
> + be created, this indicates why it was created.

s/creataion_rationale/creation_rationale/

review: Approve (code)

=== modified file 'lib/canonical/launchpad/database/account.py'
--- lib/canonical/launchpad/database/account.py	2010-05-07 20:02:33 +0000
+++ lib/canonical/launchpad/database/account.py	2010-07-02 10:41:03 +0000
@@ -190,7 +190,8 @@
             return False
         return self.preferredemail is not None
 
-    def createPerson(self, rationale, name=None, comment=None):
+    def createPerson(self, rationale, name=None, comment=None,
+                     registrant=None):
         """See `IAccount`."""
         # Need a local import because of circular dependencies.
         from lp.registry.model.person import (
@@ -204,7 +205,8 @@
             name = generate_nick(self.preferredemail.email)
         person = PersonSet()._newPerson(
             name, self.displayname, hide_email_addresses=True,
-            rationale=rationale, account=self, comment=comment)
+            rationale=rationale, account=self, comment=comment,
+            registrant=registrant)
 
         # Update all associated email addresses to point at the new person.
         result = IMasterStore(EmailAddress).find(

=== modified file 'lib/canonical/launchpad/interfaces/account.py'
--- lib/canonical/launchpad/interfaces/account.py	2009-12-24 11:06:04 +0000
+++ lib/canonical/launchpad/interfaces/account.py	2010-07-02 10:41:03 +0000
@@ -8,6 +8,7 @@
 
 __all__ = [
     'AccountStatus',
+    'AccountSuspendedError',
     'AccountCreationRationale',
     'IAccount',
     'IAccountPrivate',
@@ -27,6 +28,10 @@
 from lazr.restful.fields import CollectionField, Reference
 
 
+class AccountSuspendedError(Exception):
+    """The account being accessed has been suspended."""
+
+
 class AccountStatus(DBEnumeratedType):
     """The status of an account."""
 
@@ -179,6 +184,13 @@
         commented on.
         """)
 
+    SOFTWARE_CENTER_PURCHASE = DBItem(16, """
+        Created by purchasing commercial software through Software Center.
+
+        A purchase of commercial software (ie. subscriptions to a private
+        and commercial archive) was made via Software Center.
+        """)
+
 
 class IAccountPublic(Interface):
     """Public information on an `IAccount`."""
@@ -261,7 +273,7 @@
     password = PasswordField(
         title=_("Password."), readonly=False, required=True)
 
-    def createPerson(self, rationale, name=None, comment=None):
+    def createPerson(rationale, name=None, comment=None, registrant=None):
         """Create and return a new `IPerson` associated with this account.
 
         :param rationale: A member of `AccountCreationRationale`.
@@ -269,6 +281,7 @@
             using an automatically generated one.
         :param comment: Populate `IPerson.creation_comment`. See
             `IPerson`.
+        :param registrant: Populate `IPerson.registrant`.
         """
 
 

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-06-30 13:49:38 +0000
+++ lib/canonical/launchpad/security.py	2010-07-02 10:41:03 +0000
@@ -284,6 +284,14 @@
 class ViewAccount(EditAccountBySelfOrAdmin):
     permission = 'launchpad.View'
 
+    def checkAuthenticated(self, roles):
+        """Extend view permission to software-center-agent."""
+        if super(ViewAccount, self).checkAuthenticated(roles):
+            return True
+        else:
+            agent = getUtility(ILaunchpadCelebrities).software_center_agent
+            return agent == roles.person
+
 
 class SpecialAccount(EditAccountBySelfOrAdmin):
     permission = 'launchpad.Special'
@@ -590,6 +598,18 @@
     usedfor = IPersonSet
 
 
+class EditPersonSet(AdminByAdminsTeam):
+    permission = 'launchpad.Edit'
+    usedfor = IPersonSet
+
+    def checkAuthenticated(self, user):
+        if super(EditPersonSet, self).checkAuthenticated(user):
+            return True
+        else:
+            agent = getUtility(ILaunchpadCelebrities).software_center_agent
+            return user.person == agent
+
+
 class EditTeamByTeamOwnerOrLaunchpadAdmins(AuthorizationBase):
     permission = 'launchpad.Owner'
     usedfor = ITeam

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2010-05-15 17:43:59 +0000
+++ lib/canonical/launchpad/webapp/login.py	2010-07-02 10:41:03 +0000
@@ -32,10 +32,9 @@
 from canonical.cachedproperty import cachedproperty
 from canonical.config import config
 from canonical.launchpad import _
-from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
-from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
+from canonical.launchpad.interfaces.account import AccountSuspendedError
 from canonical.launchpad.interfaces.openidconsumer import IOpenIDConsumerStore
-from lp.registry.interfaces.person import IPerson, PersonCreationRationale
+from lp.registry.interfaces.person import IPersonSet, PersonCreationRationale
 from canonical.launchpad.readonly import is_read_only
 from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
 from canonical.launchpad.webapp.error import SystemErrorView
@@ -199,7 +198,7 @@
         return_to = "%s?%s" % (return_to, starting_url)
         form_html = self.openid_request.htmlMarkup(trust_root, return_to)
 
-        # The consumer.begin() call above will insert rows into the 
+        # The consumer.begin() call above will insert rows into the
         # OpenIDAssociations table, but since this will be a GET request, the
         # transaction would be rolled back, so we need an explicit commit
         # here.
@@ -278,23 +277,14 @@
             "No email address or full name found in sreg response")
         return email_address, full_name
 
-    def _createAccount(self, openid_identifier, email_address, full_name):
-        account, email = getUtility(IAccountSet).createAccountAndEmail(
-            email_address, PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
-            full_name, password=None, openid_identifier=openid_identifier)
-        return account
-
     def processPositiveAssertion(self):
         """Process an OpenID response containing a positive assertion.
 
-        We'll get the account with the given OpenID identifier (creating one 
-        if it doesn't already exist) and act according to the account's state:
-          - If the account is suspended, we stop and render an error page;
-          - If the account is deactivated, we reactivate it and proceed;
-          - If the account is active, we just proceed.
+        We'll get the person and account with the given OpenID
+        identifier (creating one if necessary), and then login using
+        that account.
 
-        After that we ensure there's an IPerson associated with the account
-        and login using that account.
+        If the account is suspended, we stop and render an error page.
 
         We also update the 'last_write' key in the session if we've done any
         DB writes, to ensure subsequent requests use the master DB and see
@@ -302,56 +292,23 @@
         """
         identifier = self.openid_response.identity_url.split('/')[-1]
         should_update_last_write = False
-        email_set = getUtility(IEmailAddressSet)
         # Force the use of the master database to make sure a lagged slave
         # doesn't fool us into creating a Person/Account when one already
         # exists.
+        person_set = getUtility(IPersonSet)
+        email_address, full_name = self._getEmailAddressAndFullName()
+        try:
+            person, db_updated = person_set.getOrCreateByOpenIDIdentifier(
+                identifier, email_address, full_name,
+                comment='when logging in to Launchpad.',
+                creation_rationale=(
+                    PersonCreationRationale.OWNER_CREATED_LAUNCHPAD))
+            should_update_last_write = db_updated
+        except AccountSuspendedError:
+            return self.suspended_account_template()
+
         with MasterDatabasePolicy():
-            try:
-                account = getUtility(IAccountSet).getByOpenIDIdentifier(
-                    identifier)
-            except LookupError:
-                # The two lines below are duplicated a few more lines down,
-                # but to avoid this duplication we'd have to refactor most of
-                # our tests to provide an SREG response, which would be rather
-                # painful.
-                email_address, full_name = self._getEmailAddressAndFullName()
-                email = email_set.getByEmail(email_address)
-                if email is None:
-                    # We got an OpenID response containing a positive
-                    # assertion, but we don't have an account for the
-                    # identifier or for the email address.  We'll create one.
-                    account = self._createAccount(
-                        identifier, email_address, full_name)
-                else:
-                    account = email.account
-                    assert account is not None, (
-                        "This email address should have an associated "
-                        "account.")
-                    removeSecurityProxy(account).openid_identifier = (
-                        identifier)
-                should_update_last_write = True
-
-            if account.status == AccountStatus.SUSPENDED:
-                return self.suspended_account_template()
-            elif account.status in [AccountStatus.DEACTIVATED,
-                                    AccountStatus.NOACCOUNT]:
-                comment = 'Reactivated by the user'
-                password = '' # Needed just to please reactivate() below.
-                email_address, dummy = self._getEmailAddressAndFullName()
-                email = email_set.getByEmail(email_address)
-                if email is None:
-                    email = email_set.new(email_address, account=account)
-                removeSecurityProxy(account).reactivate(
-                    comment, password, removeSecurityProxy(email))
-            else:
-                # Account is active, so nothing to do.
-                pass
-            if IPerson(account, None) is None:
-                removeSecurityProxy(account).createPerson(
-                    PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
-                should_update_last_write = True
-            self.login(account)
+            self.login(person.account)
 
         if should_update_last_write:
             # This is a GET request but we changed the database, so update

=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
--- lib/canonical/launchpad/webapp/tests/test_login.py	2010-04-27 15:50:02 +0000
+++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-07-02 10:41:03 +0000
@@ -111,7 +111,8 @@
             view_class=StubbedOpenIDCallbackView):
         openid_response = FakeOpenIDResponse(
             ITestOpenIDPersistentIdentity(account).openid_identity_url,
-            status=response_status, message=response_msg)
+            status=response_status, message=response_msg,
+            email='[e-mail]', full_name='Foo User')
         return self._createAndRenderView(
             openid_response, view_class=view_class)
 
@@ -140,7 +141,8 @@
         # In the common case we just login and redirect to the URL specified
         # in the 'starting_url' query arg.
         person = self.factory.makePerson()
-        view, html = self._createViewWithResponse(person.account)
+        with SRegResponse_fromSuccessResponse_stubbed():
+            view, html = self._createViewWithResponse(person.account)
         self.assertTrue(view.login_called)
         response = view.request.response
         self.assertEquals(httplib.TEMPORARY_REDIRECT, response.getStatus())
@@ -157,7 +159,8 @@
         # create one.
         account = self.factory.makeAccount('Test account')
         self.assertIs(None, IPerson(account, None))
-        view, html = self._createViewWithResponse(account)
+        with SRegResponse_fromSuccessResponse_stubbed():
+            view, html = self._createViewWithResponse(account)
         self.assertIsNot(None, IPerson(account, None))
         self.assertTrue(view.login_called)
         response = view.request.response
@@ -167,7 +170,7 @@
 
         # We also update the last_write flag in the session, to make sure
         # further requests use the master DB and thus see the newly created
-        # stuff. 
+        # stuff.
         self.assertLastWriteIsSet(view.request)
 
     def test_unseen_identity(self):
@@ -196,7 +199,7 @@
 
         # We also update the last_write flag in the session, to make sure
         # further requests use the master DB and thus see the newly created
-        # stuff. 
+        # stuff.
         self.assertLastWriteIsSet(view.request)
 
     def test_unseen_identity_with_registered_email(self):
@@ -230,7 +233,7 @@
 
         # We also update the last_write flag in the session, to make sure
         # further requests use the master DB and thus see the newly created
-        # stuff. 
+        # stuff.
         self.assertLastWriteIsSet(view.request)
 
     def test_deactivated_account(self):
@@ -256,7 +259,7 @@
         self.assertEquals(email, account.preferredemail.email)
         # We also update the last_write flag in the session, to make sure
         # further requests use the master DB and thus see the newly created
-        # stuff. 
+        # stuff.
         self.assertLastWriteIsSet(view.request)
 
     def test_never_used_account(self):
@@ -278,7 +281,7 @@
         self.assertEquals(email, account.preferredemail.email)
         # We also update the last_write flag in the session, to make sure
         # further requests use the master DB and thus see the newly created
-        # stuff. 
+        # stuff.
         self.assertLastWriteIsSet(view.request)
 
     def test_suspended_account(self):
@@ -286,7 +289,8 @@
         # login, but we must not allow that.
         account = self.factory.makeAccount(
             'Test account', status=AccountStatus.SUSPENDED)
-        view, html = self._createViewWithResponse(account)
+        with SRegResponse_fromSuccessResponse_stubbed():
+            view, html = self._createViewWithResponse(account)
         self.assertFalse(view.login_called)
         main_content = extract_text(find_main_content(html))
         self.assertIn('This account has been suspended', main_content)

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-06-03 14:51:07 +0000
+++ lib/lp/registry/interfaces/person.py	2010-07-02 10:41:03 +0000
@@ -297,6 +297,13 @@
         commented on.
         """)
 
+    SOFTWARE_CENTER_PURCHASE = DBItem(16, """
+        Created by purchasing commercial software through Software Center.
+
+        A purchase of commercial software (ie. subscriptions to a private
+        and commercial archive) was made via Software Center.
+        """)
+
 
 class TeamMembershipRenewalPolicy(DBEnumeratedType):
     """TeamMembership Renewal Policy.
@@ -1754,6 +1761,33 @@
         on the displayname or other arguments.
         """
 
+    def getOrCreateByOpenIDIdentifier(openid_identifier, email,
+                                      full_name, creation_rationale, comment):
+        """Get or create a person for a given OpenID identifier.
+
+        This is used when users login. We get the account with the given
+        OpenID identifier (creating one if it doesn't already exist) and
+        act according to the account's state:
+          - If the account is suspended, we stop and raise an error.
+          - If the account is deactivated, we reactivate it and proceed;
+          - If the account is active, we just proceed.
+
+        If there is no existing Launchpad person for the account, we
+        create it.
+
+        :param openid_identifier: representing the authenticated user.
+        :param email_address: the email address of the user.
+        :param full_name: the full name of the user.
+        :param creation_rationale: When an account or person needs to
+            be created, this indicates why it was created.
+        :param comment: If the account is reactivated or person created,
+            this comment indicates why.
+        :return: a tuple of `IPerson` and a boolean indicating whether the
+            database was updated.
+        :raises AccountSuspendedError: if the account associated with the
+            identifier has been suspended.
+        """
+
     @call_with(teamowner=REQUEST_USER)
     @rename_parameters_as(
         displayname='display_name', teamdescription='team_description',

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-06-22 11:19:34 +0000
+++ lib/lp/registry/model/person.py	2010-07-02 10:41:03 +0000
@@ -61,6 +61,7 @@
 from canonical.lazr.utils import get_current_browser_request, safe_hasattr
 
 from canonical.launchpad.database.account import Account, AccountPassword
+from canonical.launchpad.interfaces.account import AccountSuspendedError
 from lp.bugs.model.bugtarget import HasBugsBase
 from canonical.launchpad.database.stormsugar import StartsWith
 from lp.registry.model.karma import KarmaCategory
@@ -152,6 +153,7 @@
 
 from canonical.launchpad.validators.email import valid_email
 from canonical.launchpad.validators.name import sanitize_name, valid_name
+from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
 from lp.registry.interfaces.person import validate_public_person
 
 
@@ -2429,6 +2431,59 @@
             key=lambda obj: (obj.karma, obj.displayname, obj.id),
             reverse=True)
 
+    def getOrCreateByOpenIDIdentifier(
+        self, openid_identifier, email_address, full_name,
+        creation_rationale, comment):
+        """See `IPersonSet`."""
+        assert email_address is not None and full_name is not None, (
+                "Both email address and full name are required to "
+                "create an account.")
+        db_updated = False
+        with MasterDatabasePolicy():
+            account_set = getUtility(IAccountSet)
+            email_set = getUtility(IEmailAddressSet)
+            email = email_set.getByEmail(email_address)
+            try:
+                account = account_set.getByOpenIDIdentifier(
+                    openid_identifier)
+            except LookupError:
+                if email is None:
+                    # There is no account associated with the identifier
+                    # nor an email associated with the email address.
+                    # We'll create one.
+                    account, email = account_set.createAccountAndEmail(
+                            email_address, creation_rationale, full_name,
+                            password=None,
+                            openid_identifier=openid_identifier)
+                else:
+                    account = email.account
+                    assert account is not None, (
+                        "This email address should have an associated "
+                        "account.")
+                    removeSecurityProxy(account).openid_identifier = (
+                        openid_identifier)
+                db_updated = True
+
+            if account.status == AccountStatus.SUSPENDED:
+                raise AccountSuspendedError(
+                    "The account matching the identifier is suspended.")
+            elif account.status in [AccountStatus.DEACTIVATED,
+                                    AccountStatus.NOACCOUNT]:
+                password = '' # Needed just to please reactivate() below.
+                if email is None:
+                    email = email_set.new(email_address, account=account)
+                removeSecurityProxy(account).reactivate(
+                    comment, password, removeSecurityProxy(email))
+            else:
+                # Account is active, so nothing to do.
+                pass
+            if IPerson(account, None) is None:
+                removeSecurityProxy(account).createPerson(
+                    creation_rationale, comment=comment)
+                db_updated = True
+
+        return IPerson(account), db_updated
+
     def newTeam(self, teamowner, name, displayname, teamdescription=None,
                 subscriptionpolicy=TeamSubscriptionPolicy.MODERATED,
                 defaultmembershipperiod=None, defaultrenewalperiod=None):

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2010-03-31 18:51:32 +0000
+++ lib/lp/registry/tests/test_personset.py	2010-07-02 10:41:03 +0000
@@ -16,9 +16,12 @@
     MailingListAutoSubscribePolicy)
 from lp.registry.interfaces.person import (
     PersonCreationRationale, IPersonSet)
-from lp.testing import TestCaseWithFactory, login_person, logout
+from lp.testing import (
+    TestCaseWithFactory, login_person, logout)
 
 from canonical.database.sqlbase import cursor
+from canonical.launchpad.interfaces.account import (
+    AccountStatus, AccountSuspendedError)
 from canonical.launchpad.testing.databasehelpers import (
     remove_all_sample_data_branches)
 from canonical.testing import DatabaseFunctionalLayer
@@ -146,5 +149,93 @@
         self.assertEqual(1, self.cur.rowcount)
 
 
+class TestPersonSetGetOrCreateByOpenIDIdentifier(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonSetGetOrCreateByOpenIDIdentifier, self).setUp()
+        self.person_set = getUtility(IPersonSet)
+
+    def callGetOrCreate(self, identifier, email='[e-mail]'):
+        return self.person_set.getOrCreateByOpenIDIdentifier(
+            identifier, email, "Joe Bloggs",
+            PersonCreationRationale.SOFTWARE_CENTER_PURCHASE,
+            "when purchasing an application via Software Center.")
+
+    def test_existing_person(self):
+        person = self.factory.makePerson()
+        openid_ident = removeSecurityProxy(person.account).openid_identifier
+        person_set = getUtility(IPersonSet)
+
+        result, db_updated = self.callGetOrCreate(openid_ident)
+
+        self.assertEqual(person, result)
+        self.assertFalse(db_updated)
+
+    def test_existing_account_no_person(self):
+        # A person is created with the correct rationale.
+        account = self.factory.makeAccount('purchaser')
+        openid_ident = removeSecurityProxy(account).openid_identifier
+
+        person, db_updated = self.callGetOrCreate(openid_ident)
+
+        self.assertEqual(account, person.account)
+        # The person is created with the correct rationale, creation
+        # comment, registrant, appropriate display name.
+        self.assertEqual(
+            "when purchasing an application via Software Center.",
+            person.creation_comment)
+        self.assertEqual(
+            PersonCreationRationale.SOFTWARE_CENTER_PURCHASE,
+            person.creation_rationale)
+        self.assertTrue(db_updated)
+
+    def test_existing_deactivated_account(self):
+        # An existing deactivated account will be reactivated.
+        account = self.factory.makeAccount('purchaser',
+            status=AccountStatus.DEACTIVATED)
+        openid_ident = removeSecurityProxy(account).openid_identifier
+
+        person, db_updated = self.callGetOrCreate(openid_ident)
+        self.assertEqual(AccountStatus.ACTIVE, person.account.status)
+        self.assertTrue(db_updated)
+        self.assertEqual(
+            "when purchasing an application via Software Center.",
+            removeSecurityProxy(person.account).status_comment)
+
+    def test_existing_suspended_account(self):
+        # An existing suspended account will raise an exception.
+        account = self.factory.makeAccount('purchaser',
+            status=AccountStatus.SUSPENDED)
+        openid_ident = removeSecurityProxy(account).openid_identifier
+
+        self.assertRaises(
+            AccountSuspendedError, self.callGetOrCreate, openid_ident)
+
+    def test_no_account_or_email(self):
+        # An identifier can be used to create an account (it is assumed
+        # to be already authenticated with SSO).
+        person, db_updated = self.callGetOrCreate('openid-identifier')
+
+        self.assertEqual(
+            "openid-identifier",
+            removeSecurityProxy(person.account).openid_identifier)
+        self.assertTrue(db_updated)
+
+    def test_no_matching_account_existing_email(self):
+        # The openid_identity of the account matching the email will
+        # updated.
+        other_account = self.factory.makeAccount('test', email='[e-mail]')
+
+        person, db_updated = self.callGetOrCreate(
+            'other-openid-identifier', '[e-mail]')
+
+        self.assertEqual(other_account, person.account)
+        self.assertEqual(
+            'other-openid-identifier',
+            removeSecurityProxy(person.account).openid_identifier)
+
+
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/testopenid/browser/server.py'
--- lib/lp/testopenid/browser/server.py	2010-05-15 17:43:59 +0000
+++ lib/lp/testopenid/browser/server.py	2010-07-02 10:41:03 +0000
@@ -198,7 +198,10 @@
         else:
             response = self.openid_request.answer(True)
 
-        sreg_fields = dict(nickname=IPerson(self.account).name)
+        sreg_fields = dict(
+            nickname=IPerson(self.account).name,
+            email=self.account.preferredemail.email,
+            fullname=self.account.displayname)
         sreg_request = SRegRequest.fromOpenIDRequest(self.openid_request)
         sreg_response = SRegResponse.extractResponse(
             sreg_request, sreg_fields)
@@ -224,7 +227,7 @@
     This class implements an OpenID endpoint using the python-openid
     library.  In addition to the normal modes of operation, it also
     implements the OpenID 2.0 identifier select mode.
-    
+
     Note that the checkid_immediate mode is not supported.
     """