launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06139
[Merge] lp:~wgrant/launchpad/bug-918843 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-918843 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #918843 in Launchpad itself: "IntegrityError: new row for relation "person" violates check constraint "teams_have_no_account" "
https://bugs.launchpad.net/launchpad/+bug/918843
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-918843/+merge/89392
My refactoring of the login machinery last week broke some untested and probably unintentional behaviour: if the OpenID provider returned an email address associated with a team, a new person would be created and steal the team's address. After my refactoring this case crashes instead.
Since stealing email addresses from teams is probably not something that we want to happen accidentally, this branch rejects login attempts that try to use a team email address. feedback@ can then sort out the SSO account or team email address to let authentication succeed.
SCA also uses that API, but https://pastebin.canonical.com/58485/ and https://pastebin.canonical.com/58486/ suggest that it will handle it like AccountSuspendedError, and this is done before payment.
A proper solution that does not induce nausea is achievable, but as discussed in bug #556680 it is somewhat non-trivial.
--
https://code.launchpad.net/~wgrant/launchpad/bug-918843/+merge/89392
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-918843 into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2012-01-15 11:52:24 +0000
+++ lib/lp/registry/interfaces/person.py 2012-01-20 07:03:32 +0000
@@ -35,6 +35,7 @@
'PersonalStanding',
'PRIVATE_TEAM_PREFIX',
'TeamContactMethod',
+ 'TeamEmailAddressError',
'TeamMembershipRenewalPolicy',
'TeamSubscriptionPolicy',
'validate_person',
@@ -2542,6 +2543,10 @@
_message_prefix = "No such person"
+class TeamEmailAddressError(Exception):
+ """The person cannot be created as a team owns its email address."""
+
+
# Fix value_type.schema of IPersonViewRestricted attributes.
for name in [
'all_members_prepopulated',
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2012-01-17 23:55:44 +0000
+++ lib/lp/registry/model/person.py 2012-01-20 07:03:32 +0000
@@ -180,6 +180,7 @@
PersonalStanding,
PersonCreationRationale,
PersonVisibility,
+ TeamEmailAddressError,
TeamMembershipRenewalPolicy,
TeamSubscriptionPolicy,
validate_public_person,
@@ -3094,6 +3095,13 @@
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
=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py 2012-01-12 08:13:59 +0000
+++ lib/lp/registry/tests/test_personset.py 2012-01-20 07:03:32 +0000
@@ -33,6 +33,7 @@
IPersonSet,
PersonCreationRationale,
PersonVisibility,
+ TeamEmailAddressError,
)
from lp.registry.interfaces.personnotification import IPersonNotificationSet
from lp.registry.model.accesspolicy import AccessPolicyGrant
@@ -793,6 +794,18 @@
self.assertIs(found.account, self.identifier.account)
self.assertIn(self.identifier, list(found.account.openid_identifiers))
+ def testTeamEmailAddress(self):
+ # If the EmailAddress is linked to a team, login fails. There is
+ # no way to automatically recover -- someone must manually fix
+ # the email address of the team or the SSO account.
+ self.factory.makeTeam(email="foo@xxxxxxx")
+
+ self.assertRaises(
+ TeamEmailAddressError,
+ self.person_set.getOrCreateByOpenIDIdentifier,
+ u'other-openid-identifier', 'foo@xxxxxxx', 'New Name',
+ PersonCreationRationale.UNKNOWN, 'No Comment')
+
class TestCreatePersonAndEmail(TestCase):
"""Test `IPersonSet`.createPersonAndEmail()."""
=== modified file 'lib/lp/registry/tests/test_xmlrpc.py'
--- lib/lp/registry/tests/test_xmlrpc.py 2012-01-05 00:23:45 +0000
+++ lib/lp/registry/tests/test_xmlrpc.py 2012-01-20 07:03:32 +0000
@@ -86,7 +86,7 @@
removeSecurityProxy(
person.account).openid_identifiers.any().identifier)
- def test_getOrCreateSoftwareCenterCustomer_xmlrpc_error(self):
+ def test_getOrCreateSoftwareCenterCustomer_xmlrpc_suspended(self):
# A suspended account results in an appropriate xmlrpc fault.
suspended_person = self.factory.makePerson(
displayname='Joe Blogs', email='a@xxxxx',
@@ -106,6 +106,22 @@
self.assertTrue(fault_raised)
+ def test_getOrCreateSoftwareCenterCustomer_xmlrpc_team(self):
+ # A team email address results in an appropriate xmlrpc fault.
+ self.factory.makeTeam(email='a@xxxxx')
+
+ # assertRaises doesn't let us check the type of Fault.
+ fault_raised = False
+ try:
+ self.rpc_proxy.getOrCreateSoftwareCenterCustomer(
+ 'foo', 'a@xxxxx', 'Joe Blogs')
+ except xmlrpclib.Fault, e:
+ fault_raised = True
+ self.assertEqual(400, e.faultCode)
+ self.assertIn('a@xxxxx', e.faultString)
+
+ self.assertTrue(fault_raised)
+
def test_not_available_on_public_api(self):
# The person set api is not available on the public xmlrpc
# service.
=== modified file 'lib/lp/registry/xmlrpc/softwarecenteragent.py'
--- lib/lp/registry/xmlrpc/softwarecenteragent.py 2012-01-01 02:58:52 +0000
+++ lib/lp/registry/xmlrpc/softwarecenteragent.py 2012-01-20 07:03:32 +0000
@@ -17,6 +17,7 @@
ISoftwareCenterAgentAPI,
ISoftwareCenterAgentApplication,
PersonCreationRationale,
+ TeamEmailAddressError,
)
from lp.services.identity.interfaces.account import AccountSuspendedError
from lp.services.webapp import LaunchpadXMLRPCView
@@ -38,6 +39,8 @@
"when purchasing an application via Software Center.")
except AccountSuspendedError:
return faults.AccountSuspended(openid_identifier)
+ except TeamEmailAddressError:
+ return faults.TeamEmailAddress(email, openid_identifier)
return person.name
@@ -47,4 +50,3 @@
implements(ISoftwareCenterAgentApplication)
title = "Software Center Agent API"
-
=== modified file 'lib/lp/services/webapp/login.py'
--- lib/lp/services/webapp/login.py 2012-01-03 11:08:31 +0000
+++ lib/lp/services/webapp/login.py 2012-01-20 07:03:32 +0000
@@ -42,6 +42,7 @@
from lp.registry.interfaces.person import (
IPersonSet,
PersonCreationRationale,
+ TeamEmailAddressError,
)
from lp.services.config import config
from lp.services.database.readonly import is_read_only
@@ -295,6 +296,9 @@
suspended_account_template = ViewPageTemplateFile(
'templates/login-suspended-account.pt')
+ team_email_address_template = ViewPageTemplateFile(
+ 'templates/login-team-email-address.pt')
+
def _gather_params(self, request):
params = dict(request.form)
for key, value in request.query_string_params.iteritems():
@@ -381,6 +385,8 @@
should_update_last_write = db_updated
except AccountSuspendedError:
return self.suspended_account_template()
+ except TeamEmailAddressError:
+ return self.team_email_address_template()
with MasterDatabasePolicy():
self.login(person)
=== added file 'lib/lp/services/webapp/templates/login-team-email-address.pt'
--- lib/lp/services/webapp/templates/login-team-email-address.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/services/webapp/templates/login-team-email-address.pt 2012-01-20 07:03:32 +0000
@@ -0,0 +1,22 @@
+<html
+ xmlns="http://www.w3.org/1999/xhtml";
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+ metal:use-macro="view/macro:page/locationless"
+ i18n:domain="launchpad">
+
+ <body>
+ <div class="top-portlet" metal:fill-slot="main">
+
+ <h1>Team email address conflict</h1>
+ <p class="error">
+ Your can't log in because your OpenID account's email address is
+ associated with a Launchpad team.
+ Contact <a href="mailto:feedback@xxxxxxxxxxxxx?subject=Team%20email%20address%20conflict";
+ >Launchpad Support</a> about this issue.
+ </p>
+
+ </div>
+ </body>
+</html>
=== modified file 'lib/lp/services/webapp/tests/test_login.py'
--- lib/lp/services/webapp/tests/test_login.py 2012-01-14 12:47:39 +0000
+++ lib/lp/services/webapp/tests/test_login.py 2012-01-20 07:03:32 +0000
@@ -424,6 +424,20 @@
main_content = extract_text(find_main_content(html))
self.assertIn('This account has been suspended', main_content)
+ def test_account_with_team_email_address(self):
+ # If the email address from the OpenID provider is owned by a
+ # team, there's not much we can do. See bug #556680 for
+ # discussions about a proper solution.
+ self.factory.makeTeam(email="foo@xxxxxxx")
+ person = self.factory.makePerson()
+
+ with SRegResponse_fromSuccessResponse_stubbed():
+ view, html = self._createViewWithResponse(
+ person.account, email="foo@xxxxxxx")
+ self.assertFalse(view.login_called)
+ main_content = extract_text(find_main_content(html))
+ self.assertIn('Team email address conflict', main_content)
+
def test_negative_openid_assertion(self):
# The OpenID provider responded with a negative assertion, so the
# login error page is shown.
=== modified file 'lib/lp/xmlrpc/configure.zcml'
--- lib/lp/xmlrpc/configure.zcml 2011-12-24 17:49:30 +0000
+++ lib/lp/xmlrpc/configure.zcml 2012-01-20 07:03:32 +0000
@@ -201,6 +201,9 @@
<class class="lp.xmlrpc.faults.AccountSuspended">
<require like_class="xmlrpclib.Fault" />
</class>
+ <class class="lp.xmlrpc.faults.TeamEmailAddress">
+ <require like_class="xmlrpclib.Fault" />
+ </class>
<class class="lp.xmlrpc.faults.InvalidSourcePackageName">
<require like_class="xmlrpclib.Fault" />
</class>
=== modified file 'lib/lp/xmlrpc/faults.py'
--- lib/lp/xmlrpc/faults.py 2011-08-11 21:24:28 +0000
+++ lib/lp/xmlrpc/faults.py 2012-01-20 07:03:32 +0000
@@ -39,6 +39,7 @@
'NotInTeam',
'NoUrlForBranch',
'RequiredParameterMissing',
+ 'TeamEmailAddress',
'UnexpectedStatusReport',
]
@@ -487,3 +488,17 @@
def __init__(self, name):
self.name = name
LaunchpadFault.__init__(self, name=name)
+
+
+class TeamEmailAddress(LaunchpadFault):
+ """Raised by `ISoftwareCenterAgentAPI` when an email address is a team."""
+
+ error_code = 400
+ msg_template = (
+ 'The email address \'%(email)s\' '
+ '(openid_identifier \'%(openid_identifier)s\') '
+ 'is linked to a Launchpad team.')
+
+ def __init__(self, email, openid_identifier):
+ LaunchpadFault.__init__(
+ self, email=email, openid_identifier=openid_identifier)
