launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-private-code-import into lp:launchpad.

Commit message:
Fix incorrect visibility check that broke code imports targeted at private Git repositories.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1789424 in Launchpad itself: "Can't import a public github branch into a Launchpad proprietary Git branch"
  https://bugs.launchpad.net/launchpad/+bug/1789424

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-private-code-import/+merge/353874
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-private-code-import into lp:launchpad.

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2016-10-14 11:48:57 +0000
+++ lib/lp/code/xmlrpc/git.py	2018-08-28 14:05:19 +0000
@@ -108,7 +108,8 @@
             # The authentication parameters specifically grant access to
             # this repository, so we can bypass other checks.
             # For the time being, this only works for code imports.
-            assert repository.repository_type == GitRepositoryType.IMPORTED
+            assert (
+                naked_repository.repository_type == GitRepositoryType.IMPORTED)
             hosting_path = naked_repository.getInternalPath()
             writable = True
             private = naked_repository.private

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2016-10-14 11:48:57 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2018-08-28 14:05:19 +0000
@@ -53,13 +53,14 @@
         self.repository_set = getUtility(IGitRepositorySet)
 
     def assertGitRepositoryNotFound(self, requester, path, permission="read",
-                                    can_authenticate=False):
+                                    can_authenticate=False, macaroon_raw=None):
         """Assert that the given path cannot be translated."""
         if requester is not None:
             requester = requester.id
-        fault = self.git_api.translatePath(
-            path, permission,
-            {"uid": requester, "can-authenticate": can_authenticate})
+        auth_params = {"uid": requester, "can-authenticate": can_authenticate}
+        if macaroon_raw is not None:
+            auth_params["macaroon"] = macaroon_raw
+        fault = self.git_api.translatePath(path, permission, auth_params)
         self.assertEqual(
             faults.GitRepositoryNotFound(path.strip("/")), fault)
 
@@ -145,8 +146,8 @@
         translation = self.git_api.translatePath(path, permission, auth_params)
         login(ANONYMOUS)
         self.assertEqual(
-            {"path": repository.getInternalPath(), "writable": writable,
-             "trailing": trailing, "private": private},
+            {"path": removeSecurityProxy(repository).getInternalPath(),
+             "writable": writable, "trailing": trailing, "private": private},
             translation)
 
     def assertCreates(self, requester, path, can_authenticate=False,
@@ -663,6 +664,47 @@
         self.assertPermissionDenied(
             None, path, permission="write", macaroon_raw="nonsense")
 
+    def test_translatePath_private_code_import(self):
+        # A code import worker with a suitable macaroon can write to 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]
+        path = u"/%s" % code_imports[0].git_repository.unique_name
+        self.assertPermissionDenied(
+            None, path, permission="write", macaroon_raw=macaroons[0])
+        with celebrity_logged_in("vcs_imports"):
+            getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
+        self.assertTranslates(
+            None, path, code_imports[0].git_repository, True,
+            permission="write", macaroon_raw=macaroons[0].serialize(),
+            private=True)
+        # The expected faults are slightly different from the public case,
+        # because we deny the existence of private repositories.
+        self.assertGitRepositoryNotFound(
+            None, path, permission="write",
+            macaroon_raw=macaroons[1].serialize())
+        self.assertGitRepositoryNotFound(
+            None, path, permission="write",
+            macaroon_raw=Macaroon(
+                location=config.vhost.mainsite.hostname,
+                identifier="another", key="another-secret").serialize())
+        self.assertGitRepositoryNotFound(
+            None, path, permission="write", macaroon_raw="nonsense")
+
     def test_notify(self):
         # The notify call creates a GitRefScanJob.
         repository = self.factory.makeGitRepository()