launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22984
Re: [Merge] lp:~twom/launchpad/rework-git-permissions-for-shadowing into lp:launchpad
Review: Needs Fixing
Diff comments:
>
> === modified file 'lib/lp/code/interfaces/gitrule.py'
> --- lib/lp/code/interfaces/gitrule.py 2018-10-16 15:29:37 +0000
> +++ lib/lp/code/interfaces/gitrule.py 2018-10-18 13:49:45 +0000
> @@ -71,6 +71,16 @@
>
> grants = Attribute("The access grants for this rule.")
>
> + def findRuleGrantsByGrantee(grantee):
> + """Find the grants for a grantee applied to this repository.
> +
> + :param grantee: The Person affected
> + """
> +
> + def findRuleGrantsForRepositoryOwner():
> + """Find the grants of type REPOSITORY_OWNER applied to this repository.
> + """
s/this repository/this rule/ on both these docstrings, if you're leaving them here. (But see below; I think it might be better to move this back to IGitRepository.)
> +
>
> class IGitRuleEditableAttributes(Interface):
> """`IGitRule` attributes that can be edited.
>
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py 2018-10-17 10:09:33 +0000
> +++ lib/lp/code/xmlrpc/git.py 2018-10-18 13:49:45 +0000
> @@ -414,3 +418,70 @@
> self._listRefRules,
> translated_path,
> )
> +
> + def make_regex(self, pattern):
> + return re.compile(self.glob_to_re(pattern.rstrip(b'\n')))
> +
> + def glob_to_re(self, s):
> + """Convert a glob to a regular expression.
> +
> + The only wildcard supported is "*", to match any path segment.
This is now any number of characters, rather than just any path segment.
However, should we consider just using the standard library's `fnmatch` instead? That's what I'd specified before I realised that turnip had its own implementation, and I think it probably still makes sense: it's easy enough to explain, it's a bit more useful, and all its metacharacters are invalid in ref names.
> + """
> + return b'^%s\Z' % (
> + b''.join(b'.*' if c == b'*' else re.escape(c) for c in s))
> +
> + def _find_matching_rules(self, repository, ref_path):
> + """Find all matching rules for a given ref path"""
> +
Extra blank line.
> + matching_rules = []
> + for rule in repository.rules:
> + regex = self.make_regex(rule.ref_pattern)
> + if regex.match(ref_path):
> + matching_rules.append(rule)
> + return matching_rules
> +
> + def _checkRefPermissions(self, requester, translated_path, ref_paths):
> + repository = removeSecurityProxy(
> + getUtility(IGitLookup).getByHostingPath(translated_path))
> + is_owner = requester.inTeam(repository.owner)
> + result = {}
> + for ref in ref_paths:
> + matching_rules = self._find_matching_rules(repository, ref)
> + if is_owner and not matching_rules:
> + result[ref] = ['create', 'push', 'force_push']
> + continue
> + seen_grantees = []
This should be a set, so that membership checks are cheap.
> + union_permissions = set()
> + for rule in matching_rules:
> + grants = rule.findRuleGrantsByGrantee(requester)
> + if is_owner:
> + owner_grants = rule.findRuleGrantsForRepositoryOwner()
> + grants = grants.union(owner_grants)
This is potentially quite a lot of database queries: it's one or two per matching rule per ref being pushed. Instead, could you collect all the grants across the whole repository up-front, organise them into a dict indexed by rule, and look them up in that dict as you go? The DB indexes are arranged with the intent of making this an efficient operation.
> +
> + for grant in grants:
> + if (grant.grantee, grant.grantee_type) in seen_grantees:
> + continue
> + permissions = self._buildPermissions(grant)
> + union_permissions.update(permissions)
> + seen_grantees.append(
> + (grant.grantee, grant.grantee_type)
> + )
Could be on one line.
> +
> + owner_grantees = any(x[1] == GitGranteeType.REPOSITORY_OWNER
> + for x in seen_grantees)
> + if is_owner and not owner_grantees:
if is_owner and (None, GitGranteeType.REPOSITORY_OWNER) not in seen_grantees:
> + union_permissions.update(['create', 'push'])
> +
> + sorted_permissions = self._sortPermissions(union_permissions)
> + result[ref] = sorted_permissions
> + return result
> +
> + def checkRefPermissions(self, translated_path, ref_paths, auth_params):
> + """ See `IGitAPI`"""
> + requester_id = auth_params.get("uid")
> + return run_with_login(
> + requester_id,
> + self._checkRefPermissions,
> + translated_path,
> + ref_paths
> + )
>
> === modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
> --- lib/lp/code/xmlrpc/tests/test_git.py 2018-10-17 10:09:33 +0000
> +++ lib/lp/code/xmlrpc/tests/test_git.py 2018-10-18 13:49:45 +0000
> @@ -669,7 +669,7 @@
> }),
> ]))
>
> - def test_listRefRules_grantee_example_one(self):
> + def _make_example_repository(self):
It might be worth making this table-driven, especially once you add the second set of examples.
> user_a = self.factory.makePerson()
> user_b = self.factory.makePerson()
> user_c = self.factory.makePerson()
> @@ -710,247 +714,106 @@
> self.factory.makeGitRuleGrant(
> rule=rule, grantee=stable_team, can_create=True)
>
> - results = self.git_api.listRefRules(
> + return user_a, user_b, user_c, stable_team, next_team, repository
> +
> + def test_listRefRules_grantee_example_one(self):
Please rename all these tests to `test_checkRefPermissions_...` to avoid confusion.
> + test_ref_paths = [
> + 'refs/heads/stable/next', 'refs/heads/stable/protected',
> + 'refs/heads/stable/foo', 'refs/heads/archived/foo',
> + 'refs/heads/foo/next', 'refs/heads/unprotected',
> + 'refs/tags/1.0',
> + ]
`test_ref_paths` is common to each set of examples for this set of rules, so could you factor this out (maybe add it to the already large pile of stuff returned from `_make_example_repository`)?
> + #test_ref_paths = ['refs/heads/stable/next']
Don't leave commented code lying around.
> + user_a, _, _, _, _, repository = self._make_example_repository()
> +
> + results = self.git_api.checkRefPermissions(
> repository.getInternalPath(),
> + test_ref_paths,
> {'uid': user_a.id})
>
> - self.assertThat(results, MatchesListwise([
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/stable/next'),
> - 'permissions': Equals(['push', 'force_push']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/archived/*'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/stable/*'),
> - 'permissions': Equals(['create', 'push']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/*/next'),
> - 'permissions': Equals(['create', 'push']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/tags/*'),
> - 'permissions': Equals(['create']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('*'),
> - 'permissions': Equals(['create', 'push', 'force_push']),
> - }),
> - ]))
> + self.assertThat(results, MatchesDict({
> + 'refs/heads/stable/next': Equals(['push', 'force_push']),
> + 'refs/heads/stable/protected': Equals(['create', 'push']),
> + 'refs/heads/stable/foo': Equals(['create', 'push']),
> + 'refs/heads/archived/foo': Equals([]),
> + 'refs/heads/foo/next': Equals(['create', 'push']),
> + 'refs/heads/unprotected': Equals(['create', 'push', 'force_push']),
> + 'refs/tags/1.0': Equals(['create']),
> + }))
>
> def test_listRefRules_grantee_example_two(self):
> - user_a = self.factory.makePerson()
> - user_b = self.factory.makePerson()
> - user_c = self.factory.makePerson()
> - stable_team = self.factory.makeTeam(members=[user_a, user_b])
> - next_team = self.factory.makeTeam(members=[user_b, user_c])
> -
> - repository = removeSecurityProxy(
> - self.factory.makeGitRepository(owner=user_a))
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/stable/next')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a, can_force_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/archived/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a)
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_b, can_create=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/stable/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=stable_team, can_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/*/next')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=next_team, can_force_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/tags/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a, can_create=True)
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=stable_team, can_create=True)
> -
> - results = self.git_api.listRefRules(
> +
> + test_ref_paths = [
> + 'refs/heads/stable/next', 'refs/heads/stable/protected',
> + 'refs/heads/stable/foo', 'refs/heads/archived/foo',
> + 'refs/heads/foo/next', 'refs/heads/unprotected',
> + 'refs/tags/1.0',
> + ]
> + _, user_b, _, _, _, repository = self._make_example_repository()
> +
> + results = self.git_api.checkRefPermissions(
> repository.getInternalPath(),
> + test_ref_paths,
> {'uid': user_b.id})
>
> - self.assertThat(results, MatchesListwise([
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/stable/next'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/archived/*'),
> - 'permissions': Equals(['create']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/stable/*'),
> - 'permissions': Equals(['push']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/*/next'),
> - 'permissions': Equals(['push', 'force_push']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/tags/*'),
> - 'permissions': Equals(['create']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('*'),
> - 'permissions': Equals([]),
> - }),
> - ]))
> + self.assertThat(results, MatchesDict({
> + 'refs/heads/stable/next': Equals(['push', 'force_push']),
> + 'refs/heads/stable/protected': Equals([]),
> + 'refs/heads/stable/foo': Equals(['push']),
> + 'refs/heads/archived/foo': Equals(['create']),
> + 'refs/heads/foo/next': Equals(['push', 'force_push']),
> + 'refs/heads/unprotected': Equals([]),
> + 'refs/tags/1.0': Equals(['create']),
> + }))
>
> def test_listRefRules_grantee_example_three(self):
> - user_a = self.factory.makePerson()
> - user_b = self.factory.makePerson()
> - user_c = self.factory.makePerson()
> - stable_team = self.factory.makeTeam(members=[user_a, user_b])
> - next_team = self.factory.makeTeam(members=[user_b, user_c])
> -
> - repository = removeSecurityProxy(
> - self.factory.makeGitRepository(owner=user_a))
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/stable/next')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a, can_force_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/archived/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a)
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_b, can_create=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/stable/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=stable_team, can_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/*/next')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=next_team, can_force_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/tags/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a, can_create=True)
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=stable_team, can_create=True)
> -
> - results = self.git_api.listRefRules(
> + test_ref_paths = [
> + 'refs/heads/stable/next', 'refs/heads/stable/protected',
> + 'refs/heads/stable/foo', 'refs/heads/archived/foo',
> + 'refs/heads/foo/next', 'refs/heads/unprotected',
> + 'refs/tags/1.0',
> + ]
> + _, _, user_c, _, _, repository = self._make_example_repository()
> +
> + results = self.git_api.checkRefPermissions(
> repository.getInternalPath(),
> + test_ref_paths,
> {'uid': user_c.id})
>
> - self.assertThat(results, MatchesListwise([
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/stable/next'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/archived/*'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/stable/*'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/*/next'),
> - 'permissions': Equals(['push', 'force_push']),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/tags/*'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('*'),
> - 'permissions': Equals([]),
> - }),
> - ]))
> + self.assertThat(results, MatchesDict({
> + 'refs/heads/stable/next': Equals(['push', 'force_push']),
> + 'refs/heads/stable/protected': Equals([]),
> + 'refs/heads/stable/foo': Equals([]),
> + 'refs/heads/archived/foo': Equals([]),
> + 'refs/heads/foo/next': Equals(['push', 'force_push']),
> + 'refs/heads/unprotected': Equals([]),
> + 'refs/tags/1.0': Equals([]),
> + }))
>
> def test_listRefRules_grantee_example_four(self):
> - user_a = self.factory.makePerson()
> - user_b = self.factory.makePerson()
> - user_c = self.factory.makePerson()
> user_d = self.factory.makePerson()
> - stable_team = self.factory.makeTeam(members=[user_a, user_b])
> - next_team = self.factory.makeTeam(members=[user_b, user_c])
> -
> - repository = removeSecurityProxy(
> - self.factory.makeGitRepository(owner=user_a))
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/stable/next')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a, can_force_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/archived/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a)
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_b, can_create=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/stable/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=stable_team, can_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/heads/*/next')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=next_team, can_force_push=True)
> -
> - rule = self.factory.makeGitRule(
> - repository, ref_pattern=u'refs/tags/*')
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=user_a, can_create=True)
> - self.factory.makeGitRuleGrant(
> - rule=rule, grantee=stable_team, can_create=True)
> -
> - results = self.git_api.listRefRules(
> + test_ref_paths = [
> + 'refs/heads/stable/next', 'refs/heads/stable/protected',
> + 'refs/heads/stable/foo', 'refs/heads/archived/foo',
> + 'refs/heads/foo/next', 'refs/heads/unprotected',
> + 'refs/tags/1.0',
> + ]
> + _, _, user_c, _, _, repository = self._make_example_repository()
> +
> + results = self.git_api.checkRefPermissions(
> repository.getInternalPath(),
> + test_ref_paths,
> {'uid': user_d.id})
>
> - self.assertThat(results, MatchesListwise([
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/stable/next'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/archived/*'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/stable/*'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/heads/*/next'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('refs/tags/*'),
> - 'permissions': Equals([]),
> - }),
> - MatchesDict(
> - {'ref_pattern': Equals('*'),
> - 'permissions': Equals([]),
> - }),
> - ]))
> + self.assertThat(results, MatchesDict({
> + 'refs/heads/stable/next': Equals([]),
> + 'refs/heads/stable/protected': Equals([]),
> + 'refs/heads/stable/foo': Equals([]),
> + 'refs/heads/archived/foo': Equals([]),
> + 'refs/heads/foo/next': Equals([]),
> + 'refs/heads/unprotected': Equals([]),
> + 'refs/tags/1.0': Equals([]),
> + }))
Could you also add the other example (under "Alternatively:") in the spec? It tests some things that I'm not completely sure this one does: at any rate it's all been sufficiently subtle that I'd rather have them all.
>
>
> class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
--
https://code.launchpad.net/~twom/launchpad/rework-git-permissions-for-shadowing/+merge/357091
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
