launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py	2018-10-22 10:37:03 +0000
> +++ lib/lp/code/xmlrpc/git.py	2018-10-23 16:17:49 +0000
> @@ -103,7 +103,7 @@
>          else:
>              return issuer.checkMacaroonIssuer(macaroon)
>  
> -    def _performLookup(self, path, auth_params):
> +    def _performLookup(self, path, auth_params, requester):

Maybe move requester first, for consistency with _createRepository etc.?

>          repository, extra_path = getUtility(IGitLookup).getByPath(path)
>          if repository is None:
>              return None
> @@ -282,11 +289,11 @@
>          if requester == LAUNCHPAD_ANONYMOUS:
>              requester = None
>          try:
> -            result = self._performLookup(path, auth_params)
> +            result = self._performLookup(path, auth_params, requester)
>              if (result is None and requester is not None and
>                  permission == "write"):
> -                self._createRepository(requester, path)
> -                result = self._performLookup(path, auth_params)
> +                self._createRepository(requester, path, requester)

This looks like a mistake, and will do weird stuff because we'll end up with clone_from=requester.

> +                result = self._performLookup(path, auth_params, requester)
>              if result is None:
>                  raise faults.GitRepositoryNotFound(path)
>              if permission != "read" and not result["writable"]:
> 
> === modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
> --- lib/lp/code/xmlrpc/tests/test_git.py	2018-10-18 15:07:49 +0000
> +++ lib/lp/code/xmlrpc/tests/test_git.py	2018-10-23 16:17:49 +0000
> @@ -266,6 +266,34 @@
>          self.assertEqual(
>              initial_count, getUtility(IAllGitRepositories).count())
>  
> +    def test_translatePath_grant_to_other(self):
> +        requester = self.factory.makePerson()
> +        other_person = self.factory.makePerson()
> +        repository = self.factory.makeGitRepository(owner=requester)
> +        rule = self.factory.makeGitRule(
> +            repository, ref_pattern=u'refs/heads/stable/next')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=other_person,
> +            can_force_push=True)
> +        path = u"/%s" % repository.unique_name
> +        self.assertTranslates(
> +            other_person, path, repository, True, private=False)

I'd also like to see a test for the case where there are grants but they don't match the requester.

> +
> +    def test_translatePath_grant_to_other_private(self):
> +        requester = self.factory.makePerson()
> +        other_person = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=requester, information_type=InformationType.USERDATA))
> +        rule = self.factory.makeGitRule(
> +            repository, ref_pattern=u'refs/heads/stable/next')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=other_person,
> +            can_force_push=True)
> +        path = u"/%s" % repository.unique_name
> +        self.assertGitRepositoryNotFound(
> +            other_person, path, can_authenticate=True)
> +
>      def _make_scenario_one_repository(self):
>          user_a = self.factory.makePerson()
>          user_b = self.factory.makePerson()


-- 
https://code.launchpad.net/~twom/launchpad/widen-performLookup-account-for-grants/+merge/357706
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.