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
=== 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 @@
190 return False190 return False
191 return self.preferredemail is not None191 return self.preferredemail is not None
192192
193 def createPerson(self, rationale, name=None, comment=None):193 def createPerson(self, rationale, name=None, comment=None,
194 registrant=None):
194 """See `IAccount`."""195 """See `IAccount`."""
195 # Need a local import because of circular dependencies.196 # Need a local import because of circular dependencies.
196 from lp.registry.model.person import (197 from lp.registry.model.person import (
@@ -204,7 +205,8 @@
204 name = generate_nick(self.preferredemail.email)205 name = generate_nick(self.preferredemail.email)
205 person = PersonSet()._newPerson(206 person = PersonSet()._newPerson(
206 name, self.displayname, hide_email_addresses=True,207 name, self.displayname, hide_email_addresses=True,
207 rationale=rationale, account=self, comment=comment)208 rationale=rationale, account=self, comment=comment,
209 registrant=registrant)
208210
209 # Update all associated email addresses to point at the new person.211 # Update all associated email addresses to point at the new person.
210 result = IMasterStore(EmailAddress).find(212 result = IMasterStore(EmailAddress).find(
211213
=== 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 @@
88
9__all__ = [9__all__ = [
10 'AccountStatus',10 'AccountStatus',
11 'AccountSuspendedError',
11 'AccountCreationRationale',12 'AccountCreationRationale',
12 'IAccount',13 'IAccount',
13 'IAccountPrivate',14 'IAccountPrivate',
@@ -27,6 +28,10 @@
27from lazr.restful.fields import CollectionField, Reference28from lazr.restful.fields import CollectionField, Reference
2829
2930
31class AccountSuspendedError(Exception):
32 """The account being accessed has been suspended."""
33
34
30class AccountStatus(DBEnumeratedType):35class AccountStatus(DBEnumeratedType):
31 """The status of an account."""36 """The status of an account."""
3237
@@ -179,6 +184,13 @@
179 commented on.184 commented on.
180 """)185 """)
181186
187 SOFTWARE_CENTER_PURCHASE = DBItem(16, """
188 Created by purchasing commercial software through Software Center.
189
190 A purchase of commercial software (ie. subscriptions to a private
191 and commercial archive) was made via Software Center.
192 """)
193
182194
183class IAccountPublic(Interface):195class IAccountPublic(Interface):
184 """Public information on an `IAccount`."""196 """Public information on an `IAccount`."""
@@ -261,7 +273,7 @@
261 password = PasswordField(273 password = PasswordField(
262 title=_("Password."), readonly=False, required=True)274 title=_("Password."), readonly=False, required=True)
263275
264 def createPerson(self, rationale, name=None, comment=None):276 def createPerson(rationale, name=None, comment=None, registrant=None):
265 """Create and return a new `IPerson` associated with this account.277 """Create and return a new `IPerson` associated with this account.
266278
267 :param rationale: A member of `AccountCreationRationale`.279 :param rationale: A member of `AccountCreationRationale`.
@@ -269,6 +281,7 @@
269 using an automatically generated one.281 using an automatically generated one.
270 :param comment: Populate `IPerson.creation_comment`. See282 :param comment: Populate `IPerson.creation_comment`. See
271 `IPerson`.283 `IPerson`.
284 :param registrant: Populate `IPerson.registrant`.
272 """285 """
273286
274287
275288
=== 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 @@
284class ViewAccount(EditAccountBySelfOrAdmin):284class ViewAccount(EditAccountBySelfOrAdmin):
285 permission = 'launchpad.View'285 permission = 'launchpad.View'
286286
287 def checkAuthenticated(self, roles):
288 """Extend view permission to software-center-agent."""
289 if super(ViewAccount, self).checkAuthenticated(roles):
290 return True
291 else:
292 agent = getUtility(ILaunchpadCelebrities).software_center_agent
293 return agent == roles.person
294
287295
288class SpecialAccount(EditAccountBySelfOrAdmin):296class SpecialAccount(EditAccountBySelfOrAdmin):
289 permission = 'launchpad.Special'297 permission = 'launchpad.Special'
@@ -590,6 +598,18 @@
590 usedfor = IPersonSet598 usedfor = IPersonSet
591599
592600
601class EditPersonSet(AdminByAdminsTeam):
602 permission = 'launchpad.Edit'
603 usedfor = IPersonSet
604
605 def checkAuthenticated(self, user):
606 if super(EditPersonSet, self).checkAuthenticated(user):
607 return True
608 else:
609 agent = getUtility(ILaunchpadCelebrities).software_center_agent
610 return user.person == agent
611
612
593class EditTeamByTeamOwnerOrLaunchpadAdmins(AuthorizationBase):613class EditTeamByTeamOwnerOrLaunchpadAdmins(AuthorizationBase):
594 permission = 'launchpad.Owner'614 permission = 'launchpad.Owner'
595 usedfor = ITeam615 usedfor = ITeam
596616
=== 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 @@
32from canonical.cachedproperty import cachedproperty32from canonical.cachedproperty import cachedproperty
33from canonical.config import config33from canonical.config import config
34from canonical.launchpad import _34from canonical.launchpad import _
35from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet35from canonical.launchpad.interfaces.account import AccountSuspendedError
36from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
37from canonical.launchpad.interfaces.openidconsumer import IOpenIDConsumerStore36from canonical.launchpad.interfaces.openidconsumer import IOpenIDConsumerStore
38from lp.registry.interfaces.person import IPerson, PersonCreationRationale37from lp.registry.interfaces.person import IPersonSet, PersonCreationRationale
39from canonical.launchpad.readonly import is_read_only38from canonical.launchpad.readonly import is_read_only
40from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy39from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
41from canonical.launchpad.webapp.error import SystemErrorView40from canonical.launchpad.webapp.error import SystemErrorView
@@ -199,7 +198,7 @@
199 return_to = "%s?%s" % (return_to, starting_url)198 return_to = "%s?%s" % (return_to, starting_url)
200 form_html = self.openid_request.htmlMarkup(trust_root, return_to)199 form_html = self.openid_request.htmlMarkup(trust_root, return_to)
201200
202 # The consumer.begin() call above will insert rows into the 201 # The consumer.begin() call above will insert rows into the
203 # OpenIDAssociations table, but since this will be a GET request, the202 # OpenIDAssociations table, but since this will be a GET request, the
204 # transaction would be rolled back, so we need an explicit commit203 # transaction would be rolled back, so we need an explicit commit
205 # here.204 # here.
@@ -278,23 +277,14 @@
278 "No email address or full name found in sreg response")277 "No email address or full name found in sreg response")
279 return email_address, full_name278 return email_address, full_name
280279
281 def _createAccount(self, openid_identifier, email_address, full_name):
282 account, email = getUtility(IAccountSet).createAccountAndEmail(
283 email_address, PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
284 full_name, password=None, openid_identifier=openid_identifier)
285 return account
286
287 def processPositiveAssertion(self):280 def processPositiveAssertion(self):
288 """Process an OpenID response containing a positive assertion.281 """Process an OpenID response containing a positive assertion.
289282
290 We'll get the account with the given OpenID identifier (creating one 283 We'll get the person and account with the given OpenID
291 if it doesn't already exist) and act according to the account's state:284 identifier (creating one if necessary), and then login using
292 - If the account is suspended, we stop and render an error page;285 that account.
293 - If the account is deactivated, we reactivate it and proceed;
294 - If the account is active, we just proceed.
295286
296 After that we ensure there's an IPerson associated with the account287 If the account is suspended, we stop and render an error page.
297 and login using that account.
298288
299 We also update the 'last_write' key in the session if we've done any289 We also update the 'last_write' key in the session if we've done any
300 DB writes, to ensure subsequent requests use the master DB and see290 DB writes, to ensure subsequent requests use the master DB and see
@@ -302,56 +292,23 @@
302 """292 """
303 identifier = self.openid_response.identity_url.split('/')[-1]293 identifier = self.openid_response.identity_url.split('/')[-1]
304 should_update_last_write = False294 should_update_last_write = False
305 email_set = getUtility(IEmailAddressSet)
306 # Force the use of the master database to make sure a lagged slave295 # Force the use of the master database to make sure a lagged slave
307 # doesn't fool us into creating a Person/Account when one already296 # doesn't fool us into creating a Person/Account when one already
308 # exists.297 # exists.
298 person_set = getUtility(IPersonSet)
299 email_address, full_name = self._getEmailAddressAndFullName()
300 try:
301 person, db_updated = person_set.getOrCreateByOpenIDIdentifier(
302 identifier, email_address, full_name,
303 comment='when logging in to Launchpad.',
304 creation_rationale=(
305 PersonCreationRationale.OWNER_CREATED_LAUNCHPAD))
306 should_update_last_write = db_updated
307 except AccountSuspendedError:
308 return self.suspended_account_template()
309
309 with MasterDatabasePolicy():310 with MasterDatabasePolicy():
310 try:311 self.login(person.account)
311 account = getUtility(IAccountSet).getByOpenIDIdentifier(
312 identifier)
313 except LookupError:
314 # The two lines below are duplicated a few more lines down,
315 # but to avoid this duplication we'd have to refactor most of
316 # our tests to provide an SREG response, which would be rather
317 # painful.
318 email_address, full_name = self._getEmailAddressAndFullName()
319 email = email_set.getByEmail(email_address)
320 if email is None:
321 # We got an OpenID response containing a positive
322 # assertion, but we don't have an account for the
323 # identifier or for the email address. We'll create one.
324 account = self._createAccount(
325 identifier, email_address, full_name)
326 else:
327 account = email.account
328 assert account is not None, (
329 "This email address should have an associated "
330 "account.")
331 removeSecurityProxy(account).openid_identifier = (
332 identifier)
333 should_update_last_write = True
334
335 if account.status == AccountStatus.SUSPENDED:
336 return self.suspended_account_template()
337 elif account.status in [AccountStatus.DEACTIVATED,
338 AccountStatus.NOACCOUNT]:
339 comment = 'Reactivated by the user'
340 password = '' # Needed just to please reactivate() below.
341 email_address, dummy = self._getEmailAddressAndFullName()
342 email = email_set.getByEmail(email_address)
343 if email is None:
344 email = email_set.new(email_address, account=account)
345 removeSecurityProxy(account).reactivate(
346 comment, password, removeSecurityProxy(email))
347 else:
348 # Account is active, so nothing to do.
349 pass
350 if IPerson(account, None) is None:
351 removeSecurityProxy(account).createPerson(
352 PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
353 should_update_last_write = True
354 self.login(account)
355312
356 if should_update_last_write:313 if should_update_last_write:
357 # This is a GET request but we changed the database, so update314 # This is a GET request but we changed the database, so update
358315
=== 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 @@
111 view_class=StubbedOpenIDCallbackView):111 view_class=StubbedOpenIDCallbackView):
112 openid_response = FakeOpenIDResponse(112 openid_response = FakeOpenIDResponse(
113 ITestOpenIDPersistentIdentity(account).openid_identity_url,113 ITestOpenIDPersistentIdentity(account).openid_identity_url,
114 status=response_status, message=response_msg)114 status=response_status, message=response_msg,
115 email='non-existent@example.com', full_name='Foo User')
115 return self._createAndRenderView(116 return self._createAndRenderView(
116 openid_response, view_class=view_class)117 openid_response, view_class=view_class)
117118
@@ -140,7 +141,8 @@
140 # In the common case we just login and redirect to the URL specified141 # In the common case we just login and redirect to the URL specified
141 # in the 'starting_url' query arg.142 # in the 'starting_url' query arg.
142 person = self.factory.makePerson()143 person = self.factory.makePerson()
143 view, html = self._createViewWithResponse(person.account)144 with SRegResponse_fromSuccessResponse_stubbed():
145 view, html = self._createViewWithResponse(person.account)
144 self.assertTrue(view.login_called)146 self.assertTrue(view.login_called)
145 response = view.request.response147 response = view.request.response
146 self.assertEquals(httplib.TEMPORARY_REDIRECT, response.getStatus())148 self.assertEquals(httplib.TEMPORARY_REDIRECT, response.getStatus())
@@ -157,7 +159,8 @@
157 # create one.159 # create one.
158 account = self.factory.makeAccount('Test account')160 account = self.factory.makeAccount('Test account')
159 self.assertIs(None, IPerson(account, None))161 self.assertIs(None, IPerson(account, None))
160 view, html = self._createViewWithResponse(account)162 with SRegResponse_fromSuccessResponse_stubbed():
163 view, html = self._createViewWithResponse(account)
161 self.assertIsNot(None, IPerson(account, None))164 self.assertIsNot(None, IPerson(account, None))
162 self.assertTrue(view.login_called)165 self.assertTrue(view.login_called)
163 response = view.request.response166 response = view.request.response
@@ -167,7 +170,7 @@
167170
168 # We also update the last_write flag in the session, to make sure171 # We also update the last_write flag in the session, to make sure
169 # further requests use the master DB and thus see the newly created172 # further requests use the master DB and thus see the newly created
170 # stuff. 173 # stuff.
171 self.assertLastWriteIsSet(view.request)174 self.assertLastWriteIsSet(view.request)
172175
173 def test_unseen_identity(self):176 def test_unseen_identity(self):
@@ -196,7 +199,7 @@
196199
197 # We also update the last_write flag in the session, to make sure200 # We also update the last_write flag in the session, to make sure
198 # further requests use the master DB and thus see the newly created201 # further requests use the master DB and thus see the newly created
199 # stuff. 202 # stuff.
200 self.assertLastWriteIsSet(view.request)203 self.assertLastWriteIsSet(view.request)
201204
202 def test_unseen_identity_with_registered_email(self):205 def test_unseen_identity_with_registered_email(self):
@@ -230,7 +233,7 @@
230233
231 # We also update the last_write flag in the session, to make sure234 # We also update the last_write flag in the session, to make sure
232 # further requests use the master DB and thus see the newly created235 # further requests use the master DB and thus see the newly created
233 # stuff. 236 # stuff.
234 self.assertLastWriteIsSet(view.request)237 self.assertLastWriteIsSet(view.request)
235238
236 def test_deactivated_account(self):239 def test_deactivated_account(self):
@@ -256,7 +259,7 @@
256 self.assertEquals(email, account.preferredemail.email)259 self.assertEquals(email, account.preferredemail.email)
257 # We also update the last_write flag in the session, to make sure260 # We also update the last_write flag in the session, to make sure
258 # further requests use the master DB and thus see the newly created261 # further requests use the master DB and thus see the newly created
259 # stuff. 262 # stuff.
260 self.assertLastWriteIsSet(view.request)263 self.assertLastWriteIsSet(view.request)
261264
262 def test_never_used_account(self):265 def test_never_used_account(self):
@@ -278,7 +281,7 @@
278 self.assertEquals(email, account.preferredemail.email)281 self.assertEquals(email, account.preferredemail.email)
279 # We also update the last_write flag in the session, to make sure282 # We also update the last_write flag in the session, to make sure
280 # further requests use the master DB and thus see the newly created283 # further requests use the master DB and thus see the newly created
281 # stuff. 284 # stuff.
282 self.assertLastWriteIsSet(view.request)285 self.assertLastWriteIsSet(view.request)
283286
284 def test_suspended_account(self):287 def test_suspended_account(self):
@@ -286,7 +289,8 @@
286 # login, but we must not allow that.289 # login, but we must not allow that.
287 account = self.factory.makeAccount(290 account = self.factory.makeAccount(
288 'Test account', status=AccountStatus.SUSPENDED)291 'Test account', status=AccountStatus.SUSPENDED)
289 view, html = self._createViewWithResponse(account)292 with SRegResponse_fromSuccessResponse_stubbed():
293 view, html = self._createViewWithResponse(account)
290 self.assertFalse(view.login_called)294 self.assertFalse(view.login_called)
291 main_content = extract_text(find_main_content(html))295 main_content = extract_text(find_main_content(html))
292 self.assertIn('This account has been suspended', main_content)296 self.assertIn('This account has been suspended', main_content)
293297
=== 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 @@
297 commented on.297 commented on.
298 """)298 """)
299299
300 SOFTWARE_CENTER_PURCHASE = DBItem(16, """
301 Created by purchasing commercial software through Software Center.
302
303 A purchase of commercial software (ie. subscriptions to a private
304 and commercial archive) was made via Software Center.
305 """)
306
300307
301class TeamMembershipRenewalPolicy(DBEnumeratedType):308class TeamMembershipRenewalPolicy(DBEnumeratedType):
302 """TeamMembership Renewal Policy.309 """TeamMembership Renewal Policy.
@@ -1754,6 +1761,33 @@
1754 on the displayname or other arguments.1761 on the displayname or other arguments.
1755 """1762 """
17561763
1764 def getOrCreateByOpenIDIdentifier(openid_identifier, email,
1765 full_name, creation_rationale, comment):
1766 """Get or create a person for a given OpenID identifier.
1767
1768 This is used when users login. We get the account with the given
1769 OpenID identifier (creating one if it doesn't already exist) and
1770 act according to the account's state:
1771 - If the account is suspended, we stop and raise an error.
1772 - If the account is deactivated, we reactivate it and proceed;
1773 - If the account is active, we just proceed.
1774
1775 If there is no existing Launchpad person for the account, we
1776 create it.
1777
1778 :param openid_identifier: representing the authenticated user.
1779 :param email_address: the email address of the user.
1780 :param full_name: the full name of the user.
1781 :param creation_rationale: When an account or person needs to
1782 be created, this indicates why it was created.
1783 :param comment: If the account is reactivated or person created,
1784 this comment indicates why.
1785 :return: a tuple of `IPerson` and a boolean indicating whether the
1786 database was updated.
1787 :raises AccountSuspendedError: if the account associated with the
1788 identifier has been suspended.
1789 """
1790
1757 @call_with(teamowner=REQUEST_USER)1791 @call_with(teamowner=REQUEST_USER)
1758 @rename_parameters_as(1792 @rename_parameters_as(
1759 displayname='display_name', teamdescription='team_description',1793 displayname='display_name', teamdescription='team_description',
17601794
=== 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 @@
61from canonical.lazr.utils import get_current_browser_request, safe_hasattr61from canonical.lazr.utils import get_current_browser_request, safe_hasattr
6262
63from canonical.launchpad.database.account import Account, AccountPassword63from canonical.launchpad.database.account import Account, AccountPassword
64from canonical.launchpad.interfaces.account import AccountSuspendedError
64from lp.bugs.model.bugtarget import HasBugsBase65from lp.bugs.model.bugtarget import HasBugsBase
65from canonical.launchpad.database.stormsugar import StartsWith66from canonical.launchpad.database.stormsugar import StartsWith
66from lp.registry.model.karma import KarmaCategory67from lp.registry.model.karma import KarmaCategory
@@ -152,6 +153,7 @@
152153
153from canonical.launchpad.validators.email import valid_email154from canonical.launchpad.validators.email import valid_email
154from canonical.launchpad.validators.name import sanitize_name, valid_name155from canonical.launchpad.validators.name import sanitize_name, valid_name
156from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
155from lp.registry.interfaces.person import validate_public_person157from lp.registry.interfaces.person import validate_public_person
156158
157159
@@ -2429,6 +2431,59 @@
2429 key=lambda obj: (obj.karma, obj.displayname, obj.id),2431 key=lambda obj: (obj.karma, obj.displayname, obj.id),
2430 reverse=True)2432 reverse=True)
24312433
2434 def getOrCreateByOpenIDIdentifier(
2435 self, openid_identifier, email_address, full_name,
2436 creation_rationale, comment):
2437 """See `IPersonSet`."""
2438 assert email_address is not None and full_name is not None, (
2439 "Both email address and full name are required to "
2440 "create an account.")
2441 db_updated = False
2442 with MasterDatabasePolicy():
2443 account_set = getUtility(IAccountSet)
2444 email_set = getUtility(IEmailAddressSet)
2445 email = email_set.getByEmail(email_address)
2446 try:
2447 account = account_set.getByOpenIDIdentifier(
2448 openid_identifier)
2449 except LookupError:
2450 if email is None:
2451 # There is no account associated with the identifier
2452 # nor an email associated with the email address.
2453 # We'll create one.
2454 account, email = account_set.createAccountAndEmail(
2455 email_address, creation_rationale, full_name,
2456 password=None,
2457 openid_identifier=openid_identifier)
2458 else:
2459 account = email.account
2460 assert account is not None, (
2461 "This email address should have an associated "
2462 "account.")
2463 removeSecurityProxy(account).openid_identifier = (
2464 openid_identifier)
2465 db_updated = True
2466
2467 if account.status == AccountStatus.SUSPENDED:
2468 raise AccountSuspendedError(
2469 "The account matching the identifier is suspended.")
2470 elif account.status in [AccountStatus.DEACTIVATED,
2471 AccountStatus.NOACCOUNT]:
2472 password = '' # Needed just to please reactivate() below.
2473 if email is None:
2474 email = email_set.new(email_address, account=account)
2475 removeSecurityProxy(account).reactivate(
2476 comment, password, removeSecurityProxy(email))
2477 else:
2478 # Account is active, so nothing to do.
2479 pass
2480 if IPerson(account, None) is None:
2481 removeSecurityProxy(account).createPerson(
2482 creation_rationale, comment=comment)
2483 db_updated = True
2484
2485 return IPerson(account), db_updated
2486
2432 def newTeam(self, teamowner, name, displayname, teamdescription=None,2487 def newTeam(self, teamowner, name, displayname, teamdescription=None,
2433 subscriptionpolicy=TeamSubscriptionPolicy.MODERATED,2488 subscriptionpolicy=TeamSubscriptionPolicy.MODERATED,
2434 defaultmembershipperiod=None, defaultrenewalperiod=None):2489 defaultmembershipperiod=None, defaultrenewalperiod=None):
24352490
=== 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 @@
16 MailingListAutoSubscribePolicy)16 MailingListAutoSubscribePolicy)
17from lp.registry.interfaces.person import (17from lp.registry.interfaces.person import (
18 PersonCreationRationale, IPersonSet)18 PersonCreationRationale, IPersonSet)
19from lp.testing import TestCaseWithFactory, login_person, logout19from lp.testing import (
20 TestCaseWithFactory, login_person, logout)
2021
21from canonical.database.sqlbase import cursor22from canonical.database.sqlbase import cursor
23from canonical.launchpad.interfaces.account import (
24 AccountStatus, AccountSuspendedError)
22from canonical.launchpad.testing.databasehelpers import (25from canonical.launchpad.testing.databasehelpers import (
23 remove_all_sample_data_branches)26 remove_all_sample_data_branches)
24from canonical.testing import DatabaseFunctionalLayer27from canonical.testing import DatabaseFunctionalLayer
@@ -146,5 +149,93 @@
146 self.assertEqual(1, self.cur.rowcount)149 self.assertEqual(1, self.cur.rowcount)
147150
148151
152class TestPersonSetGetOrCreateByOpenIDIdentifier(TestCaseWithFactory):
153
154 layer = DatabaseFunctionalLayer
155
156 def setUp(self):
157 super(TestPersonSetGetOrCreateByOpenIDIdentifier, self).setUp()
158 self.person_set = getUtility(IPersonSet)
159
160 def callGetOrCreate(self, identifier, email='a@b.com'):
161 return self.person_set.getOrCreateByOpenIDIdentifier(
162 identifier, email, "Joe Bloggs",
163 PersonCreationRationale.SOFTWARE_CENTER_PURCHASE,
164 "when purchasing an application via Software Center.")
165
166 def test_existing_person(self):
167 person = self.factory.makePerson()
168 openid_ident = removeSecurityProxy(person.account).openid_identifier
169 person_set = getUtility(IPersonSet)
170
171 result, db_updated = self.callGetOrCreate(openid_ident)
172
173 self.assertEqual(person, result)
174 self.assertFalse(db_updated)
175
176 def test_existing_account_no_person(self):
177 # A person is created with the correct rationale.
178 account = self.factory.makeAccount('purchaser')
179 openid_ident = removeSecurityProxy(account).openid_identifier
180
181 person, db_updated = self.callGetOrCreate(openid_ident)
182
183 self.assertEqual(account, person.account)
184 # The person is created with the correct rationale, creation
185 # comment, registrant, appropriate display name.
186 self.assertEqual(
187 "when purchasing an application via Software Center.",
188 person.creation_comment)
189 self.assertEqual(
190 PersonCreationRationale.SOFTWARE_CENTER_PURCHASE,
191 person.creation_rationale)
192 self.assertTrue(db_updated)
193
194 def test_existing_deactivated_account(self):
195 # An existing deactivated account will be reactivated.
196 account = self.factory.makeAccount('purchaser',
197 status=AccountStatus.DEACTIVATED)
198 openid_ident = removeSecurityProxy(account).openid_identifier
199
200 person, db_updated = self.callGetOrCreate(openid_ident)
201 self.assertEqual(AccountStatus.ACTIVE, person.account.status)
202 self.assertTrue(db_updated)
203 self.assertEqual(
204 "when purchasing an application via Software Center.",
205 removeSecurityProxy(person.account).status_comment)
206
207 def test_existing_suspended_account(self):
208 # An existing suspended account will raise an exception.
209 account = self.factory.makeAccount('purchaser',
210 status=AccountStatus.SUSPENDED)
211 openid_ident = removeSecurityProxy(account).openid_identifier
212
213 self.assertRaises(
214 AccountSuspendedError, self.callGetOrCreate, openid_ident)
215
216 def test_no_account_or_email(self):
217 # An identifier can be used to create an account (it is assumed
218 # to be already authenticated with SSO).
219 person, db_updated = self.callGetOrCreate('openid-identifier')
220
221 self.assertEqual(
222 "openid-identifier",
223 removeSecurityProxy(person.account).openid_identifier)
224 self.assertTrue(db_updated)
225
226 def test_no_matching_account_existing_email(self):
227 # The openid_identity of the account matching the email will
228 # updated.
229 other_account = self.factory.makeAccount('test', email='a@b.com')
230
231 person, db_updated = self.callGetOrCreate(
232 'other-openid-identifier', 'a@b.com')
233
234 self.assertEqual(other_account, person.account)
235 self.assertEqual(
236 'other-openid-identifier',
237 removeSecurityProxy(person.account).openid_identifier)
238
239
149def test_suite():240def test_suite():
150 return TestLoader().loadTestsFromName(__name__)241 return TestLoader().loadTestsFromName(__name__)
151242
=== 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 @@
198 else:198 else:
199 response = self.openid_request.answer(True)199 response = self.openid_request.answer(True)
200200
201 sreg_fields = dict(nickname=IPerson(self.account).name)201 sreg_fields = dict(
202 nickname=IPerson(self.account).name,
203 email=self.account.preferredemail.email,
204 fullname=self.account.displayname)
202 sreg_request = SRegRequest.fromOpenIDRequest(self.openid_request)205 sreg_request = SRegRequest.fromOpenIDRequest(self.openid_request)
203 sreg_response = SRegResponse.extractResponse(206 sreg_response = SRegResponse.extractResponse(
204 sreg_request, sreg_fields)207 sreg_request, sreg_fields)
@@ -224,7 +227,7 @@
224 This class implements an OpenID endpoint using the python-openid227 This class implements an OpenID endpoint using the python-openid
225 library. In addition to the normal modes of operation, it also228 library. In addition to the normal modes of operation, it also
226 implements the OpenID 2.0 identifier select mode.229 implements the OpenID 2.0 identifier select mode.
227 230
228 Note that the checkid_immediate mode is not supported.231 Note that the checkid_immediate mode is not supported.
229 """232 """
230233

Subscribers

People subscribed via source and target branches

to status/vote changes: