launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09581
[Merge] lp:~stevenk/launchpad/new-branch-visibility into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/new-branch-visibility into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/new-branch-visibility/+merge/113484
Switch IBranch.visibleByUser() and IBranchCollection.visibleByUser() (sort of), to make use of a new function get_branch_privacy_filter, which backs onto AccessPolicyGrant and AccessArtifactGrant rather than relying on subscriptions to the branch.
--
https://code.launchpad.net/~stevenk/launchpad/new-branch-visibility/+merge/113484
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/new-branch-visibility into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2012-07-04 22:38:14 +0000
+++ database/schema/security.cfg 2012-07-05 05:11:24 +0000
@@ -693,6 +693,7 @@
public.accessartifact = SELECT, INSERT, DELETE
public.accesspolicy = SELECT
public.accesspolicyartifact = SELECT, INSERT, DELETE
+public.accesspolicygrant = SELECT
public.branch = SELECT, INSERT, UPDATE
public.branchrevision = SELECT, INSERT
public.branchsubscription = SELECT, INSERT
=== modified file 'lib/lp/code/browser/tests/test_product.py'
--- lib/lp/code/browser/tests/test_product.py 2012-05-31 03:54:13 +0000
+++ lib/lp/code/browser/tests/test_product.py 2012-07-05 05:11:24 +0000
@@ -112,15 +112,16 @@
def test_initial_branches_contains_dev_focus_branch(self):
product, branch = self.makeProductAndDevelopmentFocusBranch()
- view = create_initialized_view(product, '+code-index',
- rootsite='code')
+ view = create_initialized_view(
+ product, '+code-index', rootsite='code')
self.assertIn(branch, view.initial_branches)
def test_initial_branches_does_not_contain_private_dev_focus_branch(self):
product, branch = self.makeProductAndDevelopmentFocusBranch(
information_type=InformationType.USERDATA)
- view = create_initialized_view(product, '+code-index',
- rootsite='code')
+ login(ANONYMOUS)
+ view = create_initialized_view(
+ product, '+code-index', rootsite='code')
self.assertNotIn(branch, view.initial_branches)
def test_committer_count_with_revision_authors(self):
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2012-06-27 22:31:38 +0000
+++ lib/lp/code/model/branch.py 2012-07-05 05:11:24 +0000
@@ -7,6 +7,7 @@
__all__ = [
'Branch',
'BranchSet',
+ 'get_branch_privacy_filter',
]
from datetime import datetime
@@ -25,9 +26,11 @@
)
from storm.expr import (
And,
+ Coalesce,
Count,
Desc,
Insert,
+ Join,
NamedFunc,
Not,
Or,
@@ -36,6 +39,7 @@
from storm.locals import (
AutoReload,
Int,
+ List,
Reference,
)
from storm.store import Store
@@ -145,7 +149,11 @@
validate_person,
validate_public_person,
)
-from lp.registry.model.accesspolicy import reconcile_access_for_artifact
+from lp.registry.model.accesspolicy import (
+ AccessPolicyGrant,
+ reconcile_access_for_artifact,
+ )
+from lp.registry.model.teammembership import TeamParticipation
from lp.services.config import config
from lp.services.database.bulk import load_related
from lp.services.database.constants import (
@@ -160,6 +168,10 @@
SQLBase,
sqlvalues,
)
+from lp.services.database.stormexpr import (
+ ArrayAgg,
+ ArrayIntersects,
+ )
from lp.services.helpers import shortlist
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
@@ -188,6 +200,8 @@
mirror_status_message = StringCol(default=None)
information_type = EnumCol(
enum=InformationType, default=InformationType.PUBLIC)
+ access_policy = IntCol()
+ access_grants = List(type=Int())
# These can die after the UI is dropped.
@property
@@ -243,6 +257,15 @@
raise BranchCannotBePublic()
self.information_type = information_type
self._reconcileAccess()
+ if information_type in PRIVATE_INFORMATION_TYPES:
+ # Grant the subscriber access if they can't see the branch.
+ service = getUtility(IService, 'sharing')
+ blind_subscribers = service.getPeopleWithoutAccess(
+ self, self.subscribers)
+ if len(blind_subscribers):
+ service.ensureAccessGrants(
+ blind_subscribers, who, branches=[self],
+ ignore_permissions=True)
registrant = ForeignKey(
dbName='registrant', foreignKey='Person',
@@ -1267,13 +1290,12 @@
return True
if user is None:
return False
- if user.inTeam(self.owner):
+ if user.inTeam(self.owner) or user_has_special_branch_access(user):
return True
- for subscriber in self.subscribers:
- if user.inTeam(subscriber):
- return True
- return user_has_special_branch_access(user)
-
+ return not Store.of(self).find(
+ Branch, Branch.id == self.id,
+ get_branch_privacy_filter(user)).is_empty()
+
def visibleByUser(self, user, checked_branches=None):
"""See `IBranch`."""
if checked_branches is None:
@@ -1518,3 +1540,32 @@
"""
update_trigger_modified_fields(branch)
send_branch_modified_notifications(branch, event)
+
+
+def get_branch_privacy_filter(user, branch_class=Branch):
+ public_branch_filter = (
+ branch_class.information_type.is_in(PUBLIC_INFORMATION_TYPES))
+
+ if user is None:
+ return [public_branch_filter]
+
+ artifact_grant_query = Coalesce(
+ ArrayIntersects(branch_class.access_grants,
+ Select(
+ ArrayAgg(TeamParticipation.teamID),
+ tables=TeamParticipation,
+ where=(TeamParticipation.person == user)
+ )), False)
+
+ policy_grant_query = branch_class.access_policy.is_in(
+ Select(
+ AccessPolicyGrant.policy_id,
+ tables=(AccessPolicyGrant,
+ Join(TeamParticipation,
+ TeamParticipation.teamID ==
+ AccessPolicyGrant.grantee_id)),
+ where=(TeamParticipation.person == user)
+ ))
+
+ return [
+ Or(public_branch_filter, artifact_grant_query, policy_grant_query)]
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2012-06-06 06:56:17 +0000
+++ lib/lp/code/model/branchcollection.py 2012-07-05 05:11:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Implementations of `IBranchCollection`."""
@@ -49,7 +49,10 @@
from lp.code.interfaces.seriessourcepackagebranch import (
IFindOfficialBranchLinks,
)
-from lp.code.model.branch import Branch
+from lp.code.model.branch import (
+ Branch,
+ get_branch_privacy_filter,
+ )
from lp.code.model.branchmergeproposal import BranchMergeProposal
from lp.code.model.branchsubscription import BranchSubscription
from lp.code.model.codeimport import CodeImport
@@ -787,7 +790,6 @@
asymmetric_filter_expressions=asymmetric_filter_expressions,
asymmetric_tables=asymmetric_tables)
self._user = user
- self._private_branch_ids = self._getPrivateBranchSubQuery()
def _filterBy(self, expressions, table=None, join=None,
exclude_from_search=None, symmetric=True):
@@ -829,57 +831,14 @@
asymmetric_expr,
asymmetric_tables)
- def _getPrivateBranchSubQuery(self):
- """Return a subquery to get the private branches the user can see.
-
- If the user is None (which is used for anonymous access), then there
- is no subquery. Otherwise return the branch ids for the private
- branches that the user owns or is subscribed to.
- """
- # Everyone can see public branches.
- person = self._user
- if person is None:
- # Anonymous users can only see the public branches.
- return None
-
- # A union is used here rather than the more simplistic simple joins
- # due to the query plans generated. If we just have a simple query
- # then we are joining across TeamParticipation and BranchSubscription.
- # This creates a bad plan, hence the use of a union.
- private_branches = Union(
- # Private branches the person owns (or a team the person is in).
- Select(Branch.id,
- And(Branch.owner == TeamParticipation.teamID,
- TeamParticipation.person == person,
- Branch.information_type.is_in(
- PRIVATE_INFORMATION_TYPES))),
- # Private branches the person is subscribed to, either directly or
- # indirectly.
- Select(Branch.id,
- And(BranchSubscription.branch == Branch.id,
- BranchSubscription.person ==
- TeamParticipation.teamID,
- TeamParticipation.person == person,
- Branch.information_type.is_in(
- PRIVATE_INFORMATION_TYPES))))
- return private_branches
-
def _getBranchVisibilityExpression(self, branch_class=Branch):
"""Return the where clauses for visibility.
:param branch_class: The Branch class to use - permits using
ClassAliases.
"""
- public_branches = branch_class.information_type.is_in(
- PUBLIC_INFORMATION_TYPES)
- if self._private_branch_ids is None:
- # Public only.
- return [public_branches]
- else:
- public_or_private = Or(
- public_branches,
- branch_class.id.is_in(self._private_branch_ids))
- return [public_or_private]
+ return get_branch_privacy_filter(
+ self._user, branch_class=branch_class)
def _getCandidateBranchesWith(self):
"""Return WITH clauses defining candidate branches.
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2012-06-11 10:25:46 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2012-07-05 05:11:24 +0000
@@ -636,20 +636,6 @@
sorted([self.public_branch, self.private_branch1]),
sorted(branches.getBranches()))
- def test_owner_member_sees_own_branches(self):
- # Members of teams that own branches can see branches owned by those
- # teams, as well as public branches.
- team_owner = self.factory.makePerson()
- team = self.factory.makeTeam(team_owner)
- private_branch = self.factory.makeAnyBranch(
- owner=team, stacked_on=self.private_stacked_on_branch,
- name='team')
- removeSecurityProxy(private_branch).unsubscribe(team, team_owner)
- branches = self.all_branches.visibleByUser(team_owner)
- self.assertEqual(
- sorted([self.public_branch, private_branch]),
- sorted(branches.getBranches()))
-
def test_launchpad_services_sees_all(self):
# The LAUNCHPAD_SERVICES special user sees *everything*.
branches = self.all_branches.visibleByUser(LAUNCHPAD_SERVICES)
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2012-06-11 10:25:46 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2012-07-05 05:11:24 +0000
@@ -967,13 +967,13 @@
CodeReviewNotificationLevel.FULL, charlie)
# Make both branches private.
for branch in (bmp.source_branch, bmp.target_branch):
- removeSecurityProxy(branch).information_type = (
- InformationType.USERDATA)
+ branch.transitionToInformationType(
+ InformationType.USERDATA, branch.owner, verify_policy=False)
recipients = bmp.getNotificationRecipients(
CodeReviewNotificationLevel.FULL)
- self.assertFalse(bob in recipients)
- self.assertFalse(eric in recipients)
- self.assertTrue(charlie in recipients)
+ self.assertNotIn(bob, recipients)
+ self.assertNotIn(eric, recipients)
+ self.assertIn(charlie, recipients)
class TestGetAddress(TestCaseWithFactory):
=== modified file 'lib/lp/code/model/tests/test_branchsubscription.py'
--- lib/lp/code/model/tests/test_branchsubscription.py 2012-06-27 22:31:38 +0000
+++ lib/lp/code/model/tests/test_branchsubscription.py 2012-07-05 05:11:24 +0000
@@ -95,7 +95,7 @@
None, CodeReviewNotificationLevel.NOEMAIL, owner)
self.assertTrue(branch.visibleByUser(subscribee))
- def test_unsubscribe_gives_access(self):
+ def test_unsubscribe_removes_access(self):
"""Unsubscibing a user to a branch removes their access."""
owner = self.factory.makePerson()
branch = self.factory.makeBranch(
=== modified file 'lib/lp/code/stories/feeds/xx-branch-atom.txt'
--- lib/lp/code/stories/feeds/xx-branch-atom.txt 2011-12-30 06:14:56 +0000
+++ lib/lp/code/stories/feeds/xx-branch-atom.txt 2012-07-05 05:11:24 +0000
@@ -102,8 +102,7 @@
>>> from zope.component import getUtility
>>> from zope.security.proxy import removeSecurityProxy
>>> from lp.code.model.branch import Branch
- >>> from lp.code.interfaces.branchcollection import (
- ... IAllBranches)
+ >>> from lp.code.interfaces.branchcollection import IAllBranches
>>> from lp.registry.interfaces.person import IPersonSet
>>> login(ANONYMOUS)
>>> person_set = getUtility(IPersonSet)
=== modified file 'lib/lp/security.py'
--- lib/lp/security.py 2012-06-28 01:14:33 +0000
+++ lib/lp/security.py 2012-07-05 05:11:24 +0000
@@ -2022,8 +2022,8 @@
"""Controls visibility of branches.
A person can see the branch if the branch is public, they are the owner
- of the branch, they are in the team that owns the branch, subscribed to
- the branch, or a launchpad administrator.
+ of the branch, they are in the team that owns the branch, they have an
+ access grant to the branch, or a launchpad administrator.
"""
permission = 'launchpad.View'
usedfor = IBranch
Follow ups
