launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/blueprint-private-traversal-deptree into lp:launchpad with lp:~jcsackett/launchpad/blueprint-private-traversal as a prerequisite.

Commit message:
Adjusts Specification:+deptree to filter for private specs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1085215 in Launchpad itself: "Specification:+deptree 403s with mixed specification information_types"
  https://bugs.launchpad.net/launchpad/+bug/1085215

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/blueprint-private-traversal-deptree/+merge/137390

Summary
=======
Much as with the SVG graph in Specification:+index, Specification:+deptree
is not privacy aware and 403s when a person who can't see a dependency or
blocked specification tries to access it. It should, as with the graph, merely
remove the specs the user doesn't have permission for.

Preimp
======
None--extension of previous work.

Implementation
==============
The deptree view has been updated to use the filtered all_deps and all_blocked
methods created in the branch this branch depends on. It has also had new
methods added, "dependencies" and "blocked_specs" that filter the attributes
of the same name from the context. As these are all used multiple times in the
template, they are cachedproperties.

The blocked_specs and dependency portlets used on this page are also adapted
to accept the view rather than the context, so that they can use the new view
methods.

Tests
=====
bin/test -vvct TestDepTree

QA
==
Ensure Specification:+deptree loads appropriately in situations of mixed
blueprint visibilities.

LoC
===
Part of private projects.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/templates/specification-deptree.pt
  lib/lp/blueprints/browser/tests/test_specificationdependency.py
  lib/lp/blueprints/doc/specgraph.txt
  lib/lp/blueprints/model/tests/test_specification.py
  lib/lp/blueprints/model/specification.py
  lib/lp/blueprints/browser/specificationdependency.py
  lib/lp/blueprints/browser/configure.zcml
  lib/lp/blueprints/browser/specification.py
  lib/lp/blueprints/interfaces/specification.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/blueprint-private-traversal-deptree/+merge/137390
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/blueprint-private-traversal-deptree into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/configure.zcml'
--- lib/lp/blueprints/browser/configure.zcml	2012-10-15 04:59:17 +0000
+++ lib/lp/blueprints/browser/configure.zcml	2012-12-01 16:07:22 +0000
@@ -268,14 +268,18 @@
             <browser:page
                 name="+listing-compact"
                 template="../templates/specification-listing-compact.pt"/>
-            <browser:page
-                name="+portlet-dependencies"
-                template="../templates/specification-portlet-dependencies.pt"/>
-            <browser:page
-                name="+portlet-blocked"
-                template="../templates/specification-portlet-blocked.pt"/>
         </browser:pages>
         <browser:page
+            for="lp.blueprints.browser.specificationdependency.SpecificationDependencyTreeView"
+            name="+portlet-dependencies"
+            template="../templates/specification-portlet-dependencies.pt"
+            permission="zope.Public"/>
+        <browser:page
+            for="lp.blueprints.browser.specificationdependency.SpecificationDependencyTreeView"
+            name="+portlet-blocked"
+            template="../templates/specification-portlet-blocked.pt"
+            permission="zope.Public"/>
+        <browser:page
             name="+secrecy"
             for="lp.blueprints.interfaces.specification.ISpecification"
             class="lp.blueprints.browser.specification.SpecificationInformationTypeEditView"

=== modified file 'lib/lp/blueprints/browser/specificationdependency.py'
--- lib/lp/blueprints/browser/specificationdependency.py	2012-01-01 02:58:52 +0000
+++ lib/lp/blueprints/browser/specificationdependency.py	2012-12-01 16:07:22 +0000
@@ -12,17 +12,21 @@
     ]
 
 from lazr.restful.interface import copy_field
+from zope.component import getUtility
 from zope.interface import Interface
 
 from lp import _
+from lp.app.enums import InformationType
 from lp.app.browser.launchpadform import (
     action,
     LaunchpadFormView,
     )
+from lp.app.interfaces.services import IService
 from lp.blueprints.interfaces.specificationdependency import (
     ISpecificationDependency,
     ISpecificationDependencyRemoval,
     )
+from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
     canonical_url,
     LaunchpadView,
@@ -92,8 +96,43 @@
 
 
 class SpecificationDependencyTreeView(LaunchpadView):
+
     label = "Blueprint dependency tree"
 
+    def __init__(self, *args, **kwargs):
+        super(SpecificationDependencyTreeView, self).__init__(*args, **kwargs)
+        self.service = getUtility(IService, 'sharing')
+
     @property
     def page_title(self):
         return self.label
+
+    @cachedproperty
+    def all_blocked(self):
+        return self.context.all_blocked(self.user)
+
+    @cachedproperty
+    def all_deps(self):
+        return self.context.all_deps(self.user)
+
+    @cachedproperty
+    def dependencies(self):
+        deps = list(self.context.dependencies)
+        if self.user:
+            (ignore, ignore, deps) = self.service.getVisibleArtifacts(
+                self.user, specifications=deps)
+        else:
+            deps = [d for d in deps if
+                d.information_type == InformationType.PUBLIC]
+        return deps
+
+    @cachedproperty
+    def blocked_specs(self):
+        blocked = list(self.context.blocked_specs)
+        if self.user:
+            (ignore, ignore, blocked) = self.service.getVisibleArtifacts(
+                self.user, specifications=blocked)
+        else:
+            blocked = [b for b in blocked if
+                b.information_type == InformationType.PUBLIC]
+        return blocked

