launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22983
[Merge] lp:~twom/launchpad/rework-git-permissions-for-shadowing into lp:launchpad
Tom Wardill has proposed merging lp:~twom/launchpad/rework-git-permissions-for-shadowing into lp:launchpad.
Commit message:
Rework git branch permissions to improve shadowing for multiple grants
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~twom/launchpad/rework-git-permissions-for-shadowing/+merge/357091
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~twom/launchpad/rework-git-permissions-for-shadowing into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/gitapi.py'
--- lib/lp/code/interfaces/gitapi.py 2018-09-28 13:53:41 +0000
+++ lib/lp/code/interfaces/gitapi.py 2018-10-18 13:49:45 +0000
@@ -73,3 +73,10 @@
:returns: A list of rules for the user in the specified repository
"""
+
+ def checkRefPermissions(self, repository, ref_paths, user):
+ """Return a list of ref rules for a `user` in a `repository` that
+ match the input refs.
+
+ :returns: A list of rules for the user in the specified repository
+ """
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2018-10-16 10:10:10 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2018-10-18 13:49:45 +0000
@@ -748,16 +748,6 @@
:param user: The `IPerson` who is moving the rule.
"""
- def findRuleGrantsByGrantee(grantee):
- """Find the grants for a grantee applied to this repository.
-
- :param grantee: The Person affected
- """
-
- def findRuleGrantsForRepositoryOwner():
- """Find the grants of type REPOSITORY_OWNER applied to this repository.
- """
-
@export_read_operation()
@operation_for_version("devel")
def canBeDeleted():
=== modified file 'lib/lp/code/interfaces/gitrule.py'
--- lib/lp/code/interfaces/gitrule.py 2018-10-16 15:29:37 +0000
+++ lib/lp/code/interfaces/gitrule.py 2018-10-18 13:49:45 +0000
@@ -71,6 +71,16 @@
grants = Attribute("The access grants for this rule.")
+ def findRuleGrantsByGrantee(grantee):
+ """Find the grants for a grantee applied to this repository.
+
+ :param grantee: The Person affected
+ """
+
+ def findRuleGrantsForRepositoryOwner():
+ """Find the grants of type REPOSITORY_OWNER applied to this repository.
+ """
+
class IGitRuleEditableAttributes(Interface):
"""`IGitRule` attributes that can be edited.
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2018-10-16 10:10:10 +0000
+++ lib/lp/code/model/gitrepository.py 2018-10-18 13:49:45 +0000
@@ -72,7 +72,6 @@
from lp.app.interfaces.services import IService
from lp.code.enums import (
BranchMergeProposalStatus,
- GitGranteeType,
GitObjectType,
GitRepositoryType,
)
@@ -1194,20 +1193,6 @@
return Store.of(self).find(
GitRuleGrant, GitRuleGrant.repository_id == self.id)
- def findRuleGrantsByGrantee(self, grantee):
- """See `IGitRepository`."""
- clauses = [
- GitRuleGrant.grantee_type == GitGranteeType.PERSON,
- TeamParticipation.person == grantee,
- GitRuleGrant.grantee == TeamParticipation.teamID
- ]
- return self.grants.find(*clauses).config(distinct=True)
-
- def findRuleGrantsForRepositoryOwner(self):
- """See `IGitRepository`."""
- return self.grants.find(
- GitRuleGrant.grantee_type == GitGranteeType.REPOSITORY_OWNER)
-
def getActivity(self, changed_after=None):
"""See `IGitRepository`."""
clauses = [GitActivity.repository_id == self.id]
=== modified file 'lib/lp/code/model/gitrule.py'
--- lib/lp/code/model/gitrule.py 2018-10-16 15:29:37 +0000
+++ lib/lp/code/model/gitrule.py 2018-10-18 13:49:45 +0000
@@ -54,6 +54,7 @@
validate_person,
validate_public_person,
)
+from lp.registry.model.teammembership import TeamParticipation
from lp.services.database.constants import (
DEFAULT,
UTC_NOW,
@@ -127,6 +128,20 @@
return Store.of(self).find(
GitRuleGrant, GitRuleGrant.rule_id == self.id)
+ def findRuleGrantsByGrantee(self, grantee):
+ """See `IGitRule`."""
+ clauses = [
+ GitRuleGrant.grantee_type == GitGranteeType.PERSON,
+ TeamParticipation.person == grantee,
+ GitRuleGrant.grantee == TeamParticipation.teamID
+ ]
+ return self.grants.find(*clauses).config(distinct=True)
+
+ def findRuleGrantsForRepositoryOwner(self):
+ """See `IGitRule`."""
+ return self.grants.find(
+ GitRuleGrant.grantee_type == GitGranteeType.REPOSITORY_OWNER)
+
def addGrant(self, grantee, grantor, can_create=False, can_push=False,
can_force_push=False):
"""See `IGitRule`."""
=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py 2018-10-17 10:09:33 +0000
+++ lib/lp/code/xmlrpc/git.py 2018-10-18 13:49:45 +0000
@@ -8,6 +8,7 @@
'GitAPI',
]
+import re
import sys
from pymacaroons import Macaroon
@@ -24,7 +25,10 @@
from lp.app.errors import NameLookupFailed
from lp.app.validators import LaunchpadValidationError
-from lp.code.enums import GitRepositoryType
+from lp.code.enums import (
+ GitGranteeType,
+ GitRepositoryType,
+ )
from lp.code.errors import (
GitRepositoryCreationException,
GitRepositoryCreationFault,
@@ -414,3 +418,70 @@
self._listRefRules,
translated_path,
)
+
+ def make_regex(self, pattern):
+ return re.compile(self.glob_to_re(pattern.rstrip(b'\n')))
+
+ def glob_to_re(self, s):
+ """Convert a glob to a regular expression.
+
+ The only wildcard supported is "*", to match any path segment.
+ """
+ return b'^%s\Z' % (
+ b''.join(b'.*' if c == b'*' else re.escape(c) for c in s))
+
+ def _find_matching_rules(self, repository, ref_path):
+ """Find all matching rules for a given ref path"""
+
+ matching_rules = []
+ for rule in repository.rules:
+ regex = self.make_regex(rule.ref_pattern)
+ if regex.match(ref_path):
+ matching_rules.append(rule)
+ return matching_rules
+
+ def _checkRefPermissions(self, requester, translated_path, ref_paths):
+ repository = removeSecurityProxy(
+ getUtility(IGitLookup).getByHostingPath(translated_path))
+ is_owner = requester.inTeam(repository.owner)
+ result = {}
+ for ref in ref_paths:
+ matching_rules = self._find_matching_rules(repository, ref)
+ if is_owner and not matching_rules:
+ result[ref] = ['create', 'push', 'force_push']
+ continue
+ seen_grantees = []
+ union_permissions = set()
+ for rule in matching_rules:
+ grants = rule.findRuleGrantsByGrantee(requester)
+ if is_owner:
+ owner_grants = rule.findRuleGrantsForRepositoryOwner()
+ grants = grants.union(owner_grants)
+
+ for grant in grants:
+ if (grant.grantee, grant.grantee_type) in seen_grantees:
+ continue
+ permissions = self._buildPermissions(grant)
+ union_permissions.update(permissions)
+ seen_grantees.append(
+ (grant.grantee, grant.grantee_type)
+ )
+
+ owner_grantees = any(x[1] == GitGranteeType.REPOSITORY_OWNER
+ for x in seen_grantees)
+ if is_owner and not owner_grantees:
+ union_permissions.update(['create', 'push'])
+
+ sorted_permissions = self._sortPermissions(union_permissions)
+ result[ref] = sorted_permissions
+ return result
+
+ def checkRefPermissions(self, translated_path, ref_paths, auth_params):
+ """ See `IGitAPI`"""
+ requester_id = auth_params.get("uid")
+ return run_with_login(
+ requester_id,
+ self._checkRefPermissions,
+ translated_path,
+ ref_paths
+ )
=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py 2018-10-17 10:09:33 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py 2018-10-18 13:49:45 +0000
@@ -669,7 +669,7 @@
}),
]))
- def test_listRefRules_grantee_example_one(self):
+ def _make_example_repository(self):
user_a = self.factory.makePerson()
user_b = self.factory.makePerson()
user_c = self.factory.makePerson()
@@ -686,6 +686,10 @@
can_force_push=True)
rule = self.factory.makeGitRule(
+ repository, ref_pattern=u'refs/heads/stable/protected')
+ self.factory.makeGitRuleGrant(rule=rule, grantee=stable_team)
+
+ rule = self.factory.makeGitRule(
repository, ref_pattern=u'refs/heads/archived/*')
self.factory.makeGitRuleGrant(
rule=rule, grantee=GitGranteeType.REPOSITORY_OWNER)
@@ -710,247 +714,106 @@
self.factory.makeGitRuleGrant(
rule=rule, grantee=stable_team, can_create=True)
- results = self.git_api.listRefRules(
+ return user_a, user_b, user_c, stable_team, next_team, repository
+
+ def test_listRefRules_grantee_example_one(self):
+ test_ref_paths = [
+ 'refs/heads/stable/next', 'refs/heads/stable/protected',
+ 'refs/heads/stable/foo', 'refs/heads/archived/foo',
+ 'refs/heads/foo/next', 'refs/heads/unprotected',
+ 'refs/tags/1.0',
+ ]
+ #test_ref_paths = ['refs/heads/stable/next']
+ user_a, _, _, _, _, repository = self._make_example_repository()
+
+ results = self.git_api.checkRefPermissions(
repository.getInternalPath(),
+ test_ref_paths,
{'uid': user_a.id})
- self.assertThat(results, MatchesListwise([
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/stable/next'),
- 'permissions': Equals(['push', 'force_push']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/archived/*'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/stable/*'),
- 'permissions': Equals(['create', 'push']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/*/next'),
- 'permissions': Equals(['create', 'push']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/tags/*'),
- 'permissions': Equals(['create']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('*'),
- 'permissions': Equals(['create', 'push', 'force_push']),
- }),
- ]))
+ self.assertThat(results, MatchesDict({
+ 'refs/heads/stable/next': Equals(['push', 'force_push']),
+ 'refs/heads/stable/protected': Equals(['create', 'push']),
+ 'refs/heads/stable/foo': Equals(['create', 'push']),
+ 'refs/heads/archived/foo': Equals([]),
+ 'refs/heads/foo/next': Equals(['create', 'push']),
+ 'refs/heads/unprotected': Equals(['create', 'push', 'force_push']),
+ 'refs/tags/1.0': Equals(['create']),
+ }))
def test_listRefRules_grantee_example_two(self):
- user_a = self.factory.makePerson()
- user_b = self.factory.makePerson()
- user_c = self.factory.makePerson()
- stable_team = self.factory.makeTeam(members=[user_a, user_b])
- next_team = self.factory.makeTeam(members=[user_b, user_c])
-
- repository = removeSecurityProxy(
- self.factory.makeGitRepository(owner=user_a))
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/stable/next')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a, can_force_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/archived/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a)
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_b, can_create=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/stable/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=stable_team, can_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/*/next')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=next_team, can_force_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/tags/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a, can_create=True)
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=stable_team, can_create=True)
-
- results = self.git_api.listRefRules(
+
+ test_ref_paths = [
+ 'refs/heads/stable/next', 'refs/heads/stable/protected',
+ 'refs/heads/stable/foo', 'refs/heads/archived/foo',
+ 'refs/heads/foo/next', 'refs/heads/unprotected',
+ 'refs/tags/1.0',
+ ]
+ _, user_b, _, _, _, repository = self._make_example_repository()
+
+ results = self.git_api.checkRefPermissions(
repository.getInternalPath(),
+ test_ref_paths,
{'uid': user_b.id})
- self.assertThat(results, MatchesListwise([
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/stable/next'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/archived/*'),
- 'permissions': Equals(['create']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/stable/*'),
- 'permissions': Equals(['push']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/*/next'),
- 'permissions': Equals(['push', 'force_push']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/tags/*'),
- 'permissions': Equals(['create']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('*'),
- 'permissions': Equals([]),
- }),
- ]))
+ self.assertThat(results, MatchesDict({
+ 'refs/heads/stable/next': Equals(['push', 'force_push']),
+ 'refs/heads/stable/protected': Equals([]),
+ 'refs/heads/stable/foo': Equals(['push']),
+ 'refs/heads/archived/foo': Equals(['create']),
+ 'refs/heads/foo/next': Equals(['push', 'force_push']),
+ 'refs/heads/unprotected': Equals([]),
+ 'refs/tags/1.0': Equals(['create']),
+ }))
def test_listRefRules_grantee_example_three(self):
- user_a = self.factory.makePerson()
- user_b = self.factory.makePerson()
- user_c = self.factory.makePerson()
- stable_team = self.factory.makeTeam(members=[user_a, user_b])
- next_team = self.factory.makeTeam(members=[user_b, user_c])
-
- repository = removeSecurityProxy(
- self.factory.makeGitRepository(owner=user_a))
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/stable/next')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a, can_force_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/archived/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a)
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_b, can_create=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/stable/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=stable_team, can_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/*/next')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=next_team, can_force_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/tags/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a, can_create=True)
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=stable_team, can_create=True)
-
- results = self.git_api.listRefRules(
+ test_ref_paths = [
+ 'refs/heads/stable/next', 'refs/heads/stable/protected',
+ 'refs/heads/stable/foo', 'refs/heads/archived/foo',
+ 'refs/heads/foo/next', 'refs/heads/unprotected',
+ 'refs/tags/1.0',
+ ]
+ _, _, user_c, _, _, repository = self._make_example_repository()
+
+ results = self.git_api.checkRefPermissions(
repository.getInternalPath(),
+ test_ref_paths,
{'uid': user_c.id})
- self.assertThat(results, MatchesListwise([
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/stable/next'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/archived/*'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/stable/*'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/*/next'),
- 'permissions': Equals(['push', 'force_push']),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/tags/*'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('*'),
- 'permissions': Equals([]),
- }),
- ]))
+ self.assertThat(results, MatchesDict({
+ 'refs/heads/stable/next': Equals(['push', 'force_push']),
+ 'refs/heads/stable/protected': Equals([]),
+ 'refs/heads/stable/foo': Equals([]),
+ 'refs/heads/archived/foo': Equals([]),
+ 'refs/heads/foo/next': Equals(['push', 'force_push']),
+ 'refs/heads/unprotected': Equals([]),
+ 'refs/tags/1.0': Equals([]),
+ }))
def test_listRefRules_grantee_example_four(self):
- user_a = self.factory.makePerson()
- user_b = self.factory.makePerson()
- user_c = self.factory.makePerson()
user_d = self.factory.makePerson()
- stable_team = self.factory.makeTeam(members=[user_a, user_b])
- next_team = self.factory.makeTeam(members=[user_b, user_c])
-
- repository = removeSecurityProxy(
- self.factory.makeGitRepository(owner=user_a))
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/stable/next')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a, can_force_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/archived/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a)
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_b, can_create=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/stable/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=stable_team, can_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/heads/*/next')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=next_team, can_force_push=True)
-
- rule = self.factory.makeGitRule(
- repository, ref_pattern=u'refs/tags/*')
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=user_a, can_create=True)
- self.factory.makeGitRuleGrant(
- rule=rule, grantee=stable_team, can_create=True)
-
- results = self.git_api.listRefRules(
+ test_ref_paths = [
+ 'refs/heads/stable/next', 'refs/heads/stable/protected',
+ 'refs/heads/stable/foo', 'refs/heads/archived/foo',
+ 'refs/heads/foo/next', 'refs/heads/unprotected',
+ 'refs/tags/1.0',
+ ]
+ _, _, user_c, _, _, repository = self._make_example_repository()
+
+ results = self.git_api.checkRefPermissions(
repository.getInternalPath(),
+ test_ref_paths,
{'uid': user_d.id})
- self.assertThat(results, MatchesListwise([
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/stable/next'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/archived/*'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/stable/*'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/heads/*/next'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('refs/tags/*'),
- 'permissions': Equals([]),
- }),
- MatchesDict(
- {'ref_pattern': Equals('*'),
- 'permissions': Equals([]),
- }),
- ]))
+ self.assertThat(results, MatchesDict({
+ 'refs/heads/stable/next': Equals([]),
+ 'refs/heads/stable/protected': Equals([]),
+ 'refs/heads/stable/foo': Equals([]),
+ 'refs/heads/archived/foo': Equals([]),
+ 'refs/heads/foo/next': Equals([]),
+ 'refs/heads/unprotected': Equals([]),
+ 'refs/tags/1.0': Equals([]),
+ }))
class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
Follow ups
