launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@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.
> +        """

Done

> +
>      @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')

Ah, thanks!

> +        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

changed.

> +        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

The first block should be possible to collapse into the lower owner_grants check, but I'd prefer to do that separately as it requires some other refactoring to ensure it's in the right order, as this check must occur before the one below it.
The second block (`if not matching_grants`) is required as it stands, it's the difference between no result (no grant for this ref) and an empty list of permissions (explicit rule that removes all permissions).

> +            # 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.