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
1=== modified file 'lib/lp/registry/interfaces/person.py'
2--- lib/lp/registry/interfaces/person.py 2011-05-31 03:49:23 +0000
3+++ lib/lp/registry/interfaces/person.py 2011-07-28 09:30:36 +0000
4@@ -1400,16 +1400,20 @@
5 @operation_parameters(
6 name=TextLine(required=True, constraint=name_validator),
7 displayname=TextLine(required=False),
8- description=TextLine(required=False))
9+ description=TextLine(required=False),
10+ private=Bool(required=False),
11+ )
12 @export_factory_operation(Interface, []) # Really IArchive.
13 @operation_for_version("beta")
14- def createPPA(name=None, displayname=None, description=None):
15+ def createPPA(name=None, displayname=None, description=None, private=False):
16 """Create a PPA.
17
18 :param name: A string with the name of the new PPA to create. If
19 not specified, defaults to 'ppa'.
20 :param displayname: The displayname for the new PPA.
21 :param description: The description for the new PPA.
22+ :param private: Whether or not to create a private PPA. Defaults to
23+ False, which means the PPA will be public.
24 :raises: `PPACreationError` if an error is encountered
25
26 :return: a PPA `IArchive` record.
27
28=== modified file 'lib/lp/registry/model/person.py'
29--- lib/lp/registry/model/person.py 2011-06-16 20:12:00 +0000
30+++ lib/lp/registry/model/person.py 2011-07-28 09:30:36 +0000
31@@ -2739,17 +2739,17 @@
32 """See `IPerson`."""
33 return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)
34
35- def createPPA(self, name=None, displayname=None, description=None):
36+ def createPPA(self, name=None, displayname=None, description=None,
37+ private=False):
38 """See `IPerson`."""
39- errors = Archive.validatePPA(self, name)
40+ errors = Archive.validatePPA(self, name, private)
41 if errors:
42 raise PPACreationError(errors)
43 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
44- ppa = getUtility(IArchiveSet).new(
45+ return getUtility(IArchiveSet).new(
46 owner=self, purpose=ArchivePurpose.PPA,
47 distribution=ubuntu, name=name, displayname=displayname,
48 description=description)
49- return ppa
50
51 def isBugContributor(self, user=None):
52 """See `IPerson`."""
53
54=== modified file 'lib/lp/soyuz/configure.zcml'
55--- lib/lp/soyuz/configure.zcml 2011-07-14 15:23:28 +0000
56+++ lib/lp/soyuz/configure.zcml 2011-07-28 09:30:36 +0000
57@@ -405,6 +405,12 @@
58 permission="launchpad.Edit"
59 interface="lp.soyuz.interfaces.archive.IArchiveEdit"
60 set_attributes="description displayname publish status"/>
61+ <!--
62+ NOTE: The 'private' permission controls who can turn a public
63+ archive into a private one, and vice versa. The logic that says this
64+ requires launchpad.Commercial permissions is duplicated in
65+ IArchive.validatePPA.
66+ -->
67 <require
68 permission="launchpad.Commercial"
69 interface="lp.soyuz.interfaces.archive.IArchiveCommercial"
70
71=== modified file 'lib/lp/soyuz/model/archive.py'
72--- lib/lp/soyuz/model/archive.py 2011-07-22 13:28:35 +0000
73+++ lib/lp/soyuz/model/archive.py 2011-07-28 09:30:36 +0000
74@@ -1947,8 +1947,18 @@
75 self.enabled_restricted_families = restricted
76
77 @classmethod
78- def validatePPA(self, person, proposed_name):
79+ def validatePPA(self, person, proposed_name, private=False):
80 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
81+ if private:
82+ # NOTE: This duplicates the policy in lp/soyuz/configure.zcml
83+ # which says that one needs 'launchpad.Commercial' permission to
84+ # set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
85+ # which determines who is granted launchpad.Commercial
86+ # permissions.
87+ commercial = getUtility(ILaunchpadCelebrities).commercial_admin
88+ admin = getUtility(ILaunchpadCelebrities).admin
89+ if not person.inTeam(commercial) and not person.inTeam(admin):
90+ return '%s is not allowed to make private PPAs' % (person.name,)
91 if person.isTeam() and (
92 person.subscriptionpolicy in OPEN_TEAM_POLICY):
93 return "Open teams cannot have PPAs."
94
95=== modified file 'lib/lp/soyuz/stories/webservice/xx-person-createppa.txt'
96--- lib/lp/soyuz/stories/webservice/xx-person-createppa.txt 2011-02-13 22:54:48 +0000
97+++ lib/lp/soyuz/stories/webservice/xx-person-createppa.txt 2011-07-28 09:30:36 +0000
98@@ -30,3 +30,31 @@
99 Status: 400 Bad Request
100 ...
101 A PPA cannot have the same name as its distribution.
102+
103+== Creating private PPAs ==
104+
105+Our PPA owner now has a single PPA.
106+
107+ >>> print_self_link_of_entries(webservice.get(
108+ ... ppa_owner['ppas_collection_link']).jsonBody())
109+ http://api.launchpad.dev/beta/~.../+archive/ppa
110+
111+They aren't a commercial admin, so they cannot create private PPAs.
112+
113+ >>> print ppa_owner_webservice.named_post(
114+ ... ppa_owner['self_link'], 'createPPA', {}, name='whatever',
115+ ... displayname='My secret new PPA', description='Secretness!',
116+ ... private=True,
117+ ... )
118+ HTTP/1.1 400 Bad Request
119+ Status: 400
120+ ...
121+ ... is not allowed to make private PPAs
122+
123+After attempting and failing to create a private PPA, they still have the same
124+single PPA they had at the beginning:
125+
126+ >>> print_self_link_of_entries(webservice.get(
127+ ... ppa_owner['ppas_collection_link']).jsonBody())
128+ http://api.launchpad.dev/beta/~.../+archive/ppa
129+
130
131=== modified file 'lib/lp/soyuz/tests/test_archive.py'
132--- lib/lp/soyuz/tests/test_archive.py 2011-07-22 13:31:35 +0000
133+++ lib/lp/soyuz/tests/test_archive.py 2011-07-28 09:30:36 +0000
134@@ -1624,7 +1624,7 @@
135 set(archive.getComponentsForUploader(person)))
136
137
138-class TestvalidatePPA(TestCaseWithFactory):
139+class TestValidatePPA(TestCaseWithFactory):
140
141 layer = DatabaseFunctionalLayer
142
143@@ -1639,6 +1639,30 @@
144 'A PPA cannot have the same name as its distribution.',
145 Archive.validatePPA(ppa_owner, 'ubuntu'))
146
147+ def test_private_ppa_non_commercial_admin(self):
148+ ppa_owner = self.factory.makePerson()
149+ self.assertEqual(
150+ '%s is not allowed to make private PPAs' % (ppa_owner.name,),
151+ Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
152+ private=True))
153+
154+ def test_private_ppa_commercial_admin(self):
155+ ppa_owner = self.factory.makePerson()
156+ with celebrity_logged_in('admin'):
157+ comm = getUtility(ILaunchpadCelebrities).commercial_admin
158+ comm.addMember(ppa_owner, comm.teamowner)
159+ self.assertIs(
160+ None,
161+ Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
162+ private=True))
163+
164+ def test_private_ppa_admin(self):
165+ ppa_owner = self.factory.makeAdministrator()
166+ self.assertIs(
167+ None,
168+ Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
169+ private=True))
170+
171 def test_two_ppas(self):
172 ppa = self.factory.makeArchive(name='ppa')
173 self.assertEqual("You already have a PPA named 'ppa'.",