Merge lp:~jcsackett/launchpad/bad-bugtasks-how-dare-you-break-details into lp:launchpad

[j.c.sackett]

Filters out bugs the user doesn't have authorization to see in the sharing details view.

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

[Steve Kowalik (stevenk)]

This looks like great work, my only concerns are the imports you've added on lines 11 to 17 of the diff -- I'd like to see a comment explaining circular imports, and I really don't think the zope import belongs there at all. Please try and move the imports if you can, but I won't block on it.

review: Approve (code)

[j.c.sackett (jcsackett)]

Oh lord, those were put there as expedient with the notion I'd move them; I don't believe it's a circular issue at all, just a miss on my part. I'll move those before landing, thanks.

=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py	2012-04-05 18:47:58 +0000
+++ lib/lp/registry/browser/pillar.py	2012-04-09 20:45:27 +0000
@@ -44,13 +44,23 @@
     StructuralSubscriptionMenuMixin,
     )
 from lp.bugs.interfaces.bug import IBug
+from lp.bugs.interfaces.bugtask import (
+    BugTaskSearchParams,
+    IBugTaskSet,
+    )
 from lp.code.interfaces.branch import IBranch
+from lp.registry.interfaces.accesspolicy import (
+    IAccessPolicyGrantFlatSource,
+    IAccessPolicySource,
+    )
+from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
     )
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pillar import IPillar
+from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.model.pillar import PillarPerson
 from lp.services.config import config
@@ -400,21 +410,43 @@
         cache.objects['sharing_write_enabled'] = (write_flag_enabled
             and check_permission('launchpad.Edit', self.pillar))
 
+    def _getSafeBugs(self, bugs):
+        """Uses the bugsearch tools to safely get the list of bugs the user is
+        allowed to see."""
+        if bugs == set([]):
+            return []
+        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, request):
         branch_data = []
@@ -426,26 +458,17 @@
                 branch_id=branch.id))
         return branch_data
 
-    def _build_bug_template_data(self, bugs, request):
+    def _build_bug_template_data(self, bugtasks, request):
         bug_data = []
-        for bug in bugs:
-            [bugtask] = [task for task in bug.bugtasks if
-                            task.target == self.pillar]
-            if bugtask is not None:
-                web_link = canonical_url(bugtask, path_only_if_possible=True)
-                self_link = absoluteURL(bug, request)
-                importance = bugtask.importance.title.lower()
-            else:
-                # This shouldn't ever happen, but if it does there's no reason
-                # to crash.
-                web_link = canonical_url(bug, path_only_if_possible=True)
-                self_link = absoluteURL(bug, request)
-                importance = bug.default_bugtask.importance.title.lower()
+        for bugtask in bugtasks:
+            web_link = canonical_url(bugtask, path_only_if_possible=True)
+            self_link = absoluteURL(bugtask.bug, request)
+            importance = bugtask.importance.title.lower()
 
             bug_data.append(dict(
                 self_link=self_link,
                 web_link=web_link,
-                bug_summary=bug.title,
-                bug_id=bug.id,
+                bug_summary=bugtask.bug.title,
+                bug_id=bugtask.bug.id,
                 bug_importance=importance))
         return bug_data

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-04-05 18:47:58 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-04-09 20:45:27 +0000
@@ -54,14 +54,15 @@
 
     layer = DatabaseFunctionalLayer
 
-    def getPillarPerson(self, person=None, with_sharing=True):
-        if person is None:
-            person = self.factory.makePerson()
-        if with_sharing:
+    def _create_sharing(self, grantee, security=False):
+            if security:
+                owner = self.factory.makePerson()
+            else:
+                owner = self.pillar.owner
             if self.pillar_type == 'product':
                 self.bug = self.factory.makeBug(
                     product=self.pillar,
-                    owner=self.pillar.owner,
+                    owner=owner,
                     private=True)
                 self.branch = self.factory.makeBranch(
                     product=self.pillar,
@@ -71,25 +72,46 @@
                 self.branch = None
                 self.bug = self.factory.makeBug(
                     distribution=self.pillar,
-                    owner=self.pillar.owner,
+                    owner=owner,
                     private=True)
             artifact = self.factory.makeAccessArtifact(concrete=self.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)
+                artifact=artifact, grantee=grantee, grantor=self.pillar.owner)
+
             if self.branch:
                 artifact = self.factory.makeAccessArtifact(
                     concrete=self.branch)
                 self.factory.makeAccessPolicyArtifact(
                     artifact=artifact, policy=policy)
                 self.factory.makeAccessArtifactGrant(
-                    artifact=artifact, grantee=person,
+                    artifact=artifact, grantee=grantee,
                     grantor=self.pillar.owner)
 
+    def getPillarPerson(self, person=None, with_sharing=True):
+        if person is None:
+            person = self.factory.makePerson()
+        if with_sharing:
+            self._create_sharing(person)
+
         return PillarPerson(self.pillar, person)
 
+    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/+sharing/person
         with FeatureFixture(DETAILS_ENABLED_FLAG):
@@ -130,6 +152,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)
 
     def test_view_data_model(self):
         # Test that the json request cache contains the view data model.