launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/copy-allows-queue-admin into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1006917 in Launchpad itself: "Distribution archive owners cannot necessarily copy packages"
  https://bugs.launchpad.net/launchpad/+bug/1006917

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/copy-allows-queue-admin/+merge/116731

== Summary ==

Bug 1006917: people with queue admin permissions cannot necessarily copy packages; they require upload permissions too.  This makes it hard to wean various automated scripts run by ubuntu-archive off direct database access and onto the API, because they have to be given blanket upload access, which makes me feel dirty.  If nothing else, certain types of mistakes are harder to make when the permissions are narrower.

== Proposed fix ==

Many of the kinds of things we do with queue admin permissions are associated with copies (e.g. releasing stable updates), so it makes at least a degree of sense to allow those with queue admin permissions to copy packages.  We trust all such people in Ubuntu anyway, and I don't think anyone else relies on queue admin permissions.

To attempt to minimise duplication, I beefed up Archive.canAdministerQueue to be able to handle multiple components, and re-expressed EditPackageUpload in terms of this.  That makes it easy for check_copy_permissions to reuse this logic (although I had to break up the call to Archive.checkUpload into two pieces to distinguish "closed pocket" from "insufficient permissions"), and will support near-future work on per-pocket queue admin permissions.

== LOC Rationale ==

