Merge lp:~wgrant/launchpad/dedupe-bugnotification-canApprove into lp:launchpad

[William Grant]

[r=benji][bug=451390] BugNomination.canApprove now uses Archive.verifyUpload, allowing it to respect Packageset upload permissions.

BugNomination.canApprove has long duplicated logic from archiveuploader and elsewhere to allow people to approve series nominations for packages they can upload. This has lead to a number of deficiencies, but most recently a lack of support for packageset permissions.

This branch changes BugNomination.canApprove to use Archive.verifyUpload (which checks against the package, its component, and any relevant packagesets) instead of Archive.checkArchivePermission (which just checks the package or component that it is given).

I replaced the canApprove doctest with unittests, and added a new one to ensure that packageset permissions are respected.

[Benji York (benji)]

This branch looks good.

I do have one question: I haven't seen "with
celebrity_logged_in('admin'):" used in non-test code before (and a grep
through the LP code doesn't turn any up). That doesn't mean it's wrong,
but perhaps something that warrants some thought. I'm a little worried
that logging or other audit trail might be polluted by faking the active
user. Perhaps judicious use of removeSecurityProxy is an alternative.

review: Approve (code)

[William Grant (wgrant)]

That's in lp.testing.factory.

[Benji York (benji)]

On Mon, May 9, 2011 at 7:04 PM, William Grant <email address hidden> wrote:
> That's in lp.testing.factory.

Ha! Right you are.
--
Benji York

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-05-04 17:14:18 +0000
+++ database/schema/security.cfg	2011-05-10 04:03:36 +0000
@@ -1652,6 +1652,7 @@
 public.distroseries                     = SELECT
 public.emailaddress                     = SELECT
 public.faq                              = SELECT
+public.flatpackagesetinclusion          = SELECT
 public.gpgkey                           = SELECT
 public.job                              = SELECT, INSERT, UPDATE
 public.karma                            = SELECT, INSERT
@@ -1664,6 +1665,10 @@
 public.messagechunk                     = SELECT, INSERT
 public.milestone                        = SELECT
 public.packagebugsupervisor             = SELECT
+public.packageset                       = SELECT
+public.packagesetgroup                  = SELECT
+public.packagesetinclusion              = SELECT
+public.packagesetsources                = SELECT
 public.person                           = SELECT, UPDATE
 public.personlanguage                   = SELECT
 public.personsettings                   = SELECT

=== modified file 'lib/lp/bugs/doc/bug-nomination.txt'
--- lib/lp/bugs/doc/bug-nomination.txt	2011-03-28 20:54:50 +0000
+++ lib/lp/bugs/doc/bug-nomination.txt	2011-05-10 04:03:36 +0000
@@ -352,131 +352,6 @@
     >>> ubuntu_warty.driver = None
 
 
-=== Permissions to approve a nomination ===
-
-Since approving a nomination can affect release planning, not everyone
-is allowed to do it. For large projects, like distributions, it's not
-suitable to restrict approving nominations to only release managers,
-instead anyone that can upload a package may approve a nomination.  The
-package upload permission system is a bit complex. Someone can be
-allowed to either upload a single source package, or all packages in a
-component, or all packages in a packageset.
-
-    >>> from lp.soyuz.interfaces.archivepermission import (
-    ...     IArchivePermissionSet)
-    >>> from lp.soyuz.model.component import Component
-    >>> permission_set = getUtility(IArchivePermissionSet)
-    >>> main = Component.byName('main')
-    >>> main_uploader = factory.makePerson()
-    >>> permission_set.newComponentUploader(
-    ...     ubuntu.main_archive, main_uploader, main)
-    <ArchivePermission at ...>
-
-    >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
-    >>> ubuntu_evolution = ubuntu.getSourcePackage('evolution')
-    >>> evolution_uploader = factory.makePerson()
-    >>> getUtility(IArchivePermissionSet).newPackageUploader(
-    ...     ubuntu.main_archive, evolution_uploader,
-    ...     ubuntu_evolution.sourcepackagename)
-    <ArchivePermission at ...>
-    >>> ubuntu_cnews = ubuntu.getSourcePackage('cnews')
-    >>> cnews_uploader = factory.makePerson()
-    >>> getUtility(IArchivePermissionSet).newPackageUploader(
-    ...     ubuntu.main_archive, cnews_uploader,
-    ...     ubuntu_cnews.sourcepackagename)
-    <ArchivePermission at ...>
-
-For a bug nominated to a package from the 'main' component in Ubuntu,
-people that are allowed to upload to 'main' are allowed to approve the
-nomination.
-
-    >>> ubuntu_evolution = ubuntu.getSourcePackage('evolution')
-    >>> main_bugtask = factory.makeBugTask(target=ubuntu_evolution)
-    >>> main_bug = main_bugtask.bug
-    >>> ubuntu_hoary = ubuntu.getSeries('hoary')
-    >>> hoary_evolution = ubuntu_hoary.getSourcePackage('evolution')
-    >>> hoary_evolution.latest_published_component.name
-    u'main'
-    >>> nominator = factory.makePerson()
-    >>> hoary_main_nomination = main_bug.addNomination(
-    ...     nominator, ubuntu_hoary)
-
-    >>> hoary_main_nomination.canApprove(nominator)
-    False
-    >>> hoary_main_nomination.canApprove(main_uploader)
-    True
-
-People that have been granted rights to upload that specific package are
-also allowed.
-
-    >>> hoary_main_nomination.canApprove(evolution_uploader)
-    True
-    >>> hoary_main_nomination.canApprove(cnews_uploader)
-    False
-
-Only uploaders to the relevant component are allowed to approve it. For
-example, people who are allowed only to upload to 'universe' may not
-approve 'main' nominations.
-
-    >>> universe_uploader = factory.makePerson()
-    >>> universe = Component.byName('universe')
-    >>> permission_set.newComponentUploader(
-    ...     ubuntu.main_archive, universe_uploader, universe)
-    <ArchivePermission at ...>
-
-    >>> hoary_main_nomination.canApprove(universe_uploader)
-    False
-
-If the bug is targeted to source packages in more than one component,
-anyone that can upload to any of the components or packages may approve
-the nomination. This is because a nomination is tied to a release only,
-not to source packages.
-
-    >>> hoary_cnews = ubuntu_hoary.getSourcePackage('cnews')
-    >>> hoary_cnews.latest_published_component.name
-    u'universe'
-    >>> universe_task = factory.makeBugTask(
-    ...     bug=main_bug, target=hoary_cnews)
-    >>> hoary_main_nomination.canApprove(universe_uploader)
-    True
-    >>> hoary_main_nomination.canApprove(main_uploader)
-    True
-    >>> hoary_main_nomination.canApprove(evolution_uploader)
-    True
-    >>> hoary_main_nomination.canApprove(cnews_uploader)
-    True
-
-Drivers may approve nominations as well.
-
-    >>> hoary_driver = factory.makePerson()
-    >>> ubuntu_hoary.driver = hoary_driver
-    >>> hoary_main_nomination.canApprove(hoary_driver)
-    True
-
-    >>> ubuntu_driver = factory.makePerson()
-    >>> ubuntu.driver = ubuntu_driver
-    >>> hoary_main_nomination.canApprove(ubuntu_driver)
-    True
-
-If a bug isn't targeted to a specific source package, any component
-uploader can approve a nomination.
-
-    >>> ubuntu_bugtask = factory.makeBugTask(target=ubuntu)
-    >>> bug = ubuntu_bugtask.bug
-    >>> hoary_nomination = bug.addNomination(nominator, ubuntu_hoary)
-    >>> hoary_nomination.canApprove(nominator)
-    False
-
-    >>> hoary_nomination.canApprove(main_uploader)
-    True
-    >>> hoary_nomination.canApprove(universe_uploader)
-    True
-    >>> hoary_nomination.canApprove(evolution_uploader)
-    False
-    >>> hoary_nomination.canApprove(cnews_uploader)
-    False
-
-
 == Declining a nomination ==
 
 Declining a nomination simply sets its status to DECLINED. No tasks are

=== modified file 'lib/lp/bugs/model/bugnomination.py'
--- lib/lp/bugs/model/bugnomination.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bugnomination.py	2011-05-10 04:03:36 +0000
@@ -127,33 +127,30 @@
                 return True
 
         if self.distroseries is not None:
-            # For distributions anyone that can upload to the
-            # distribution may approve nominations.
-            bug_packagenames_and_components = set()
             distribution = self.distroseries.distribution
+            # An uploader to any of the packages can approve the
+            # nomination. Compile a list of possibilities, and check
+            # them all.
+            package_names = []
             for bugtask in self.bug.bugtasks:
                 if (bugtask.distribution == distribution
                     and bugtask.sourcepackagename is not None):
-                    source_package = self.distroseries.getSourcePackage(
-                        bugtask.sourcepackagename)
-                    bug_packagenames_and_components.add(
-                        bugtask.sourcepackagename)
-                    if source_package.latest_published_component is not None:
-                        bug_packagenames_and_components.add(
-                            source_package.latest_published_component)
-            if len(bug_packagenames_and_components) == 0:
+                    package_names.append(bugtask.sourcepackagename)
+            if len(package_names) == 0:
                 # If the bug isn't targeted to a source package, allow
-                # any uploader to approve the nomination.
-                bug_packagenames_and_components = set(
-                    upload_component.component
-                    for upload_component in distribution.uploaders)
-            for packagename_or_component in bug_packagenames_and_components:
-                if distribution.main_archive.checkArchivePermission(
-                    person, packagename_or_component):
+                # any component uploader to approve the nomination, like
+                # a new package.
+                return distribution.main_archive.verifyUpload(
+                    person, None, None, None, strict_component=False) is None
+            for name in package_names:
+                component = self.distroseries.getSourcePackage(
+                    name).latest_published_component
+                if distribution.main_archive.verifyUpload(
+                    person, name, component, self.distroseries) is None:
                     return True
-
         return False
 
+
 class BugNominationSet:
     """See IBugNominationSet."""
     implements(IBugNominationSet)

=== modified file 'lib/lp/bugs/tests/test_bugnomination.py'
--- lib/lp/bugs/tests/test_bugnomination.py	2011-03-23 16:28:51 +0000
+++ lib/lp/bugs/tests/test_bugnomination.py	2011-05-10 04:03:36 +0000
@@ -10,7 +10,11 @@
     logout,
     )
 from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.testing import TestCaseWithFactory
+from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.testing import (
+    celebrity_logged_in,
+    TestCaseWithFactory,
+    )
 
 
 class CanBeNominatedForTestMixin:
@@ -101,3 +105,114 @@
         sp_bug.addTask(
             self.eric, self.series.distribution.getSourcePackage(spn))
         self.assertTrue(sp_bug.canBeNominatedFor(self.series))
+
+
+class TestCanApprove(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_normal_user_cannot_approve(self):
+        nomination = self.factory.makeBugNomination(
+            target=self.factory.makeProductSeries())
+        self.assertFalse(nomination.canApprove(self.factory.makePerson()))
+
+    def test_driver_can_approve(self):
+        product = self.factory.makeProduct(driver=self.factory.makePerson())
+        nomination = self.factory.makeBugNomination(
+            target=self.factory.makeProductSeries(product=product))
+        self.assertTrue(nomination.canApprove(product.driver))
+
+    def publishSource(self, series, sourcepackagename, component):
+        return self.factory.makeSourcePackagePublishingHistory(
+            archive=series.main_archive,
+            distroseries=series,
+            sourcepackagename=sourcepackagename,
+            component=component,
+            status=PackagePublishingStatus.PUBLISHED)
+
+    def test_component_uploader_can_approve(self):
+        # A component uploader can approve a nomination for a package in
+        # that component, but not those in other components
+        series = self.factory.makeDistroSeries()
+        package_name = self.factory.makeSourcePackageName()
+        with celebrity_logged_in('admin'):
+            perm = series.main_archive.newComponentUploader(
+                self.factory.makePerson(), self.factory.makeComponent())
+            other_perm = series.main_archive.newComponentUploader(
+                self.factory.makePerson(), self.factory.makeComponent())
+        nomination = self.factory.makeBugNomination(
+            target=series.getSourcePackage(package_name))
+
+        # Publish the package in one of the uploaders' components. The
+        # uploader for the other component cannot approve the nomination.
+        self.publishSource(series, package_name, perm.component)
+        self.assertFalse(nomination.canApprove(other_perm.person))
+        self.assertTrue(nomination.canApprove(perm.person))
+
+    def test_any_component_uploader_can_approve_for_no_package(self):
+        # An uploader for any component can approve a nomination without
+        # a package.
+        series = self.factory.makeDistroSeries()
+        with celebrity_logged_in('admin'):
+            perm = series.main_archive.newComponentUploader(
+                self.factory.makePerson(), self.factory.makeComponent())
+        nomination = self.factory.makeBugNomination(target=series)
+
+        self.assertFalse(nomination.canApprove(self.factory.makePerson()))
+        self.assertTrue(nomination.canApprove(perm.person))
+
+    def test_package_uploader_can_approve(self):
+        # A package uploader can approve a nomination for that package,
+        # but not others.
+        series = self.factory.makeDistroSeries()
+        package_name = self.factory.makeSourcePackageName()
+        with celebrity_logged_in('admin'):
+            perm = series.main_archive.newPackageUploader(
+                self.factory.makePerson(), package_name)
+            other_perm = series.main_archive.newPackageUploader(
+                self.factory.makePerson(),
+                self.factory.makeSourcePackageName())
+        nomination = self.factory.makeBugNomination(
+            target=series.getSourcePackage(package_name))
+
+        self.assertFalse(nomination.canApprove(other_perm.person))
+        self.assertTrue(nomination.canApprove(perm.person))
+
+    def test_packageset_uploader_can_approve(self):
+        # A packageset uploader can approve a nomination for anything in
+        # that packageset.
+        series = self.factory.makeDistroSeries()
+        package_name = self.factory.makeSourcePackageName()
+        ps = self.factory.makePackageset(
+            distroseries=series, packages=[package_name])
+        with celebrity_logged_in('admin'):
+            perm = series.main_archive.newPackagesetUploader(
+                self.factory.makePerson(), ps)
+        nomination = self.factory.makeBugNomination(
+            target=series.getSourcePackage(package_name))
+
+        self.assertFalse(nomination.canApprove(self.factory.makePerson()))
+        self.assertTrue(nomination.canApprove(perm.person))
+
+    def test_any_uploader_can_approve(self):
+        # If there are multiple tasks for a distribution, an uploader to
+        # any of the involved packages or components can approve the
+        # nomination.
+        series = self.factory.makeDistroSeries()
+        package_name = self.factory.makeSourcePackageName()
+        comp_package_name = self.factory.makeSourcePackageName()
+        with celebrity_logged_in('admin'):
+            package_perm = series.main_archive.newPackageUploader(
+                self.factory.makePerson(), package_name)
+            comp_perm = series.main_archive.newComponentUploader(
+                self.factory.makePerson(), self.factory.makeComponent())
+        nomination = self.factory.makeBugNomination(
+            target=series.getSourcePackage(package_name))
+        self.factory.makeBugTask(
+            bug=nomination.bug,
+            target=series.distribution.getSourcePackage(comp_package_name))
+
+        self.publishSource(series, package_name, comp_perm.component)
+        self.assertFalse(nomination.canApprove(self.factory.makePerson()))
+        self.assertTrue(nomination.canApprove(package_perm.person))
+        self.assertTrue(nomination.canApprove(comp_perm.person))

=== modified file 'lib/lp/soyuz/model/component.py'
--- lib/lp/soyuz/model/component.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/model/component.py	2011-05-10 04:03:36 +0000
@@ -34,6 +34,9 @@
 
     name = StringCol(notNull=True, alternateID=True)
 
+    def __repr__(self):
+        return "<%s '%s'>" % (self.__class__.__name__, self.name)
+
 
 class ComponentSelection(SQLBase):
     """See IComponentSelection."""
@@ -75,8 +78,6 @@
             return component
         return self.new(name)
 
-
     def new(self, name):
         """See IComponentSet."""
         return Component(name=name)
-

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-05-09 22:53:33 +0000
+++ lib/lp/testing/factory.py	2011-05-10 04:03:36 +0000
@@ -920,7 +920,8 @@
         self, name=None, project=None, displayname=None,
         licenses=None, owner=None, registrant=None,
         title=None, summary=None, official_malone=None,
-        translations_usage=None, bug_supervisor=None):
+        translations_usage=None, bug_supervisor=None,
+        driver=None):
         """Create and return a new, arbitrary Product."""
         if owner is None:
             owner = self.makePerson()
@@ -947,14 +948,15 @@
             licenses=licenses,
             project=project,
             registrant=registrant)
+        naked_product = removeSecurityProxy(product)
         if official_malone is not None:
-            removeSecurityProxy(product).official_malone = official_malone
+            naked_product.official_malone = official_malone
         if translations_usage is not None:
-            naked_product = removeSecurityProxy(product)
             naked_product.translations_usage = translations_usage
         if bug_supervisor is not None:
-            naked_product = removeSecurityProxy(product)
             naked_product.bug_supervisor = bug_supervisor
+        if driver is not None:
+            naked_product.driver = driver
         return product
 
     def makeProductSeries(self, product=None, name=None, owner=None,
@@ -1713,6 +1715,28 @@
 
         return removeSecurityProxy(bug).addTask(owner, target)
 
+    def makeBugNomination(self, bug=None, target=None):
+        """Create and return a BugNomination.
+
+        Will create a non-series task if it does not already exist.
+
+        :param bug: The `IBug` the nomination should be for. If None,
+            one will be created.
+        :param target: The `IProductSeries`, `IDistroSeries` or
+            `ISourcePackage` to nominate for.
+        """
+        if ISourcePackage.providedBy(target):
+            non_series = target.distribution_sourcepackage
+            series = target.distroseries
+        else:
+            non_series = target.parent
+            series = target
+        with celebrity_logged_in('admin'):
+            bug = self.makeBugTask(bug=bug, target=non_series).bug
+            nomination = bug.addNomination(
+                getUtility(ILaunchpadCelebrities).admin, series)
+        return nomination
+
     def makeBugTracker(self, base_url=None, bugtrackertype=None, title=None,
                        name=None):
         """Make a new bug tracker."""