launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-checkRefPermissions-macaroon into lp:launchpad.

Commit message:
Make GitAPI.checkRefPermissions work for pushes from code import workers.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1517559 in Launchpad itself: "git fine-grained permissions"
  https://bugs.launchpad.net/launchpad/+bug/1517559

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-checkRefPermissions-macaroon/+merge/357732

See e.g. https://oops.canonical.com/oops/?oopsid=OOPS-030d238eea8c79e6d52e0205deafd603.

I've tried to follow the structure of translatePath and similar here.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-checkRefPermissions-macaroon into lp:launchpad.

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2018-10-23 16:36:25 +0000
+++ lib/lp/code/xmlrpc/git.py	2018-10-24 01:17:40 +0000
@@ -366,18 +366,35 @@
                 matching_rules.append(rule)
         return matching_rules
 
-    def _checkRefPermissions(self, requester, translated_path, ref_paths):
+    def _checkRefPermissions(self, requester, translated_path, ref_paths,
+                             auth_params):
+        if requester == LAUNCHPAD_ANONYMOUS:
+            requester = None
         repository = removeSecurityProxy(
             getUtility(IGitLookup).getByHostingPath(translated_path))
-        is_owner = requester.inTeam(repository.owner)
         result = {}
 
         grants_for_user = defaultdict(list)