+88, but this is one of the remaining pieces needed to remove copy-package.py (once this or a similar branch lands, I'll be able to propose a fix for bug 1006871, which is the other piece needed for us to remove our last known reliance on it); and in any case this is more than offset by the -179 in https://code.launchpad.net/~cjwatson/launchpad/reorg-copy-package-tests-2/+merge/116729 which I just put up for review.

== Tests ==

bin/test -vvct archive.txt -t test_archivepermission -t archivepermission.txt -t browser.tests.test_queue -t xx-archive.txt -t test_archive.TestSyncSource

== Demo and Q/A ==

Try copying packages in each of the following cases:

 * person with appropriate upload permissions (yes)
 * person with appropriate queue admin permissions (yes)
 * person with queue admin permissions, but only to the wrong component (no)
 * person with neither type of permission (no)

== Lint ==

Just a pre-existing false positive due to pocketlint not understanding property setters:

./lib/lp/soyuz/model/archive.py
     346: redefinition of function 'private' from line 342
-- 
https://code.launchpad.net/~cjwatson/launchpad/copy-allows-queue-admin/+merge/116731
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/copy-allows-queue-admin into lp:launchpad.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-07-09 17:17:58 +0000
+++ lib/lp/security.py	2012-07-25 19:29:20 +0000
@@ -1730,19 +1730,8 @@
             archive_append = AppendArchive(self.obj.archive)
             return archive_append.checkAuthenticated(user)
 
-        permission_set = getUtility(IArchivePermissionSet)
-        permissions = permission_set.componentsForQueueAdmin(
-            self.obj.archive, user.person)
-        if permissions.count() == 0:
-            return False
-        allowed_components = set(
-            permission.component for permission in permissions)
-        existing_components = self.obj.components
-        # The intersection of allowed_components and
-        # existing_components must be equal to existing_components
-        # to allow the operation to go ahead.
-        return (allowed_components.intersection(existing_components)
-                == existing_components)
+        return self.obj.archive.canAdministerQueue(
+            user.person, self.obj.components)
 
 
 class AdminByBuilddAdmin(AuthorizationBase):

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-07-02 16:40:17 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-07-25 19:29:20 +0000
@@ -680,14 +680,15 @@
             None otherwise.
         """
 
-    def canAdministerQueue(person, component):
+    def canAdministerQueue(person, components):
         """Check to see if person is allowed to administer queue items.
 
-        :param person: An `IPerson` whom should be checked for authenticate.
-        :param component: The context `IComponent` for the check.
+        :param person: An `IPerson` who should be checked for authentication.
+        :param components: The context `IComponent`(s) for the check.
 
         :return: True if 'person' is allowed to administer the package upload
-        queue for items with 'component'.
+        queue for all given 'components'.  If 'components' is empty or None,
+        check if 'person' has any queue admin permissions for this archive.
         """
 
     def getFileByName(filename):

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-07-23 11:51:21 +0000
+++ lib/lp/soyuz/model/archive.py	2012-07-25 19:29:20 +0000
@@ -1347,10 +1347,20 @@
 
         return None
 
-    def canAdministerQueue(self, user, component):
+    def canAdministerQueue(self, user, components):
         """See `IArchive`."""
-        return self._authenticate(
-            user, component, ArchivePermissionType.QUEUE_ADMIN)
+        if components is None:
+            components = []
+        elif IComponent.providedBy(components):
+            components = [components]
+        permissions = self.getComponentsForQueueAdmin(user)
+        if permissions.count() == 0:
+            return False
+        allowed_components = set(
+            permission.component for permission in permissions)
+        # The intersection of allowed_components and components must be
+        # equal to components to allow the operation to go ahead.
+        return allowed_components.intersection(components) == set(components)
 
     def _authenticate(self, user, item, permission):
         """Private helper method to check permissions."""

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2012-07-05 09:43:58 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2012-07-25 19:29:20 +0000
@@ -256,16 +256,22 @@
         except IndexError:
             destination_component = None
 
+        # Is the destination pocket open at all?
+        reason = archive.checkUploadToPocket(dest_series, pocket)
+        if reason is not None:
+            raise CannotCopy(reason)
+
         # If destination_component is not None, make sure the person
         # has upload permission for this component.  Otherwise, any
         # upload permission on this archive will do.
         strict_component = destination_component is not None
-        reason = archive.checkUpload(
-            person, dest_series, spn, destination_component, pocket,
-            strict_component=strict_component)
-
+        reason = archive.verifyUpload(
+            person, spn, destination_component, dest_series,
+            strict_component=strict_component, pocket=pocket)
         if reason is not None:
-            raise CannotCopy(reason)
+            # Queue admins are allowed to copy even if they can't upload.
+            if not archive.canAdministerQueue(person, destination_component):
+                raise CannotCopy(reason)
 
 
 class CopyChecker:

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-07-23 11:51:21 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-07-25 19:29:20 +0000
@@ -2315,6 +2315,70 @@
             to_pocket.name, to_series=to_series.name, include_binaries=False,
             person=person)
 
+    def test_copyPackage_allows_queue_admins_for_new_packages(self):
+        # If a package does not exist in the target archive and series,
+        # people with queue admin permissions to any component may copy it.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data(
+            target_purpose=ArchivePurpose.PRIMARY)
+        person = self.factory.makePerson()
+        with person_logged_in(target_archive.distribution.owner):
+            target_archive.newQueueAdmin(person, "universe")
+        target_archive.copyPackage(
+            source_name, version, source_archive, to_pocket.name,
+            to_series=to_series.name, include_binaries=False,
+            person=person)
+
+        # There should be one copy job.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_job = job_source.getActiveJobs(target_archive).one()
+        self.assertEqual(target_archive, copy_job.target_archive)
+
+    def test_copyPackage_allows_queue_admins_for_correct_component(self):
+        # If a package already exists in the target archive and series,
+        # queue admins of its component may copy it.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data(
+            target_purpose=ArchivePurpose.PRIMARY)
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=to_series, archive=target_archive,
+            status=PackagePublishingStatus.PUBLISHED,
+            sourcepackagename=source_name, version="%s~" % version,
+            component="main")
+        person = self.factory.makePerson()
+        with person_logged_in(target_archive.distribution.owner):
+            target_archive.newQueueAdmin(person, "main")
+        target_archive.copyPackage(
+            source_name, version, source_archive, to_pocket.name,
+            to_series=to_series.name, include_binaries=False,
+            person=person)
+
+        # There should be one copy job.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_job = job_source.getActiveJobs(target_archive).one()
+        self.assertEqual(target_archive, copy_job.target_archive)
+
+    def test_copyPackage_disallows_queue_admins_for_incorrect_component(self):
+        # If a package already exists in the target archive and series,
+        # people who only have queue admin permissions to some other
+        # component may not copy it.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data(
+            target_purpose=ArchivePurpose.PRIMARY)
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=to_series, archive=target_archive,
+            status=PackagePublishingStatus.PUBLISHED,
+            sourcepackagename=source_name, version="%s~" % version,
+            component="main")
+        person = self.factory.makePerson()
+        with person_logged_in(target_archive.distribution.owner):
+            target_archive.newQueueAdmin(person, "universe")
+        self.assertRaises(
+            CannotCopy,
+            target_archive.copyPackage, source_name, version, source_archive,
+            to_pocket.name, to_series=to_series.name, include_binaries=False,
+            person=person)
+
     def test_copyPackage_disallows_non_release_target_pocket_for_PPA(self):
         (source, source_archive, source_name, target_archive, to_pocket,
          to_series, version) = self._setup_copy_data()
@@ -2450,7 +2514,7 @@
         self.assertEqual(target_archive, copy_job.target_archive)
 
     def test_copyPackages_disallows_non_PPA_owners(self):
-        # Only people with launchpad.Append are allowed to call copyPackage.
+        # Only people with launchpad.Append are allowed to call copyPackages.
         (source, source_archive, source_name, target_archive, to_pocket,
          to_series, version) = self._setup_copy_data()
         person = self.factory.makePerson()
@@ -2461,6 +2525,24 @@
             to_pocket.name, to_series=to_series.name, include_binaries=False,
             person=person)
 
+    def test_copyPackages_allows_queue_admins(self):
+        # Queue admins without upload permissions may still copy packages.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data(
+            target_purpose=ArchivePurpose.PRIMARY)
+        person = self.factory.makePerson()
+        with person_logged_in(target_archive.distribution.owner):
+            target_archive.newQueueAdmin(person, "universe")
+        target_archive.copyPackages(
+            [source_name], source_archive, to_pocket.name,
+            to_series=to_series.name, include_binaries=False,
+            person=person)
+
+        # There should be one copy job.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_job = job_source.getActiveJobs(target_archive).one()
+        self.assertEqual(target_archive, copy_job.target_archive)
+
     def test_copyPackages_with_multiple_distroseries(self):
         # The from_series parameter selects a source distroseries.
         (source, source_archive, source_name, target_archive, to_pocket,