launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23034
[Merge] lp:~cjwatson/launchpad/git-permissions-check-api into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-permissions-check-api into lp:launchpad.
Commit message:
Add webservice APIs to allow checking a user's effective permissions on Git refs.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1517559 in Launchpad itself: "git fine-grained permissions"
https://bugs.launchpad.net/launchpad/+bug/1517559
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-permissions-check-api/+merge/357842
I ended up pushing most of the guts of GitAPI._checkRefPermissions down to GitRepository, since we need almost all of the same logic except for looking up translated paths, handling authentication parameters, and the slightly different output encoding.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-permissions-check-api into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py 2018-10-22 13:10:21 +0000
+++ lib/lp/code/interfaces/gitref.py 2018-10-25 19:12:27 +0000
@@ -433,6 +433,19 @@
Other grants may apply via wildcard rules.
"""
+ @operation_parameters(
+ person=Reference(title=_("Person to check"), schema=IPerson))
+ @export_read_operation()
+ @operation_for_version("devel")
+ def checkPermissions(person):
+ """Check a person's permissions on this reference.
+
+ :param person: An `IPerson` to check.
+ :return: A list of zero or more of "create", "push", and
+ "force-push", indicating the requested person's effective
+ permissions on this reference.
+ """
+
class IGitRef(IGitRefView, IGitRefEdit):
"""A reference in a Git repository."""
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2018-10-23 16:15:37 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2018-10-25 19:12:27 +0000
@@ -25,6 +25,7 @@
export_as_webservice_collection,
export_as_webservice_entry,
export_destructor_operation,
+ export_operation_as,
export_read_operation,
export_write_operation,
exported,
@@ -809,6 +810,35 @@
def setRules(rules, user):
"""Set the access rules for this repository."""
+ def checkRefPermissions(person, ref_paths):
+ """Check a person's permissions on some references in this repository.
+
+ :param person: An `IPerson` to check, or
+ `GitGranteeType.REPOSITORY_OWNER` to check an anonymous
+ repository owner.
+ :param ref_paths: An iterable of reference paths.
+ :return: A dict mapping reference paths to sets of
+ `GitPermissionType`, corresponding to the requested person's
+ effective permissions on each of the requested references.
+ """
+
+ @operation_parameters(
+ person=Reference(title=_("Person to check"), schema=IPerson),
+ paths=List(title=_("Reference paths"), value_type=TextLine()))
+ @export_operation_as("checkRefPermissions")
+ @export_read_operation()
+ @operation_for_version("devel")
+ def api_checkRefPermissions(person, paths):
+ """Check a person's permissions on some references in this repository.
+
+ :param person: An `IPerson` to check.
+ :param paths: An iterable of reference paths.
+ :return: A dict mapping reference paths to lists of zero or more of
+ "create", "push", and "force-push", indicating the requested
+ person's effective permissions on each of the requested
+ references.
+ """
+
@export_read_operation()
@operation_for_version("devel")
def canBeDeleted():
=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py 2018-10-22 12:43:55 +0000
+++ lib/lp/code/model/gitref.py 2018-10-25 19:12:27 +0000
@@ -65,6 +65,7 @@
IGitRef,
IGitRefRemoteSet,
)
+from lp.code.interfaces.gitrule import describe_git_permissions
from lp.code.model.branchmergeproposal import (
BranchMergeProposal,
BranchMergeProposalGetter,
@@ -441,6 +442,12 @@
rule = self.repository.addRule(self.path, user)
rule.setGrants(grants, user)
+ def checkPermissions(self, person):
+ """See `IGitRef`."""
+ return describe_git_permissions(
+ self.repository.checkRefPermissions(
+ person, [self.path])[self.path])
+
@implementer(IGitRef)
class GitRef(StormBase, GitRefMixin):
@@ -823,6 +830,7 @@
return []
setGrants = _unimplemented
+ checkPermissions = _unimplemented
def __eq__(self, other):
return (
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2018-10-22 16:24:46 +0000
+++ lib/lp/code/model/gitrepository.py 2018-10-25 19:12:27 +0000
@@ -9,9 +9,13 @@
'parse_git_commits',
]
-from collections import OrderedDict
+from collections import (
+ defaultdict,
+ OrderedDict,
+ )
from datetime import datetime
import email
+from fnmatch import fnmatch
from functools import partial
from itertools import (
chain,
@@ -48,7 +52,10 @@
Reference,
Unicode,
)
-from storm.store import Store
+from storm.store import (
+ EmptyResultSet,
+ Store,
+ )
from zope.component import getUtility
from zope.event import notify
from zope.interface import (
@@ -81,6 +88,7 @@
BranchMergeProposalStatus,
GitGranteeType,
GitObjectType,
+ GitPermissionType,
GitRepositoryType,
)
from lp.code.errors import (
@@ -114,6 +122,7 @@
IGitRepositorySet,
user_has_special_git_repository_access,
)
+from lp.code.interfaces.gitrule import describe_git_permissions
from lp.code.interfaces.revision import IRevisionSet
from lp.code.mail.branch import send_git_repository_modified_notifications
from lp.code.model.branchmergeproposal import BranchMergeProposal
@@ -1279,6 +1288,68 @@
"(got %s instead)" %
(requested_rule_order, observed_rule_order))
+ def checkRefPermissions(self, person, ref_paths):
+ """See `IGitRepository`."""
+ result = {}
+
+ rules = list(self.rules)
+ grants_for_user = defaultdict(list)
+ grants = EmptyResultSet()
+ is_owner = False
+ if IPerson.providedBy(person):
+ grants = grants.union(self.findRuleGrantsByGrantee(person))
+ if person.inTeam(self.owner):
+ is_owner = True
+ elif person == GitGranteeType.REPOSITORY_OWNER:
+ is_owner = True
+ if is_owner:
+ grants = grants.union(
+ self.findRuleGrantsByGrantee(GitGranteeType.REPOSITORY_OWNER))
+ for grant in grants:
+ grants_for_user[grant.rule].append(grant)
+
+ for ref_path in ref_paths:
+ matching_rules = [
+ rule for rule in rules if fnmatch(ref_path, rule.ref_pattern)]
+ if is_owner and not matching_rules:
+ # If there are no matching rules, then the repository owner
+ # can do anything.
+ result[ref_path] = {
+ GitPermissionType.CAN_CREATE, GitPermissionType.CAN_PUSH,
+ GitPermissionType.CAN_FORCE_PUSH,
+ }
+ continue
+
+ seen_grantees = set()
+ union_permissions = set()
+ for rule in matching_rules:
+ for grant in grants_for_user[rule]:
+ if (grant.grantee, grant.grantee_type) in seen_grantees:
+ continue
+ union_permissions.update(grant.permissions)
+ seen_grantees.add((grant.grantee, grant.grantee_type))
+
+ owner_type = (None, GitGranteeType.REPOSITORY_OWNER)
+ if is_owner and owner_type not in seen_grantees:
+ union_permissions.update(
+ {GitPermissionType.CAN_CREATE, GitPermissionType.CAN_PUSH})
+
+ # Permission to force-push implies permission to push.
+ if GitPermissionType.CAN_FORCE_PUSH in union_permissions:
+ union_permissions.add(GitPermissionType.CAN_PUSH)
+
+ result[ref_path] = union_permissions
+
+ return result
+
+ def api_checkRefPermissions(self, person, paths):
+ """See `IGitRepository`."""
+ return {
+ path: describe_git_permissions(permissions)
+ for path, permissions in self.checkRefPermissions(
+ person, paths).items()
+ }
+
def getActivity(self, changed_after=None):
"""See `IGitRepository`."""
clauses = [GitActivity.repository_id == self.id]
=== modified file 'lib/lp/code/model/tests/test_gitref.py'
--- lib/lp/code/model/tests/test_gitref.py 2018-10-16 15:29:37 +0000
+++ lib/lp/code/model/tests/test_gitref.py 2018-10-25 19:12:27 +0000
@@ -875,3 +875,30 @@
can_push=Is(True),
can_force_push=Is(False)))),
]))
+
+ def test_checkPermissions(self):
+ [ref] = self.factory.makeGitRefs()
+ owner = ref.owner
+ grantees = [self.factory.makePerson() for _ in range(2)]
+ self.factory.makeGitRuleGrant(
+ repository=ref.repository, ref_pattern=ref.path,
+ grantee=grantees[0], can_create=True)
+ self.factory.makeGitRuleGrant(
+ repository=ref.repository, ref_pattern="*",
+ grantee=grantees[1], can_force_push=True)
+ with person_logged_in(owner):
+ ref_url = api_url(ref)
+ owner_url = api_url(owner)
+ grantee_urls = [api_url(grantee) for grantee in grantees]
+ webservice = webservice_for_person(
+ owner, permission=OAuthPermission.WRITE_PUBLIC)
+ webservice.default_api_version = "devel"
+ response = webservice.named_get(
+ ref_url, "checkPermissions", person=owner_url)
+ self.assertEqual(["create", "push"], json.loads(response.body))
+ response = webservice.named_get(
+ ref_url, "checkPermissions", person=grantee_urls[0])
+ self.assertEqual(["create"], json.loads(response.body))
+ response = webservice.named_get(
+ ref_url, "checkPermissions", person=grantee_urls[1])
+ self.assertEqual(["push", "force-push"], json.loads(response.body))
=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py 2018-10-22 16:24:46 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py 2018-10-25 19:12:27 +0000
@@ -3633,3 +3633,48 @@
can_push=Is(True),
can_force_push=Is(False)))),
]))
+
+ def test_checkRefPermissions(self):
+ repository = self.factory.makeGitRepository()
+ owner = repository.owner
+ grantees = [self.factory.makePerson() for _ in range(2)]
+ master_rule = self.factory.makeGitRule(
+ repository=repository, ref_pattern="refs/heads/master")
+ self.factory.makeGitRuleGrant(
+ rule=master_rule, grantee=grantees[0], can_create=True)
+ self.factory.makeGitRuleGrant(
+ rule=master_rule, grantee=grantees[1], can_push=True)
+ self.factory.makeGitRuleGrant(
+ repository=repository, ref_pattern="refs/heads/*",
+ grantee=grantees[1], can_force_push=True)
+ with person_logged_in(owner):
+ repository_url = api_url(repository)
+ owner_url = api_url(owner)
+ grantee_urls = [api_url(grantee) for grantee in grantees]
+ webservice = webservice_for_person(
+ owner, permission=OAuthPermission.WRITE_PUBLIC)
+ webservice.default_api_version = "devel"
+ response = webservice.named_get(
+ repository_url, "checkRefPermissions", person=owner_url,
+ paths=["refs/heads/master", "refs/heads/next", "refs/other"])
+ self.assertThat(json.loads(response.body), MatchesDict({
+ "refs/heads/master": Equals(["create", "push"]),
+ "refs/heads/next": Equals(["create", "push"]),
+ "refs/other": Equals(["create", "push", "force-push"]),
+ }))
+ response = webservice.named_get(
+ repository_url, "checkRefPermissions", person=grantee_urls[0],
+ paths=["refs/heads/master", "refs/heads/next", "refs/other"])
+ self.assertThat(json.loads(response.body), MatchesDict({
+ "refs/heads/master": Equals(["create"]),
+ "refs/heads/next": Equals([]),
+ "refs/other": Equals([]),
+ }))
+ response = webservice.named_get(
+ repository_url, "checkRefPermissions", person=grantee_urls[1],
+ paths=["refs/heads/master", "refs/heads/next", "refs/other"])
+ self.assertThat(json.loads(response.body), MatchesDict({
+ "refs/heads/master": Equals(["push"]),
+ "refs/heads/next": Equals(["push", "force-push"]),
+ "refs/other": Equals([]),
+ }))
=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py 2018-10-24 00:42:39 +0000
+++ lib/lp/code/xmlrpc/git.py 2018-10-25 19:12:27 +0000
@@ -8,8 +8,6 @@
'GitAPI',
]
-from collections import defaultdict
-from fnmatch import fnmatch
import sys
from pymacaroons import Macaroon
@@ -350,31 +348,13 @@
permissions.append('force_push')
return permissions
- def _buildPermissions(self, grant):
- """Build a set of the available permissions from a GitRuleGrant"""
- permissions = set(grant.permissions)
- if GitPermissionType.CAN_FORCE_PUSH in permissions:
- # Permission to force-push implies permission to push.
- permissions.add(GitPermissionType.CAN_PUSH)
- return permissions
-
- def _findMatchingRules(self, repository, ref_path):
- """Find all matching rules for a given ref path"""
- matching_rules = []
- for rule in repository.rules:
- if fnmatch(ref_path, rule.ref_pattern):
- matching_rules.append(rule)
- return matching_rules
-
def _checkRefPermissions(self, requester, translated_path, ref_paths,
auth_params):
if requester == LAUNCHPAD_ANONYMOUS:
requester = None
repository = removeSecurityProxy(
getUtility(IGitLookup).getByHostingPath(translated_path))
- result = {}
- grants_for_user = defaultdict(list)
macaroon_raw = auth_params.get("macaroon")
if (macaroon_raw is not None and
self._verifyMacaroon(macaroon_raw, repository)):
@@ -382,44 +362,13 @@
# repository owner.
# For the time being, this only works for code imports.
assert repository.repository_type == GitRepositoryType.IMPORTED
- is_owner = True
- grants = repository.findRuleGrantsByGrantee(
- GitGranteeType.REPOSITORY_OWNER)
- elif requester is None:
- is_owner = False
- grants = []
- else:
- is_owner = requester.inTeam(repository.owner)
- grants = repository.findRuleGrantsByGrantee(requester)
- if is_owner:
- owner_grants = repository.findRuleGrantsByGrantee(
- GitGranteeType.REPOSITORY_OWNER)
- grants = grants.union(owner_grants)
- for grant in grants:
- grants_for_user[grant.rule].append(grant)
-
- for ref in ref_paths:
- matching_rules = self._findMatchingRules(repository, ref)
- if is_owner and not matching_rules:
- result[ref] = ['create', 'push', 'force_push']
- continue
- seen_grantees = set()
- union_permissions = set()
- for rule in matching_rules:
- for grant in grants_for_user[rule]:
- if (grant.grantee, grant.grantee_type) in seen_grantees:
- continue
- permissions = self._buildPermissions(grant)
- union_permissions.update(permissions)
- seen_grantees.add((grant.grantee, grant.grantee_type))
-
- owner_type = (None, GitGranteeType.REPOSITORY_OWNER)
- if is_owner and owner_type not in seen_grantees:
- union_permissions.update(
- [GitPermissionType.CAN_CREATE, GitPermissionType.CAN_PUSH])
-
- result[ref] = self._renderPermissions(union_permissions)
- return result
+ requester = GitGranteeType.REPOSITORY_OWNER
+
+ return {
+ ref_path: self._renderPermissions(permissions)
+ for ref_path, permissions in repository.checkRefPermissions(
+ requester, ref_paths).items()
+ }
def checkRefPermissions(self, translated_path, ref_paths, auth_params):
""" See `IGitAPI`"""
Follow ups
