launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'lib/lp/code/interfaces/gitapi.py'
> --- lib/lp/code/interfaces/gitapi.py	2015-03-31 04:18:22 +0000
> +++ lib/lp/code/interfaces/gitapi.py	2018-09-27 15:27:58 +0000
> @@ -67,3 +67,9 @@
>          :returns: An `Unauthorized` fault, as password authentication is
>              not yet supported.
>          """
> +
> +    def listRefRules(self, repository, user):
> +        """Return the list of RefRules for `user` in `repository`

Nitpicks: RefRule isn't a class, so it's probably best not to make it look like one here (just "ref rules" or similar).

> +
> +        :returns: A list of rules for the user in the specified repository
> +        """
> 
> === modified file 'lib/lp/code/model/gitlookup.py'
> --- lib/lp/code/model/gitlookup.py	2018-07-23 10:28:33 +0000
> +++ lib/lp/code/model/gitlookup.py	2018-09-27 15:27:58 +0000
> @@ -34,6 +34,7 @@
>  from lp.code.interfaces.gitrepository import IGitRepositorySet
>  from lp.code.interfaces.hasgitrepositories import IHasGitRepositories
>  from lp.code.model.gitrepository import GitRepository
> +from lp.code.model.gitrule import GitRuleGrant

This is unnecessary now.

>  from lp.registry.errors import NoSuchSourcePackageName
>  from lp.registry.interfaces.distribution import IDistribution
>  from lp.registry.interfaces.distributionsourcepackage import (
> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2018-09-27 15:27:58 +0000
> +++ lib/lp/code/model/gitrepository.py	2018-09-27 15:27:58 +0000
> @@ -1185,6 +1185,11 @@
>          return Store.of(self).find(
>              GitRuleGrant, GitRuleGrant.repository_id == self.id)
>  
> +    def findGrantsByGrantee(self, grantee):
> +        clauses = [TeamParticipation.person == grantee,
> +                   GitRuleGrant.grantee == TeamParticipation.teamID]
> +        return self.grants.find(*clauses)

This doesn't look right in the case where the grantee is an ordinary person rather than a team.

> +
>      def canBeDeleted(self):
>          """See `IGitRepository`."""
>          # Can't delete if the repository is associated with anything.
> 
> === 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-09-27 15:27:58 +0000
> @@ -325,3 +326,57 @@
>          else:
>              # Only macaroons are supported for password authentication.
>              return faults.Unauthorized()
> +
> +    def _isRepositoryOwner(self, requester, repository):
> +        try:
> +            return requester.inTeam(repository.owner)
> +        except Unauthorized:
> +            return False

As I suggested in an earlier review, I think the right answer is to use removeSecurityProxy(repository) rather than try / except Unauthorized, since this code is doing its own security checks.  Given that, you can probably inline this helper method.

> +
> +    def _buildPermissions(self, grants):
> +        permissions = []
> +        if any(grant.can_create for grant in grants):
> +            permissions.append("create")
> +        if any(grant.can_push for grant in grants):
> +            permissions.append("push")
> +        if any(grant.can_force_push for grant in grants):
> +            permissions.append("force-push")
> +        return permissions
> +
> +    def _listRefRules(self, requester, translated_path):
> +        repository = getUtility(IGitLookup).getByHostingPath(translated_path)
> +        try:
> +            grants = repository.findGrantsByGrantee(requester)
> +        except Unauthorized:
> +            return []
> +
> +        lines = []
> +        # Check for duplicate rules, and sort them by position
> +        rules = sorted(
> +            {x.rule for x in grants},
> +            key=attrgetter('position'),
> +            reverse=True)
> +        for rule in rules:
> +            # Find all the grants that apply for that rule
> +            matching_grants = [x for x in grants if x.rule == rule]
> +            # Collapse the permissions for all applicable grants
> +            permissions = self._buildPermissions(matching_grants)
> +            lines.append({'ref_pattern': rule.ref_pattern,
> +                          'permissions': permissions,
> +                         })

I think there are a couple of subtle logic errors here.  Here's what the spec says:

"""
If no GitRule matches a ref, then the repository owner (or members of the owning team) can create, push, or force-push that ref, but nobody else can modify it in any way.