-        grants = repository.findRuleGrantsByGrantee(requester)
-        if is_owner:
-            owner_grants = repository.findRuleGrantsByGrantee(
+        macaroon_raw = auth_params.get("macaroon")
+        if (macaroon_raw is not None and
+                self._verifyMacaroon(macaroon_raw, repository)):
+            # The authentication parameters grant access as an anonymous
+            # repository owner.
+            # For the time being, this only works for code imports.
+            assert repository.repository_type == GitRepositoryType.IMPORTED
+            is_owner = True
+            grants = repository.findRuleGrantsByGrantee(
                 GitGranteeType.REPOSITORY_OWNER)
-            grants = grants.union(owner_grants)
+        elif requester is None:
+            is_owner = False
+            grants = []
+        else:
+            is_owner = requester.inTeam(repository.owner)
+            grants = repository.findRuleGrantsByGrantee(requester)
+            if is_owner:
+                owner_grants = repository.findRuleGrantsByGrantee(
+                    GitGranteeType.REPOSITORY_OWNER)
+                grants = grants.union(owner_grants)
         for grant in grants:
             grants_for_user[grant.rule].append(grant)
 
@@ -407,9 +424,11 @@
     def checkRefPermissions(self, translated_path, ref_paths, auth_params):
         """ See `IGitAPI`"""
         requester_id = auth_params.get("uid")
+        if requester_id is None:
+            requester_id = LAUNCHPAD_ANONYMOUS
         return run_with_login(
             requester_id,
             self._checkRefPermissions,
             translated_path,
-            ref_paths
-        )
+            ref_paths,
+            auth_params)

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2018-10-23 16:36:45 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2018-10-24 01:17:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the internal Git API."""
@@ -8,7 +8,6 @@
 from pymacaroons import Macaroon
 from testtools.matchers import (
     Equals,
-    MatchesListwise,
     MatchesDict,
     )
 from zope.component import getUtility
@@ -187,6 +186,20 @@
             {"clone_from": cloned_from.getInternalPath()},
             self.hosting_fixture.create.extract_kwargs()[0])
 
+    def assertHasRefPermissions(self, requester, repository, ref_paths,
+                                permissions, macaroon_raw=None):
+        if requester is not None:
+            requester = requester.id
+        auth_params = {"uid": requester}
+        if macaroon_raw is not None:
+            auth_params["macaroon"] = macaroon_raw
+        translated_path = removeSecurityProxy(repository).getInternalPath()
+        results = self.git_api.checkRefPermissions(
+            translated_path, ref_paths, auth_params)
+        self.assertThat(results, MatchesDict({
+            ref_path: Equals(ref_permissions)
+            for ref_path, ref_permissions in permissions.items()}))
+
     def test_translatePath_private_repository(self):
         requester = self.factory.makePerson()
         repository = removeSecurityProxy(
@@ -893,7 +906,8 @@
             removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
         path = u"/%s" % code_imports[0].git_repository.unique_name
         self.assertPermissionDenied(
-            None, path, permission="write", macaroon_raw=macaroons[0])
+            None, path, permission="write",
+            macaroon_raw=macaroons[0].serialize())
         with celebrity_logged_in("vcs_imports"):
             getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
         self.assertTranslates(
@@ -931,7 +945,8 @@
             removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
         path = u"/%s" % code_imports[0].git_repository.unique_name
         self.assertPermissionDenied(
-            None, path, permission="write", macaroon_raw=macaroons[0])
+            None, path, permission="write",
+            macaroon_raw=macaroons[0].serialize())
         with celebrity_logged_in("vcs_imports"):
             getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
         self.assertTranslates(
@@ -1003,6 +1018,89 @@
             self.git_api.authenticateWithPassword("", "nonsense"),
             faults.Unauthorized)
 
+    def test_checkRefPermissions_code_import(self):
+        # A code import worker with a suitable macaroon has repository owner
+        # privileges on a repository associated with a running code import
+        # job.
+        self.pushConfig("codeimport", macaroon_secret_key="some-secret")
+        machine = self.factory.makeCodeImportMachine(set_online=True)
+        code_imports = [
+            self.factory.makeCodeImport(
+                target_rcs_type=TargetRevisionControlSystems.GIT)
+            for _ in range(2)]
+        with celebrity_logged_in("vcs_imports"):
+            jobs = [
+                self.factory.makeCodeImportJob(code_import=code_import)
+                for code_import in code_imports]
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroons = [
+            removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
+        repository = code_imports[0].git_repository
+        ref_path = "refs/heads/master"
+        self.assertHasRefPermissions(
+            None, repository, [ref_path], {ref_path: []},
+            macaroon_raw=macaroons[0].serialize())
+        with celebrity_logged_in("vcs_imports"):
+            getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
+        self.assertHasRefPermissions(
+            None, repository, [ref_path],
+            {ref_path: ["create", "push", "force_push"]},
+            macaroon_raw=macaroons[0].serialize())
+        self.assertHasRefPermissions(
+            None, repository, [ref_path], {ref_path: []},
+            macaroon_raw=macaroons[1].serialize())
+        self.assertHasRefPermissions(
+            None, repository, [ref_path], {ref_path: []},
+            macaroon_raw=Macaroon(
+                location=config.vhost.mainsite.hostname, identifier="another",
+                key="another-secret").serialize())
+        self.assertHasRefPermissions(
+            None, repository, [ref_path], {ref_path: []},
+            macaroon_raw="nonsense")
+
+    def test_checkRefPermissions_private_code_import(self):
+        # A code import worker with a suitable macaroon has repository owner
+        # privileges on a repository associated with a running private code
+        # import job.
+        self.pushConfig("codeimport", macaroon_secret_key="some-secret")
+        machine = self.factory.makeCodeImportMachine(set_online=True)
+        code_imports = [
+            self.factory.makeCodeImport(
+                target_rcs_type=TargetRevisionControlSystems.GIT)
+            for _ in range(2)]
+        private_repository = code_imports[0].git_repository
+        removeSecurityProxy(private_repository).transitionToInformationType(
+            InformationType.PRIVATESECURITY, private_repository.owner)
+        with celebrity_logged_in("vcs_imports"):
+            jobs = [
+                self.factory.makeCodeImportJob(code_import=code_import)
+                for code_import in code_imports]
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroons = [
+            removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
+        repository = code_imports[0].git_repository
+        ref_path = "refs/heads/master"
+        self.assertHasRefPermissions(
+            None, repository, [ref_path], {ref_path: []},
+            macaroon_raw=macaroons[0])
+        with celebrity_logged_in("vcs_imports"):
+            getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
+        self.assertHasRefPermissions(
+            None, repository, [ref_path],
+            {ref_path: ["create", "push", "force_push"]},
+            macaroon_raw=macaroons[0].serialize())
+        self.assertHasRefPermissions(
+            None, repository, [ref_path], {ref_path: []},
+            macaroon_raw=macaroons[1].serialize())
+        self.assertHasRefPermissions(
+            None, repository, [ref_path], {ref_path: []},
+            macaroon_raw=Macaroon(
+                location=config.vhost.mainsite.hostname, identifier="another",
+                key="another-secret").serialize())
+        self.assertHasRefPermissions(
+            None, repository, [ref_path], {ref_path: []},
+            macaroon_raw="nonsense")
+
 
 class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory):
     """Slow tests for `IGitAPI`.