launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

Thanks, this is looking better now.  Just a few more bits and pieces.

A quick pass over the whole diff to conform to the prevailing style would be good (e.g. public methods of model objects get a """See `IFoo`.""" docstring, no blank line at the start of methods, multi-line displays generally have a newline after the opening punctuation and then a four-space indent for their contents rather than the contents being indented to the opening punctuation, etc.).

Diff comments:

> 
> === 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-01 16:28:22 +0000
> @@ -325,3 +328,91 @@
>          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 list of the available permissions from a GitRuleGrant"""
> +        permissions = []

This could be an actual set, since you're going to sort it later anyway, and especially since the caller updates a set with this anyway.

> +        if grant.can_create:
> +            permissions.append('create')
> +        if grant.can_push:
> +            permissions.append('push')
> +        if grant.can_force_push and not grant.can_push:
> +            permissions.append('push')
> +            permissions.append('force_push')
> +        elif grant.can_force_push and grant.can_push:
> +            permissions.append('force_push')
> +        return permissions
> +
> +    def _listRefRules(self, requester, translated_path):
> +        repository = removeSecurityProxy(
> +            getUtility(IGitLookup).getByHostingPath(translated_path))
> +        grants = repository.findGrantsByGrantee(requester)
> +        is_owner = requester.inTeam(repository.owner)
> +        lines = []

`lines` feels like an odd name, since this isn't a text-line-based protocol.  Maybe `result` or something?

> +
> +        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:
> +                lines.append({'ref_pattern': rule.ref_pattern,
> +                              'permissions': ['create', 'push'],
> +                             })
> +                continue

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
> +            owner_grant = any(
> +                (g.grantee_type == GitGranteeType.REPOSITORY_OWNER or
> +                 g.grantee.inTeam(repository.owner))

Please remove the `g.grantee.inTeam(repository.owner)` condition.  These aren't equivalent: a grant to REPOSITORY_OWNER tracks the current owner of the repository, which is mutable, whereas a grant to the person/team that happens to own the repository right now is fixed to that particular person/team unless the grant itself is changed.  The intent of the spec was that only a grant to REPOSITORY_OWNER overrides the default create+push permissions.

> +                 for g in matching_grants)

It seems awkward that we have to check this both in findGrantsForGrantee and here.  How about splitting out a findGrantForRepositoryOwner which could be called only if the requester participates in repository ownership?  I think that would result in clearer logic.

You don't have to do it in this MP, but I wonder if promoting the create / push / force_push strings to a proper EnumeratedType (e.g. GitRulePermission) that's formatted only at the boundary would result in clearer code.  We could then have findPermissionsForRepositoryOwner which could handle the default behaviour.

> +            if is_owner and not owner_grant:
> +                union_permissions.update(['create', 'push'])
> +
> +            # Sort the permissions from the set for consistency
> +            sorted_permissions = self._sortPermissions(union_permissions)
> +            lines.append({'ref_pattern': rule.ref_pattern,
> +                          'permissions': sorted_permissions,
> +                         })
> +
> +        # The last rule for a repository owner is a default permission
> +        # for everything. This is overriden by any matching rules previously

"overridden"

> +        # in the list
> +        if is_owner:
> +            lines.append({'ref_pattern': '*',
> +                          'permissions': ['create', 'push', 'force_push'],
> +                         })
> +
> +        return lines
> +
> +    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.