launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04689
lp:~julian-edwards/launchpad/perms-for-changing-uploaders--bug-828894 into lp:launchpad
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/perms-for-changing-uploaders--bug-828894 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #828894 in Launchpad itself: "Changing upload/queue admin permissions is broken"
https://bugs.launchpad.net/launchpad/+bug/828894
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/perms-for-changing-uploaders--bug-828894/+merge/72220
= Summary =
Fix the permissions required to change uploader-related permissions
on a distribution.
== Proposed fix ==
There are three types of upload permissions for a distribution's primary
archive:
1. component-based
2. package-based
3. packageset-based
Right now to change #1 and #2 you need to be in the team that owns the archive
object. To change #3 you need to be an admin or a member of Ubuntu Techboard.
These 2 rules are basically useless for anything other than Ubuntu because
* There's no way to alter the archive owner other than via SQL
* Derived distros don't want Ubuntu Techboard changing their permissions!
This branch fixes this situation so that the two different rules are unified
so that you need to be in the *distribution's* owning team (this is Maintainer
on the distro page) to change these permissions.
== Pre-implementation notes ==
Discussed with Gavin as I had a PEBKAC moment working out how the heck
we'd got by without an EditArchive security adapter up until now but it turns
out that it inherits the IHasOwner magic.
== Implementation details ==
Several changes were needed:
* Add a new security adapter for IArchive/launchpad.Edit that allows
distro main archives to be edited by the distro owner or an admin, and
leaves PPAs editable by their archive owners.
* Remove the EditArchivePermissionSet security adapter, it is redundant
because it can never have enough contextual information to be useful (it
doesn't know what the archive is). We now rely on the utility only getting
called via IArchive code.
* Change the zcml declaration for IArchivePermissionSet to allow any caller
to call its methods.
* Remove unnecessary testing in lib/lp/soyuz/doc/archivepermission.txt
* Add a warning in lib/lp/soyuz/interfaces/archivepermission.py about never
exporting ArchivePermissionSet on the webservice because it has no
good security model.
* Fix lib/lp/soyuz/stories/webservice/xx-archive.txt to handle the new
security checks.
* Fix lib/lp/soyuz/tests/test_archive.py to not rely on the old checks
to alter some of the uploader permissions.
== Tests ==
bin/test -cvvt test_archive -t xx-archive.txt -t archivepermission.txt
And probably more, I've not run this through ec2 yet.
== Demo and Q/A ==
Dogfood.
--
https://code.launchpad.net/~julian-edwards/launchpad/perms-for-changing-uploaders--bug-828894/+merge/72220
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/perms-for-changing-uploaders--bug-828894 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2011-07-27 15:02:34 +0000
+++ lib/canonical/launchpad/security.py 2011-08-19 16:38:26 +0000
@@ -2246,6 +2246,22 @@
return not self.obj.private and self.obj.enabled
+class EditArchive(AuthorizationBase):
+ """Restrict archive editing operations.
+
+ If the archive a primary archive then we check the user is in the
+ distribution's owning team, otherwise we check the archive owner.
+ """
+ permission = 'launchpad.Edit'
+ usedfor = IArchive
+
+ def checkAuthenticated(self, user):
+ if self.obj.is_main:
+ return user.isOwner(self.obj.distribution) or user.in_admin
+
+ return user.isOwner(self.obj) or user.in_admin
+
+
class AppendArchive(AuthorizationBase):
"""Restrict appending (upload and copy) operations on archives.
@@ -2535,15 +2551,6 @@
usedfor = IWikiName
-class EditArchivePermissionSet(AuthorizationBase):
- permission = 'launchpad.Edit'
- usedfor = IArchivePermissionSet
-
- def checkAuthenticated(self, user):
- """Users must be an admin or a member of the tech board."""
- return user.in_admin or user.in_ubuntu_techboard
-
-
class ViewPackageset(AnonymousAuthorization):
"""Anyone can view an IPackageset."""
usedfor = IPackageset
=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml 2011-07-28 14:46:35 +0000
+++ lib/lp/soyuz/configure.zcml 2011-08-19 16:38:26 +0000
@@ -471,31 +471,7 @@
class="lp.soyuz.model.archivepermission.ArchivePermissionSet"
provides="lp.soyuz.interfaces.archivepermission.IArchivePermissionSet">
<allow
- attributes="
- checkAuthenticated
- componentsForUploader
- uploadersForComponent
- packagesForUploader
- uploadersForPackage
- queueAdminsForComponent
- componentsForQueueAdmin
- permissionsForPerson
- uploadersForPackageset
- packagesetsForUploader
- packagesetsForSource
- packagesetsForSourceUploader
- isSourceUploadAllowed
- newPackageUploader
- newComponentUploader
- newQueueAdmin
- deletePackageUploader
- deleteComponentUploader
- deleteQueueAdmin"/>
- <require
- permission="launchpad.Edit"
- attributes="
- newPackagesetUploader
- deletePackagesetUploader"/>
+ interface="lp.soyuz.interfaces.archivepermission.IArchivePermissionSet"/>
</securedutility>
<!-- BinaryPackageBuild -->
=== modified file 'lib/lp/soyuz/doc/archivepermission.txt'
--- lib/lp/soyuz/doc/archivepermission.txt 2011-06-16 20:12:00 +0000
+++ lib/lp/soyuz/doc/archivepermission.txt 2011-08-19 16:38:26 +0000
@@ -266,46 +266,9 @@
~~~~~~~~~~~~~~~~~~~~
There are some methods that will enable the caller to add and delete
-PackageSet based permissions. They currently require launchpad.Edit
-permission to use, which enforces the user to be an admin or a member
-of the "techboard" (Ubuntu Technical Board) team.
-
-Set up a helper function to check access:
-
- >>> from zope.security.checker import canAccess
- >>> from zope.security.proxy import removeSecurityProxy
- >>> restricted_methods = (
- ... 'newPackagesetUploader',
- ... 'deletePackagesetUploader',
- ... )
- >>> def checkAccess(true_or_false):
- ... for method in restricted_methods:
- ... assert(canAccess(permission_set, method) == true_or_false)
- >>> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
- >>> from lp.registry.interfaces.person import IPersonSet
- >>> from lp.registry.interfaces.teammembership import (
- ... TeamMembershipStatus)
- >>> personset = getUtility(IPersonSet)
- >>> techboard = getUtility(ILaunchpadCelebrities).ubuntu_techboard
- >>> techboard = removeSecurityProxy(techboard)
-
-Ordinary users have no access:
-
- >>> login("test@xxxxxxxxxxxxx")
- >>> checkAccess(False)
-
-Admins have access:
-
- >>> login("foo.bar@xxxxxxxxxxxxx")
- >>> checkAccess(True)
-
-Now add "test@xxxxxxxxxxxxx" to the techboard team and log in as him.
-
- >>> person = personset.getByEmail("test@xxxxxxxxxxxxx")
- >>> ignored = techboard.addMember(
- ... person, reviewer=person, status=TeamMembershipStatus.APPROVED,
- ... force_team_add=True)
- >>> login_person(person)
+PackageSet based permissions. They require no special permission to use
+because these methods should only ever be called from inside other security
+proxied objects like IArchive.
newPackageUploader() creates a permission for a person to upload to a
specific package:
=== modified file 'lib/lp/soyuz/interfaces/archivepermission.py'
--- lib/lp/soyuz/interfaces/archivepermission.py 2011-01-31 16:01:21 +0000
+++ lib/lp/soyuz/interfaces/archivepermission.py 2011-08-19 16:38:26 +0000
@@ -129,6 +129,15 @@
class IArchivePermissionSet(Interface):
"""The interface for `ArchivePermissionSet`."""
+ # Do not export this utility directly on the webservice. There is
+ # no reasonable security model we can implement for it because it
+ # requires the archive context to be able to make an informed
+ # security decision.
+ #
+ # For this reason, the security declaration in the zcml is
+ # deliberately permissive. We don't expect anything to access this
+ # utility except the IArchive code, which is appropriately protected.
+
def checkAuthenticated(person, archive, permission, item):
"""The `ArchivePermission` records that authenticate the person.
=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt 2011-06-23 16:05:30 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt 2011-08-19 16:38:26 +0000
@@ -1,7 +1,7 @@
Archives
========
-Representations for IArchive can be fetch via the API for PPAs and
+Representations for IArchive can be fetched via the API for PPAs and
distribution archives.
>>> cprov_archive = webservice.get("/~cprov/+archive/ppa").jsonBody()
@@ -270,24 +270,49 @@
>>> from canonical.launchpad.testing.pages import webservice_for_person
>>> from canonical.launchpad.webapp.interfaces import OAuthPermission
>>> from lp.registry.interfaces.distribution import IDistributionSet
+ >>> from lp.testing.factory import LaunchpadObjectFactory
+ >>> factory = LaunchpadObjectFactory()
- >>> login(ANONYMOUS)
+ >>> login('foo.bar@xxxxxxxxxxxxx')
>>> cjwatson = getUtility(IPersonSet).getByName('kamion')
>>> ubuntu_db = getUtility(IDistributionSet).getByName('ubuntu')
>>> cjwatson.inTeam(ubuntu_db.main_archive.owner)
True
+Let's also make a new Person to own the Ubuntu distro.
+
+ >>> ubuntu_owner = factory.makePerson()
+ >>> ubuntu_db.owner = ubuntu_owner
+
>>> logout()
>>> cjwatson_webservice = webservice_for_person(
... cjwatson, permission=OAuthPermission.WRITE_PUBLIC)
-
+ >>> ubuntu_owner_webservice = webservice_for_person(
+ ... ubuntu_owner, permission=OAuthPermission.WRITE_PUBLIC)
+
+To be able to amend any permissions on a distribution archive,
+you need to be one of the distribution owners - not one of the archive
+owners. Here, cjwatson cannot make a new package uploader:
+
+ >>> name12 = webservice.get("/~name12").jsonBody()
+ >>> response = cjwatson_webservice.named_post(
+ ... ubuntu['main_archive_link'], 'newPackageUploader', {},
+ ... person=name12['self_link'],
+ ... source_package_name='mozilla-firefox')
+ >>> print response
+ HTTP/1.1 401 Unauthorized
+ ...
+ (<Archive at ...>, 'newPackageUploader', 'launchpad.Edit')
+
+From here on we'll use ubuntu_owner, who does have permission as Ubuntu's
+owner.
`newPackageUploader` is a factory function that adds a new permission
for a person to upload a package.
>>> name12 = webservice.get("/~name12").jsonBody()
- >>> response = cjwatson_webservice.named_post(
+ >>> response = ubuntu_owner_webservice.named_post(
... ubuntu['main_archive_link'], 'newPackageUploader', {},
... person=name12['self_link'],
... source_package_name='mozilla-firefox')
@@ -306,7 +331,7 @@
deletePackageUploader() removes that permission:
- >>> print cjwatson_webservice.named_post(
+ >>> print ubuntu_owner_webservice.named_post(
... ubuntu['main_archive_link'], 'deletePackageUploader', {},
... person=name12['self_link'],
... source_package_name='mozilla-firefox')
@@ -348,7 +373,7 @@
newComponentUploader adds a new permission for a person to upload to a
component.
- >>> response = cjwatson_webservice.named_post(
+ >>> response = ubuntu_owner_webservice.named_post(
... ubuntu['main_archive_link'], 'newComponentUploader', {},
... person=name12['self_link'],
... component_name='restricted')
@@ -382,7 +407,7 @@
deleteComponentUploader() removes that permission:
- >>> print cjwatson_webservice.named_post(
+ >>> print ubuntu_owner_webservice.named_post(
... ubuntu['main_archive_link'], 'deleteComponentUploader', {},
... person=name12['self_link'],
... component_name='restricted')
@@ -411,7 +436,7 @@
this distribution's primary archive. Did you mean to upload to a PPA?
-Only the archive owners can add or remove component-uploaders.
+For PPAs, only the archive owners can add or remove component-uploaders.
>>> no_priv = webservice.get("/~no-priv").jsonBody()
@@ -479,7 +504,7 @@
newQueueAdmin adds a new permission for a person to administer distroseries
queues in a particular component.
- >>> response = webservice.named_post(
+ >>> response = ubuntu_owner_webservice.named_post(
... ubuntu['main_archive_link'], 'newQueueAdmin', {},
... person=name12['self_link'],
... component_name='partner')
@@ -487,7 +512,7 @@
HTTP/1.1 201 Created
...
- >>> new_permission = webservice.get(
+ >>> new_permission = ubuntu_owner_webservice.get(
... response.getHeader('Location')).jsonBody()
>>> print new_permission['self_link']
http://.../ubuntu/+archive/primary/+queue-admin/name12?type=component&item=partner
@@ -501,7 +526,7 @@
deleteQueueAdmin removes that permission.
- >>> print webservice.named_post(
+ >>> print ubuntu_owner_webservice.named_post(
... ubuntu['main_archive_link'], 'deleteQueueAdmin', {},
... person=name12['self_link'],
... component_name='partner')
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py 2011-08-17 11:43:06 +0000
+++ lib/lp/soyuz/tests/test_archive.py 2011-08-19 16:38:26 +0000
@@ -480,6 +480,9 @@
def test_checkArchivePermission_distro_archive(self):
# Regular users can not upload to ubuntu
archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+ # The factory sets the archive owner the same as the distro owner,
+ # change that here to ensure the security adapter checks are right.
+ removeSecurityProxy(archive).owner = self.factory.makePerson()
main = getUtility(IComponentSet)["main"]
# A regular user doesn't have access
somebody = self.factory.makePerson()
@@ -487,7 +490,7 @@
archive.checkArchivePermission(somebody, main))
# An ubuntu core developer does have access
coredev = self.factory.makePerson()
- with person_logged_in(archive.owner):
+ with person_logged_in(archive.distribution.owner):
archive.newComponentUploader(coredev, main.name)
self.assertEquals(True, archive.checkArchivePermission(coredev, main))
@@ -2137,7 +2140,7 @@
to_series, version) = self._setup_copy_data(
target_purpose=ArchivePurpose.PRIMARY)
person = self.factory.makePerson()
- with person_logged_in(target_archive.owner):
+ with person_logged_in(target_archive.distribution.owner):
target_archive.newComponentUploader(person, "universe")
target_archive.copyPackage(
source_name, version, source_archive, to_pocket.name,
@@ -2244,7 +2247,7 @@
to_series, version) = self._setup_copy_data(
target_purpose=ArchivePurpose.PRIMARY)
person = self.factory.makePerson()
- with person_logged_in(target_archive.owner):
+ with person_logged_in(target_archive.distribution.owner):
target_archive.newComponentUploader(person, "universe")
target_archive.copyPackages(
[source_name], source_archive, to_pocket.name,
Follow ups
