launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18087
[Merge] lp:~wgrant/launchpad/sca-api into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/sca-api into lp:launchpad.
Commit message:
Implement PersonSet.getOrCreateSoftwareCenterCustomer and export it on the webservice. The xmlrpc-private method is deprecated.
Requested reviews:
Michael Nelson (michael.nelson)
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/sca-api/+merge/252420
Implement PersonSet.getOrCreateSoftwareCenterCustomer and export it on the webservice.
It will replace the xmlrpc-private method of the same name with something that's a bit less insecure. SCA will no longer be able to compromise existing active accounts by attaching new OpenID identifiers or email addresses to them. It retains the ability to:
- Look up an existing active account by OpenID identifier
- Activate an unactivated account by OpenID identifier and with the given email address
- Create a new account with the given OpenID identifier and email address
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/sca-api into lp:launchpad.
=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py 2015-01-07 00:35:53 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py 2015-03-10 11:46:53 +0000
@@ -1,8 +1,9 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
+from storm.store import Store
from testtools.matchers import Equals
from zope.component import getUtility
from zope.security.management import endInteraction
@@ -14,6 +15,7 @@
)
from lp.registry.interfaces.teammembership import ITeamMembershipSet
from lp.services.identity.interfaces.account import AccountStatus
+from lp.services.openid.model.openididentifier import OpenIdIdentifier
from lp.services.webapp.interfaces import OAuthPermission
from lp.testing import (
admin_logged_in,
@@ -282,3 +284,61 @@
'identifier=http://login1.dev/%%2Bid/%s'
% person_openid,
api_version='devel').jsonBody()['name'])
+
+ def getOrCreateSoftwareCenterCustomer(self, user):
+ webservice = webservice_for_person(
+ user, permission=OAuthPermission.WRITE_PRIVATE)
+ response = webservice.named_post(
+ '/people', 'getOrCreateSoftwareCenterCustomer',
+ openid_identifier='somebody',
+ email_address='somebody@xxxxxxxxxxx', display_name='Somebody',
+ api_version='devel')
+ return response
+
+ def test_getOrCreateSoftwareCenterCustomer(self):
+ # Software Center Agent (SCA) can get or create people by OpenID
+ # identifier.
+ with admin_logged_in():
+ sca = getUtility(IPersonSet).getByName('software-center-agent')
+ response = self.getOrCreateSoftwareCenterCustomer(sca)
+ self.assertEqual('Somebody', response.jsonBody()['display_name'])
+ with admin_logged_in():
+ person = getUtility(IPersonSet).getByEmail('somebody@xxxxxxxxxxx')
+ self.assertEqual('Somebody', person.displayname)
+ self.assertEqual(
+ ['somebody'],
+ [oid.identifier for oid in person.account.openid_identifiers])
+ self.assertEqual(
+ 'somebody@xxxxxxxxxxx', person.preferredemail.email)
+
+ def test_getOrCreateSoftwareCenterCustomer_is_restricted(self):
+ # The method may only be invoked by the ~software-center-agent
+ # celebrity user, as it is security-sensitive.
+ with admin_logged_in():
+ random = self.factory.makePerson()
+ response = self.getOrCreateSoftwareCenterCustomer(random)
+ self.assertEqual(401, response.status)
+
+ def test_getOrCreateSoftwareCenterCustomer_rejects_email_conflicts(self):
+ # An unknown OpenID identifier with a known email address causes
+ # the request to fail with 409 Conflict, as we'd otherwise end
+ # up linking the OpenID identifier to an existing account.
+ with admin_logged_in():
+ self.factory.makePerson(email='somebody@xxxxxxxxxxx')
+ sca = getUtility(IPersonSet).getByName('software-center-agent')
+ response = self.getOrCreateSoftwareCenterCustomer(sca)
+ self.assertEqual(409, response.status)
+
+ def test_getOrCreateSoftwareCenterCustomer_rejects_suspended(self):
+ # Suspended accounts are not returned.
+ with admin_logged_in():
+ existing = self.factory.makePerson(
+ email='somebody@xxxxxxxxxxx',
+ account_status=AccountStatus.SUSPENDED)
+ oid = OpenIdIdentifier()
+ oid.account = existing.account
+ oid.identifier = u'somebody'
+ Store.of(existing).add(oid)
+ sca = getUtility(IPersonSet).getByName('software-center-agent')
+ response = self.getOrCreateSoftwareCenterCustomer(sca)
+ self.assertEqual(400, response.status)
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2015-02-26 11:34:47 +0000
+++ lib/lp/registry/interfaces/person.py 2015-03-10 11:46:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Person interfaces."""
@@ -2173,6 +2173,30 @@
identifier has been suspended.
"""
+ @call_with(user=REQUEST_USER)
+ @operation_parameters(
+ openid_identifier=TextLine(
+ title=_("OpenID identifier suffix"), required=True),
+ email_address=TextLine(title=_("Email address"), required=True),
+ display_name=TextLine(title=_("Display name"), required=True))
+ @export_write_operation()
+ @operation_for_version("devel")
+ def getOrCreateSoftwareCenterCustomer(user, openid_identifier,
+ email_address, display_name):
+ """Restricted person creation API for Software Center Agent.
+
+ This method can only be called by Software Center Agent. It gets
+ a person by OpenID identifier or creates a new Launchpad person
+ from the OpenID identifier, email address and display name.
+
+ :param user: the `IPerson` performing the operation. Only the
+ software-center-agent celebrity is allowed.
+ :param openid_identifier: OpenID identifier suffix for the user.
+ This is *not* the full URL, just the unique suffix portion.
+ :param email_address: the email address of the user.
+ :param full_name: the full name of the user.
+ """
+
@call_with(teamowner=REQUEST_USER)
@rename_parameters_as(
displayname='display_name', teamdescription='team_description',
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2015-03-04 19:05:47 +0000
+++ lib/lp/registry/model/person.py 2015-03-10 11:46:53 +0000
@@ -268,6 +268,7 @@
INACTIVE_ACCOUNT_STATUSES,
)
from lp.services.identity.interfaces.emailaddress import (
+ EmailAddressAlreadyTaken,
EmailAddressStatus,
IEmailAddress,
IEmailAddressSet,
@@ -3317,8 +3318,26 @@
return IPerson(account)
def getOrCreateByOpenIDIdentifier(self, openid_identifier, email_address,
- full_name, creation_rationale, comment):
+ full_name, creation_rationale, comment,
+ trust_email=True):
"""See `IPersonSet`."""
+ # trust_email is an internal flag used by
+ # getOrCreateSoftwareCenterCustomer. We don't want SCA to be
+ # able to use the API to associate arbitrary OpenID identifiers
+ # and email addresses when that could compromise existing
+ # accounts.
+ #
+ # To that end, if trust_email is not set then the given
+ # email address will only be used to create an account. It will
+ # never be used to look up an account, nor will it be added to
+ # an existing account. This causes two additional cases to be
+ # rejected: unknown OpenID identifier but known email address,
+ # and deactivated account.
+ #
+ # Exempting account creation and activation from this rule opens
+ # us to a potential account fixation attack, but the risk is
+ # minimal.
+
assert email_address is not None and full_name is not None, (
"Both email address and full name are required to create an "
"account.")
@@ -3341,13 +3360,19 @@
# 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:
+ if email is None:
person_set = getUtility(IPersonSet)
person, email = person_set.createPersonAndEmail(
email_address, creation_rationale, comment=comment,
displayname=full_name)
+ elif trust_email:
+ person = email.person
+ else:
+ # The email address originated from a source that's
+ # not completely trustworth (eg. SCA), so we can't
+ # use it to link the OpenID identifier to an
+ # existing person.
+ raise EmailAddressAlreadyTaken()
# It's possible that the email address is owned by a
# team. Reject the login attempt, and wait for the user
@@ -3373,10 +3398,20 @@
person = IPerson(identifier.account, None)
assert person is not None, ('Received a personless account.')
- if person.account.status == AccountStatus.SUSPENDED:
+ status = person.account.status
+ if status == AccountStatus.ACTIVE:
+ # Account is active, so nothing to do.
+ pass
+ elif status == AccountStatus.SUSPENDED:
raise AccountSuspendedError(
"The account matching the identifier is suspended.")
-
+ elif not trust_email and status != AccountStatus.NOACCOUNT:
+ # If the email address is not completely trustworthy
+ # (ie. it comes from SCA) and the account has already
+ # been used, then we don't want to proceed as we might
+ # end up adding a malicious OpenID identifier to an
+ # existing account.
+ raise AccountSuspendedError()
elif person.account.status in [AccountStatus.DEACTIVATED,
AccountStatus.NOACCOUNT]:
removeSecurityProxy(person.account).reactivate(comment)
@@ -3386,11 +3421,24 @@
removeSecurityProxy(person).setPreferredEmail(email)
db_updated = True
else:
- # Account is active, so nothing to do.
- pass
+ raise AssertionError(
+ "Unhandled account status: %r" % person.account.status)
return person, db_updated
+ def getOrCreateSoftwareCenterCustomer(self, user, openid_identifier,
+ email_address, display_name):
+ """See `IPersonSet`."""
+ if user != getUtility(ILaunchpadCelebrities).software_center_agent:
+ raise Unauthorized()
+ person, _ = getUtility(
+ IPersonSet).getOrCreateByOpenIDIdentifier(
+ openid_identifier, email_address, display_name,
+ PersonCreationRationale.SOFTWARE_CENTER_PURCHASE,
+ "when purchasing an application via Software Center.",
+ trust_email=False)
+ return person
+
def newTeam(self, teamowner, name, displayname, teamdescription=None,
membership_policy=TeamMembershipPolicy.MODERATED,
defaultmembershipperiod=None, defaultrenewalperiod=None,
=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py 2015-01-07 00:35:53 +0000
+++ lib/lp/registry/tests/test_personset.py 2015-03-10 11:46:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for PersonSet."""
@@ -9,6 +9,7 @@
from testtools.matchers import LessThan
import transaction
from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
from lp.code.tests.helpers import remove_all_sample_data_branches
@@ -41,13 +42,17 @@
from lp.services.identity.interfaces.emailaddress import (
EmailAddressAlreadyTaken,
EmailAddressStatus,
+ IEmailAddressSet,
InvalidEmailAddress,
)
from lp.services.identity.model.account import Account
from lp.services.identity.model.emailaddress import EmailAddress
from lp.services.openid.model.openididentifier import OpenIdIdentifier
+from lp.services.webapp.interfaces import ILaunchBag
from lp.testing import (
+ admin_logged_in,
ANONYMOUS,
+ anonymous_logged_in,
login,
logout,
person_logged_in,
@@ -590,3 +595,135 @@
u'other-openid-identifier' in [
identifier.identifier for identifier in removeSecurityProxy(
person.account).openid_identifiers])
+
+
+class TestPersonSetGetOrCreateSoftwareCenterCustomer(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestPersonSetGetOrCreateSoftwareCenterCustomer, self).setUp()
+ self.sca = getUtility(IPersonSet).getByName('software-center-agent')
+
+ def makeOpenIdIdentifier(self, account, identifier):
+ openid_identifier = OpenIdIdentifier()
+ openid_identifier.identifier = identifier
+ openid_identifier.account = account
+ return IStore(OpenIdIdentifier).add(openid_identifier)
+
+ def test_restricted_to_sca(self):
+ # Only the software-center-agent celebrity can invoke this
+ # privileged method.
+ def do_it():
+ return getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer(
+ getUtility(ILaunchBag).user, u'somebody',
+ 'somebody@xxxxxxxxxxx', 'Example')
+ random = self.factory.makePerson()
+ admin = self.factory.makePerson(
+ member_of=[getUtility(IPersonSet).getByName('admins')])
+
+ # Anonymous, random or admin users can't invoke the method.
+ with anonymous_logged_in():
+ self.assertRaises(Unauthorized, do_it)
+ with person_logged_in(random):
+ self.assertRaises(Unauthorized, do_it)
+ with person_logged_in(admin):
+ self.assertRaises(Unauthorized, do_it)
+
+ with person_logged_in(self.sca):
+ person = do_it()
+ self.assertIsInstance(person, Person)
+
+ def test_finds_by_openid(self):
+ # A Person with the requested OpenID identifier is returned.
+ somebody = self.factory.makePerson()
+ self.makeOpenIdIdentifier(somebody.account, u'somebody')
+ with person_logged_in(self.sca):
+ got = getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer(
+ self.sca, u'somebody', 'somebody@xxxxxxxxxxx', 'Example')
+ self.assertEqual(somebody, got)
+
+ # The email address doesn't get linked, as that could change how
+ # future logins work.
+ self.assertIs(
+ None,
+ getUtility(IEmailAddressSet).getByEmail('somebody@xxxxxxxxxxx'))
+
+ def test_creates_new(self):
+ # If an unknown OpenID identifier and email address are
+ # provided, a new account is created and returned.
+ with person_logged_in(self.sca):
+ made = getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer(
+ self.sca, u'somebody', 'somebody@xxxxxxxxxxx', 'Example')
+ with admin_logged_in():
+ self.assertEqual('Example', made.displayname)
+ self.assertEqual('somebody@xxxxxxxxxxx', made.preferredemail.email)
+
+ # The email address is linked, since it can't compromise an
+ # account that is being created just for it.
+ email = getUtility(IEmailAddressSet).getByEmail('somebody@xxxxxxxxxxx')
+ self.assertEqual(made, email.person)
+
+ def test_activates_unactivated(self):
+ # An unactivated account should be treated just like a new
+ # account -- it gets activated with the given email address.
+ somebody = self.factory.makePerson(
+ email='existing@xxxxxxxxxxx',
+ account_status=AccountStatus.NOACCOUNT)
+ self.makeOpenIdIdentifier(somebody.account, u'somebody')
+ self.assertEqual(AccountStatus.NOACCOUNT, somebody.account.status)
+ with person_logged_in(self.sca):
+ got = getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer(
+ self.sca, u'somebody', 'somebody@xxxxxxxxxxx', 'Example')
+ self.assertEqual(somebody, got)
+ with admin_logged_in():
+ self.assertEqual(AccountStatus.ACTIVE, somebody.account.status)
+ self.assertEqual(
+ 'somebody@xxxxxxxxxxx', somebody.preferredemail.email)
+
+ def test_fails_if_email_is_already_registered(self):
+ # Only the OpenID identifier is used to look up an account. If
+ # the OpenID identifier isn't already registered by the email
+ # address is, the request is rejected to avoid potentially
+ # adding an unwanted OpenID identifier to the address' account.
+ #
+ # The user must log into Launchpad directly first to register
+ # their OpenID identifier.
+ other = self.factory.makePerson(email='other@xxxxxxxxxxx')
+ with person_logged_in(self.sca):
+ self.assertRaises(
+ EmailAddressAlreadyTaken,
+ getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer,
+ self.sca, u'somebody', 'other@xxxxxxxxxxx', 'Example')
+
+ # The email address stays with the old owner.
+ email = getUtility(IEmailAddressSet).getByEmail('other@xxxxxxxxxxx')
+ self.assertEqual(other, email.person)
+
+ def test_fails_if_account_is_suspended(self):
+ # Suspended accounts cannot be returned.
+ somebody = self.factory.makePerson()
+ self.makeOpenIdIdentifier(somebody.account, u'somebody')
+ with admin_logged_in():
+ somebody.setAccountStatus(
+ AccountStatus.SUSPENDED, None, "Go away!")
+ with person_logged_in(self.sca):
+ self.assertRaises(
+ AccountSuspendedError,
+ getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer,
+ self.sca, u'somebody', 'somebody@xxxxxxxxxxx', 'Example')
+
+ def test_fails_if_account_is_deactivated(self):
+ # We don't want to reactivate explicitly deactivated accounts,
+ # nor do we want to potentially compromise them with a bad email
+ # address.
+ somebody = self.factory.makePerson()
+ self.makeOpenIdIdentifier(somebody.account, u'somebody')
+ with admin_logged_in():
+ somebody.setAccountStatus(
+ AccountStatus.DEACTIVATED, None, "Goodbye cruel world.")
+ with person_logged_in(self.sca):
+ self.assertRaises(
+ AccountSuspendedError,
+ getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer,
+ self.sca, u'somebody', 'somebody@xxxxxxxxxxx', 'Example')
=== modified file 'lib/lp/services/identity/interfaces/account.py'
--- lib/lp/services/identity/interfaces/account.py 2015-01-07 00:35:53 +0000
+++ lib/lp/services/identity/interfaces/account.py 2015-03-10 11:46:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Account interfaces."""
@@ -18,11 +18,13 @@
'INACTIVE_ACCOUNT_STATUSES',
]
+import httplib
from lazr.enum import (
DBEnumeratedType,
DBItem,
)
+from lazr.restful.declarations import error_status
from zope.interface import (
Attribute,
Interface,
@@ -40,6 +42,7 @@
from lp.services.fields import StrippedTextLine
+@error_status(httplib.BAD_REQUEST)
class AccountSuspendedError(Exception):
"""The account being accessed has been suspended."""
=== modified file 'lib/lp/services/identity/interfaces/emailaddress.py'
--- lib/lp/services/identity/interfaces/emailaddress.py 2013-01-07 02:40:55 +0000
+++ lib/lp/services/identity/interfaces/emailaddress.py 2015-03-10 11:46:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""EmailAddress interfaces."""
@@ -12,11 +12,14 @@
'InvalidEmailAddress',
'VALID_EMAIL_STATUSES']
+import httplib
+
from lazr.enum import (
DBEnumeratedType,
DBItem,
)
from lazr.restful.declarations import (
+ error_status,
export_as_webservice_entry,
exported,
)
@@ -36,6 +39,7 @@
"""The email address is not valid."""
+@error_status(httplib.CONFLICT)
class EmailAddressAlreadyTaken(Exception):
"""The email address is already registered in Launchpad."""
