launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/bad-bugtasks-how-dare-you-break-details into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #971241 in Launchpad itself: "Sharing details page breaks when an inaccessible bug is shown"
  https://bugs.launchpad.net/launchpad/+bug/971241

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/bad-bugtasks-how-dare-you-break-details/+merge/100677

Summary
=======
The sharing details view currently attempts to show all shared bugs in a
project. However, as the details view requires a permission of
`launchpad.Driver`, bugs exist that cannot be seen by everyone who can see
sharing details.

To remedy this, bugs should be filtered using the searching machinery to
safely filter out anything that the user shouldn't be able to see.

Preimp
======
Spoke with William Grant.

Additional discussion can be found in the #launchpad-dev irclogs. [1]

Implementation
==============

The bug ids found by querying the flattened artifact source are now passed
into a new method which uses `BugTaskSet.search` to get back a list of safe
bugtasks. These are then used to build the mustache data.

Additionally, testing this branch demonstrated that if an artifact was shared
via two policies, it would be listed twice; as a drive by, this branch
switches to using sets to accumulate the artifacts rather than lists,
preventing duplication.

Tests
=====
bin/test -vvct pillar_sharing

QA
==
Open https://qastaging.launchpad.net/launchpad/+sharingdetails/adeuring. It
should open safely, showing only bugs you, as the user, should be able to see.  

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/pillar.py
  lib/lp/registry/browser/tests/test_pillar_sharing.py


[1]: http://irclogs.ubuntu.com/2012/04/02/%23launchpad-dev.html#t03:28
-- 
https://code.launchpad.net/~jcsackett/launchpad/bad-bugtasks-how-dare-you-break-details/+merge/100677
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/bad-bugtasks-how-dare-you-break-details into lp:launchpad.

=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py	2012-03-31 11:32:15 +0000
+++ lib/lp/registry/browser/pillar.py	2012-04-03 19:40:27 +0000
@@ -381,21 +381,48 @@
         cache.objects['bugs'] = bug_data
         cache.objects['branches'] = branch_data
 
+    def _getSafeBugs(self, bugs):
+        """Uses the bugsearch tools to safely get the list of bugs the user is
+        allowed to see."""
+        from lp.registry.interfaces.product import IProduct
+        from lp.registry.interfaces.distribution import IDistribution
+        from lp.bugs.interfaces.bugtask import (
+            BugTaskSearchParams,
+            IBugTaskSet,
+            )
+        from zope.component import getUtility
+        params = []
+        for b in bugs:
+            param = BugTaskSearchParams(user=self.user, bug=b)
+            if IProduct.providedBy(self.pillar):
+                param.setProduct(self.pillar)
+            elif IDistribution.providedBy(self.pillar):
+                param.setDistribution(self.pillar)
+            params.append(param)
+
+        safe_bugs = getUtility(IBugTaskSet).search(params[0], *params[1:])
+        return list(safe_bugs)
+
     def _loadSharedArtifacts(self):
-        bugs = []
-        branches = []
+        # As a concrete can by linked via more than one policy, we use sets to
+        # filter out dupes.
+        bugs = set()
+        branches = set()
         for artifact in self.sharing_service.getSharedArtifacts(
                             self.pillar, self.person):
             concrete = artifact.concrete_artifact
             if IBug.providedBy(concrete):
-                bugs.append(concrete)
+                bugs.add(concrete)
             elif IBranch.providedBy(concrete):
-                branches.append(concrete)
+                branches.add(concrete)
 
-        self.bugs = bugs
+        # For security reasons, the bugs have to be refetched by ID through
+        # the normal querying mechanism. This prevents bugs the user shouldn't
+        # be able to see from being displayed.
+        self.bugs = self._getSafeBugs(bugs)
         self.branches = branches
-        self.shared_bugs_count = len(bugs)
-        self.shared_branches_count = len(branches)
+        self.shared_bugs_count = len(self.bugs)
+        self.shared_branches_count = len(self.branches)
 
     def _build_branch_template_data(self, branches):
         branch_data = []
@@ -409,16 +436,8 @@
     def _build_bug_template_data(self, bugs):
         bug_data = []
         for bug in bugs:
-            [bugtask] = [task for task in bug.bugtasks if
-                            task.target == self.pillar]
-            if bugtask is not None:
-                url = canonical_url(bugtask, path_only_if_possible=True)
-                importance = bugtask.importance.title.lower()
-            else:
-                # This shouldn't ever happen, but if it does there's no reason
-                # to crash.
-                url = canonical_url(bug, path_only_if_possible=True)
-                importance = bug.default_bugtask.importance.title.lower()
+            url = canonical_url(bug, path_only_if_possible=True)
+            importance = bug.importance.title.lower()
 
             bug_data.append(dict(
                 bug_link=url,

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-03-31 11:32:15 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-04-03 19:40:27 +0000
@@ -54,24 +54,45 @@
         if person is None:
             person = self.factory.makePerson()
         if with_sharing:
+            self._create_sharing(person)
+
+        return PillarPerson(self.pillar, person)
+
+    def _create_sharing(self, grantee, security=False):
+            if security:
+                owner = self.factory.makePerson()
+            else:
+                owner = self.pillar.owner
             if self.pillar_type == 'product':
                 bug = self.factory.makeBug(
                     product=self.pillar,
-                    owner=self.pillar.owner,
+                    owner=owner,
                     private=True)
             elif self.pillar_type == 'distribution':
                 bug = self.factory.makeBug(
                     distribution=self.pillar,
-                    owner=self.pillar.owner,
+                    owner=owner,
                     private=True)
             artifact = self.factory.makeAccessArtifact(concrete=bug)
             policy = self.factory.makeAccessPolicy(pillar=self.pillar)
             self.factory.makeAccessPolicyArtifact(
                 artifact=artifact, policy=policy)
             self.factory.makeAccessArtifactGrant(
-                artifact=artifact, grantee=person, grantor=self.pillar.owner)
-
-        return PillarPerson(self.pillar, person)
+                artifact=artifact, grantee=grantee, grantor=self.pillar.owner)
+
+    def test_view_filters_security_wisely(self):
+        # There are bugs in the sharingdetails view that not everyone with
+        # `launchpad.Driver` -- the permission level for the page -- should be
+        # able to see.
+        with FeatureFixture(DETAILS_ENABLED_FLAG):
+            pillarperson = self.getPillarPerson(with_sharing=False)
+            self._create_sharing(grantee=pillarperson.person, security=True)
+            view = create_initialized_view(pillarperson, '+index')
+            # The page loads
+            self.assertEqual(pillarperson.person.displayname, view.page_title)
+            # The bug, which is not shared with the owner, is not included.
+
+            self.assertEqual(0, view.shared_bugs_count)
 
     def test_view_traverses_plus_sharingdetails(self):
         # The traversed url in the app is pillar/+sharingdetails/person
@@ -109,6 +130,7 @@
             pillarperson = self.getPillarPerson()
             view = create_initialized_view(pillarperson, '+index')
             self.assertEqual(pillarperson.person.displayname, view.page_title)
+            self.assertEqual(1, view.shared_bugs_count)
 
 
 class TestProductSharingDetailsView(