Merge lp:~julian-edwards/launchpad/perms-for-changing-uploaders--bug-828894 into lp:launchpad

[Julian Edwards]

[r=sinzui][bug=828894] Fix the permissions required to change uploader-related permissions on a distribution.

= 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.

[Curtis Hovey (sinzui)]

Thank you for this fix. I believe with your change that there is exactly one call for user.in_ubuntu_techboard Which is used in security.py and defined in lp/registry/interfaces/role.py. Is there any chance you could also change EditPackagesetSet to not use that role and remove the role from code?

review: Approve (code)

[Julian Edwards (julian-edwards)]

Thanks Curtis. I won't do that change in this branch, but I'll file a bug and fix it soon, I'd forgotten about that.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-08-24 00:47:25 +0000
+++ lib/canonical/launchpad/security.py	2011-08-25 10:17:54 +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-08-24 00:47:25 +0000
+++ lib/lp/soyuz/configure.zcml	2011-08-25 10:17:54 +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-08-24 00:47:25 +0000
+++ lib/lp/soyuz/doc/archivepermission.txt	2011-08-25 10:17:54 +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("[e-mail]")
-    >>> checkAccess(False)
-
-Admins have access:
-
-    >>> login("[e-mail]")
-    >>> checkAccess(True)
-
-Now add "[e-mail]" to the techboard team and log in as him.
-
-    >>> person = personset.getByEmail("[e-mail]")
-    >>> 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/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2011-08-01 15:28:09 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2011-08-25 10:17:54 +0000
@@ -711,29 +711,6 @@
         """
 
     @operation_parameters(
-        person=Reference(schema=IPerson),
-        # Really IPackageset, corrected in _schema_circular_imports to avoid
-        # circular import.
-        packageset=Reference(
-            Interface, title=_("Package set"), required=True),
-        explicit=Bool(
-            title=_("Explicit"), required=False))
-    # Really IArchivePermission, set in _schema_circular_imports to avoid
-    # circular import.
-    @export_factory_operation(Interface, [])
-    def newPackagesetUploader(person, packageset, explicit=False):
-        """Add a package set based permission for a person.
-
-        :param person: An `IPerson` for whom you want to add permission.
-        :param packageset: An `IPackageset`.
-        :param explicit: True if the package set in question requires
-            specialist skills for proper handling.
-
-        :return: The new `ArchivePermission`, or the existing one if it
-            already exists.
-        """
-
-    @operation_parameters(
         # Really IPackageset, corrected in _schema_circular_imports to avoid
         # circular import.
         packageset=Reference(
@@ -757,24 +734,6 @@
         """
 
     @operation_parameters(
-        person=Reference(schema=IPerson),
-        # Really IPackageset, corrected in _schema_circular_imports to avoid
-        # circular import.
-        packageset=Reference(
-            Interface, title=_("Package set"), required=True),
-        explicit=Bool(
-            title=_("Explicit"), required=False))
-    @export_write_operation()
-    def deletePackagesetUploader(person, packageset, explicit=False):
-        """Revoke upload permissions for a person.
-
-        :param person: An `IPerson` for whom you want to revoke permission.
-        :param packageset: An `IPackageset`.
-        :param explicit: The value of the 'explicit' flag for the permission
-            to be revoked.
-        """
-
-    @operation_parameters(
         person=Reference(schema=IPerson))
     # Really IArchivePermission, set in _schema_circular_imports to avoid
     # circular import.
@@ -1490,6 +1449,29 @@
 
     @operation_parameters(
         person=Reference(schema=IPerson),
+        # Really IPackageset, corrected in _schema_circular_imports to avoid
+        # circular import.
+        packageset=Reference(
+            Interface, title=_("Package set"), required=True),
+        explicit=Bool(
+            title=_("Explicit"), required=False))
+    # Really IArchivePermission, set in _schema_circular_imports to avoid
+    # circular import.
+    @export_factory_operation(Interface, [])
+    def newPackagesetUploader(person, packageset, explicit=False):
+        """Add a package set based permission for a person.
+
+        :param person: An `IPerson` for whom you want to add permission.
+        :param packageset: An `IPackageset`.
+        :param explicit: True if the package set in question requires
+            specialist skills for proper handling.
+
+        :return: The new `ArchivePermission`, or the existing one if it
+            already exists.
+        """
+
+    @operation_parameters(
+        person=Reference(schema=IPerson),
         source_package_name=TextLine(
             title=_("Source Package Name"), required=True))
     @export_write_operation()
@@ -1528,6 +1510,24 @@
         :param component: An `IComponent` or textual component name.
         """
 
+    @operation_parameters(
+        person=Reference(schema=IPerson),
+        # Really IPackageset, corrected in _schema_circular_imports to avoid
+        # circular import.
+        packageset=Reference(
+            Interface, title=_("Package set"), required=True),
+        explicit=Bool(
+            title=_("Explicit"), required=False))
+    @export_write_operation()
+    def deletePackagesetUploader(person, packageset, explicit=False):
+        """Revoke upload permissions for a person.
+
+        :param person: An `IPerson` for whom you want to revoke permission.
+        :param packageset: An `IPackageset`.
+        :param explicit: The value of the 'explicit' flag for the permission
+            to be revoked.
+        """
+
     def enable():
         """Enable the archive."""
 

=== modified file 'lib/lp/soyuz/interfaces/archivepermission.py'
--- lib/lp/soyuz/interfaces/archivepermission.py	2011-08-24 00:47:25 +0000
+++ lib/lp/soyuz/interfaces/archivepermission.py	2011-08-25 10:17:54 +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-08-24 00:47:25 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt	2011-08-25 10:17:54 +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,80 @@
     >>> 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('[e-mail]')
     >>> 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)
+    >>> name12 = webservice.get("/~name12").jsonBody()
+
+And here's a packageset to play with later:
+
+    >>> print webservice.named_post(
+    ...     '/package-sets', 'new', {},
+    ...     name=u'umbrella', description=u'Contains all source packages',
+    ...     owner=name12['self_link'])
+    HTTP/1.1 201 Created
+    ...
+
+    >>> packageset = webservice.get("/package-sets/hoary/umbrella").jsonBody()
+
+
+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, packageset
+uploader or component uploader.
+
+    >>> 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')
+
+    >>> response = cjwatson_webservice.named_post(
+    ...     ubuntu['main_archive_link'], 'newPackagesetUploader', {},
+    ...     person=name12['self_link'],
+    ...     packageset=packageset['self_link'])
+    >>> print response
+    HTTP/1.1 401 Unauthorized
+    ...
+    (<Archive at ...>, 'newPackagesetUploader', 'launchpad.Edit')
+
+    >>> response = cjwatson_webservice.named_post(
+    ...     ubuntu['main_archive_link'], 'newComponentUploader', {},
+    ...     person=name12['self_link'],
+    ...     component_name='restricted')
+    >>> print response
+    HTTP/1.1 401 Unauthorized
+    ...
+    (<Archive at ...>, 'newComponentUploader', '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 +362,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 +404,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 +438,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 +467,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 +535,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 +543,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 +557,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-24 00:47:25 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2011-08-25 10:17:54 +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))
 
@@ -679,8 +682,7 @@
         packageset = self.factory.makePackageset(
             distroseries=distroseries, packages=packages)
         person = self.factory.makePerson()
-        techboard = getUtility(ILaunchpadCelebrities).ubuntu_techboard
-        with person_logged_in(techboard):
+        with person_logged_in(archive.distribution.owner):
             archive.newPackagesetUploader(person, packageset)
         return person, packageset
 
@@ -2137,7 +2139,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 +2246,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,