launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14655
[Merge] lp:~jcsackett/launchpad/blueprint-private-traversal into lp:launchpad
j.c.sackett has proposed merging lp:~jcsackett/launchpad/blueprint-private-traversal into lp:launchpad with lp:~jcsackett/launchpad/just-use-all_deps-and-all_blocked as a prerequisite.
Commit message:
Strips nonpublic specifications from the dependency graph on Specification:+index if the user doesn't have permission to see them.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1081773 in Launchpad itself: "Unauthorized on public blueprint which is a dependancy to an embargoed one"
https://bugs.launchpad.net/launchpad/+bug/1081773
For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/blueprint-private-traversal/+merge/137330
Summary
=======
Specifications can have dependencies with different permissions (either
information_type or grants) than the root node; likewise, it can block
specifications with the same conditions.
In this situation the page currently 403s. Per our usual rules we want to show
the page without the information a user shouldn't see.
Preimp
======
Spoke with Orange Squad and William Grant.
Implementation
==============
The SpecGraph is updated to use all_deps and all_blocked; these are easy to
modify to filter out blueprints the user can't see.
Those two properties are converted to methods that take the user as a
parameter, which is then used with the sharing service to modify the results
of the query to only include the ones the user can see.
The SpecGraph object is modified to properly use the all_deps/all_blocked
methods, and to recieve the user as an argument to __init__.
Tests
=====
bin/test -vvc -t TestSpecificationDependencies -t specgraph
QA
==
Follow the setup in the bug; you should see the page without a 403, and
without see nonpublic data that you have no grant for.
LoC
===
Part of private projects.
Lint
====
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/blueprints/doc/specgraph.txt
lib/lp/blueprints/model/tests/test_specification.py
lib/lp/blueprints/model/specification.py
lib/lp/blueprints/browser/specification.py
lib/lp/blueprints/interfaces/specification.py
--
https://code.launchpad.net/~jcsackett/launchpad/blueprint-private-traversal/+merge/137330
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/blueprint-private-traversal into lp:launchpad.
=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py 2012-11-30 21:39:22 +0000
+++ lib/lp/blueprints/browser/specification.py 2012-11-30 21:39:22 +0000
@@ -1112,10 +1112,11 @@
# containing one '%s' replacement marker.
url_pattern_for_testing = None
- def __init__(self):
+ def __init__(self, user=None):
self.nodes = set()
self.edges = set()
self.root_node = None
+ self.user = user
def newNode(self, spec, root=False):
"""Return a new node based on the given spec.
@@ -1166,7 +1167,8 @@
"""Add nodes for the specs that the given spec depends on,
transitively.
"""
- get_related_specs_fn = attrgetter('all_deps')
+ def get_related_specs_fn(spec):
+ return spec.all_deps(user=self.user)
def link_nodes_fn(node, dependency):
self.link(dependency, node)
@@ -1174,7 +1176,8 @@
def addBlockedNodes(self, spec):
"""Add nodes for specs that the given spec blocks, transitively."""
- get_related_specs_fn = attrgetter('all_blocked')
+ def get_related_specs_fn(spec):
+ return spec.all_blocked(user=self.user)
def link_nodes_fn(node, blocked_spec):
self.link(node, blocked_spec)
@@ -1197,10 +1200,7 @@
current_spec = to_search.pop()
visited.add(current_spec)
node = self.newOrExistingNode(current_spec)
- try:
- related_specs = set(get_related_specs_fn(current_spec))
- except AttributeError:
- import pdb; pdb.set_trace()
+ related_specs = set(get_related_specs_fn(current_spec))
for related_spec in related_specs:
link_nodes_fn(node, self.newOrExistingNode(related_spec))
to_search.update(related_specs.difference(visited))
@@ -1408,7 +1408,7 @@
def makeSpecGraph(self):
"""Return a SpecGraph object rooted on the spec that is self.context.
"""
- graph = SpecGraph()
+ graph = SpecGraph(self.user)
graph.newNode(self.context, root=True)
graph.addDependencyNodes(self.context)
graph.addBlockedNodes(self.context)
=== modified file 'lib/lp/blueprints/doc/specgraph.txt'
--- lib/lp/blueprints/doc/specgraph.txt 2012-11-30 21:39:22 +0000
+++ lib/lp/blueprints/doc/specgraph.txt 2012-11-30 21:39:22 +0000
@@ -27,12 +27,16 @@
... self.title = title or name
... self.is_complete = is_complete
... self.assignee = assignee
- ... # Use lists here to ensure that the code converts them
- ... # to sets explicitly, like it has to do for SelectResults.
- ... self.all_deps = []
- ... # This is a hack for testing: we can set up dependencies,
- ... # and simply use their "mirror image" for blocked specs.
- ... self.all_blocked = self.all_deps
+ ... # For purposes of this doc, we're only stubbing the
+ ... # dependency methods.
+ ... self._all_deps = []
+ ... self._all_blocked = self._all_deps
+ ...
+ ... def all_deps(self, user):
+ ... return self._all_deps
+ ...
+ ... def all_blocked(self, user):
+ ... return self._all_blocked
>>> foo = Spec('foo', title='something with " and \n in it')
>>> root = g.newNode(foo, root=True)
@@ -70,16 +74,16 @@
<fnord-foo>:
>>> foo1 = Spec('foo1')
- >>> foo.all_deps.append(foo1)
+ >>> foo._all_deps.append(foo1)
>>> foo2 = Spec('foo2')
- >>> foo.all_deps.append(foo2)
+ >>> foo._all_deps.append(foo2)
>>> foo11 = Spec('foo11')
- >>> foo1.all_deps.append(foo11)
+ >>> foo1._all_deps.append(foo11)
>>> foo111 = Spec('foo111')
- >>> foo11.all_deps.append(foo111)
+ >>> foo11._all_deps.append(foo111)
>>> def make_graph(dependency, blocked):
- ... g = SpecGraph()
+ ... g = SpecGraph(user=None)
... g.url_pattern_for_testing = 'http://whatever/%s'
... g.newNode(foo, root=True)
... if dependency:
@@ -175,11 +179,11 @@
The graph grows when specifications gain more dependencies.
- >>> foo1.all_deps.append(foo)
- >>> foo111.all_deps.append(foo1)
- >>> foo111.all_deps.append(foo)
- >>> foo2.all_deps.append(foo1)
- >>> foo1.all_deps.append(foo2)
+ >>> foo1._all_deps.append(foo)
+ >>> foo111._all_deps.append(foo1)
+ >>> foo111._all_deps.append(foo)
+ >>> foo2._all_deps.append(foo1)
+ >>> foo1._all_deps.append(foo2)
>>> print_graph()
Root is <fnord-foo>
<fnord-foo>:
=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py 2012-11-26 08:40:20 +0000
+++ lib/lp/blueprints/interfaces/specification.py 2012-11-30 21:39:22 +0000
@@ -404,10 +404,6 @@
readonly=True),
as_of="devel")
blocked_specs = Attribute('Specs for which this spec is a dependency.')
- all_deps = Attribute(
- "All the dependencies, including dependencies of dependencies.")
- all_blocked = Attribute(
- "All specs blocked on this, and those blocked on the blocked ones.")
linked_branches = exported(
CollectionField(
title=_("Branches associated with this spec, usually "
@@ -453,6 +449,18 @@
readonly=True),
as_of="devel")
+ def all_deps():
+ """All the dependencies, including dependencies of dependencies.
+
+ If a user is provided, filters to only dependencies the user can see.
+ """
+ def all_blocked():
+ """All specs blocked on this, and those blocked on the blocked ones.
+
+ If a user is provided, filters to only blocked dependencies the user
+ can see.
+ """
+
def validateMove(target):
"""Check that the specification can be moved to the target."""
=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py 2012-11-28 19:45:50 +0000
+++ lib/lp/blueprints/model/specification.py 2012-11-30 21:39:22 +0000
@@ -89,8 +89,8 @@
from lp.registry.enums import SpecificationSharingPolicy
from lp.registry.errors import CannotChangeInformationType
from lp.registry.interfaces.accesspolicy import (
+ IAccessArtifactSource,
IAccessArtifactGrantSource,
- IAccessArtifactSource,
)
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.distroseries import IDistroSeries
@@ -822,25 +822,53 @@
SpecificationDependency.delete(deplink.id)
return deplink
- @property
- def all_deps(self):
- return Store.of(self).with_(
+ def all_deps(self, user=None):
+ public_clause = True
+ if user is None:
+ public_clause = (
+ Specification.information_type == InformationType.PUBLIC,
+ )
+
+ results = Store.of(self).with_(
SQL(recursive_dependent_query(self))).find(
Specification,
Specification.id != self.id,
- SQL('Specification.id in (select id from dependencies)')
+ SQL('Specification.id in (select id from dependencies)'),
+ public_clause,
).order_by(Specification.name, Specification.id)
- @property
- def all_blocked(self):
+ results = list(results)
+
+ if user:
+ service = getUtility(IService, 'sharing')
+ (ignore, ignore, results) = service.getVisibleArtifacts(
+ user, specifications=results)
+ return results
+
+ def all_blocked(self, user=None):
"""See `ISpecification`."""
- return Store.of(self).with_(
+ public_clause = True
+ if user is None:
+ public_clause = (
+ Specification.information_type == InformationType.PUBLIC,
+ )
+
+ results = Store.of(self).with_(
SQL(recursive_blocked_query(self))).find(
Specification,
Specification.id != self.id,
- SQL('Specification.id in (select id from blocked)')
+ SQL('Specification.id in (select id from blocked)'),
+ public_clause,
).order_by(Specification.name, Specification.id)
+ results = list(results)
+
+ if user:
+ service = getUtility(IService, 'sharing')
+ (ignore, ignore, results) = service.getVisibleArtifacts(
+ user, specifications=results)
+ return results
+
# branches
def getBranchLink(self, branch):
return SpecificationBranch.selectOneBy(
=== modified file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py 2012-11-26 08:33:03 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py 2012-11-30 21:39:22 +0000
@@ -53,18 +53,18 @@
def test_no_deps(self):
blueprint = self.factory.makeBlueprint()
self.assertThat(list(blueprint.dependencies), Equals([]))
- self.assertThat(list(blueprint.all_deps), Equals([]))
+ self.assertThat(list(blueprint.all_deps()), Equals([]))
self.assertThat(list(blueprint.blocked_specs), Equals([]))
- self.assertThat(list(blueprint.all_blocked), Equals([]))
+ self.assertThat(list(blueprint.all_blocked()), Equals([]))
def test_single_dependency(self):
do_first = self.factory.makeBlueprint()
do_next = self.factory.makeBlueprint()
do_next.createDependency(do_first)
self.assertThat(list(do_first.blocked_specs), Equals([do_next]))
- self.assertThat(list(do_first.all_blocked), Equals([do_next]))
+ self.assertThat(list(do_first.all_blocked()), Equals([do_next]))
self.assertThat(list(do_next.dependencies), Equals([do_first]))
- self.assertThat(list(do_next.all_deps), Equals([do_first]))
+ self.assertThat(list(do_next.all_deps()), Equals([do_first]))
def test_linear_dependency(self):
do_first = self.factory.makeBlueprint()
@@ -74,11 +74,11 @@
do_last.createDependency(do_next)
self.assertThat(sorted(do_first.blocked_specs), Equals([do_next]))
self.assertThat(
- sorted(do_first.all_blocked),
+ sorted(do_first.all_blocked()),
Equals(sorted([do_next, do_last])))
self.assertThat(sorted(do_last.dependencies), Equals([do_next]))
self.assertThat(
- sorted(do_last.all_deps),
+ sorted(do_last.all_deps()),
Equals(sorted([do_first, do_next])))
def test_diamond_dependency(self):
@@ -99,15 +99,62 @@
sorted(do_first.blocked_specs),
Equals(sorted([do_next_lhs, do_next_rhs])))
self.assertThat(
- sorted(do_first.all_blocked),
+ sorted(do_first.all_blocked()),
Equals(sorted([do_next_lhs, do_next_rhs, do_last])))
self.assertThat(
sorted(do_last.dependencies),
Equals(sorted([do_next_lhs, do_next_rhs])))
self.assertThat(
- sorted(do_last.all_deps),
+ sorted(do_last.all_deps()),
Equals(sorted([do_first, do_next_lhs, do_next_rhs])))
+ def test_all_deps_filters(self):
+ # all_deps, when provided a user, shows only the dependencies the user
+ # can 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 (no user) requests only get public dependencies
+ self.assertEqual([public_dep], root.all_deps())
+ # The owner of the product can see everything.
+ self.assertEqual(
+ [proprietary_dep, public_dep], root.all_deps(user=owner))
+ # A random person can't see the proprietary dependency.
+ self.assertEqual(
+ [public_dep], root.all_deps(user=self.factory.makePerson()))
+
+ def test_all_blocked_filters(self):
+ # all_blocked, when provided a user, shows only the blocked specs the
+ # user can 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 (no user) requests only get public blocked specs.
+ self.assertEqual(
+ [public_blocked], root.all_blocked())
+ # The owner of the product can see everything.
+ self.assertEqual(
+ [proprietary_blocked, public_blocked],
+ root.all_blocked(user=owner))
+ # A random person can't see the proprietary blocked spec.
+ self.assertEqual(
+ [public_blocked],
+ root.all_blocked(user=self.factory.makePerson()))
+
class TestSpecificationSubscriptionSort(TestCaseWithFactory):
