launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14903
[Merge] lp:~stevenk/launchpad/openid-identifier-is-the-new-black into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/openid-identifier-is-the-new-black into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #556680 in Launchpad itself: "attempting to create a new account with an existing team email address at login.ubuntu.com oopses"
https://bugs.launchpad.net/launchpad/+bug/556680
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/openid-identifier-is-the-new-black/+merge/142641
Rewrite IPersonSet.getOrCreateByOpenIDIdentifier() so that the OpenID identifier is trusted as the first class object, rather than using the e-mail address and then potentially stealing the OpenID identifier from another account. Do not allow blank OpenID identifiers, and do not create unknown e-mail addresses if the OpenID identifier is known -- the workflow where both the OpenID identifier and the e-mail address are unknown is unchanged.
Secondly, no longer create e-mail addresses from each unrevoked UID when a GPG key is imported.
A bunch of cleanup, with the exception of xx-gpg-coc.txt, since that test is a mess, and make lint has a large amount of problems with it, and I didn't want to confuse this complex diff with non-required changes.
--
https://code.launchpad.net/~stevenk/launchpad/openid-identifier-is-the-new-black/+merge/142641
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/openid-identifier-is-the-new-black into lp:launchpad.
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2012-12-10 16:46:16 +0000
+++ lib/lp/registry/model/person.py 2013-01-10 06:27:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Implementation classes for a Person."""
@@ -610,9 +610,9 @@
account_status_comment = property(
_get_account_status_comment, _set_account_status_comment)
- teamowner = ForeignKey(dbName='teamowner', foreignKey='Person',
- default=None,
- storm_validator=validate_public_person)
+ teamowner = ForeignKey(
+ dbName='teamowner', foreignKey='Person', default=None,
+ storm_validator=validate_public_person)
sshkeys = SQLMultipleJoin('SSHKey', joinColumn='person')
@@ -620,13 +620,12 @@
enum=TeamMembershipRenewalPolicy,
default=TeamMembershipRenewalPolicy.NONE)
membership_policy = EnumCol(
- dbName='subscriptionpolicy',
- enum=TeamMembershipPolicy,
+ dbName='subscriptionpolicy', enum=TeamMembershipPolicy,
default=TeamMembershipPolicy.RESTRICTED,
storm_validator=validate_membership_policy)
defaultrenewalperiod = IntCol(dbName='defaultrenewalperiod', default=None)
- defaultmembershipperiod = IntCol(dbName='defaultmembershipperiod',
- default=None)
+ defaultmembershipperiod = IntCol(
+ dbName='defaultmembershipperiod', default=None)
mailing_list_auto_subscribe_policy = EnumCol(
enum=MailingListAutoSubscribePolicy,
default=MailingListAutoSubscribePolicy.ON_REGISTRATION)
@@ -647,13 +646,11 @@
jabberids = SQLMultipleJoin('JabberID', joinColumn='person')
visibility = EnumCol(
- enum=PersonVisibility,
- default=PersonVisibility.PUBLIC,
+ enum=PersonVisibility, default=PersonVisibility.PUBLIC,
storm_validator=validate_person_visibility)
personal_standing = EnumCol(
- enum=PersonalStanding, default=PersonalStanding.UNKNOWN,
- notNull=True)
+ enum=PersonalStanding, default=PersonalStanding.UNKNOWN, notNull=True)
personal_standing_reason = StringCol(default=None)
@@ -3337,89 +3334,62 @@
return None
return IPerson(account)
- def getOrCreateByOpenIDIdentifier(
- self, openid_identifier, email_address, full_name,
- creation_rationale, comment):
+ 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.")
+ "Both email address and full name are required to create an "
+ "account.")
db_updated = False
assert isinstance(openid_identifier, unicode)
+ assert openid_identifier != u'', (
+ "OpenID identifier must not be empty.")
# Load the EmailAddress, Account and OpenIdIdentifier records
# from the master (if they exist). We use the master to avoid
# possible replication lag issues but this might actually be
# unnecessary.
with MasterDatabasePolicy():
- email, person = (
- getUtility(IPersonSet).getByEmails(
- [email_address],
- filter_status=False).one()
- or (None, None))
identifier = IStore(OpenIdIdentifier).find(
OpenIdIdentifier, identifier=openid_identifier).one()
-
- # XXX wgrant 2012-01-20 bug=556680: This is awful, as it can
- # lock people out of their account until they change their
- # SSO address. But stealing addresses from other accounts is
- # probably worse.
- if email is not None and email.person.is_team:
- raise TeamEmailAddressError()
-
- if email is None:
- if identifier is None:
- # Neither the Email Address not the OpenId Identifier
- # exist in the database. Create the email address,
- # account, and associated info. OpenIdIdentifier is
- # created later.
+ email = getUtility(IEmailAddressSet).getByEmail(email_address)
+
+ if identifier is None:
+ # We don't know about the OpenID identifier yet, so try
+ # to match a person by email address, or as a last
+ # resort create a new one.
+ if email is not None:
+ person = email.person
+ else:
person_set = getUtility(IPersonSet)
person, email = person_set.createPersonAndEmail(
email_address, creation_rationale, comment=comment,
displayname=full_name)
- db_updated = True
- else:
- # The Email Address does not exist in the database,
- # but the OpenId Identifier does. Create the Email
- # Address and link it to the person.
- person = IPerson(identifier.account, None)
- assert person is not None, (
- 'Received a personless account.')
- emailaddress_set = getUtility(IEmailAddressSet)
- email = emailaddress_set.new(email_address, person=person)
- db_updated = True
- elif email.person.account is None:
- # Email address and person exist, but there is no
- # account. Create and link it.
- account_set = getUtility(IAccountSet)
- account = account_set.new(
- AccountCreationRationale.OWNER_CREATED_LAUNCHPAD,
- full_name)
- removeSecurityProxy(email.person).account = account
- db_updated = True
-
- person = email.person
- assert person.account is not None
-
- if identifier is None:
- # This is the first time we have seen that
- # OpenIdIdentifier. Link it.
+
+ # It's possible that the email address is owned by a
+ # team. Reject the login attempt, and wait for the user
+ # to change their address.
+ if person.is_team:
+ raise TeamEmailAddressError()
+
+ # Some autocreated Persons won't have a corresponding
+ # Account yet.
+ if not person.account:
+ removeSecurityProxy(email.person).account = (
+ getUtility(IAccountSet).new(
+ AccountCreationRationale.OWNER_CREATED_LAUNCHPAD,
+ full_name))
+
+ # Create the identifier, and link it.
identifier = OpenIdIdentifier()
identifier.account = person.account
identifier.identifier = openid_identifier
IStore(OpenIdIdentifier).add(identifier)
db_updated = True
- elif identifier.account != person.account:
- # The ISD OpenId server may have linked this OpenId
- # identifier to a new email address, or the user may
- # have transfered their email address to a different
- # Launchpad Account. If that happened, repair the
- # link - we trust the ISD OpenId server.
- identifier.account = person.account
- db_updated = True
- # We now have an account, email address, and openid identifier.
+ person = IPerson(identifier.account, None)
+ assert person is not None, ('Received a personless account.')
if person.account.status == AccountStatus.SUSPENDED:
raise AccountSuspendedError(
@@ -3434,7 +3404,7 @@
# Account is active, so nothing to do.
pass
- return email.person, db_updated
+ return person, db_updated
def newTeam(self, teamowner, name, displayname, teamdescription=None,
membership_policy=TeamMembershipPolicy.MODERATED,
@@ -3461,9 +3431,9 @@
teamowner, team, TeamMembershipStatus.ADMIN, teamowner)
return team
- def createPersonAndEmail(
- self, email, rationale, comment=None, name=None, displayname=None,
- hide_email_addresses=False, registrant=None):
+ def createPersonAndEmail(self, email, rationale, comment=None, name=None,
+ displayname=None, hide_email_addresses=False,
+ registrant=None):
"""See `IPersonSet`."""
# This check is also done in EmailAddressSet.new() and also
@@ -3492,9 +3462,8 @@
return person, email
- def createPersonWithoutEmail(
- self, name, rationale, comment=None, displayname=None,
- registrant=None):
+ def createPersonWithoutEmail(self, name, rationale, comment=None,
+ displayname=None, registrant=None):
"""Create and return a new Person without using an email address.
See `IPersonSet`.
=== modified file 'lib/lp/registry/stories/gpg-coc/xx-gpg-coc.txt'
--- lib/lp/registry/stories/gpg-coc/xx-gpg-coc.txt 2012-11-15 19:02:53 +0000
+++ lib/lp/registry/stories/gpg-coc/xx-gpg-coc.txt 2013-01-10 06:27:21 +0000
@@ -133,7 +133,6 @@
>>> for message in get_feedback_messages(browser.contents):
... print message
The key 1024D/DFD20543 was successfully validated.
- ...
Certify the key is imported:
@@ -141,17 +140,6 @@
>>> browser.getControl(name='DEACTIVATE_GPGKEY').displayOptions
['1024D/DFD20543']
-Make sure that UIDs were properly processed; in particular, check that
-"sample.revoked@xxxxxxxxxxxxx", the revoked UID in the key, wasn't
-added as an email address:
-
- >>> from lp.testing.pages import strip_label
-
- >>> browser.open("http://launchpad.dev/~name12/+editemails";)
- >>> control = browser.getControl(name='field.UNVALIDATED_SELECTED')
- >>> [strip_label(label) for label in sorted(control.displayOptions)]
- ['sample.person@xxxxxxxxxxxxx', 'testtest@xxxxxxxxxxxxx']
-
Verify that the key was imported with the "can encrypt" flag set:
>>> from lp.registry.model.gpgkey import GPGKey
@@ -309,14 +297,9 @@
>>> browser.getControl('Continue').click()
>>> browser.url
'http://launchpad.dev/~name12'
-
-The address associated with the key is not associated with his
-Launchpad account, so Launchpad gives him a message to that effect.
-
>>> for message in get_feedback_messages(browser.contents):
... print message
The key 1024D/17B05A8F was successfully validated.
- Some of your key's UIDs (sign.only@xxxxxxxxxxxxx) are not registered...
Now that the key has been validated, the login token is consumed:
=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py 2012-11-01 19:23:02 +0000
+++ lib/lp/registry/tests/test_personset.py 2013-01-10 06:27:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for PersonSet."""
@@ -33,6 +33,7 @@
)
from lp.registry.interfaces.nameblacklist import INameBlacklistSet
from lp.registry.interfaces.person import (
+ IPerson,
IPersonSet,
PersonCreationRationale,
TeamEmailAddressError,
@@ -806,30 +807,6 @@
in self.account.openid_identifiers]
self.assertIn(new_identifier, identifiers)
- def testNewEmailAddress(self):
- # Account looked up by OpenId identifier and new EmailAddress
- # attached. We can do this because we trust our OpenId Provider.
- new_email = u'new_email@xxxxxxxxxxx'
- found, updated = self.person_set.getOrCreateByOpenIDIdentifier(
- self.identifier.identifier, new_email, 'Ignored Name',
- PersonCreationRationale.UNKNOWN, 'No Comment')
- found = removeSecurityProxy(found)
-
- self.assertIs(True, updated)
- self.assertIs(self.person, found)
- self.assertIs(self.account, found.account)
- self.assertEqual(
- [self.identifier], list(self.account.openid_identifiers))
-
- # The old email address is still there and correctly linked.
- self.assertIs(self.email, found.preferredemail)
- self.assertIs(self.email.person, self.person)
-
- # The new email address is there too and correctly linked.
- new_email = self.store.find(EmailAddress, email=new_email).one()
- self.assertIs(new_email.person, self.person)
- self.assertEqual(EmailAddressStatus.NEW, new_email.status)
-
def testNewAccountAndIdentifier(self):
# If neither the OpenId Identifier nor the email address are
# found, we create everything.
@@ -862,7 +839,7 @@
PersonCreationRationale.UNKNOWN, 'No Comment')
found = removeSecurityProxy(found)
- self.assertIs(True, updated)
+ self.assertTrue(updated)
self.assertIsNot(None, found.account)
self.assertEqual(
@@ -870,14 +847,10 @@
self.assertIs(self.email.person, found)
self.assertEqual(EmailAddressStatus.PREFERRED, self.email.status)
- def testMovedEmailAddress(self):
- # The EmailAddress and OpenId Identifier are both in the
- # database, but they are not linked to the same account. The
- # identifier needs to be relinked to the correct account - the
- # user able to log into the trusted SSO with that email address
- # should be able to log into Launchpad with that email address.
- # This lets us cope with the SSO migrating email addresses
- # between SSO accounts.
+ def testEmailAddressAccountAndOpenIDAccountAreDifferent(self):
+ # The EmailAddress and OpenId Identifier are both in the database,
+ # but they are not linked to the same account. In this case, the
+ # OpenId Identifier trumps the EmailAddress's account.
self.identifier.account = self.store.find(
Account, displayname='Foo Bar').one()
@@ -886,12 +859,18 @@
PersonCreationRationale.UNKNOWN, 'No Comment')
found = removeSecurityProxy(found)
- self.assertIs(True, updated)
- self.assertIs(self.person, found)
+ self.assertFalse(updated)
+ self.assertIs(IPerson(self.identifier.account), found)
self.assertIs(found.account, self.identifier.account)
self.assertIn(self.identifier, list(found.account.openid_identifiers))
+ def testEmptyOpenIDIdentifier(self):
+ self.assertRaises(
+ AssertionError,
+ self.person_set.getOrCreateByOpenIDIdentifier, u'', 'foo@xxxxxxx',
+ 'New Name', PersonCreationRationale.UNKNOWN, 'No Comment')
+
def testTeamEmailAddress(self):
# If the EmailAddress is linked to a team, login fails. There is
# no way to automatically recover -- someone must manually fix
=== modified file 'lib/lp/services/verification/browser/logintoken.py'
--- lib/lp/services/verification/browser/logintoken.py 2012-12-12 04:59:52 +0000
+++ lib/lp/services/verification/browser/logintoken.py 2013-01-10 06:27:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -339,39 +339,19 @@
def _activateGPGKey(self, key, can_encrypt):
person_url = canonical_url(self.context.requester)
- lpkey, new, created, owned_by_others = self.context.activateGPGKey(
- key, can_encrypt)
+ lpkey, new, = self.context.activateGPGKey(key, can_encrypt)
- if not new:
+ if new:
+ self.request.response.addInfoNotification(_(
+ "The key ${lpkey} was successfully validated. ",
+ mapping=dict(lpkey=lpkey.displayname)))
+ else:
msgid = _(
'Key ${lpkey} successfully reactivated. '
'<a href="${url}/+editpgpkeys">See more Information'
'</a>',
mapping=dict(lpkey=lpkey.displayname, url=person_url))
self.request.response.addInfoNotification(structured(msgid))
- return
-
- self.request.response.addInfoNotification(_(
- "The key ${lpkey} was successfully validated. ",
- mapping=dict(lpkey=lpkey.displayname)))
-
- if len(created):
- msgid = _(
- "<p>Some of your key's UIDs (<code>${emails}</code>) are "
- "not registered in Launchpad. If you want to use them in "
- 'Launchpad, you will need to <a href="${url}/+editemails">'
- 'confirm them</a> first.</p>',
- mapping=dict(emails=', '.join(created), url=person_url))
- self.request.response.addInfoNotification(structured(msgid))
-
- if len(owned_by_others):
- msgid = _(
- "<p>Also, some of them (<code>${emails}</code>) are "
- "associated with other profile(s) in Launchpad, so you may "
- 'want to <a href="/people/+requestmerge">merge them</a> into '
- "your current one.</p>",
- mapping=dict(emails=', '.join(owned_by_others)))
- self.request.response.addInfoNotification(structured(msgid))
def _getGPGKey(self):
"""Look up the OpenPGP key for this login token.
=== modified file 'lib/lp/services/verification/interfaces/logintoken.py'
--- lib/lp/services/verification/interfaces/logintoken.py 2013-01-07 02:40:55 +0000
+++ lib/lp/services/verification/interfaces/logintoken.py 2013-01-10 06:27:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Login token interfaces."""
@@ -83,19 +83,7 @@
def activateGPGKey(key, can_encrypt):
"""Activate a GPG key.
- :return: A Launchpad key, whether it's new, email addresses that were
- created, email addresses owned by someone else.
- """
-
- def createEmailAddresses(uids):
- """Create EmailAddresses for the GPG UIDs that do not exist yet.
-
- For each of the given UIDs, check if it is already registered and, if
- not, register it.
-
- Return a tuple containing the list of newly created emails (as
- strings) and the emails that exist and are already assigned to another
- person (also as strings).
+ :return: A Launchpad key, and whether it's new.
"""
=== modified file 'lib/lp/services/verification/model/logintoken.py'
--- lib/lp/services/verification/model/logintoken.py 2013-01-07 02:40:55 +0000
+++ lib/lp/services/verification/model/logintoken.py 2013-01-10 06:27:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -7,8 +7,6 @@
'LoginTokenSet',
]
-from itertools import chain
-
import pytz
from sqlobject import (
ForeignKey,
@@ -18,7 +16,6 @@
from storm.expr import And
from zope.component import getUtility
from zope.interface import implements
-from zope.security.proxy import removeSecurityProxy
from lp.app.errors import NotFoundError
from lp.app.validators.email import valid_email
@@ -38,7 +35,6 @@
sqlvalues,
)
from lp.services.gpg.interfaces import IGPGHandler
-from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
from lp.services.mail.helpers import get_email_template
from lp.services.mail.sendmail import (
format_address,
@@ -68,8 +64,7 @@
token = StringCol(dbName='token', unique=True)
tokentype = EnumCol(dbName='tokentype', notNull=True, enum=LoginTokenType)
date_created = UtcDateTimeCol(dbName='created', notNull=True)
- fingerprint = StringCol(dbName='fingerprint', notNull=False,
- default=None)
+ fingerprint = StringCol(dbName='fingerprint', notNull=False, default=None)
date_consumed = UtcDateTimeCol(default=None)
password = '' # Quick fix for Bug #2481
@@ -253,56 +248,10 @@
def activateGPGKey(self, key, can_encrypt):
"""See `ILoginToken`."""
- gpgkeyset = getUtility(IGPGKeySet)
- lpkey, new = gpgkeyset.activate(self.requester, key, can_encrypt)
-
+ lpkey, new = getUtility(IGPGKeySet).activate(
+ self.requester, key, can_encrypt)
self.consume()
-
- if not new:
- return lpkey, new, [], []
-
- created, owned_by_others = self.createEmailAddresses(key.emails)
- return lpkey, new, created, owned_by_others
-
- def createEmailAddresses(self, uids):
- """Create EmailAddresses for the GPG UIDs that do not exist yet.
-
- For each of the given UIDs, check if it is already registered and, if
- not, register it.
-
- Return a tuple containing the list of newly created emails (as
- strings) and the emails that exist and are already assigned to another
- person (also as strings).
- """
- emailset = getUtility(IEmailAddressSet)
- requester = self.requester
- emails = chain(requester.validatedemails, [requester.preferredemail])
- # Must remove the security proxy because the user may not be logged in
- # and thus won't be allowed to view the requester's email addresses.
- emails = [
- removeSecurityProxy(email).email.lower() for email in emails]
-
- created = []
- existing_and_owned_by_others = []
- for uid in uids:
- # Here we use .lower() because the case of email addresses's chars
- # don't matter to us (e.g. 'foo@xxxxxxx' is the same as
- # 'Foo@xxxxxxx'). However, note that we use the original form
- # when creating a new email.
- if uid.lower() not in emails:
- # EmailAddressSet.getByEmail() is not case-sensitive, so
- # there's no need to do uid.lower() here.
- if emailset.getByEmail(uid) is not None:
- # This email address is registered but does not belong to
- # our user.
- existing_and_owned_by_others.append(uid)
- else:
- # The email is not yet registered, so we register it for
- # our user.
- email = emailset.new(uid, requester)
- created.append(uid)
-
- return created, existing_and_owned_by_others
+ return lpkey, new
class LoginTokenSet:
