launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-796867 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #796867 in Launchpad itself: "Cannot modify upstream of packages I can upload in Ubuntu"
  https://bugs.launchpad.net/launchpad/+bug/796867

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-796867/+merge/65209

This branch fixes bug 796867: Cannot modify upstream of packages I can upload in Ubuntu

This problem is caused by a bad permission check in Packaging.userCanDelete(): Allowing package_maintainer to delete a link is nonsensical. As discussed in the bur report, it is better to check if a person can edit the source package. The new implementation does this by checking if the current user can access sourcepackage.setbranch(), which is guarded by the Launchpad.Edit permission.

test: ./bin/test registry -vvt lp.registry.tests.test_packaging

no lint
-- 
https://code.launchpad.net/~adeuring/launchpad/bug-796867/+merge/65209
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-796867 into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/packaging.py'
--- lib/lp/registry/interfaces/packaging.py	2011-04-07 20:40:29 +0000
+++ lib/lp/registry/interfaces/packaging.py	2011-06-20 14:51:41 +0000
@@ -96,7 +96,12 @@
 
     def userCanDelete():
         """True, if the current user is allowed to delete this packaging,
-        else False."""
+        else False.
+
+        People who created the packaging can delete it, as well as
+        people with upload rights for a source package, distribution
+        owners, members of the registry team and LP admin team.
+        """
 
 
 class IPackagingUtil(Interface):

=== modified file 'lib/lp/registry/model/packaging.py'
--- lib/lp/registry/model/packaging.py	2011-05-27 21:12:25 +0000
+++ lib/lp/registry/model/packaging.py	2011-06-20 14:51:41 +0000
@@ -73,15 +73,12 @@
         user = getUtility(ILaunchBag).user
         if user is None:
             return False
-        currentrelease = self.sourcepackage.currentrelease
-        package_maintainer = (
-            currentrelease.maintainer if currentrelease is not None
-            else None)
         admin = getUtility(ILaunchpadCelebrities).admin
         registry_experts = (
             getUtility(ILaunchpadCelebrities).registry_experts)
         return (
-            user.inTeam(self.owner) or user.inTeam(package_maintainer) or
+            user.inTeam(self.owner) or
+            user.canAccess(self.sourcepackage, 'setBranch') or
             user.inTeam(registry_experts) or user.inTeam(admin))
 
     def destroySelf(self):

=== modified file 'lib/lp/registry/tests/test_packaging.py'
--- lib/lp/registry/tests/test_packaging.py	2011-04-08 14:14:44 +0000
+++ lib/lp/registry/tests/test_packaging.py	2011-06-20 14:51:41 +0000
@@ -29,7 +29,6 @@
 from lp.registry.model.packaging import (
     Packaging,
     )
-from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
     EventRecorder,
     login,
@@ -53,7 +52,6 @@
 
     def test_destroySelf_notifies(self):
         """destroySelf creates a notification."""
-        packaging_util = getUtility(IPackagingUtil)
         packaging = self.factory.makePackagingLink()
         with person_logged_in(packaging.owner):
             with EventRecorder() as recorder:
@@ -96,26 +94,37 @@
             packaging_util.packagingEntryExists(
                 sourcepackagename, distroseries, productseries))
 
-    def test_destroySelf__allowed_for_package_maintainer(self):
-        """A package maintainer can delete a packaging."""
-        # The package maintainer is the, strictly speaking, the
-        # maintainer of the current release. SoyuzTestPublisher
-        # knows how to create a current release.
-        publisher = SoyuzTestPublisher()
-        distroseries = publisher.setUpDefaultDistroSeries()
-        publisher.getPubSource(version='0.9')
-        sourcepackage = distroseries.getSourcePackage(
-            publisher.default_package_name)
-        sourcepackagename = sourcepackage.sourcepackagename
-        packaging = publisher.factory.makePackagingLink(
-            sourcepackagename=sourcepackagename, distroseries=distroseries)
+    def test_destroySelf__allowed_for_distro_owner(self):
+        """A distribution owner can delete a packaging link."""
+        packaging = self.factory.makePackagingLink()
+        sourcepackagename = packaging.sourcepackagename
+        distroseries = packaging.distroseries
         productseries = packaging.productseries
         packaging_util = getUtility(IPackagingUtil)
+        with person_logged_in(distroseries.distribution.owner):
+            packaging_util.deletePackaging(
+                productseries, sourcepackagename, distroseries)
+        self.assertFalse(
+            packaging_util.packagingEntryExists(
+                sourcepackagename, distroseries, productseries))
 
-        with person_logged_in(sourcepackage.currentrelease.maintainer):
+    def test_destroySelf__allowed_for_uploader(self):
+        """A person with upload rights for the sourcepackage can
+        delete a packaging link.
+        """
+        packaging = self.factory.makePackagingLink()
+        sourcepackagename = packaging.sourcepackagename
+        sourcepackage = packaging.sourcepackage
+        distroseries = packaging.distroseries
+        productseries = packaging.productseries
+        uploader = self.factory.makePerson()
+        archive = sourcepackage.get_default_archive()
+        with person_logged_in(distroseries.distribution.main_archive.owner):
+            archive.newPackageUploader(uploader, sourcepackage.name)
+        packaging_util = getUtility(IPackagingUtil)
+        with person_logged_in(uploader):
             packaging_util.deletePackaging(
-                packaging.productseries, packaging.sourcepackagename,
-                packaging.distroseries)
+                productseries, sourcepackagename, distroseries)
         self.assertFalse(
             packaging_util.packagingEntryExists(
                 sourcepackagename, distroseries, productseries))