Launchpad itself

Merge lp:~jml/launchpad/create-private-ppa-814567 into lp:launchpad

  1. create-private-ppa-814567
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 13545
Proposed branch: lp:~jml/launchpad/create-private-ppa-814567
Merge into: lp:launchpad
Diff against target: 173 lines (+80/-8)
6 files modified
lib/lp/registry/interfaces/person.py (+6/-2)
lib/lp/registry/model/person.py (+4/-4)
lib/lp/soyuz/configure.zcml (+6/-0)
lib/lp/soyuz/model/archive.py (+11/-1)
lib/lp/soyuz/stories/webservice/xx-person-createppa.txt (+28/-0)
lib/lp/soyuz/tests/test_archive.py (+25/-1)
To merge this branch: bzr merge lp:~jml/launchpad/create-private-ppa-814567
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+69539@code.launchpad.net

Commit message

[r=julian-edwards][bug=814567] Add an API to create private PPAs, rather than changing privacy status post creation.

Description of the change

This branch makes it possible to create private PPAs over the webservice.

I couldn't find a good way to do the check on creation. Currently the ability to change privacy is controlled by a Zope attribute security check. I don't know how that works wrt object creation, and I *certainly* don't know how that works given that Archive.private is a property with side effects.

Thus, I duplicated the check that takes place in the security permission.

I also couldn't find a good set of tests that I could repurpose to make sure that setting .private and specifying private=True behaved the same.

Note that setting .private over the API raises 401 Unauthorized, but specifying private to createPPA raises 400 Bad Request.

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

