Merge lp:~jml/launchpad/validate-ppa-owner into lp:launchpad

[Jonathan Lange]

Restrict IPerson.createPPA to those with launchpad.Edit permissions.

I noticed that createPPA can be called by anyone on anyone, since it's only got ViewRestricted permissions. I don't think this is a major vulnerability, since it doesn't grant upload permissions to that created PPA. Thus, I'm submitting this in the clear. I hope that's OK.

This branch changes createPPA to be on IPersonEditRestricted, thus requiring launchpad.Edit permissions on the team (person) that the PPA is being created for. This matches the behaviour in the web UI, which only shows PPA creation screens for users with launchpad.Edit on a team (person).

Curtis was concerned that this might break behaviour for key users: CA, OEM, OpenStack. He suggested that we do an audit first. However, this was when we believed that only team members could create PPAs via API, rather than the more restrictive web UI, which only allows team admins. Now that our understanding has changed, I think this patch should be landed now.

It also changes the web UI code to also use createPPA. This leaves Launchpad with one consistent way to create PPAs, which certainly makes it easier for *me* to understand.

This add 24 lines to the code base, including the one added by the dependent branch. I'm fairly sure that the CA PPA arc is in credit. Will get exact numbers soon.

[Curtis Hovey (sinzui)]

Thank's jml. You discovered and fixed a disclosure feature mistake (and we might think of the LOC point to also be in the disclosure domain.). This is good to land.

review: Approve (code)

[Jonathan Lange (jml)]

Thanks. I'd like to keep track of it in our own arc, if you don't mind.

