launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #11195
[Merge] lp:~wallyworld/launchpad/private-teams-junk-branches-1024990 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/private-teams-junk-branches-1024990 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1024990 in Launchpad itself: "private teams cannot access their +junk branches without a subscription"
https://bugs.launchpad.net/launchpad/+bug/1024990
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/private-teams-junk-branches-1024990/+merge/120691
== Implementation ==
Teams which have private junk branches can see the branches because a subscription is created for branch owners. But if the subscription goes away, the branch becomes invisible.
This branch solves the problem by introducing personal access policies. The work requires that https://code.launchpad.net/~wallyworld/launchpad/accesspolicy-private-teams-1024990/+merge/120507 is approved and landed first, since that mp contains the required schema changes.
Access polices are extended to have a person attribute. Currently, only private teams have such policies created. The general workflow is:
1. When a team is made private, a new personal access policy is created, and also a corresponding access policy grant for the team. This is done in Person.transitionVisibility().
Theoretically, the above approach allows others to be granted access to any private junk branches, but we don't support that yet.
2. Each time a private branch is created, or the target changes, or the information type changes, a check is done to see if the branch is a private personal branch. If so, an access policy artifact is set up to link the branch to the access policy, conferring access to the branch owner. If not, any existing APAs are removed.
Only private teams are supposed to have private junk branches. But we never enforced this and it could be circumvented vis the API. So I added a check to Branch.setTarget() to enforce this rule.
I considered doing the required access policy/grant work in namespace.createBranch(), but I liked the idea of doing the logic in the place closest to the relevant business rules ie AP, APG in transitionVisibility; APA in reconcileAccess.
New methods were added to IAccessPolicySource:
- createForTeams
- findByTeam
These are analogous to the pillar equivalents, but work with teams to create/find personal access policies.
== Tests ==
Add tests to test_accesspolicy for the new IAccessPolicySource methods.
Add tests to test_person to ensure that personal access policies are only created for private teams.
Add test to test_branch to check that reconcileAccess creates the expected APA.
Add test to test_branch to ensure that only private teams can have private junk branches.
Add test to test_branch to ensure that the expected APA is created when a branch is converted from a product branch to a junk branch.
Add test to test_branchcollection to ensure an unsubscribed private team can see their own junk branches.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/model/branch.py
lib/lp/code/model/tests/test_branch.py
lib/lp/code/model/tests/test_branchcollection.py
lib/lp/registry/interfaces/accesspolicy.py
lib/lp/registry/model/accesspolicy.py
lib/lp/registry/model/person.py
lib/lp/registry/tests/test_accesspolicy.py
lib/lp/registry/tests/test_person.py
lib/lp/testing/factory.py
--
https://code.launchpad.net/~wallyworld/launchpad/private-teams-junk-branches-1024990/+merge/120691
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/private-teams-junk-branches-1024990 into lp:launchpad.
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2012-08-07 02:31:56 +0000
+++ lib/lp/code/model/branch.py 2012-08-22 03:58:32 +0000
@@ -134,12 +134,14 @@
from lp.codehosting.safe_open import safe_open
from lp.registry.enums import (
InformationType,
+ PersonVisibility,
PRIVATE_INFORMATION_TYPES,
PUBLIC_INFORMATION_TYPES,
)
from lp.registry.interfaces.accesspolicy import (
IAccessArtifactGrantSource,
IAccessArtifactSource,
+ IAccessPolicySource,
)
from lp.registry.interfaces.person import (
validate_person,
@@ -222,13 +224,24 @@
Takes the information_type and target and makes the related
AccessArtifact and AccessPolicyArtifacts match.
"""
- # We haven't yet quite worked out how distribution privacy
- # works, so only work for products for now.
- if self.product is not None:
- pillars = [self.product]
+ wanted_links = None
+ pillars = []
+ # For private +junk branches, we calculate the wanted grants.
+ if (not self.product and
+ not self.sourcepackagename and
+ not self.information_type in PUBLIC_INFORMATION_TYPES):
+ aasource = getUtility(IAccessArtifactSource)
+ [abstract_artifact] = aasource.ensure([self])
+ wanted_links = set(
+ (abstract_artifact, policy) for policy in
+ getUtility(IAccessPolicySource).findByTeam([self.owner]))
else:
- pillars = []
- reconcile_access_for_artifact(self, self.information_type, pillars)
+ # We haven't yet quite worked out how distribution privacy
+ # works, so only work for products for now.
+ if self.product is not None:
+ pillars = [self.product]
+ reconcile_access_for_artifact(
+ self, self.information_type, pillars, wanted_links)
def setPrivate(self, private, user):
"""See `IBranch`."""
@@ -360,7 +373,11 @@
source_package)
else:
target = IBranchTarget(self.owner)
- # Person targets are always valid.
+ if (self.information_type in PRIVATE_INFORMATION_TYPES and
+ (not self.owner.is_team or
+ self.owner.visibility != PersonVisibility.PRIVATE)):
+ raise BranchTargetError(
+ 'Only private teams may have personal private branches.')
namespace = target.getNamespace(self.owner)
namespace.moveBranch(self, user, rename_if_necessary=True)
self._reconcileAccess()
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2012-08-20 14:03:01 +0000
+++ lib/lp/code/model/tests/test_branch.py 2012-08-22 03:58:32 +0000
@@ -2415,6 +2415,16 @@
removeSecurityProxy(branch)._reconcileAccess()
self.assertEqual([], get_policies_for_artifact(branch))
+ def test__reconcileAccess_for_personal_branch(self):
+ # _reconcileAccess uses a person policy for a personal branch.
+ team_owner = self.factory.makeTeam()
+ branch = self.factory.makePersonalBranch(
+ owner=team_owner, information_type=InformationType.USERDATA)
+ removeSecurityProxy(branch)._reconcileAccess()
+ self.assertContentEqual(
+ getUtility(IAccessPolicySource).findByTeam([team_owner]),
+ get_policies_for_artifact(branch))
+
class TestBranchGetAllowedInformationTypes(TestCaseWithFactory):
"""Test Branch.getAllowedInformationTypes."""
@@ -2958,6 +2968,26 @@
branch.setTarget(user=branch.owner)
self.assertEqual(branch.owner, branch.target.context)
+ def test_private_junk_branches_forbidden_for_public_teams(self):
+ # Only private teams can have private junk branches.
+ owner = self.factory.makeTeam()
+ branch = self.factory.makeBranch(
+ owner=owner,
+ information_type=InformationType.USERDATA)
+ with admin_logged_in():
+ self.assertRaises(
+ BranchTargetError, branch.setTarget, branch.owner)
+
+ def test_private_junk_branches_allowed_for_private_teams(self):
+ # Only private teams can have private junk branches.
+ owner = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
+ with admin_logged_in():
+ branch = self.factory.makeBranch(
+ owner=owner,
+ information_type=InformationType.USERDATA)
+ branch.setTarget(user=branch.owner)
+ self.assertEqual(branch.owner, branch.target.context)
+
def test_reconciles_access(self):
# setTarget calls _reconcileAccess to make the sharing schema
# match the new target.
@@ -2969,6 +2999,18 @@
self.assertEqual(
new_product, get_policies_for_artifact(branch)[0].pillar)
+ def test_reconciles_access_junk_branch(self):
+ # setTarget calls _reconcileAccess to make the sharing schema
+ # correct for a private junk branch.
+ owner = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
+ branch = self.factory.makeBranch(
+ owner=owner,
+ information_type=InformationType.USERDATA)
+ with admin_logged_in():
+ branch.setTarget(user=branch.owner)
+ self.assertEqual(
+ owner, get_policies_for_artifact(branch)[0].person)
+
def make_proposal_and_branch_revision(factory, revno, revision_id,
userdata_target=False):
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2012-08-13 19:34:10 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2012-08-22 03:58:32 +0000
@@ -32,7 +32,7 @@
from lp.code.model.branch import Branch
from lp.code.model.branchcollection import GenericBranchCollection
from lp.code.tests.helpers import remove_all_sample_data_branches
-from lp.registry.enums import InformationType
+from lp.registry.enums import InformationType, PersonVisibility
from lp.registry.interfaces.person import TeamMembershipPolicy
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.services.webapp.interfaces import (
@@ -733,6 +733,30 @@
sorted([self.public_branch, private_branch]),
sorted(branches.getBranches()))
+ def test_private_teams_see_own_private_junk_branches(self):
+ # Private teams are given an acess grant to see their private +junk
+ # branches.
+ team_owner = self.factory.makePerson()
+ team = self.factory.makeTeam(
+ visibility=PersonVisibility.PRIVATE,
+ membership_policy=TeamMembershipPolicy.MODERATED,
+ owner=team_owner)
+ with person_logged_in(team_owner):
+ personal_branch = self.factory.makePersonalBranch(
+ owner=team,
+ information_type=InformationType.USERDATA)
+ # The team is automatically subscribed to the branch since they are
+ # the owner. We want to unsubscribe them so that they lose access
+ # conferred via subscription and rely instead on the APG.
+ personal_branch.unsubscribe(team, team_owner, True)
+ # Make another junk branch the team can't see.
+ self.factory.makePersonalBranch(
+ information_type=InformationType.USERDATA)
+ branches = self.all_branches.visibleByUser(team)
+ self.assertEqual(
+ sorted([self.public_branch, personal_branch]),
+ sorted(branches.getBranches()))
+
class TestExtendedBranchRevisionDetails(TestCaseWithFactory):
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-07-19 03:18:37 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-08-22 03:58:32 +0000
@@ -62,6 +62,7 @@
id = Attribute("ID")
pillar = Attribute("Pillar")
type = Attribute("Type")
+ person = Attribute("Person")
class IAccessPolicyArtifact(Interface):
@@ -187,6 +188,14 @@
:return: a collection of the created `IAccessPolicy` objects.
"""
+ def createForTeams(teams):
+ """Create an `IAccessPolicy` for the given teams.
+
+ :param teams: a collection of teams to create `IAccessPolicy`
+ objects for.
+ :return: a collection of the created `IAccessPolicy` objects.
+ """
+
def find(pillars_and_types):
"""Return the `IAccessPolicy`s for the given pillars and types.
@@ -201,6 +210,9 @@
def findByPillar(pillars):
"""Return a `ResultSet` of all `IAccessPolicy`s for the pillars."""
+ def findByTeam(teams):
+ """Return a `ResultSet` of all `IAccessPolicy`s for the teams."""
+
class IAccessPolicyGrantSource(Interface):
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-08-06 03:47:42 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-08-22 03:58:32 +0000
@@ -58,7 +58,8 @@
from lp.services.database.stormbase import StormBase
-def reconcile_access_for_artifact(artifact, information_type, pillars):
+def reconcile_access_for_artifact(artifact, information_type, pillars,
+ wanted_links=None):
if information_type in PUBLIC_INFORMATION_TYPES:
# If it's public we can delete all the access information.
# IAccessArtifactSource handles the cascade.
@@ -67,12 +68,13 @@
[abstract_artifact] = getUtility(IAccessArtifactSource).ensure([artifact])
# Now determine the existing and desired links, and make them
- # match.
+ # match. The caller may have provided the wanted_links.
apasource = getUtility(IAccessPolicyArtifactSource)
- wanted_links = set(
- (abstract_artifact, policy) for policy in
- getUtility(IAccessPolicySource).find(
- (pillar, information_type) for pillar in pillars))
+ wanted_links = (wanted_links or
+ set(
+ (abstract_artifact, policy) for policy in
+ getUtility(IAccessPolicySource).find(
+ (pillar, information_type) for pillar in pillars)))
existing_links = set([
(apa.abstract_artifact, apa.policy)
for apa in apasource.findByArtifact([abstract_artifact])])
@@ -167,6 +169,8 @@
distribution_id = Int(name='distribution')
distribution = Reference(distribution_id, 'Distribution.id')
type = DBEnum(allow_none=True, enum=InformationType)
+ person_id = Int(name='person')
+ person = Reference(person_id, 'Person.id')
@property
def pillar(self):
@@ -190,6 +194,17 @@
get_objects=True)
@classmethod
+ def createForTeams(cls, teams):
+ insert_values = []
+ for team in teams:
+ if team is None or not team.is_team:
+ raise ValueError("A team must be specified")
+ insert_values.append((None, None, None, team))
+ return create(
+ (cls.product, cls.distribution, cls.type, cls.person),
+ insert_values, get_objects=True)
+
+ @classmethod
def _constraintForPillar(cls, pillar):
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.product import IProduct
@@ -226,6 +241,13 @@
Or(*(cls._constraintForPillar(pillar) for pillar in pillars)))
@classmethod
+ def findByTeam(cls, teams):
+ """See `IAccessPolicySource`."""
+ return IStore(cls).find(
+ cls,
+ Or(*(cls.person == team for team in teams)))
+
+ @classmethod
def findByPillarAndGrantee(cls, pillars):
"""See `IAccessPolicySource`."""
return IStore(cls).find(
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2012-08-15 17:45:15 +0000
+++ lib/lp/registry/model/person.py 2012-08-22 03:58:32 +0000
@@ -165,6 +165,10 @@
PPACreationError,
TeamMembershipPolicyError,
)
+from lp.registry.interfaces.accesspolicy import (
+ IAccessPolicyGrantSource,
+ IAccessPolicySource,
+ )
from lp.registry.interfaces.codeofconduct import ISignedCodeOfConductSet
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.gpg import IGPGKeySet
@@ -3159,6 +3163,22 @@
if not user.checkAllowVisibility():
raise ImmutableVisibilityError()
self.visibility = visibility
+ self._ensurePolicies()
+
+ def _ensurePolicies(self):
+ # Ensure that private teams have an access policy grant enabling them
+ # to see any private +junk branches.
+ if self.visibility == PersonVisibility.PUBLIC or not self.is_team:
+ return
+ aps = getUtility(IAccessPolicySource)
+ existing_policy = list(aps.findByTeam([self]))
+ if existing_policy:
+ return
+ # Create the personal access policy.
+ [policy] = getUtility(IAccessPolicySource).createForTeams([self])
+ # Create the required access policy grant.
+ grants = [(policy, self, self)]
+ getUtility(IAccessPolicyGrantSource).grant(grants)
class PersonSet:
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-08-08 07:22:51 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-08-22 03:58:32 +0000
@@ -133,6 +133,33 @@
for ap in getUtility(IAccessPolicySource).findByPillar(
[product])])
+ def test_createForTeams(self):
+ # Test createForTeams.
+ teams = [self.factory.makeTeam()]
+ policies = getUtility(IAccessPolicySource).createForTeams(teams)
+ self.assertThat(
+ policies,
+ AllMatch(Provides(IAccessPolicy)))
+ self.assertContentEqual(
+ teams,
+ [policy.person for policy in policies])
+
+ def test_findByTeam(self):
+ # findByTeam finds only the relevant policies.
+ team = self.factory.makeTeam()
+ other_team = self.factory.makeTeam()
+ aps = getUtility(IAccessPolicySource)
+ aps.createForTeams([team])
+ self.assertContentEqual(
+ [team],
+ [ap.person
+ for ap in getUtility(IAccessPolicySource).findByTeam(
+ [team, other_team])])
+ self.assertContentEqual(
+ [team],
+ [ap.person
+ for ap in getUtility(IAccessPolicySource).findByTeam([team])])
+
class TestAccessArtifact(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2012-08-14 21:52:26 +0000
+++ lib/lp/registry/tests/test_person.py 2012-08-22 03:58:32 +0000
@@ -35,6 +35,7 @@
TeamMembershipPolicy,
)
from lp.registry.errors import PrivatePersonLinkageError
+from lp.registry.interfaces.accesspolicy import IAccessPolicySource
from lp.registry.interfaces.karma import IKarmaCacheManager
from lp.registry.interfaces.person import (
ImmutableVisibilityError,
@@ -621,6 +622,22 @@
ImmutableVisibilityError, person.transitionVisibility,
PersonVisibility.PRIVATE, person)
+ def test_private_team_has_personal_access_policy(self):
+ # Private teams have a personal access policy.
+ team = self.factory.makeTeam()
+ admin = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
+ team.transitionVisibility(PersonVisibility.PRIVATE, admin)
+ self.assertContentEqual(
+ [team],
+ [ap.person
+ for ap in getUtility(IAccessPolicySource).findByTeam([team])])
+
+ def test_public_team_has_no_personal_access_policy(self):
+ # Public teams do not have a personal access policy.
+ team = self.factory.makeTeam()
+ self.assertContentEqual(
+ [], getUtility(IAccessPolicySource).findByTeam([team]))
+
class TestPersonStates(TestCaseWithFactory):
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2012-08-16 13:08:43 +0000
+++ lib/lp/testing/factory.py 2012-08-22 03:58:32 +0000
@@ -789,6 +789,7 @@
# Visibility is normally restricted to launchpad.Commercial, so
# removing the security proxy as we don't care here.
naked_team.visibility = visibility
+ naked_team._ensurePolicies()
if email is not None:
removeSecurityProxy(team).setContactAddress(
getUtility(IEmailAddressSet).new(email, team))
Follow ups
