launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07557
lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-2 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-2 into lp:launchpad with lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #992315 in Launchpad itself: "When revoking access to a project or artifact, user needs to be unsubscribed also"
https://bugs.launchpad.net/launchpad/+bug/992315
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/revoke-access-delete-subscriptions-job-2/+merge/104522
== Implementation ==
This branch adds a method to IAccessPolicyGrantFlatSource which will be used by the job to remove subscriptions to bugs and branches a user can no longer access if their grant is revoked. In so doing a little refactoring was done to put some common code into a method.
The new findIndirectGranteePermissionsByPolicy() method will find information types for which a user can see artifacts, including where the user can see those artifacts by virtue of the fact that they belong to a team with access. The method also returns the teams via which the user has access. These are not used right now, but will be later for the audit sharing functionality.
This branch will not be landed directly. There's still a couple more to go before the work is finished.
== Tests ==
Add tests to test_accesspolicy
- test_findIndirectGranteePermissionsByPolicy
- test_findIndirectGranteePermissionsByPolicy_filter_grantees
- test_findIndirectGranteePermissionsMultiTeam
- test_findIndirectGranteePermissionsDirect
- test_findIndirectGranteePermissionsMultiTeamPermissions
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/interfaces/accesspolicy.py
lib/lp/registry/model/accesspolicy.py
lib/lp/registry/tests/test_accesspolicy.py
--
https://code.launchpad.net/~wallyworld/launchpad/revoke-access-delete-subscriptions-job-2/+merge/104522
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/revoke-access-delete-subscriptions-job-2 into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py 2012-04-26 08:12:23 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py 2012-05-03 10:51:01 +0000
@@ -252,6 +252,31 @@
type.
"""
+ def findIndirectGranteePermissionsByPolicy(policies, grantees=None):
+ """Find teams or users with access grants for the policies.
+
+ This method is similar to findGranteePermissionsByPolicy, but the
+ results contain people who have access by virtue of team membership.
+
+ :param policies: a collection of `IAccessPolicy`s.
+ :param grantees: if not None, the result only includes people in the
+ specified list of grantees.
+ :return: a collection of
+ (`IPerson` sharee, `IAccessPolicy`, permission,
+ [`ITeam`] via_teams, shared_artifact_types)
+ where
+ sharee is the person or team with access
+ permission is a SharingPermission enum value.
+ ALL means the person has an access policy grant and can see all
+ artifacts for the associated pillar.
+ SOME means the person only has specified access artifact grants.
+ via_teams is the team the sharee belongs to in order to gain
+ access. If via is None, then the sharee has direct access.
+ shared_artifact_types contains the information_types for which the
+ user has been granted access for one or more artifacts of that
+ type.
+ """
+
def findArtifactsByGrantee(grantee, policies):
"""Find the `IAccessArtifact`s for grantee and policies.
=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py 2012-04-26 08:12:23 +0000
+++ lib/lp/registry/model/accesspolicy.py 2012-05-03 10:51:01 +0000
@@ -14,12 +14,15 @@
from collections import defaultdict
import pytz
+from storm import Undef
from storm.expr import (
And,
In,
+ Join,
Or,
Select,
SQL,
+ With,
)
from storm.properties import (
DateTime,
@@ -43,6 +46,7 @@
IAccessPolicyGrant,
)
from lp.registry.model.person import Person
+from lp.registry.model.teammembership import TeamParticipation
from lp.services.database.bulk import create
from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.database.enumcol import DBEnum
@@ -359,39 +363,21 @@
Person, Person.id == cls.grantee_id, cls.policy_id.is_in(ids))
@classmethod
- def findGranteePermissionsByPolicy(cls, policies, grantees=None):
- """See `IAccessPolicyGrantFlatSource`."""
- policies_by_id = dict((policy.id, policy) for policy in policies)
-
- # A cache for the sharing permissions, keyed on grantee
- permissions_cache = defaultdict(dict)
- # Information types for which there are shared artifacts.
- shared_artifact_info_types = defaultdict(list)
-
- def set_permission(person):
- # Lookup the permissions from the previously loaded cache.
- return (
- person[0],
- permissions_cache[person[0]],
- shared_artifact_info_types[person[0]])
-
- def load_permissions(people):
- # We now have the grantees and policies we want in the result so
- # load any corresponding permissions and cache them.
- people_by_id = dict(
- (person[0].id, person[0]) for person in people)
+ def _populatePermissionsCache(cls, permissions_cache,
+ shared_artifact_info_types, grantee_ids,
+ policies_by_id, persons_by_id):
all_permission_term = SQL("bool_or(artifact IS NULL) as all")
some_permission_term = SQL(
"bool_or(artifact IS NOT NULL) as some")
constraints = [
- cls.grantee_id.is_in(people_by_id.keys()),
+ cls.grantee_id.is_in(grantee_ids),
cls.policy_id.is_in(policies_by_id.keys())]
result_set = IStore(cls).find(
(cls.grantee_id, cls.policy_id, all_permission_term,
some_permission_term),
*constraints).group_by(cls.grantee_id, cls.policy_id)
for (person_id, policy_id, has_all, has_some) in result_set:
- person = people_by_id[person_id]
+ person = persons_by_id[person_id]
policy = policies_by_id[policy_id]
permissions_cache[person][policy] = (
SharingPermission.ALL if has_all
@@ -399,6 +385,32 @@
if has_some:
shared_artifact_info_types[person].append(policy.type)
+ @classmethod
+ def findGranteePermissionsByPolicy(cls, policies, grantees=None):
+ """See `IAccessPolicyGrantFlatSource`."""
+ policies_by_id = dict((policy.id, policy) for policy in policies)
+
+ # A cache for the sharing permissions, keyed on grantee
+ permissions_cache = defaultdict(dict)
+ # Information types for which there are shared artifacts.
+ shared_artifact_info_types = defaultdict(list)
+
+ def set_permission(person):
+ # Lookup the permissions from the previously loaded cache.
+ return (
+ person[0],
+ permissions_cache[person[0]],
+ shared_artifact_info_types[person[0]])
+
+ def load_permissions(people):
+ # We now have the grantees and policies we want in the result so
+ # load any corresponding permissions and cache them.
+ people_by_id = dict(
+ (person[0].id, person[0]) for person in people)
+ cls._populatePermissionsCache(
+ permissions_cache, shared_artifact_info_types,
+ people_by_id.keys(), policies_by_id, people_by_id)
+
constraints = [cls.policy_id.is_in(policies_by_id.keys())]
if grantees:
grantee_ids = [grantee.id for grantee in grantees]
@@ -417,6 +429,121 @@
result_decorator=set_permission, pre_iter_hook=load_permissions)
@classmethod
+ def _populateIndirectGranteePermissions(cls,
+ policies_by_id, result_set):
+ # A cache for the sharing permissions, keyed on grantee.
+ permissions_cache = defaultdict(dict)
+ # A cache of teams belonged to, keyed by grantee.
+ via_teams_cache = defaultdict(list)
+ grantees_by_id = defaultdict()
+ # Information types for which there are shared artifacts.
+ shared_artifact_info_types = defaultdict(list)
+
+ def set_permission(grantee):
+ # Lookup the permissions from the previously loaded cache.
+ via_team_ids = via_teams_cache[grantee[0].id]
+ via_teams = sorted(
+ [grantees_by_id[team_id] for team_id in via_team_ids],
+ key=lambda x: x.displayname)
+ permissions = permissions_cache[grantee[0]]
+ shared_info_types = shared_artifact_info_types[grantee[0]]
+ # For access via teams, we need to use the team permissions. If a
+ # person has access via more than one team, we use the most
+ # powerful permission of all that are there.
+ for team in via_teams:
+ team_permissions = permissions_cache[team]
+ shared_info_types = []
+ for info_type, permission in team_permissions.items():
+ permission_to_use = permissions.get(info_type, permission)
+ if permission == SharingPermission.ALL:
+ permission_to_use = permission
+ elif permission == SharingPermission.SOME:
+ shared_info_types.append(info_type.type)
+ permissions[info_type] = permission_to_use
+ result = (
+ grantee[0], permissions, via_teams or None,
+ shared_info_types)
+ return result
+
+ def load_teams_and_permissions(grantees):
+ # We now have the grantees we want in the result so load any
+ # associated team memberships and permissions and cache them.
+ if permissions_cache:
+ return
+ store = IStore(cls)
+ for grantee in grantees:
+ grantees_by_id[grantee[0].id] = grantee[0]
+ # Find any teams associated with the grantees. If grantees is a
+ # sliced list (for batching), it may contain indirect grantees but
+ # not the team they belong to so that needs to be fixed below.
+ with_expr = With("grantees", store.find(
+ cls.grantee_id, cls.policy_id.is_in(policies_by_id.keys())
+ ).config(distinct=True)._get_select())
+ result_set = store.with_(with_expr).find(
+ (TeamParticipation.teamID, TeamParticipation.personID),
+ TeamParticipation.personID.is_in(grantees_by_id.keys()),
+ TeamParticipation.teamID.is_in(
+ Select(
+ (SQL("grantees.grantee"),),
+ tables="grantees",
+ distinct=True)))
+ team_ids = set()
+ direct_grantee_ids = set()
+ for team_id, team_member_id in result_set:
+ if team_member_id == team_id:
+ direct_grantee_ids.add(team_member_id)
+ else:
+ via_teams_cache[team_member_id].append(team_id)
+ team_ids.add(team_id)
+ # Remove from the via_teams cache all the direct grantees.
+ for direct_grantee_id in direct_grantee_ids:
+ if direct_grantee_id in via_teams_cache:
+ del via_teams_cache[direct_grantee_id]
+ # Load and cache the additional required teams.
+ persons = store.find(Person, Person.id.is_in(team_ids))
+ for person in persons:
+ grantees_by_id[person.id] = person
+
+ cls._populatePermissionsCache(
+ permissions_cache, shared_artifact_info_types,
+ grantees_by_id.keys(), policies_by_id, grantees_by_id)
+
+ return DecoratedResultSet(
+ result_set,
+ result_decorator=set_permission,
+ pre_iter_hook=load_teams_and_permissions)
+
+ @classmethod
+ def findIndirectGranteePermissionsByPolicy(cls, policies,
+ grantees=None):
+ """See `IAccessPolicyGrantFlatSource`."""
+ policies_by_id = dict((policy.id, policy) for policy in policies)
+
+ grantee_filter = Undef
+ if grantees:
+ grantee_ids = [grantee.id for grantee in grantees]
+ grantee_filter = TeamParticipation.personID.is_in(grantee_ids)
+
+ store = IStore(cls)
+ with_expr = With("grantees", store.find(
+ cls.grantee_id,
+ cls.policy_id.is_in(policies_by_id.keys()))
+ .config(distinct=True)._get_select())
+ result_set = store.with_(with_expr).find(
+ (Person,),
+ In(
+ Person.id,
+ Select(
+ (TeamParticipation.personID,),
+ tables=(TeamParticipation, Join("grantees",
+ SQL("grantees.grantee = TeamParticipation.team"))),
+ where=grantee_filter,
+ distinct=True)))
+
+ return cls._populateIndirectGranteePermissions(
+ policies_by_id, result_set)
+
+ @classmethod
def findArtifactsByGrantee(cls, grantee, policies):
"""See `IAccessPolicyGrantFlatSource`."""
ids = [policy.id for policy in policies]
=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py 2012-04-26 07:54:34 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py 2012-05-03 10:51:01 +0000
@@ -596,6 +596,216 @@
apgfs.findGranteePermissionsByPolicy(
[policy], [grantee_in_result]))
+ def test_findIndirectGranteePermissionsByPolicy(self):
+ # findIndirectGranteePermissionsByPolicy() returns anyone with a grant
+ # for any of the policies or the policies' artifacts. The result
+ # includes members of teams with access and a list of teams via which
+ # they have gained access.
+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
+
+ # People with grants on the policy show up.
+ policy_with_no_grantees = self.factory.makeAccessPolicy()
+ policy = self.factory.makeAccessPolicy()
+ indirect_person_grantee = self.factory.makePerson()
+ team_grantee = self.factory.makeTeam(members=[indirect_person_grantee])
+ self.factory.makeAccessPolicyGrant(policy=policy, grantee=team_grantee)
+ policy_info = {policy: SharingPermission.ALL}
+ expected_grantees = [
+ (member, policy_info, [team_grantee], [])
+ for member in team_grantee.activemembers]
+ expected_grantees.append(
+ (team_grantee, policy_info, None, []))
+ self.assertContentEqual(
+ expected_grantees,
+ apgfs.findIndirectGranteePermissionsByPolicy(
+ [policy, policy_with_no_grantees]))
+
+ # But not people with grants on artifacts.
+ artifact = self.factory.makeAccessArtifact()
+ artifact_grant = self.factory.makeAccessArtifactGrant(
+ artifact=artifact, grantee=team_grantee)
+ other_artifact_grant = self.factory.makeAccessArtifactGrant(
+ artifact=artifact)
+ self.assertContentEqual(
+ expected_grantees,
+ apgfs.findIndirectGranteePermissionsByPolicy(
+ [policy, policy_with_no_grantees]))
+
+ # Unless the artifacts are linked to the policy.
+ another_policy = self.factory.makeAccessPolicy()
+ self.factory.makeAccessPolicyArtifact(
+ artifact=artifact_grant.abstract_artifact, policy=another_policy)
+ policy_info = {
+ policy: SharingPermission.ALL,
+ another_policy: SharingPermission.SOME}
+ expected_grantees = [
+ (member, policy_info, [team_grantee], [another_policy.type])
+ for member in team_grantee.activemembers]
+ expected_grantees.append(
+ (team_grantee, policy_info, None, [another_policy.type]))
+ expected_grantees.append(
+ (other_artifact_grant.grantee,
+ {another_policy: SharingPermission.SOME}, None,
+ [another_policy.type]))
+ self.assertContentEqual(
+ expected_grantees,
+ apgfs.findIndirectGranteePermissionsByPolicy([
+ policy, another_policy, policy_with_no_grantees]))
+
+ # Slicing works by person, not by (person, policy).
+ self.assertContentEqual(
+ [(indirect_person_grantee, {
+ policy: SharingPermission.ALL,
+ another_policy: SharingPermission.SOME}, [team_grantee],
+ [another_policy.type])],
+ apgfs.findIndirectGranteePermissionsByPolicy([
+ policy, another_policy, policy_with_no_grantees]).order_by(
+ Person.id)[:1])
+
+ def test_findIndirectGranteePermissionsByPolicy_filter_grantees(
+ self):
+ # findIndirectGranteePermissionsByPolicy() returns anyone with a grant
+ # for any of the policies or the policies' artifacts so long as the
+ # grantee is in the specified list of grantees.
+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
+
+ # People with grants on the policy show up.
+ policy = self.factory.makeAccessPolicy()
+ grantee_not_in_result = self.factory.makePerson()
+ indirect_person_grantee = self.factory.makePerson()
+ team_grantee = self.factory.makeTeam(members=[indirect_person_grantee])
+ self.factory.makeAccessPolicyGrant(policy=policy, grantee=team_grantee)
+ self.factory.makeAccessPolicyGrant(
+ policy=policy, grantee=grantee_not_in_result)
+ self.assertContentEqual(
+ [(indirect_person_grantee, {policy: SharingPermission.ALL},
+ [team_grantee], [])],
+ apgfs.findIndirectGranteePermissionsByPolicy(
+ [policy], [indirect_person_grantee]))
+
+ def test_findIndirectGranteePermissionsMultiTeam(self):
+ # Test that findIndirectGranteePermissionsByPolicy() works correctly
+ # when a user is granted access via membership of more than one team.
+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
+
+ policy_with_no_grantees = self.factory.makeAccessPolicy()
+ policy = self.factory.makeAccessPolicy()
+ # Make an indirect grantee belonging to team1 and team2.
+ indirect_person_grantee = self.factory.makePerson()
+ team_grantee1 = self.factory.makeTeam(
+ members=[indirect_person_grantee])
+ # Make a team for indirect grantee which should not appear in the
+ # results.
+ self.factory.makeTeam(members=[indirect_person_grantee])
+ team_grantee2 = self.factory.makeTeam(
+ members=[indirect_person_grantee])
+ self.factory.makeAccessPolicyGrant(
+ policy=policy, grantee=team_grantee1)
+ self.factory.makeAccessPolicyGrant(
+ policy=policy, grantee=team_grantee2)
+ policy_info = {policy: SharingPermission.ALL}
+ # Indirect grantee has access.
+ expected_grantees = [
+ (indirect_person_grantee, policy_info,
+ sorted([team_grantee1, team_grantee2],
+ key=lambda x: x.displayname), [])]
+ # All other team members have access.
+ expected_grantees.extend([(member, policy_info, [team_grantee1], [])
+ for member in team_grantee1.activemembers
+ if member != indirect_person_grantee])
+ expected_grantees.extend([(member, policy_info, [team_grantee2], [])
+ for member in team_grantee2.activemembers
+ if member != indirect_person_grantee])
+ # The team itself has access also.
+ expected_grantees.append((team_grantee1, policy_info, None, []))
+ expected_grantees.append((team_grantee2, policy_info, None, []))
+ self.assertContentEqual(
+ expected_grantees,
+ apgfs.findIndirectGranteePermissionsByPolicy(
+ [policy, policy_with_no_grantees]))
+
+ def test_findIndirectGranteePermissionsDirect(self):
+ # Test that findIndirectGranteePermissionsByPolicy() works correctly
+ # when a user is granted access directly as well as via membership of a
+ # team.
+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
+
+ policy_with_no_grantees = self.factory.makeAccessPolicy()
+ policy = self.factory.makeAccessPolicy()
+ # Make an direct grantee.
+ person_grantee = self.factory.makePerson()
+ self.factory.makeAccessPolicyGrant(
+ policy=policy, grantee=person_grantee)
+ # Make a team grantee with the person grantee as a member.
+ team_grantee = self.factory.makeTeam(owner=self.factory.makePerson(),
+ members=[person_grantee])
+ self.factory.makeAccessPolicyGrant(
+ policy=policy, grantee=team_grantee)
+ # Make a team for indirect grantee which should not appear in the
+ # results.
+ self.factory.makeTeam(members=[person_grantee])
+ policy_info = {policy: SharingPermission.ALL}
+ # Direct grantee has access.
+ expected_grantees = [(person_grantee, policy_info, None, [])]
+ # All other team members have indirect access.
+ expected_grantees.extend([(member, policy_info, [team_grantee], [])
+ for member in team_grantee.activemembers
+ if member != person_grantee])
+ # The team itself has access also.
+ expected_grantees.append((team_grantee, policy_info, None, []))
+ self.assertContentEqual(
+ expected_grantees,
+ apgfs.findIndirectGranteePermissionsByPolicy(
+ [policy, policy_with_no_grantees]))
+
+ def test_findIndirectGranteePermissionsMultiTeamPermissions(self):
+ # Test that findIndirectGranteePermissionsByPolicy() works correctly
+ # when a user is granted access via membership of more than one team
+ # where each team has a different level of access. If an indirect
+ # grantee has both ALL and SOME, then ALL is shown.
+ apgfs = getUtility(IAccessPolicyGrantFlatSource)
+
+ policy_with_no_grantees = self.factory.makeAccessPolicy()
+ policy = self.factory.makeAccessPolicy()
+ indirect_person_grantee = self.factory.makePerson()
+
+ # One team has ALL access.
+ team_grantee1 = self.factory.makeTeam(
+ members=[indirect_person_grantee])
+ self.factory.makeAccessPolicyGrant(
+ policy=policy, grantee=team_grantee1)
+
+ # Another team has SOME access.
+ indirect_person_grantee2 = self.factory.makePerson()
+ team_grantee2 = self.factory.makeTeam(
+ members=[indirect_person_grantee, indirect_person_grantee2])
+ artifact = self.factory.makeAccessArtifact()
+ artifact_grant = self.factory.makeAccessArtifactGrant(
+ artifact=artifact, grantee=team_grantee2)
+ self.factory.makeAccessPolicyArtifact(
+ artifact=artifact_grant.abstract_artifact, policy=policy)
+
+ policy_info = {policy: SharingPermission.ALL}
+ expected_grantees = [
+ (indirect_person_grantee, policy_info,
+ sorted([team_grantee1, team_grantee2],
+ key=lambda x: x.displayname), [policy.type])]
+ expected_grantees.extend([(member, policy_info, [team_grantee1], [])
+ for member in team_grantee1.activemembers
+ if member != indirect_person_grantee])
+ expected_grantees.append((team_grantee1, policy_info, None, []))
+ policy_info = {policy: SharingPermission.SOME}
+ expected_grantees.extend(
+ [(member, policy_info, [team_grantee2], [policy.type])
+ for member in team_grantee2.activemembers
+ if member != indirect_person_grantee])
+ expected_grantees.append(
+ (team_grantee2, policy_info, None, [policy.type]))
+ self.assertContentEqual(
+ expected_grantees,
+ apgfs.findIndirectGranteePermissionsByPolicy(
+ [policy, policy_with_no_grantees]))
+
def test_findArtifactsByGrantee(self):
# findArtifactsByGrantee() returns the artifacts for grantee for any of
# the policies.
Follow ups