If a GitRule matches a ref, and there is no explicit grant to the repository owner (or members of the owning team), then the repository owner can create or push that ref; if there is an explicit grant to the repository owner, then the repository owner has the permissions defined by that grant.  All other users have only the permissions defined by any associated grants.
"""

Consider this simple set of rules for a hypothetical repository owned by me:

  refs/heads/stable/*: [GitRuleGrant(twom, can_push=True)]

What happens if I try to push to refs/heads/stable/foo?  repository.findGrantsByGrantee(cjwatson) will return the empty list, so I'll get the default permissions of create + push + force-push; but the spec says that the repository owner should only get create + push in the case of a protected ref.

Furthermore, if the repository had this set of rules instead:

  refs/heads/stable/*: [GitRuleGrant(REPOSITORY_OWNER, can_create=True)]

... then the spec says I should only get create permission, but this code will still give me create + push + force-push because it doesn't handle repository owner grants.

You could solve the second problem with a bit more sophistication in findGrantsByGrantee, but I think that the first problem can't be solved that way, and so this method will need to be reworked somewhat.

I think a fairly minor rework should do the trick, though.  Instead of iterating over only the rules that are referenced by the grants you get back from findGrantsByGrantee, just iterate over repository.rules instead (which is already ordered by position).  You'll also need to handle the grantee_type == REPOSITORY_OWNER case somewhere, ideally in findGrantsByGrantee; and you'll need to handle the create + push default for the repository owner if there's no explicit grant to them.

> +
> +        if self._isRepositoryOwner(requester, repository):
> +            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,
> +            )
> 
> === modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
> --- lib/lp/code/xmlrpc/tests/test_git.py	2018-08-28 14:07:38 +0000
> +++ lib/lp/code/xmlrpc/tests/test_git.py	2018-09-27 15:27:58 +0000
> @@ -260,6 +265,220 @@
>          self.assertEqual(
>              initial_count, getUtility(IAllGitRepositories).count())
>  
> +    def test_listRefRules_simple(self):
> +        # Test that GitGrantRule (ref rule) can be retrieved for a user

There's no such thing as a GitGrantRule, and I'm not sure I quite understand this comment anyway.

> +        requester = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=requester, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(repository)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=requester, can_push=True, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': requester.id})
> +        self.assertThat(results, MatchesListwise([
> +            MatchesDict({
> +                'ref_pattern': Equals('refs/heads/*'),
> +                'permissions': Equals(['create', 'push']),
> +                }),
> +            MatchesDict({
> +                'ref_pattern': Equals('*'),
> +                'permissions': Equals(['create', 'push', 'force-push']),
> +                }),
> +            ]))
> +
> +    def test_listRefRules_with_other_grants(self):
> +        # Test that findGrantsByGrantee only returns relevant rules
> +        requester = self.factory.makePerson()
> +        other_user = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=requester, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(repository)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=requester, can_push=True)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=other_user, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': requester.id})
> +        self.assertThat(results, MatchesListwise([
> +            MatchesDict({
> +                'ref_pattern': Equals('refs/heads/*'),
> +                'permissions': Equals(['push']),
> +                }),
> +            MatchesDict({
> +                'ref_pattern': Equals('*'),
> +                'permissions': Equals(['create', 'push', 'force-push']),
> +                }),
> +            ]))
> +
> +    def test_listRefRules_no_grants(self):
> +        # User that has no grants and is not the owner
> +        requester = self.factory.makePerson()
> +        owner = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(repository)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': requester.id})
> +        self.assertEqual(0, len(results))
> +
> +    def test_listRefRules_owner_has_default(self):
> +        owner = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(
> +            repository=repository, ref_pattern=u'refs/heads/master')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': owner.id})
> +        self.assertEqual(len(results), 2)
> +        # Default grant should be last in pattern
> +        self.assertThat(results, MatchesListwise([
> +            MatchesDict({
> +                'ref_pattern': Equals('refs/heads/master'),
> +                'permissions': Equals(['create', 'push']),
> +                }),
> +            MatchesDict({
> +                'ref_pattern': Equals('*'),
> +                'permissions': Equals(['create', 'push', 'force-push']),
> +                }),
> +            ]))
> +
> +    def test_listRefRules_owner_is_team(self):
> +        member = self.factory.makePerson()
> +        owner = self.factory.makeTeam(members=[member])
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': member.id})
> +
> +        # Should have default grant as member of owning team
> +        self.assertThat(results, MatchesListwise([
> +            MatchesDict({
> +                'ref_pattern': Equals('*'),
> +                'permissions': Equals(['create', 'push', 'force-push']),
> +                }),
> +            ]))
> +
> +    def test_listRefRules_owner_is_team_with_grants(self):
> +        member = self.factory.makePerson()
> +        owner = self.factory.makeTeam(members=[member])
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(
> +            repository=repository, ref_pattern=u'refs/heads/master')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': member.id})
> +
> +        # Should have default grant as member of owning team
> +        self.assertThat(results, MatchesListwise([
> +            MatchesDict({
> +                'ref_pattern': Equals('refs/heads/master'),
> +                'permissions': Equals(['create', 'push']),
> +                }),
> +            MatchesDict({
> +                'ref_pattern': Equals('*'),
> +                'permissions': Equals(['create', 'push', 'force-push']),
> +                }),
> +            ]))
> +
> +    def test_listRefRules_owner_is_team_with_grants_to_person(self):
> +        member = self.factory.makePerson()
> +        other_member = self.factory.makePerson()
> +        owner = self.factory.makeTeam(members=[member, other_member])
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(
> +            repository=repository, ref_pattern=u'refs/heads/master')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True, can_create=True)
> +
> +        rule = self.factory.makeGitRule(
> +            repository=repository, ref_pattern=u'refs/heads/tags')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=member, can_create=True)
> +
> +        # This should not appear
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=other_member, can_push=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': member.id})
> +
> +        # Should have default grant as member of owning team
> +        self.assertThat(results, MatchesListwise([
> +            MatchesDict({
> +                'ref_pattern': Equals('refs/heads/tags'),
> +                'permissions': Equals(['create']),
> +                }),
> +            MatchesDict({
> +                'ref_pattern': Equals('refs/heads/master'),
> +                'permissions': Equals(['create', 'push']),
> +                }),
> +            MatchesDict({
> +                'ref_pattern': Equals('*'),
> +                'permissions': Equals(['create', 'push', 'force-push']),
> +                }),
> +            ]))
> +
> +    def test_listRefRules_multiple_grants_to_same_ref(self):
> +        member = self.factory.makePerson()
> +        owner = self.factory.makeTeam(members=[member])
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(repository=repository)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=member, can_create=True)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': member.id})
> +
> +        self.assertThat(results, MatchesListwise([
> +            MatchesDict({
> +                'ref_pattern': Equals('refs/heads/*'),
> +                'permissions': Equals(['create', 'push']),
> +                }),
> +            MatchesDict({
> +                'ref_pattern': Equals('*'),
> +                'permissions': Equals(['create', 'push', 'force-push']),
> +                }),
> +            ]))
> +
>  
>  class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
>      """Tests for the implementation of `IGitAPI`."""


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