Merge lp:~wgrant/launchpad/sca-api into lp:launchpad

[William Grant]

Implement PersonSet.getOrCreateSoftwareCenterCustomer and export it on the webservice. The xmlrpc-private method is deprecated.

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

[Colin Watson (cjwatson)]

review: Approve

[Michael Nelson (michael.nelson)]

Thanks William. Looks great - I'll see if we can get the sca work scheduled so we can close the private xmlrpc access.

review: Approve

=== 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 22:08:18 +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='[e-mail]', 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('[e-mail]')
+            self.assertEqual('Somebody', person.displayname)
+            self.assertEqual(
+                ['somebody'],
+                [oid.identifier for oid in person.account.openid_identifiers])
+            self.assertEqual(
+                '[e-mail]', 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='[e-mail]')
+            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='[e-mail]',
+                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 22:08:18 +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 22:08:18 +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,21 @@
             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 NameAlreadyTaken(
+                    "The account matching the identifier is inactive.")
             elif person.account.status in [AccountStatus.DEACTIVATED,
                                            AccountStatus.NOACCOUNT]:
                 removeSecurityProxy(person.account).reactivate(comment)
@@ -3386,11 +3422,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 22:08:18 +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',
+                '[e-mail]', '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', '[e-mail]', '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('[e-mail]'))
+
+    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', '[e-mail]', 'Example')
+        with admin_logged_in():
+            self.assertEqual('Example', made.displayname)
+            self.assertEqual('[e-mail]', 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('[e-mail]')
+        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='[e-mail]',
+            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', '[e-mail]', 'Example')
+        self.assertEqual(somebody, got)
+        with admin_logged_in():
+            self.assertEqual(AccountStatus.ACTIVE, somebody.account.status)
+            self.assertEqual(
+                '[e-mail]', 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='[e-mail]')
+        with person_logged_in(self.sca):
+            self.assertRaises(
+                EmailAddressAlreadyTaken,
+                getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer,
+                self.sca, u'somebody', '[e-mail]', 'Example')
+
+        # The email address stays with the old owner.
+        email = getUtility(IEmailAddressSet).getByEmail('[e-mail]')
+        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', '[e-mail]', '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(
+                NameAlreadyTaken,
+                getUtility(IPersonSet).getOrCreateSoftwareCenterCustomer,
+                self.sca, u'somebody', '[e-mail]', '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 22:08:18 +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 22:08:18 +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."""