https://docs.google.com/a/canonical.com/spreadsheet/ccc?key=0AsE2-aW-RE8adG5vbW1nN3dJV0o0WUVKSElFRE9UUFE#gid=0 says this will put us on a net of zero.

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2012-05-14 03:12:44 +0000
+++ lib/lp/_schema_circular_imports.py	2012-06-11 09:21:23 +0000
@@ -118,6 +118,7 @@
     )
 from lp.registry.interfaces.person import (
     IPerson,
+    IPersonEditRestricted,
     IPersonLimitedView,
     IPersonViewRestricted,
     ITeam,
@@ -309,7 +310,7 @@
 patch_reference_property(IPersonViewRestricted, 'archive', IArchive)
 patch_collection_property(IPersonViewRestricted, 'ppas', IArchive)
 patch_entry_return_type(IPersonLimitedView, 'getPPAByName', IArchive)
-patch_entry_return_type(IPersonViewRestricted, 'createPPA', IArchive)
+patch_entry_return_type(IPersonEditRestricted, 'createPPA', IArchive)
 
 IHasBuildRecords['getBuildRecords'].queryTaggedValue(
     LAZR_WEBSERVICE_EXPORTED)[
 
=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2012-05-31 03:54:13 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2012-06-11 09:21:23 +0000
@@ -39,6 +39,7 @@
 from lp.registry.interfaces.person import TeamSubscriptionPolicy
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
+from lp.registry.interfaces.teammembership import TeamMembershipStatus
 from lp.services.database.constants import UTC_NOW
 from lp.services.propertycache import clear_property_cache
 from lp.services.webapp import canonical_url
@@ -670,7 +671,8 @@
         # name specifed an error is shown.
         self.user = self.factory.makePerson(name='eric')
         # Make a PPA called 'ppa' using the default.
-        self.user.createPPA(name='foo')
+        with person_logged_in(self.user):
+            self.user.createPPA(name='foo')
         branch = self.factory.makeAnyBranch()
 
         # A new recipe can be created from the branch page.
@@ -710,6 +712,9 @@
         team = self.factory.makeTeam(
             name='vikings', members=[self.user],
             subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        with person_logged_in(team.teamowner):
+            team.setMembershipData(
+                self.user, TeamMembershipStatus.ADMIN, team.teamowner)
         branch = self.factory.makeAnyBranch(owner=team)
 
         # A new recipe can be created from the branch page.
 
=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2012-05-02 05:25:11 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2012-06-11 09:21:23 +0000
@@ -344,7 +344,8 @@
         # the team has any ppas.
 
         def setup_team(team):
-            team.createPPA()
+            with person_logged_in(team.teamowner):
+                team.createPPA()
 
         self._test_edit_team_view_expected_subscription_vocab(
             setup_team, CLOSED_TEAM_POLICY)
 
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-05-28 09:17:25 +0000
+++ lib/lp/registry/interfaces/person.py	2012-06-11 09:21:23 +0000
@@ -17,6 +17,7 @@
     'IObjectReassignment',
     'IPerson',
     'IPersonClaim',
+    'IPersonEditRestricted',
     'IPersonPublic',
     'IPersonSet',
     'IPersonSettings',
@@ -1482,33 +1483,6 @@
         :return: True if the user was subscribed, false if they weren't.
         """
 
-    @operation_parameters(
-        name=TextLine(required=True, constraint=name_validator),
-        displayname=TextLine(required=False),
-        description=TextLine(required=False),
-        private=Bool(required=False),
-        suppress_subscription_notifications=Bool(required=False),
-        )
-    @export_factory_operation(Interface, [])  # Really IArchive.
-    @operation_for_version("beta")
-    def createPPA(name=None, displayname=None, description=None,
-                  private=False, suppress_subscription_notifications=False):
-        """Create a PPA.
-
-        :param name: A string with the name of the new PPA to create. If
-            not specified, defaults to 'ppa'.
-        :param displayname: The displayname for the new PPA.
-        :param description: The description for the new PPA.
-        :param private: Whether or not to create a private PPA. Defaults to
-            False, which means the PPA will be public.
-        :param suppress_subscription_notifications: Whether or not to suppress
-            emails to new subscribers about their subscriptions.  Only
-            meaningful for private PPAs.
-        :raises: `PPACreationError` if an error is encountered
-
-        :return: a PPA `IArchive` record.
-        """
-
     def checkRename():
         """Check if a person or team can be renamed.
 
@@ -1826,6 +1800,33 @@
             about the change.
         """
 
+    @operation_parameters(
+        name=TextLine(required=True, constraint=name_validator),
+        displayname=TextLine(required=False),
+        description=TextLine(required=False),
+        private=Bool(required=False),
+        suppress_subscription_notifications=Bool(required=False),
+        )
+    @export_factory_operation(Interface, [])  # Really IArchive.
+    @operation_for_version("beta")
+    def createPPA(name=None, displayname=None, description=None,
+                  private=False, suppress_subscription_notifications=False):
+        """Create a PPA.
+
+        :param name: A string with the name of the new PPA to create. If
+            not specified, defaults to 'ppa'.
+        :param displayname: The displayname for the new PPA.
+        :param description: The description for the new PPA.
+        :param private: Whether or not to create a private PPA. Defaults to
+            False, which means the PPA will be public.
+        :param suppress_subscription_notifications: Whether or not to suppress
+            emails to new subscribers about their subscriptions.  Only
+            meaningful for private PPAs.
+        :raises: `PPACreationError` if an error is encountered
+
+        :return: a PPA `IArchive` record.
+        """
+
 
 class IPersonSpecialRestricted(Interface):
     """IPerson methods that require launchpad.Special permission to use."""
 
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-06-09 16:26:32 +0000
+++ lib/lp/registry/model/person.py	2012-06-11 09:21:23 +0000
@@ -2978,11 +2978,6 @@
     def createPPA(self, name=None, displayname=None, description=None,
                   private=False, suppress_subscription_notifications=False):
         """See `IPerson`."""
-        # XXX: We pass through the Person on whom the PPA is being created,
-        # but validate_ppa assumes that that Person is also the one creating
-        # the PPA.  This is not true in general, and particularly not for
-        # teams.  Instead, both the acting user and the target of the PPA
-        # creation ought to be passed through.
         errors = validate_ppa(self, name, private)
         if errors:
             raise PPACreationError(errors)
 
=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py	2012-05-02 05:25:11 +0000
+++ lib/lp/registry/tests/test_team.py	2012-06-11 09:21:23 +0000
@@ -500,7 +500,8 @@
         # made to set an illegal open subscription policy on a team.
         team = self.factory.makeTeam(
             subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
-        team.createPPA()
+        with person_logged_in(team.teamowner):
+            team.createPPA()
         for policy in OPEN_TEAM_POLICY:
             self.assertRaises(
                 TeamSubscriptionPolicyError,
 
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2012-06-09 16:26:32 +0000
+++ lib/lp/soyuz/browser/archive.py	2012-06-11 09:21:23 +0000
@@ -1994,6 +1994,7 @@
         # PPA name.
         name = data.get('name', None)
 
+        # XXX: jml: I think this ought to call createPPA.
         # XXX cprov 2009-03-27 bug=188564: We currently only create PPAs
         # for Ubuntu distribution. PPA creation should be revisited when we
         # start supporting other distribution (debian, mainly).
 
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-06-09 16:26:32 +0000
+++ lib/lp/soyuz/model/archive.py	2012-06-11 09:21:23 +0000
@@ -105,6 +105,7 @@
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import (
     DEFAULT_FLAVOR,
+    ILaunchBag,
     IStoreSelector,
     MAIN_STORE,
     )
@@ -2054,7 +2055,14 @@
         job.destroySelf()
 
 
-def validate_ppa(person, proposed_name, private=False):
+def validate_ppa(owner, proposed_name, private=False):
+    """Can 'person' create a PPA called 'proposed_name'?
+
+    :param owner: The proposed owner of the PPA.
+    :param proposed_name: The proposed name.
+    :param private: Whether or not to make it private.
+    """
+    creator = getUtility(ILaunchBag).user
     ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
     if private:
         # NOTE: This duplicates the policy in lp/soyuz/configure.zcml
@@ -2062,11 +2070,11 @@
         # set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
         # which determines who is granted launchpad.Commercial
         # permissions.
-        role = IPersonRoles(person)
+        role = IPersonRoles(creator)
         if not (role.in_admin or role.in_commercial_admin):
-            return '%s is not allowed to make private PPAs' % person.name
-    if person.is_team and (
-        person.subscriptionpolicy in OPEN_TEAM_POLICY):
+            return '%s is not allowed to make private PPAs' % creator.name
+    if owner.is_team and (
+        owner.subscriptionpolicy in OPEN_TEAM_POLICY):
         return "Open teams cannot have PPAs."
     if proposed_name is not None and proposed_name == ubuntu.name:
         return (
@@ -2074,14 +2082,14 @@
     if proposed_name is None:
         proposed_name = 'ppa'
     try:
-        person.getPPAByName(proposed_name)
+        owner.getPPAByName(proposed_name)
     except NoSuchPPA:
         return None
     else:
         text = "You already have a PPA named '%s'." % proposed_name
-        if person.is_team:
+        if owner.is_team:
             text = "%s already has a PPA named '%s'." % (
-                person.displayname, proposed_name)
+                owner.displayname, proposed_name)
         return text
 
 
 
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-06-09 16:26:32 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-06-11 09:21:23 +0000
@@ -1666,30 +1666,35 @@
 
     def test_private_ppa_non_commercial_admin(self):
         ppa_owner = self.factory.makePerson()
+        with person_logged_in(ppa_owner):
+            errors = validate_ppa(
+                ppa_owner, self.factory.getUniqueString(), private=True)
         self.assertEqual(
             '%s is not allowed to make private PPAs' % (ppa_owner.name,),
-            validate_ppa(
-                ppa_owner, self.factory.getUniqueString(), private=True))
+            errors)
 
     def test_private_ppa_commercial_admin(self):
         ppa_owner = self.factory.makePerson()
         with celebrity_logged_in('admin'):
             comm = getUtility(ILaunchpadCelebrities).commercial_admin
             comm.addMember(ppa_owner, comm.teamowner)
-        self.assertIsNone(
-            validate_ppa(
-                ppa_owner, self.factory.getUniqueString(), private=True))
+        with person_logged_in(ppa_owner):
+            self.assertIsNone(
+                validate_ppa(
+                    ppa_owner, self.factory.getUniqueString(), private=True))
 
     def test_private_ppa_admin(self):
         ppa_owner = self.factory.makeAdministrator()
-        self.assertIsNone(
-            validate_ppa(
-                ppa_owner, self.factory.getUniqueString(), private=True))
+        with person_logged_in(ppa_owner):
+            self.assertIsNone(
+                validate_ppa(
+                    ppa_owner, self.factory.getUniqueString(), private=True))
 
     def test_two_ppas(self):
         ppa = self.factory.makeArchive(name='ppa')
         self.assertEqual(
-            "You already have a PPA named 'ppa'.", validate_ppa(ppa.owner, 'ppa'))
+            "You already have a PPA named 'ppa'.",
+            validate_ppa(ppa.owner, 'ppa'))
 
     def test_two_ppas_with_team(self):
         team = self.factory.makeTeam(
 
=== modified file 'lib/lp/soyuz/tests/test_person_createppa.py'
--- lib/lp/soyuz/tests/test_person_createppa.py	2012-05-25 22:14:21 +0000
+++ lib/lp/soyuz/tests/test_person_createppa.py	2012-06-11 09:21:23 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from zope.security.interfaces import Unauthorized
 from lp.registry.errors import PPACreationError
 from lp.testing import (
     celebrity_logged_in,
@@ -21,13 +22,14 @@
 
     def test_default_name(self):
         person = self.factory.makePerson()
-        ppa = person.createPPA()
+        with person_logged_in(person):
+            ppa = person.createPPA()
         self.assertEqual(ppa.name, 'ppa')
 
     def test_private(self):
         with celebrity_logged_in('commercial_admin') as person:
             ppa = person.createPPA(private=True)
-            self.assertEqual(True, ppa.private)
+        self.assertEqual(True, ppa.private)
 
     def test_private_without_permission(self):
         person = self.factory.makePerson()
@@ -35,7 +37,15 @@
             self.assertRaises(
                 PPACreationError, person.createPPA, private=True)
 
+    def test_different_person(self):
+        # You cannot make a PPA on another person.
+        owner = self.factory.makePerson()
+        creator = self.factory.makePerson()
+        with person_logged_in(creator):
+            self.assertRaises(Unauthorized, getattr, owner, 'createPPA')
+
     def test_suppress_subscription_notifications(self):
         person = self.factory.makePerson()
-        ppa = person.createPPA(suppress_subscription_notifications=True)
+        with person_logged_in(person):
+            ppa = person.createPPA(suppress_subscription_notifications=True)
         self.assertEqual(True, ppa.suppress_subscription_notifications)