launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09940
[Merge] lp:~wallyworld/launchpad/view-private-stacked-branch2-393566 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/view-private-stacked-branch2-393566 into lp:launchpad.
Requested reviews:
Curtis Hovey (sinzui)
Related bugs:
Bug #393566 in Launchpad itself: "cannot view private branch if you cannot see the private branch it is stacked on"
https://bugs.launchpad.net/launchpad/+bug/393566
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/view-private-stacked-branch2-393566/+merge/115112
== Implementation ==
This is a second go at fixing this bug. The first attempt was rejected because it could be exploited.
This branch works as follows:
- when a person is subscribed to a private branch, the person is also subscribed to any private stacked on branches the person cannot access and which the subscribed_by user can edit
I had to call check_permission() from within the branch model code. check_permission calls are normally done in the view but there are other cases where this api is called from model code eg bugs.
************************************************************************************
*** What to do when a person is unsubscribed from a private branch???? ***
My first thought was to look at the private stacked on branches and revoke access for those branches the person is not subscribed to. But this is meaningless if the initial subscribe action goes ahead and subscribes the person to these stacked on branches. As far as I can tell, there's no way to easily determine which stacked on branches the person should lose access to. We can't determine which subscriptions were created as part of the initial subscribe verses those which were created explicitly.
So, the choices are:
1. Aggressively unsubscribe the person from all private stacked on branches which the unsubscribed_by user can edit and accept the person may lose access to some branches.
2. Do nothing and assume the owner of the stacked on branch data will be proactive in cleaning up subscriptions no longer required.
Thoughts?
************************************************************************************
There were a few things to do:
1. Add a getStackedOnBranches() method to Branch
2. Add a getBranchIds() method to BranchCollection. This is similar to the BugTaskSet searchBugIds() method in that it does the search but only returns the branch ids, not the hydrated branches. I also cleaned up a bit of the code so that things were less messy and common code was used to construct the core query.
3. Add a getInvisibleArtifacts() method to the sharing service (similar to the existing getVisibleArtifacts() method).
4. The branch subscribe() method takes an extra parameter 'check_stacked_visibility' defaults True. This parameter is not exposed to the web service.
== Tests ==
Basically add a few tests for each of the new methods introduced above. Also fix some existing tests in test_branchcollection where the newly visible stacked on branches needed to be added to the expected results.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/interfaces/branch.py
lib/lp/code/interfaces/branchcollection.py
lib/lp/code/model/branch.py
lib/lp/code/model/branchcollection.py
lib/lp/code/model/tests/test_branch.py
lib/lp/code/model/tests/test_branchcollection.py
lib/lp/code/model/tests/test_branchsubscription.py
lib/lp/registry/interfaces/sharingservice.py
lib/lp/registry/services/sharingservice.py
lib/lp/registry/services/tests/test_sharingservice.py
--
https://code.launchpad.net/~wallyworld/launchpad/view-private-stacked-branch2-393566/+merge/115112
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2012-07-14 07:50:13 +0000
+++ lib/lp/code/interfaces/branch.py 2012-07-16 11:16:27 +0000
@@ -641,6 +641,9 @@
def getStackedBranches():
"""The branches that are stacked on this one."""
+ def getStackedOnBranches():
+ """The branches on which this one is stacked."""
+
def getMainlineBranchRevisions(start_date, end_date=None,
oldest_first=False):
"""Return the matching mainline branch revision objects.
@@ -790,7 +793,8 @@
@export_write_operation()
@operation_for_version('beta')
def subscribe(person, notification_level, max_diff_lines,
- code_review_level, subscribed_by):
+ code_review_level, subscribed_by,
+ check_stacked_visibility=True):
"""Subscribe this person to the branch.
:param person: The `Person` to subscribe.
=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py 2012-07-14 07:50:13 +0000
+++ lib/lp/code/interfaces/branchcollection.py 2012-07-16 11:16:27 +0000
@@ -72,6 +72,9 @@
objects in the collection.
"""
+ def getBranchIds():
+ """Return a result set of all branch ids in this collection."""
+
def getMergeProposals(statuses=None, for_branches=None,
target_branch=None, eager_load=False):
"""Return a result set of merge proposals for the branches in this
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2012-07-14 07:50:13 +0000
+++ lib/lp/code/model/branch.py 2012-07-16 11:16:27 +0000
@@ -180,6 +180,7 @@
from lp.services.mail.notificationrecipientset import NotificationRecipientSet
from lp.services.propertycache import cachedproperty
from lp.services.webapp import urlappend
+from lp.services.webapp.authorization import check_permission
class Branch(SQLBase, BzrIdentityMixin):
@@ -607,6 +608,18 @@
store = Store.of(self)
return store.find(Branch, Branch.stacked_on == self)
+ def getStackedOnBranches(self):
+ """See `IBranch`."""
+ # We need to ensure we avoid being caught by accidental circular
+ # dependencies.
+ stacked_on_branches = []
+ branch = self
+ while (branch.stacked_on and
+ branch.stacked_on not in stacked_on_branches):
+ stacked_on_branches.append(branch.stacked_on)
+ branch = branch.stacked_on
+ return stacked_on_branches
+
@property
def code_is_browseable(self):
"""See `IBranch`."""
@@ -849,8 +862,13 @@
# subscriptions
def subscribe(self, person, notification_level, max_diff_lines,
- code_review_level, subscribed_by):
- """See `IBranch`."""
+ code_review_level, subscribed_by,
+ check_stacked_visibility=True):
+ """See `IBranch`.
+
+ Subscribe person to this branch and also to any editable stacked on
+ branches they cannot see.
+ """
if (person.is_team and self.information_type in
PRIVATE_INFORMATION_TYPES and person.anyone_can_join()):
raise SubscriptionPrivacyViolation(
@@ -878,6 +896,23 @@
service.ensureAccessGrants(
[person], subscribed_by, branches=[self],
ignore_permissions=True)
+
+ if not check_stacked_visibility:
+ return subscription
+
+ # We now grant access to any stacked on branches which are not
+ # currently accessible to the person but which the subscribed_by user
+ # has edit permissions for.
+ service = getUtility(IService, 'sharing')
+ ignored, invisible_stacked_branches = service.getInvisibleArtifacts(
+ person, branches=self.getStackedOnBranches())
+ editable_stacked_on_branches = [
+ branch for branch in invisible_stacked_branches
+ if check_permission('launchpad.Edit', branch)]
+ for invisible_branch in editable_stacked_on_branches:
+ invisible_branch.subscribe(
+ person, notification_level, max_diff_lines, code_review_level,
+ subscribed_by, check_stacked_visibility=False)
return subscription
def getSubscription(self, person):
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2012-07-14 07:50:13 +0000
+++ lib/lp/code/model/branchcollection.py 2012-07-16 11:16:27 +0000
@@ -142,8 +142,7 @@
def ownerCounts(self):
"""See `IBranchCollection`."""
is_team = Person.teamowner != None
- branch_owners = self._getBranchIdQuery()
- branch_owners.columns = (Branch.ownerID,)
+ branch_owners = self._getBranchSelect((Branch.ownerID,))
counts = dict(self.store.find(
(is_team, Count(Person.id)),
Person.id.is_in(branch_owners)).group_by(is_team))
@@ -200,12 +199,10 @@
asymmetric_expr,
asymmetric_tables)
- def _getBranchIdQuery(self):
- """Return a Storm 'Select' for the branch IDs in this collection."""
- branches = self.getBranches(eager_load=False)
- select = branches.get_plain_result_set()._get_select()
- select.columns = (Branch.id,)
- return select
+ def _getBranchSelect(self, columns=(Branch.id,)):
+ """Return a Storm 'Select' for columns in this collection."""
+ branches = self.getBranches(eager_load=False, find_expr=columns)
+ return branches.get_plain_result_set()._get_select()
def _getBranchExpressions(self):
"""Return the where expressions for this collection."""
@@ -291,13 +288,13 @@
cache = caches[code_import.branchID]
cache.code_import = code_import
- def getBranches(self, eager_load=False):
+ def getBranches(self, find_expr=Branch, eager_load=False):
"""See `IBranchCollection`."""
all_tables = set(
self._tables.values() + self._asymmetric_tables.values())
tables = [Branch] + list(all_tables)
expressions = self._getBranchExpressions()
- resultset = self.store.using(*tables).find(Branch, *expressions)
+ resultset = self.store.using(*tables).find(find_expr, *expressions)
def do_eager_load(rows):
branch_ids = set(branch.id for branch in rows)
@@ -317,11 +314,16 @@
set([self._user.id]))
return branch
- eager_load_hook = do_eager_load if eager_load else None
+ eager_load_hook = (
+ do_eager_load if eager_load and find_expr == Branch else None)
return DecoratedResultSet(
resultset, pre_iter_hook=eager_load_hook,
result_decorator=cache_permission)
+ def getBranchIds(self):
+ """See `IBranchCollection`."""
+ return self.getBranches(find_expr=Branch.id).get_plain_result_set()
+
def getMergeProposals(self, statuses=None, for_branches=None,
target_branch=None, merged_revnos=None,
merged_revision=None, eager_load=False):
@@ -454,8 +456,7 @@
expressions = [
CodeReviewVoteReference.reviewer == reviewer,
- BranchMergeProposal.source_branchID.is_in(
- self._getBranchIdQuery())]
+ BranchMergeProposal.source_branchID.is_in(self._getBranchSelect())]
visibility = self._getBranchVisibilityExpression()
if visibility:
expressions.append(BranchMergeProposal.target_branchID.is_in(
@@ -541,8 +542,7 @@
# BranchCollection conceptual model, but we're not quite sure how to
# fix it just yet. Perhaps when bug 337494 is fixed, we'd be able to
# sensibly be able to move this method to another utility class.
- branch_query = self._getBranchIdQuery()
- branch_query.columns = (Branch.ownerID,)
+ branch_query = self._getBranchSelect((Branch.ownerID,))
return self.store.find(
Person,
Person.id == TeamParticipation.teamID,
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2012-07-14 07:50:13 +0000
+++ lib/lp/code/model/tests/test_branch.py 2012-07-16 11:16:27 +0000
@@ -1729,6 +1729,29 @@
self.assertEqual(
set([stacked_a, stacked_b]), set(branch.getStackedBranches()))
+ def testNoBranchesStackedOn(self):
+ # getStackedBranches returns an empty collection if there are no
+ # branches stacked on it.
+ branch = self.factory.makeAnyBranch()
+ self.assertEqual(set(), set(branch.getStackedOnBranches()))
+
+ def testSingleBranchStackedOn(self):
+ # some_branch.getStackedOnBranches returns a collection of branches
+ # on which some_branch is stacked.
+ branch = self.factory.makeAnyBranch()
+ stacked_branch = self.factory.makeAnyBranch(stacked_on=branch)
+ self.assertEqual(
+ set([branch]), set(stacked_branch.getStackedOnBranches()))
+
+ def testMultipleBranchesStackedOn(self):
+ # some_branch.getStackedOnBranches returns a collection of branches
+ # on which some_branch is stacked.
+ stacked_a = self.factory.makeAnyBranch()
+ stacked_b = self.factory.makeAnyBranch(stacked_on=stacked_a)
+ branch = self.factory.makeAnyBranch(stacked_on=stacked_b)
+ self.assertEqual(
+ set([stacked_a, stacked_b]), set(branch.getStackedOnBranches()))
+
class BranchAddLandingTarget(TestCaseWithFactory):
"""Exercise all the code paths for adding a landing target."""
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2012-07-14 07:50:13 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2012-07-16 11:16:27 +0000
@@ -147,13 +147,21 @@
product=product,
information_type=InformationType.USERDATA)
someone = self.factory.makePerson()
- getUtility(IService, 'sharing').ensureAccessGrants(
- [someone], owner, branches=[branch], ignore_permissions=True)
+ with person_logged_in(owner):
+ getUtility(IService, 'sharing').ensureAccessGrants(
+ [someone], owner, branches=[branch], ignore_permissions=True)
[branch] = list(collection.visibleByUser(someone).getBranches())
with StormStatementRecorder() as recorder:
self.assertTrue(branch.visibleByUser(someone))
self.assertThat(recorder, HasQueryCount(Equals(0)))
+ def test_getBranchIds(self):
+ branch = self.factory.makeProductBranch()
+ self.factory.makeAnyBranch()
+ collection = GenericBranchCollection(
+ self.store, [Branch.product == branch.product])
+ self.assertEqual([branch.id], list(collection.getBranchIds()))
+
def test_count(self):
# The 'count' property of a collection is the number of elements in
# the collection.
@@ -650,12 +658,14 @@
self.assertEqual([self.public_branch], list(branches.getBranches()))
def test_owner_sees_own_branches(self):
- # Users can always see the branches that they own, as well as public
- # branches.
+ # Users can always see the branches that they own, those their own
+ # branches are stacked on, as well as public branches.
owner = removeSecurityProxy(self.private_branch1).owner
branches = self.all_branches.visibleByUser(owner)
self.assertEqual(
- sorted([self.public_branch, self.private_branch1]),
+ sorted([self.public_branch, self.private_branch1,
+ self.public_stacked_on_branch,
+ self.private_stacked_on_branch]),
sorted(branches.getBranches()))
def test_launchpad_services_sees_all(self):
@@ -677,7 +687,8 @@
sorted(branches.getBranches()))
def test_subscribers_can_see_branches(self):
- # A person subscribed to a branch can see it, even if it's private.
+ # A person subscribed to a branch can see it, as well as any it is
+ # stacked on, even if it's private.
subscriber = self.factory.makePerson()
removeSecurityProxy(self.private_branch1).subscribe(
subscriber, BranchSubscriptionNotificationLevel.NOEMAIL,
@@ -686,7 +697,9 @@
subscriber)
branches = self.all_branches.visibleByUser(subscriber)
self.assertEqual(
- sorted([self.public_branch, self.private_branch1]),
+ sorted([self.public_branch, self.private_branch1,
+ self.public_stacked_on_branch,
+ self.private_stacked_on_branch]),
sorted(branches.getBranches()))
def test_subscribed_team_members_can_see_branches(self):
=== modified file 'lib/lp/code/model/tests/test_branchsubscription.py'
--- lib/lp/code/model/tests/test_branchsubscription.py 2012-07-04 10:36:37 +0000
+++ lib/lp/code/model/tests/test_branchsubscription.py 2012-07-16 11:16:27 +0000
@@ -5,10 +5,14 @@
__metaclass__ = type
+
+from zope.component import getUtility
+
from lp.app.errors import (
SubscriptionPrivacyViolation,
UserCannotUnsubscribePerson,
)
+from lp.app.interfaces.services import IService
from lp.code.enums import (
BranchSubscriptionNotificationLevel,
CodeReviewNotificationLevel,
@@ -109,6 +113,80 @@
branch.unsubscribe(subscribee, owner)
self.assertFalse(branch.visibleByUser(subscribee))
+ def test_subscribe_with_editable_stacked_branch(self):
+ # Subscribing to a branch also subscribes access to any private
+ # stacked on branches which the subscribed_by person can edit.
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=owner)
+ private_stacked_on_branch = self.factory.makeBranch(
+ product=product, owner=owner,
+ information_type=InformationType.EMBARGOEDSECURITY)
+ branch = self.factory.makeBranch(
+ product=product, owner=owner, stacked_on=private_stacked_on_branch,
+ information_type=InformationType.USERDATA)
+
+ with person_logged_in(owner):
+ # Subscribe to the top level branch.
+ grantee = self.factory.makePerson()
+ branch.subscribe(
+ grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
+ None, CodeReviewNotificationLevel.NOEMAIL, owner)
+ # The stacked on branch should be visible.
+ service = getUtility(IService, 'sharing')
+ ignored, visible_branches = service.getVisibleArtifacts(
+ grantee, branches=[private_stacked_on_branch])
+ self.assertContentEqual(
+ [private_stacked_on_branch], visible_branches)
+ self.assertIn(
+ grantee, private_stacked_on_branch.subscribers)
+
+ def test_subscribe_with_non_editable_stacked_branch(self):
+ # Subscribing to a branch ignores any stacked on private branches
+ # which the subscribed_by person can not edit.
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=owner)
+ private_stacked_on_branch = self.factory.makeBranch(
+ product=product,
+ information_type=InformationType.EMBARGOEDSECURITY)
+ branch = self.factory.makeBranch(
+ product=product, owner=owner,
+ stacked_on=private_stacked_on_branch,
+ information_type=InformationType.USERDATA)
+
+ service = getUtility(IService, 'sharing')
+ with person_logged_in(owner):
+ # Subscribe to the top level branch.
+ grantee = self.factory.makePerson()
+ branch.subscribe(
+ grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
+ None, CodeReviewNotificationLevel.NOEMAIL, owner)
+ # The stacked on branch should not be visible.
+ ignored, visible_branches = service.getVisibleArtifacts(
+ grantee, branches=[private_stacked_on_branch])
+ self.assertContentEqual([], visible_branches)
+ self.assertNotIn(
+ grantee, private_stacked_on_branch.subscribers)
+
+ def test_subscribe_with_public_stacked_branch(self):
+ # Subscribing to branches does not create subscriptions for
+ # public stacked on branches.
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct(owner=owner)
+ public_stacked_on_branch = self.factory.makeBranch(product=product)
+ branch = self.factory.makeBranch(
+ product=product, owner=owner,
+ stacked_on=public_stacked_on_branch,
+ information_type=InformationType.USERDATA)
+
+ with person_logged_in(owner):
+ # Subscribe to the top level branch.
+ grantee = self.factory.makePerson()
+ branch.subscribe(
+ grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
+ None, CodeReviewNotificationLevel.NOEMAIL, owner)
+ self.assertNotIn(
+ grantee, public_stacked_on_branch.subscribers)
+
class TestBranchSubscriptionCanBeUnsubscribedbyUser(TestCaseWithFactory):
"""Tests for BranchSubscription.canBeUnsubscribedByUser."""
=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py 2012-07-16 10:25:45 +0000
+++ lib/lp/registry/interfaces/sharingservice.py 2012-07-16 11:16:27 +0000
@@ -77,6 +77,18 @@
:return: a collection of artifacts the person can see.
"""
+ def getInvisibleArtifacts(person, branches=None, bugs=None):
+ """Return the artifacts which are not shared with person.
+
+ Given lists of artifacts, return those a person does not have access to
+ either via a policy grant or artifact grant.
+
+ :param person: the person whose access is being checked.
+ :param branches: the branches to check for which a person has access.
+ :param bugs: the bugs to check for which a person has access.
+ :return: a collection of artifacts the person can not see.
+ """
+
def getPeopleWithoutAccess(concrete_artifact, people):
"""Return the people who cannot access an artifact.
=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py 2012-07-16 10:25:45 +0000
+++ lib/lp/registry/services/sharingservice.py 2012-07-16 11:16:27 +0000
@@ -158,6 +158,38 @@
return visible_bugs, visible_branches
+ def getInvisibleArtifacts(self, person, branches=None, bugs=None):
+ """See `ISharingService`."""
+ bugs_by_id = {}
+ branches_by_id = {}
+ for bug in bugs or []:
+ bugs_by_id[bug.id] = bug
+ for branch in branches or []:
+ branches_by_id[branch.id] = branch
+
+ # Load the bugs.
+ visible_bug_ids = set()
+ if bugs_by_id:
+ param = BugTaskSearchParams(
+ user=person, bug=any(*bugs_by_id.keys()))
+ visible_bug_ids = set(getUtility(IBugTaskSet).searchBugIds(param))
+ invisible_bug_ids = set(bugs_by_id.keys()).difference(visible_bug_ids)
+ invisible_bugs = [bugs_by_id[bug_id] for bug_id in invisible_bug_ids]
+
+ # Load the branches.
+ invisible_branches = []
+ if branches_by_id:
+ all_branches = getUtility(IAllBranches)
+ visible_branch_ids = all_branches.visibleByUser(person).withIds(
+ *branches_by_id.keys()).getBranchIds()
+ invisible_branch_ids = (
+ set(branches_by_id.keys()).difference(visible_branch_ids))
+ invisible_branches = [
+ branches_by_id[branch_id]
+ for branch_id in invisible_branch_ids]
+
+ return invisible_bugs, invisible_branches
+
def getPeopleWithoutAccess(self, concrete_artifact, people):
"""See `ISharingService`."""
# Public artifacts allow everyone to have access.
=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py 2012-07-16 10:25:45 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py 2012-07-16 11:16:27 +0000
@@ -1155,8 +1155,8 @@
without_access = self.service.getPeopleWithoutAccess(bug, people)
self.assertContentEqual(people[:5], without_access)
- def test_getVisibleArtifacts(self):
- # Test the getVisibleArtifacts method.
+ def _make_Artifacts(self):
+ # Make artifacts for test (in)visible artifact methods.
owner = self.factory.makePerson()
product = self.factory.makeProduct(owner=owner)
grantee = self.factory.makePerson()
@@ -1187,23 +1187,26 @@
grant_access(bug)
for branch in branches[:5]:
grant_access(branch)
- # XXX bug=1001042 wallyworld 2012-05-18
- # for now we need to subscribe users to the branch in order
- # for the underlying BranchCollection to allow access. This will
- # no longer be the case when BranchCollection supports the new
- # access policy framework.
- branch.subscribe(
- grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
- BranchSubscriptionDiffSize.NODIFF,
- CodeReviewNotificationLevel.NOEMAIL,
- owner)
+ return grantee, branches, bugs
+ def test_getVisibleArtifacts(self):
+ # Test the getVisibleArtifacts method.
+ grantee, branches, bugs = self._make_Artifacts()
# Check the results.
shared_bugs, shared_branches = self.service.getVisibleArtifacts(
grantee, branches, bugs)
self.assertContentEqual(bugs[:5], shared_bugs)
self.assertContentEqual(branches[:5], shared_branches)
+ def test_getInvisibleArtifacts(self):
+ # Test the getInvisibleArtifacts method.
+ grantee, branches, bugs = self._make_Artifacts()
+ # Check the results.
+ not_shared_bugs, not_shared_branches = (
+ self.service.getInvisibleArtifacts(grantee, branches, bugs))
+ self.assertContentEqual(bugs[5:], not_shared_bugs)
+ self.assertContentEqual(branches[5:], not_shared_branches)
+
def _assert_getVisibleArtifacts_bug_change(self, change_callback):
# Test the getVisibleArtifacts method excludes bugs after a change of
# information_type or bugtask re-targetting.
Follow ups
