Launchpad itself

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

  1. db-598464-get-or-create-from-identity
  2. Merge into db-devel
Proposed by Michael Nelson
Status: Merged
Approved by: Michael Nelson
Approved revision: no longer in the source branch.
Merged at revision: 9526
Proposed branch: lp:~michael.nelson/launchpad/db-598464-get-or-create-from-identity
Merge into: lp:launchpad/db-devel
Prerequisite: lp:~stevenk/launchpad/db-software-center-agent-celebrity
Diff against target: 596 lines (+257/-78)
9 files modified
lib/canonical/launchpad/database/account.py (+4/-2)
lib/canonical/launchpad/interfaces/account.py (+14/-1)
lib/canonical/launchpad/security.py (+20/-0)
lib/canonical/launchpad/webapp/login.py (+20/-63)
lib/canonical/launchpad/webapp/tests/test_login.py (+13/-9)
lib/lp/registry/interfaces/person.py (+34/-0)
lib/lp/registry/model/person.py (+55/-0)
lib/lp/registry/tests/test_personset.py (+92/-1)
lib/lp/testopenid/browser/server.py (+5/-2)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/db-598464-get-or-create-from-identity
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Guilherme Salgado Pending
Review via email: mp+28885@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Guilherme Salgado (salgado) wrote :

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

Revision history for this message
Michael Nelson (michael.nelson) wrote :
Download full text (5.7 KiB)

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

Read more...

Revision history for this message
Abel Deuring (adeuring) wrote :

