launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18088
Re: [Merge] lp:~wgrant/launchpad/sca-api into lp:launchpad
Review: Approve
Diff comments:
> === 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.
The parameter name is "display_name" rather than "full_name" earlier, although the old XML-RPC interface spells it "full_name".
> + """
> +
> @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()
A slightly opaque choice. Wouldn't a new exception with @error_status(httplib.CONFLICT) be clearer, or maybe just NameAlreadyTaken?
> 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,
self.assertIsNone() to save a line break.
> + 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
Took me a couple of readings, but presumably s/by/but/.
> + # 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."""
>
>
--
https://code.launchpad.net/~wgrant/launchpad/sca-api/+merge/252420
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