<bigjools> jml: in your branch did you consider refactoring the check for commercial admin, somehow?
<bigjools> I'm not sure how I'd do it tbh :(
<jml> bigjools: the only thing that came to mind was writing a version of check_permission that took user as a parameter, rather than getting it from the session.
<bigjools> jml: I'm just wary of duplicating the check, that's all. Been there, picked up the pieces :(
<jml> bigjools: yeah. I can put a note in the new code & the zcml if you'd like, pointing one to the other.
<bigjools> jml: fair enough, thanks

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2011-05-31 03:49:23 +0000
+++ lib/lp/registry/interfaces/person.py 2011-07-28 09:30:36 +0000
@@ -1400,16 +1400,20 @@
1400 @operation_parameters(1400 @operation_parameters(
1401 name=TextLine(required=True, constraint=name_validator),1401 name=TextLine(required=True, constraint=name_validator),
1402 displayname=TextLine(required=False),1402 displayname=TextLine(required=False),
1403 description=TextLine(required=False))1403 description=TextLine(required=False),
1404 private=Bool(required=False),
1405 )
1404 @export_factory_operation(Interface, []) # Really IArchive.1406 @export_factory_operation(Interface, []) # Really IArchive.
1405 @operation_for_version("beta")1407 @operation_for_version("beta")
1406 def createPPA(name=None, displayname=None, description=None):1408 def createPPA(name=None, displayname=None, description=None, private=False):
1407 """Create a PPA.1409 """Create a PPA.
14081410
1409 :param name: A string with the name of the new PPA to create. If1411 :param name: A string with the name of the new PPA to create. If
1410 not specified, defaults to 'ppa'.1412 not specified, defaults to 'ppa'.
1411 :param displayname: The displayname for the new PPA.1413 :param displayname: The displayname for the new PPA.
1412 :param description: The description for the new PPA.1414 :param description: The description for the new PPA.
1415 :param private: Whether or not to create a private PPA. Defaults to
1416 False, which means the PPA will be public.
1413 :raises: `PPACreationError` if an error is encountered1417 :raises: `PPACreationError` if an error is encountered
14141418
1415 :return: a PPA `IArchive` record.1419 :return: a PPA `IArchive` record.
14161420
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-06-16 20:12:00 +0000
+++ lib/lp/registry/model/person.py 2011-07-28 09:30:36 +0000
@@ -2739,17 +2739,17 @@
2739 """See `IPerson`."""2739 """See `IPerson`."""
2740 return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)2740 return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)
27412741
2742 def createPPA(self, name=None, displayname=None, description=None):2742 def createPPA(self, name=None, displayname=None, description=None,
2743 private=False):
2743 """See `IPerson`."""2744 """See `IPerson`."""
2744 errors = Archive.validatePPA(self, name)2745 errors = Archive.validatePPA(self, name, private)
2745 if errors:2746 if errors:
2746 raise PPACreationError(errors)2747 raise PPACreationError(errors)
2747 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu2748 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
2748 ppa = getUtility(IArchiveSet).new(2749 return getUtility(IArchiveSet).new(
2749 owner=self, purpose=ArchivePurpose.PPA,2750 owner=self, purpose=ArchivePurpose.PPA,
2750 distribution=ubuntu, name=name, displayname=displayname,2751 distribution=ubuntu, name=name, displayname=displayname,
2751 description=description)2752 description=description)
2752 return ppa
27532753
2754 def isBugContributor(self, user=None):2754 def isBugContributor(self, user=None):
2755 """See `IPerson`."""2755 """See `IPerson`."""
27562756
=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml 2011-07-14 15:23:28 +0000
+++ lib/lp/soyuz/configure.zcml 2011-07-28 09:30:36 +0000
@@ -405,6 +405,12 @@
405 permission="launchpad.Edit"405 permission="launchpad.Edit"
406 interface="lp.soyuz.interfaces.archive.IArchiveEdit"406 interface="lp.soyuz.interfaces.archive.IArchiveEdit"
407 set_attributes="description displayname publish status"/>407 set_attributes="description displayname publish status"/>
408 <!--
409 NOTE: The 'private' permission controls who can turn a public
410 archive into a private one, and vice versa. The logic that says this
411 requires launchpad.Commercial permissions is duplicated in
412 IArchive.validatePPA.
413 -->
408 <require414 <require
409 permission="launchpad.Commercial"415 permission="launchpad.Commercial"
410 interface="lp.soyuz.interfaces.archive.IArchiveCommercial"416 interface="lp.soyuz.interfaces.archive.IArchiveCommercial"
411417
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2011-07-22 13:28:35 +0000
+++ lib/lp/soyuz/model/archive.py 2011-07-28 09:30:36 +0000
@@ -1947,8 +1947,18 @@
1947 self.enabled_restricted_families = restricted1947 self.enabled_restricted_families = restricted
19481948
1949 @classmethod1949 @classmethod
1950 def validatePPA(self, person, proposed_name):1950 def validatePPA(self, person, proposed_name, private=False):
1951 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu1951 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
1952 if private:
1953 # NOTE: This duplicates the policy in lp/soyuz/configure.zcml
1954 # which says that one needs 'launchpad.Commercial' permission to
1955 # set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
1956 # which determines who is granted launchpad.Commercial
1957 # permissions.
1958 commercial = getUtility(ILaunchpadCelebrities).commercial_admin
1959 admin = getUtility(ILaunchpadCelebrities).admin
1960 if not person.inTeam(commercial) and not person.inTeam(admin):
1961 return '%s is not allowed to make private PPAs' % (person.name,)
1952 if person.isTeam() and (1962 if person.isTeam() and (
1953 person.subscriptionpolicy in OPEN_TEAM_POLICY):1963 person.subscriptionpolicy in OPEN_TEAM_POLICY):
1954 return "Open teams cannot have PPAs."1964 return "Open teams cannot have PPAs."
19551965
=== modified file 'lib/lp/soyuz/stories/webservice/xx-person-createppa.txt'
--- lib/lp/soyuz/stories/webservice/xx-person-createppa.txt 2011-02-13 22:54:48 +0000
+++ lib/lp/soyuz/stories/webservice/xx-person-createppa.txt 2011-07-28 09:30:36 +0000
@@ -30,3 +30,31 @@
30 Status: 400 Bad Request30 Status: 400 Bad Request
31 ...31 ...
32 A PPA cannot have the same name as its distribution.32 A PPA cannot have the same name as its distribution.
33
34== Creating private PPAs ==
35
36Our PPA owner now has a single PPA.
37
38 >>> print_self_link_of_entries(webservice.get(
39 ... ppa_owner['ppas_collection_link']).jsonBody())
40 http://api.launchpad.dev/beta/~.../+archive/ppa
41
42They aren't a commercial admin, so they cannot create private PPAs.
43
44 >>> print ppa_owner_webservice.named_post(
45 ... ppa_owner['self_link'], 'createPPA', {}, name='whatever',
46 ... displayname='My secret new PPA', description='Secretness!',
47 ... private=True,
48 ... )
49 HTTP/1.1 400 Bad Request
50 Status: 400
51 ...
52 ... is not allowed to make private PPAs
53
54After attempting and failing to create a private PPA, they still have the same
55single PPA they had at the beginning:
56
57 >>> print_self_link_of_entries(webservice.get(
58 ... ppa_owner['ppas_collection_link']).jsonBody())
59 http://api.launchpad.dev/beta/~.../+archive/ppa
60
3361
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py 2011-07-22 13:31:35 +0000
+++ lib/lp/soyuz/tests/test_archive.py 2011-07-28 09:30:36 +0000
@@ -1624,7 +1624,7 @@
1624 set(archive.getComponentsForUploader(person)))1624 set(archive.getComponentsForUploader(person)))
16251625
16261626
1627class TestvalidatePPA(TestCaseWithFactory):1627class TestValidatePPA(TestCaseWithFactory):
16281628
1629 layer = DatabaseFunctionalLayer1629 layer = DatabaseFunctionalLayer
16301630
@@ -1639,6 +1639,30 @@
1639 'A PPA cannot have the same name as its distribution.',1639 'A PPA cannot have the same name as its distribution.',
1640 Archive.validatePPA(ppa_owner, 'ubuntu'))1640 Archive.validatePPA(ppa_owner, 'ubuntu'))
16411641
1642 def test_private_ppa_non_commercial_admin(self):
1643 ppa_owner = self.factory.makePerson()
1644 self.assertEqual(
1645 '%s is not allowed to make private PPAs' % (ppa_owner.name,),
1646 Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
1647 private=True))
1648
1649 def test_private_ppa_commercial_admin(self):
1650 ppa_owner = self.factory.makePerson()
1651 with celebrity_logged_in('admin'):
1652 comm = getUtility(ILaunchpadCelebrities).commercial_admin
1653 comm.addMember(ppa_owner, comm.teamowner)
1654 self.assertIs(
1655 None,
1656 Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
1657 private=True))
1658
1659 def test_private_ppa_admin(self):
1660 ppa_owner = self.factory.makeAdministrator()
1661 self.assertIs(
1662 None,
1663 Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
1664 private=True))
1665
1642 def test_two_ppas(self):1666 def test_two_ppas(self):
1643 ppa = self.factory.makeArchive(name='ppa')1667 ppa = self.factory.makeArchive(name='ppa')
1644 self.assertEqual("You already have a PPA named 'ppa'.",1668 self.assertEqual("You already have a PPA named 'ppa'.",