(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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/database/account.py'
2--- lib/canonical/launchpad/database/account.py 2010-05-07 20:02:33 +0000
3+++ lib/canonical/launchpad/database/account.py 2010-07-02 10:41:03 +0000
4@@ -190,7 +190,8 @@
5 return False
6 return self.preferredemail is not None
7
8- def createPerson(self, rationale, name=None, comment=None):
9+ def createPerson(self, rationale, name=None, comment=None,
10+ registrant=None):
11 """See `IAccount`."""
12 # Need a local import because of circular dependencies.
13 from lp.registry.model.person import (
14@@ -204,7 +205,8 @@
15 name = generate_nick(self.preferredemail.email)
16 person = PersonSet()._newPerson(
17 name, self.displayname, hide_email_addresses=True,
18- rationale=rationale, account=self, comment=comment)
19+ rationale=rationale, account=self, comment=comment,
20+ registrant=registrant)
21
22 # Update all associated email addresses to point at the new person.
23 result = IMasterStore(EmailAddress).find(
24
25=== modified file 'lib/canonical/launchpad/interfaces/account.py'
26--- lib/canonical/launchpad/interfaces/account.py 2009-12-24 11:06:04 +0000
27+++ lib/canonical/launchpad/interfaces/account.py 2010-07-02 10:41:03 +0000
28@@ -8,6 +8,7 @@
29
30 __all__ = [
31 'AccountStatus',
32+ 'AccountSuspendedError',
33 'AccountCreationRationale',
34 'IAccount',
35 'IAccountPrivate',
36@@ -27,6 +28,10 @@
37 from lazr.restful.fields import CollectionField, Reference
38
39
40+class AccountSuspendedError(Exception):
41+ """The account being accessed has been suspended."""
42+
43+
44 class AccountStatus(DBEnumeratedType):
45 """The status of an account."""
46
47@@ -179,6 +184,13 @@
48 commented on.
49 """)
50
51+ SOFTWARE_CENTER_PURCHASE = DBItem(16, """
52+ Created by purchasing commercial software through Software Center.
53+
54+ A purchase of commercial software (ie. subscriptions to a private
55+ and commercial archive) was made via Software Center.
56+ """)
57+
58
59 class IAccountPublic(Interface):
60 """Public information on an `IAccount`."""
61@@ -261,7 +273,7 @@
62 password = PasswordField(
63 title=_("Password."), readonly=False, required=True)
64
65- def createPerson(self, rationale, name=None, comment=None):
66+ def createPerson(rationale, name=None, comment=None, registrant=None):
67 """Create and return a new `IPerson` associated with this account.
68
69 :param rationale: A member of `AccountCreationRationale`.
70@@ -269,6 +281,7 @@
71 using an automatically generated one.
72 :param comment: Populate `IPerson.creation_comment`. See
73 `IPerson`.
74+ :param registrant: Populate `IPerson.registrant`.
75 """
76
77
78
79=== modified file 'lib/canonical/launchpad/security.py'
80--- lib/canonical/launchpad/security.py 2010-06-30 13:49:38 +0000
81+++ lib/canonical/launchpad/security.py 2010-07-02 10:41:03 +0000
82@@ -284,6 +284,14 @@
83 class ViewAccount(EditAccountBySelfOrAdmin):
84 permission = 'launchpad.View'
85
86+ def checkAuthenticated(self, roles):
87+ """Extend view permission to software-center-agent."""
88+ if super(ViewAccount, self).checkAuthenticated(roles):
89+ return True
90+ else:
91+ agent = getUtility(ILaunchpadCelebrities).software_center_agent
92+ return agent == roles.person
93+
94
95 class SpecialAccount(EditAccountBySelfOrAdmin):
96 permission = 'launchpad.Special'
97@@ -590,6 +598,18 @@
98 usedfor = IPersonSet
99
100
101+class EditPersonSet(AdminByAdminsTeam):
102+ permission = 'launchpad.Edit'
103+ usedfor = IPersonSet
104+
105+ def checkAuthenticated(self, user):
106+ if super(EditPersonSet, self).checkAuthenticated(user):
107+ return True
108+ else:
109+ agent = getUtility(ILaunchpadCelebrities).software_center_agent
110+ return user.person == agent
111+
112+
113 class EditTeamByTeamOwnerOrLaunchpadAdmins(AuthorizationBase):
114 permission = 'launchpad.Owner'
115 usedfor = ITeam
116
117=== modified file 'lib/canonical/launchpad/webapp/login.py'
118--- lib/canonical/launchpad/webapp/login.py 2010-05-15 17:43:59 +0000
119+++ lib/canonical/launchpad/webapp/login.py 2010-07-02 10:41:03 +0000
120@@ -32,10 +32,9 @@
121 from canonical.cachedproperty import cachedproperty
122 from canonical.config import config
123 from canonical.launchpad import _
124-from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
125-from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
126+from canonical.launchpad.interfaces.account import AccountSuspendedError
127 from canonical.launchpad.interfaces.openidconsumer import IOpenIDConsumerStore
128-from lp.registry.interfaces.person import IPerson, PersonCreationRationale
129+from lp.registry.interfaces.person import IPersonSet, PersonCreationRationale
130 from canonical.launchpad.readonly import is_read_only
131 from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
132 from canonical.launchpad.webapp.error import SystemErrorView
133@@ -199,7 +198,7 @@
134 return_to = "%s?%s" % (return_to, starting_url)
135 form_html = self.openid_request.htmlMarkup(trust_root, return_to)
136
137- # The consumer.begin() call above will insert rows into the
138+ # The consumer.begin() call above will insert rows into the
139 # OpenIDAssociations table, but since this will be a GET request, the
140 # transaction would be rolled back, so we need an explicit commit
141 # here.
142@@ -278,23 +277,14 @@
143 "No email address or full name found in sreg response")
144 return email_address, full_name
145
146- def _createAccount(self, openid_identifier, email_address, full_name):
147- account, email = getUtility(IAccountSet).createAccountAndEmail(
148- email_address, PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
149- full_name, password=None, openid_identifier=openid_identifier)
150- return account
151-
152 def processPositiveAssertion(self):
153 """Process an OpenID response containing a positive assertion.
154
155- We'll get the account with the given OpenID identifier (creating one
156- if it doesn't already exist) and act according to the account's state:
157- - If the account is suspended, we stop and render an error page;
158- - If the account is deactivated, we reactivate it and proceed;
159- - If the account is active, we just proceed.
160+ We'll get the person and account with the given OpenID
161+ identifier (creating one if necessary), and then login using
162+ that account.
163
164- After that we ensure there's an IPerson associated with the account
165- and login using that account.
166+ If the account is suspended, we stop and render an error page.
167
168 We also update the 'last_write' key in the session if we've done any
169 DB writes, to ensure subsequent requests use the master DB and see
170@@ -302,56 +292,23 @@
171 """
172 identifier = self.openid_response.identity_url.split('/')[-1]
173 should_update_last_write = False
174- email_set = getUtility(IEmailAddressSet)
175 # Force the use of the master database to make sure a lagged slave
176 # doesn't fool us into creating a Person/Account when one already
177 # exists.
178+ person_set = getUtility(IPersonSet)
179+ email_address, full_name = self._getEmailAddressAndFullName()
180+ try:
181+ person, db_updated = person_set.getOrCreateByOpenIDIdentifier(
182+ identifier, email_address, full_name,
183+ comment='when logging in to Launchpad.',
184+ creation_rationale=(
185+ PersonCreationRationale.OWNER_CREATED_LAUNCHPAD))
186+ should_update_last_write = db_updated
187+ except AccountSuspendedError:
188+ return self.suspended_account_template()
189+
190 with MasterDatabasePolicy():
191- try:
192- account = getUtility(IAccountSet).getByOpenIDIdentifier(
193- identifier)
194- except LookupError:
195- # The two lines below are duplicated a few more lines down,
196- # but to avoid this duplication we'd have to refactor most of
197- # our tests to provide an SREG response, which would be rather
198- # painful.
199- email_address, full_name = self._getEmailAddressAndFullName()
200- email = email_set.getByEmail(email_address)
201- if email is None:
202- # We got an OpenID response containing a positive
203- # assertion, but we don't have an account for the
204- # identifier or for the email address. We'll create one.
205- account = self._createAccount(
206- identifier, email_address, full_name)
207- else:
208- account = email.account
209- assert account is not None, (
210- "This email address should have an associated "
211- "account.")
212- removeSecurityProxy(account).openid_identifier = (
213- identifier)
214- should_update_last_write = True
215-
216- if account.status == AccountStatus.SUSPENDED:
217- return self.suspended_account_template()
218- elif account.status in [AccountStatus.DEACTIVATED,
219- AccountStatus.NOACCOUNT]:
220- comment = 'Reactivated by the user'
221- password = '' # Needed just to please reactivate() below.
222- email_address, dummy = self._getEmailAddressAndFullName()
223- email = email_set.getByEmail(email_address)
224- if email is None:
225- email = email_set.new(email_address, account=account)
226- removeSecurityProxy(account).reactivate(
227- comment, password, removeSecurityProxy(email))
228- else:
229- # Account is active, so nothing to do.
230- pass
231- if IPerson(account, None) is None:
232- removeSecurityProxy(account).createPerson(
233- PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
234- should_update_last_write = True
235- self.login(account)
236+ self.login(person.account)
237
238 if should_update_last_write:
239 # This is a GET request but we changed the database, so update
240
241=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
242--- lib/canonical/launchpad/webapp/tests/test_login.py 2010-04-27 15:50:02 +0000
243+++ lib/canonical/launchpad/webapp/tests/test_login.py 2010-07-02 10:41:03 +0000
244@@ -111,7 +111,8 @@
245 view_class=StubbedOpenIDCallbackView):
246 openid_response = FakeOpenIDResponse(
247 ITestOpenIDPersistentIdentity(account).openid_identity_url,
248- status=response_status, message=response_msg)
249+ status=response_status, message=response_msg,
250+ email='non-existent@example.com', full_name='Foo User')
251 return self._createAndRenderView(
252 openid_response, view_class=view_class)
253
254@@ -140,7 +141,8 @@
255 # In the common case we just login and redirect to the URL specified
256 # in the 'starting_url' query arg.
257 person = self.factory.makePerson()
258- view, html = self._createViewWithResponse(person.account)
259+ with SRegResponse_fromSuccessResponse_stubbed():
260+ view, html = self._createViewWithResponse(person.account)
261 self.assertTrue(view.login_called)
262 response = view.request.response
263 self.assertEquals(httplib.TEMPORARY_REDIRECT, response.getStatus())
264@@ -157,7 +159,8 @@
265 # create one.
266 account = self.factory.makeAccount('Test account')
267 self.assertIs(None, IPerson(account, None))
268- view, html = self._createViewWithResponse(account)
269+ with SRegResponse_fromSuccessResponse_stubbed():
270+ view, html = self._createViewWithResponse(account)
271 self.assertIsNot(None, IPerson(account, None))
272 self.assertTrue(view.login_called)
273 response = view.request.response
274@@ -167,7 +170,7 @@
275
276 # We also update the last_write flag in the session, to make sure
277 # further requests use the master DB and thus see the newly created
278- # stuff.
279+ # stuff.
280 self.assertLastWriteIsSet(view.request)
281
282 def test_unseen_identity(self):
283@@ -196,7 +199,7 @@
284
285 # We also update the last_write flag in the session, to make sure
286 # further requests use the master DB and thus see the newly created
287- # stuff.
288+ # stuff.
289 self.assertLastWriteIsSet(view.request)
290
291 def test_unseen_identity_with_registered_email(self):
292@@ -230,7 +233,7 @@
293
294 # We also update the last_write flag in the session, to make sure
295 # further requests use the master DB and thus see the newly created
296- # stuff.
297+ # stuff.
298 self.assertLastWriteIsSet(view.request)
299
300 def test_deactivated_account(self):
301@@ -256,7 +259,7 @@
302 self.assertEquals(email, account.preferredemail.email)
303 # We also update the last_write flag in the session, to make sure
304 # further requests use the master DB and thus see the newly created
305- # stuff.
306+ # stuff.
307 self.assertLastWriteIsSet(view.request)
308
309 def test_never_used_account(self):
310@@ -278,7 +281,7 @@
311 self.assertEquals(email, account.preferredemail.email)
312 # We also update the last_write flag in the session, to make sure
313 # further requests use the master DB and thus see the newly created
314- # stuff.
315+ # stuff.
316 self.assertLastWriteIsSet(view.request)
317
318 def test_suspended_account(self):
319@@ -286,7 +289,8 @@
320 # login, but we must not allow that.
321 account = self.factory.makeAccount(
322 'Test account', status=AccountStatus.SUSPENDED)
323- view, html = self._createViewWithResponse(account)
324+ with SRegResponse_fromSuccessResponse_stubbed():
325+ view, html = self._createViewWithResponse(account)
326 self.assertFalse(view.login_called)
327 main_content = extract_text(find_main_content(html))
328 self.assertIn('This account has been suspended', main_content)
329
330=== modified file 'lib/lp/registry/interfaces/person.py'
331--- lib/lp/registry/interfaces/person.py 2010-06-03 14:51:07 +0000
332+++ lib/lp/registry/interfaces/person.py 2010-07-02 10:41:03 +0000
333@@ -297,6 +297,13 @@
334 commented on.
335 """)
336
337+ SOFTWARE_CENTER_PURCHASE = DBItem(16, """
338+ Created by purchasing commercial software through Software Center.
339+
340+ A purchase of commercial software (ie. subscriptions to a private
341+ and commercial archive) was made via Software Center.
342+ """)
343+
344
345 class TeamMembershipRenewalPolicy(DBEnumeratedType):
346 """TeamMembership Renewal Policy.
347@@ -1754,6 +1761,33 @@
348 on the displayname or other arguments.
349 """
350
351+ def getOrCreateByOpenIDIdentifier(openid_identifier, email,
352+ full_name, creation_rationale, comment):
353+ """Get or create a person for a given OpenID identifier.
354+
355+ This is used when users login. We get the account with the given
356+ OpenID identifier (creating one if it doesn't already exist) and
357+ act according to the account's state:
358+ - If the account is suspended, we stop and raise an error.
359+ - If the account is deactivated, we reactivate it and proceed;
360+ - If the account is active, we just proceed.
361+
362+ If there is no existing Launchpad person for the account, we
363+ create it.
364+
365+ :param openid_identifier: representing the authenticated user.
366+ :param email_address: the email address of the user.
367+ :param full_name: the full name of the user.
368+ :param creation_rationale: When an account or person needs to
369+ be created, this indicates why it was created.
370+ :param comment: If the account is reactivated or person created,
371+ this comment indicates why.
372+ :return: a tuple of `IPerson` and a boolean indicating whether the
373+ database was updated.
374+ :raises AccountSuspendedError: if the account associated with the
375+ identifier has been suspended.
376+ """
377+
378 @call_with(teamowner=REQUEST_USER)
379 @rename_parameters_as(
380 displayname='display_name', teamdescription='team_description',
381
382=== modified file 'lib/lp/registry/model/person.py'
383--- lib/lp/registry/model/person.py 2010-06-22 11:19:34 +0000
384+++ lib/lp/registry/model/person.py 2010-07-02 10:41:03 +0000
385@@ -61,6 +61,7 @@
386 from canonical.lazr.utils import get_current_browser_request, safe_hasattr
387
388 from canonical.launchpad.database.account import Account, AccountPassword
389+from canonical.launchpad.interfaces.account import AccountSuspendedError
390 from lp.bugs.model.bugtarget import HasBugsBase
391 from canonical.launchpad.database.stormsugar import StartsWith
392 from lp.registry.model.karma import KarmaCategory
393@@ -152,6 +153,7 @@
394
395 from canonical.launchpad.validators.email import valid_email
396 from canonical.launchpad.validators.name import sanitize_name, valid_name
397+from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
398 from lp.registry.interfaces.person import validate_public_person
399
400
401@@ -2429,6 +2431,59 @@
402 key=lambda obj: (obj.karma, obj.displayname, obj.id),
403 reverse=True)
404
405+ def getOrCreateByOpenIDIdentifier(
406+ self, openid_identifier, email_address, full_name,
407+ creation_rationale, comment):
408+ """See `IPersonSet`."""
409+ assert email_address is not None and full_name is not None, (
410+ "Both email address and full name are required to "
411+ "create an account.")
412+ db_updated = False
413+ with MasterDatabasePolicy():
414+ account_set = getUtility(IAccountSet)
415+ email_set = getUtility(IEmailAddressSet)
416+ email = email_set.getByEmail(email_address)
417+ try:
418+ account = account_set.getByOpenIDIdentifier(
419+ openid_identifier)
420+ except LookupError:
421+ if email is None:
422+ # There is no account associated with the identifier
423+ # nor an email associated with the email address.
424+ # We'll create one.
425+ account, email = account_set.createAccountAndEmail(
426+ email_address, creation_rationale, full_name,
427+ password=None,
428+ openid_identifier=openid_identifier)
429+ else:
430+ account = email.account
431+ assert account is not None, (
432+ "This email address should have an associated "
433+ "account.")
434+ removeSecurityProxy(account).openid_identifier = (
435+ openid_identifier)
436+ db_updated = True
437+
438+ if account.status == AccountStatus.SUSPENDED:
439+ raise AccountSuspendedError(
440+ "The account matching the identifier is suspended.")
441+ elif account.status in [AccountStatus.DEACTIVATED,
442+ AccountStatus.NOACCOUNT]:
443+ password = '' # Needed just to please reactivate() below.
444+ if email is None:
445+ email = email_set.new(email_address, account=account)
446+ removeSecurityProxy(account).reactivate(
447+ comment, password, removeSecurityProxy(email))
448+ else:
449+ # Account is active, so nothing to do.
450+ pass
451+ if IPerson(account, None) is None:
452+ removeSecurityProxy(account).createPerson(
453+ creation_rationale, comment=comment)
454+ db_updated = True
455+
456+ return IPerson(account), db_updated
457+
458 def newTeam(self, teamowner, name, displayname, teamdescription=None,
459 subscriptionpolicy=TeamSubscriptionPolicy.MODERATED,
460 defaultmembershipperiod=None, defaultrenewalperiod=None):
461
462=== modified file 'lib/lp/registry/tests/test_personset.py'
463--- lib/lp/registry/tests/test_personset.py 2010-03-31 18:51:32 +0000
464+++ lib/lp/registry/tests/test_personset.py 2010-07-02 10:41:03 +0000
465@@ -16,9 +16,12 @@
466 MailingListAutoSubscribePolicy)
467 from lp.registry.interfaces.person import (
468 PersonCreationRationale, IPersonSet)
469-from lp.testing import TestCaseWithFactory, login_person, logout
470+from lp.testing import (
471+ TestCaseWithFactory, login_person, logout)
472
473 from canonical.database.sqlbase import cursor
474+from canonical.launchpad.interfaces.account import (
475+ AccountStatus, AccountSuspendedError)
476 from canonical.launchpad.testing.databasehelpers import (
477 remove_all_sample_data_branches)
478 from canonical.testing import DatabaseFunctionalLayer
479@@ -146,5 +149,93 @@
480 self.assertEqual(1, self.cur.rowcount)
481
482
483+class TestPersonSetGetOrCreateByOpenIDIdentifier(TestCaseWithFactory):
484+
485+ layer = DatabaseFunctionalLayer
486+
487+ def setUp(self):
488+ super(TestPersonSetGetOrCreateByOpenIDIdentifier, self).setUp()
489+ self.person_set = getUtility(IPersonSet)
490+
491+ def callGetOrCreate(self, identifier, email='a@b.com'):
492+ return self.person_set.getOrCreateByOpenIDIdentifier(
493+ identifier, email, "Joe Bloggs",
494+ PersonCreationRationale.SOFTWARE_CENTER_PURCHASE,
495+ "when purchasing an application via Software Center.")
496+
497+ def test_existing_person(self):
498+ person = self.factory.makePerson()
499+ openid_ident = removeSecurityProxy(person.account).openid_identifier
500+ person_set = getUtility(IPersonSet)
501+
502+ result, db_updated = self.callGetOrCreate(openid_ident)
503+
504+ self.assertEqual(person, result)
505+ self.assertFalse(db_updated)
506+
507+ def test_existing_account_no_person(self):
508+ # A person is created with the correct rationale.
509+ account = self.factory.makeAccount('purchaser')
510+ openid_ident = removeSecurityProxy(account).openid_identifier
511+
512+ person, db_updated = self.callGetOrCreate(openid_ident)
513+
514+ self.assertEqual(account, person.account)
515+ # The person is created with the correct rationale, creation
516+ # comment, registrant, appropriate display name.
517+ self.assertEqual(
518+ "when purchasing an application via Software Center.",
519+ person.creation_comment)
520+ self.assertEqual(
521+ PersonCreationRationale.SOFTWARE_CENTER_PURCHASE,
522+ person.creation_rationale)
523+ self.assertTrue(db_updated)
524+
525+ def test_existing_deactivated_account(self):
526+ # An existing deactivated account will be reactivated.
527+ account = self.factory.makeAccount('purchaser',
528+ status=AccountStatus.DEACTIVATED)
529+ openid_ident = removeSecurityProxy(account).openid_identifier
530+
531+ person, db_updated = self.callGetOrCreate(openid_ident)
532+ self.assertEqual(AccountStatus.ACTIVE, person.account.status)
533+ self.assertTrue(db_updated)
534+ self.assertEqual(
535+ "when purchasing an application via Software Center.",
536+ removeSecurityProxy(person.account).status_comment)
537+
538+ def test_existing_suspended_account(self):
539+ # An existing suspended account will raise an exception.
540+ account = self.factory.makeAccount('purchaser',
541+ status=AccountStatus.SUSPENDED)
542+ openid_ident = removeSecurityProxy(account).openid_identifier
543+
544+ self.assertRaises(
545+ AccountSuspendedError, self.callGetOrCreate, openid_ident)
546+
547+ def test_no_account_or_email(self):
548+ # An identifier can be used to create an account (it is assumed
549+ # to be already authenticated with SSO).
550+ person, db_updated = self.callGetOrCreate('openid-identifier')
551+
552+ self.assertEqual(
553+ "openid-identifier",
554+ removeSecurityProxy(person.account).openid_identifier)
555+ self.assertTrue(db_updated)
556+
557+ def test_no_matching_account_existing_email(self):
558+ # The openid_identity of the account matching the email will
559+ # updated.
560+ other_account = self.factory.makeAccount('test', email='a@b.com')
561+
562+ person, db_updated = self.callGetOrCreate(
563+ 'other-openid-identifier', 'a@b.com')
564+
565+ self.assertEqual(other_account, person.account)
566+ self.assertEqual(
567+ 'other-openid-identifier',
568+ removeSecurityProxy(person.account).openid_identifier)
569+
570+
571 def test_suite():
572 return TestLoader().loadTestsFromName(__name__)
573
574=== modified file 'lib/lp/testopenid/browser/server.py'
575--- lib/lp/testopenid/browser/server.py 2010-05-15 17:43:59 +0000
576+++ lib/lp/testopenid/browser/server.py 2010-07-02 10:41:03 +0000
577@@ -198,7 +198,10 @@
578 else:
579 response = self.openid_request.answer(True)
580
581- sreg_fields = dict(nickname=IPerson(self.account).name)
582+ sreg_fields = dict(
583+ nickname=IPerson(self.account).name,
584+ email=self.account.preferredemail.email,
585+ fullname=self.account.displayname)
586 sreg_request = SRegRequest.fromOpenIDRequest(self.openid_request)
587 sreg_response = SRegResponse.extractResponse(
588 sreg_request, sreg_fields)
589@@ -224,7 +227,7 @@
590 This class implements an OpenID endpoint using the python-openid
591 library. In addition to the normal modes of operation, it also
592 implements the OpenID 2.0 identifier select mode.
593-
594+
595 Note that the checkid_immediate mode is not supported.
596 """
597

Subscribers

People subscribed via source and target branches

to status/vote changes: