Merge lp:~adeuring/launchpad/bug-1079116 into lp:launchpad

[Abel Deuring]

If a branch is the series branch of a proprietary product, do not try to display data about the series in branch listings if users have access to the branch but not to the product.

This branch fixes bug 1079116: https://code.launchpad.net/~ broken if the user had worked on private projects but has no longer access to these projects

The cause of the error: A user may have an artfiact grant for a proprietary branch that is the official branch of a series of a proprieatry product. If the user does not have a policy grant for the product, accessing the series object in lp.code.browser.branchlisting.BranchListingItemsMixin.product_series_map fails.

product_series_map builds a dictionary branch_id -> [product_series, ...]; I simply added a filter so that only series instances are added which the user can view. The method series.userCanView() is anyway called by the security adapter, so there are no additional queries involved.

test:

./bin/test code -vvt test_proprietary_branch_for_series_artifact_grant

no lint

[j.c.sackett (jcsackett)]

I had a concern about the userCanView calls, but I think your logic about check_permission is correct, so this looks good. Thanks for adding that note in the proposal.

review: Approve

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2012-10-09 00:04:10 +0000
+++ lib/lp/code/browser/branchlisting.py	2012-11-29 16:58:27 +0000
@@ -340,7 +340,11 @@
             self._visible_branch_ids)
         result = {}
         for series in series_resultset:
-            result.setdefault(series.branch.id, []).append(series)
+            # Some products may be proprietary or embargoed, and users
+            # do not need to have access to them, while they may have
+            # artifact grants for the series branch.
+            if series.userCanView(self.view_user):
+                result.setdefault(series.branch.id, []).append(series)
         return result
 
     def getProductSeries(self, branch):

=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2012-10-18 12:40:40 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2012-11-29 16:58:27 +0000
@@ -20,6 +20,8 @@
 from testtools.matchers import Not
 from zope.component import getUtility
 
+from lp.app.enums import InformationType
+from lp.app.interfaces.services import IService
 from lp.code.browser.branchlisting import (
     BranchListingSort,
     BranchListingView,
@@ -31,7 +33,10 @@
 from lp.code.model.seriessourcepackagebranch import (
     SeriesSourcePackageBranchSet,
     )
-from lp.registry.enums import PersonVisibility
+from lp.registry.enums import (
+    PersonVisibility,
+    SharingPermission,
+    )
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.personproduct import IPersonProductFactory
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -41,6 +46,7 @@
 from lp.services.webapp import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
+    admin_logged_in,
     BrowserTestCase,
     login_person,
     normalize_whitespace,
@@ -258,6 +264,57 @@
         # The correct template is used for batch requests.
         self._test_batch_template(self.barney)
 
+    def test_proprietary_branch_for_series_user_has_artifact_grant(self):
+        # A user can be the owner of a branch which is the series
+        # branch of a proprietary product, and the user may only have
+        # an access grant for the branch but no policy grant for the
+        # product. In this case, the branch owner does get any information
+        #about the series.
+        product_owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=product_owner, information_type=InformationType.PROPRIETARY)
+        branch_owner = self.factory.makePerson()
+        sharing_service = getUtility(IService, 'sharing')
+        with person_logged_in(product_owner):
+            # The branch owner needs to have a policy grant at first
+            # so that they can create the branch.
+            sharing_service.sharePillarInformation(
+                product, branch_owner, product_owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+            proprietary_branch = self.factory.makeProductBranch(
+                product, owner=branch_owner, name='special-branch',
+                information_type=InformationType.PROPRIETARY)
+            series = self.factory.makeProductSeries(
+                product=product, branch=proprietary_branch)
+            sharing_service.deletePillarGrantee(
+                product, branch_owner, product_owner)
+        # Admin help is needed: Product owners do not have the
+        # permission to create artifact grants for branches they
+        # do not own, and the branch owner does have the permission
+        # to issue grants related to the product.
+        with admin_logged_in():
+            sharing_service.ensureAccessGrants(
+                [branch_owner], product_owner, branches=[proprietary_branch])
+
+        with person_logged_in(branch_owner):
+            view = create_initialized_view(
+                branch_owner, name="+branches", rootsite='code',
+                principal=branch_owner)
+            self.assertIn(proprietary_branch, view.branches().batch)
+            # The product series related to the branch is not returned
+            # for the branch owner.
+            self.assertEqual(
+                [], view.branches().getProductSeries(proprietary_branch))
+
+        with person_logged_in(product_owner):
+            # The product series related to the branch is returned
+            # for the product owner.
+            view = create_initialized_view(
+                branch_owner, name="+branches", rootsite='code',
+                principal=branch_owner)
+            self.assertEqual(
+                [series], view.branches().getProductSeries(proprietary_branch))
+
 
 class TestSimplifiedPersonBranchesView(TestCaseWithFactory):