launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py	2018-10-03 15:54:13 +0000
> +++ lib/lp/code/interfaces/gitrepository.py	2018-10-03 15:54:14 +0000
> @@ -738,6 +738,16 @@
>              end.
>          """
>  
> +    def findGrantsByGrantee(grantee):
> +        """Find the grants for a grantee applied to this repository.
> +
> +        :param grantee: The Person affected
> +        """
> +
> +    def findGrantsForRepositoryOwner():
> +        """Find the grants of type REPOSITORY_OWNER applied to this repository.
> +        """

Could you s/findGrants/findRuleGrants/g across this MP, in line with William's review of https://code.launchpad.net/~cjwatson/launchpad/git-grant-limitedview/+merge/355607 ?

> +
>      @export_read_operation()
>      @operation_for_version("devel")
>      def canBeDeleted():
> 
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py	2018-08-28 13:58:37 +0000
> +++ lib/lp/code/xmlrpc/git.py	2018-10-03 15:54:14 +0000
> @@ -325,3 +325,95 @@
>          else:
>              # Only macaroons are supported for password authentication.
>              return faults.Unauthorized()
> +
> +    def _sortPermissions(self, set_of_permissions):
> +        """Sort an iterable of permission strings for consistency"""
> +        permissions = []
> +        if 'create' in set_of_permissions:
> +            permissions.append('create')
> +        if 'push' in set_of_permissions:
> +            permissions.append('push')
> +        if 'force_push' in set_of_permissions:
> +            permissions.append('force_push')
> +        return permissions
> +
> +    def _buildPermissions(self, grant):
> +        """Build a set of the available permissions from a GitRuleGrant"""
> +        permissions = set()
> +        if grant.can_create:
> +            permissions.add('create')
> +        if grant.can_push:
> +            permissions.add('push')
> +        if grant.can_force_push and not grant.can_push:
> +            permissions.add('push')
> +            permissions.add('force_push')
> +        elif grant.can_force_push and grant.can_push:
> +            permissions.add('force_push')

This is unnecessarily complicated now that permissions is a set.  It can just be:

    if grant.can_force_push:
        # can_force_push implies can_push.
        permissions.add('push')
        permissions.add('force_push')

> +        return permissions
> +
> +    def _listRefRules(self, requester, translated_path):
> +        repository = removeSecurityProxy(
> +            getUtility(IGitLookup).getByHostingPath(translated_path))
> +        is_owner = requester.inTeam(repository.owner)
> +        grants = repository.findGrantsByGrantee(requester)
> +        # If the user is the owner, get the grants for REPOSITORY_OWNER
> +        # add add them to our available grants to match against

s/add add/and add/

> +        if is_owner:
> +            owner_grants = repository.findGrantsForRepositoryOwner()
> +            grants = grants.union(owner_grants)
> +        result = []
> +
> +        for rule in repository.rules:
> +            # Do we have any grants for this rule, for this user?
> +            matching_grants = [x for x in grants if x.rule == rule]
> +            # If we don't have any grants, but the user is the owner,
> +            # they get a default grant to the ref specified by the rule.
> +            if is_owner and not matching_grants:
> +                result.append(
> +                    {'ref_pattern': rule.ref_pattern,
> +                     'permissions': ['create', 'push'],
> +                    })
> +                continue

I made a previous review comment here that hasn't been handled yet:

This duplicates the owner_grant stuff below.  You can probably just drop the two continue blocks at the top of this for loop body, since it looks to me as though the rest of the body will already do the right thing if matching_grants is empty.  We don't need to micro-optimise this.

> +            # If we otherwise don't have any matching grants,
> +            # we can ignore this rule
> +            if not matching_grants:
> +                continue
> +
> +            # Permissions are a union of all the applicable grants
> +            union_permissions = set()
> +            for grant in matching_grants:
> +                permissions = self._buildPermissions(grant)
> +                union_permissions.update(permissions)
> +
> +            # If the user is the repository owner, they essentially have
> +            # the equivalent of a default team grant, but only
> +            # if there is no explicit grant to them otherwise specified
> +            if is_owner and not any(g for g in owner_grants if g.rule == rule):
> +                union_permissions.update(['create', 'push'])
> +
> +            # Sort the permissions from the set for consistency
> +            sorted_permissions = self._sortPermissions(union_permissions)
> +            result.append(
> +                {'ref_pattern': rule.ref_pattern,
> +                 'permissions': sorted_permissions,
> +                })
> +
> +        # The last rule for a repository owner is a default permission
> +        # for everything. This is overridden by any matching rules previously
> +        # in the list
> +        if is_owner:
> +            result.append(
> +                {'ref_pattern': '*',
> +                 'permissions': ['create', 'push', 'force_push'],
> +                })
> +
> +        return result
> +
> +    def listRefRules(self, translated_path, auth_params):
> +        """See `IGitAPI`"""
> +        requester_id = auth_params.get("uid")
> +        return run_with_login(
> +            requester_id,
> +            self._listRefRules,
> +            translated_path,
> +            )


-- 
https://code.launchpad.net/~twom/launchpad/branch-permissions-for-gitapi/+merge/355716
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.