=== modified file 'lib/lp/blueprints/browser/tests/test_specificationdependency.py'
--- lib/lp/blueprints/browser/tests/test_specificationdependency.py	2012-08-20 16:38:10 +0000
+++ lib/lp/blueprints/browser/tests/test_specificationdependency.py	2012-12-01 16:07:22 +0000
@@ -8,15 +8,21 @@
 
 __metaclass__ = type
 
+from lp.app.enums import InformationType
+from lp.registry.enums import SpecificationSharingPolicy
 from lp.services.webapp import canonical_url
 from lp.testing import (
+    anonymous_logged_in,
     BrowserTestCase,
     person_logged_in,
+    TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.views import create_view
 
 
 class TestAddDependency(BrowserTestCase):
+
     layer = DatabaseFunctionalLayer
 
     def test_add_dependency_by_url(self):
@@ -35,3 +41,76 @@
         # on ISpecification objects.
         with person_logged_in(None):
             self.assertIn(dependency, spec.dependencies)
+
+
+class TestDepTree(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_deptree_filters_dependencies(self):
+        # dep tree's blocked_specs and dependencies attributes filter
+        # blueprints the user can't see.
+        sharing_policy = SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=owner, specification_sharing_policy=sharing_policy)
+        root = self.factory.makeBlueprint(product=product)
+        proprietary_dep = self.factory.makeBlueprint(
+            product=product, information_type=InformationType.PROPRIETARY)
+        public_dep = self.factory.makeBlueprint(product=product)
+        root.createDependency(proprietary_dep)
+        root.createDependency(public_dep)
+
+        # Anonymous can see only the public
+        with anonymous_logged_in():
+            view = create_view(root, name="+deptree")
+            self.assertEqual([public_dep], view.all_deps())
+            self.assertEqual([public_dep], view.dependencies())
+
+        # The owner can see everything.
+        with person_logged_in(owner):
+            view = create_view(root, name="+deptree")
+            self.assertEqual(
+                [proprietary_dep, public_dep], view.all_deps())
+            self.assertEqual(
+                [proprietary_dep, public_dep], view.dependencies())
+
+        # A random person cannot see the propriety dep.
+        with person_logged_in(self.factory.makePerson()):
+            view = create_view(root, name="+deptree")
+            self.assertEqual([public_dep], view.all_deps())
+            self.assertEqual([public_dep], view.dependencies())
+
+    def test_deptree_filters_blocked(self):
+        # dep tree's blocked_specs and dependencies attributes filter
+        # blueprints the user can't see.
+        sharing_policy = SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=owner, specification_sharing_policy=sharing_policy)
+        root = self.factory.makeBlueprint(product=product)
+        proprietary_blocked = self.factory.makeBlueprint(
+            product=product, information_type=InformationType.PROPRIETARY)
+        public_blocked = self.factory.makeBlueprint(product=product)
+        proprietary_blocked.createDependency(root)
+        public_blocked.createDependency(root)
+
+        # Anonymous can see only the public
+        with anonymous_logged_in():
+            view = create_view(root, name="+deptree")
+            self.assertEqual([public_blocked], view.all_blocked())
+            self.assertEqual([public_blocked], view.blocked_specs())
+
+        # The owner can see everything.
+        with person_logged_in(owner):
+            view = create_view(root, name="+deptree")
+            self.assertEqual(
+                [proprietary_blocked, public_blocked], view.all_blocked())
+            self.assertEqual(
+                [proprietary_blocked, public_blocked], view.blocked_specs())
+
+        # A random person cannot see the propriety dep.
+        with person_logged_in(self.factory.makePerson()):
+            view = create_view(root, name="+deptree")
+            self.assertEqual([public_blocked], view.all_blocked())
+            self.assertEqual([public_blocked], view.blocked_specs())

=== modified file 'lib/lp/blueprints/templates/specification-deptree.pt'
--- lib/lp/blueprints/templates/specification-deptree.pt	2009-09-22 11:35:39 +0000
+++ lib/lp/blueprints/templates/specification-deptree.pt	2012-12-01 16:07:22 +0000
@@ -10,8 +10,8 @@
 <body>
 
 <div metal:fill-slot="side">
-  <div tal:replace="structure context/@@+portlet-blocked" />
-  <div tal:replace="structure context/@@+portlet-dependencies" />
+  <div tal:replace="structure view/@@+portlet-blocked" />
+  <div tal:replace="structure view/@@+portlet-dependencies" />
 </div>
 
 <div metal:fill-slot="main">
@@ -25,9 +25,9 @@
 
   <div class="portlet">
   <h2>Blueprints that must be implemented first</h2>
-    <div tal:repeat="spec context/all_deps"
+    <div tal:repeat="spec view/all_deps"
          tal:replace="structure spec/@@+listing-detailed" />
-    <p tal:condition="not: context/dependencies">
+    <p tal:condition="not: view/dependencies">
       <i>None - this blueprint does not depend on any others.</i>
     </p>
   </div>
@@ -39,9 +39,9 @@
 
   <div class="portlet">
   <h2>Blueprints that can then be implemented</h2>
-    <div tal:repeat="spec context/all_blocked"
+    <div tal:repeat="spec view/all_blocked"
          tal:replace="structure spec/@@+listing-detailed" />
-    <p tal:condition="not: context/blocked_specs">
+    <p tal:condition="not: view/blocked_specs">
       No blueprints depend on this one.
     </p>
   </div>