launchpad-reviewers team mailing list archive

[Colin Watson <mp+419984@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:git-private-packages into launchpad:master.

Commit message:
Fix Git traversal for elements of private pillars

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1969646 in Launchpad itself: "accessing code in private distributions results in unexpected error"
  https://bugs.launchpad.net/launchpad/+bug/1969646

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/419984

Correctly supporting anonymous requests to Git repositories in any of the following targets needs some work:

 * source packages in private distributions
 * OCI projects in private distributions
 * OCI projects in private projects

In order to tell anonymous requests that they need to authenticate before we can determine whether they can see repositories in these targets, we need to deny their existence when unauthorized; this is our general policy nowadays anyway (see https://warthogs.atlassian.net/browse/LP-312), and `GitAPI._translatePath` already has special handling of not-found errors for anonymous requests that advertise the ability to authenticate.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-private-packages into launchpad:master.

diff --git a/lib/lp/code/model/gitlookup.py b/lib/lp/code/model/gitlookup.py
index 6c5746f..e09427b 100644
--- a/lib/lp/code/model/gitlookup.py
+++ b/lib/lp/code/model/gitlookup.py
@@ -18,6 +18,7 @@ from zope.component import (
     queryMultiAdapter,
     )
 from zope.interface import implementer
+from zope.security.interfaces import Unauthorized
 
 from lp.app.errors import NameLookupFailed
 from lp.app.validators.name import valid_name
@@ -147,7 +148,10 @@ class ProjectGitTraversable(_BaseGitTraversable):
                 ociproject_name = next(segments)
             except StopIteration:
                 raise InvalidNamespace("/".join(segments.traversed))
-            oci_project = self.context.getOCIProject(ociproject_name)
+            try:
+                oci_project = self.context.getOCIProject(ociproject_name)
+            except Unauthorized:
+                oci_project = None
             if oci_project is None:
                 raise NoSuchOCIProjectName(ociproject_name)
             return owner, oci_project, None
@@ -184,12 +188,18 @@ class DistributionGitTraversable(_BaseGitTraversable):
         except StopIteration:
             raise InvalidNamespace("/".join(segments.traversed))
         if name == "+source":
-            distro_source_package = self.context.getSourcePackage(spn_name)
+            try:
+                distro_source_package = self.context.getSourcePackage(spn_name)
+            except Unauthorized:
+                distro_source_package = None
             if distro_source_package is None:
                 raise NoSuchSourcePackageName(spn_name)
             return owner, distro_source_package, None
         elif name == "+oci":
-            oci_project = self.context.getOCIProject(spn_name)
+            try:
+                oci_project = self.context.getOCIProject(spn_name)
+            except Unauthorized:
+                oci_project = None
             if oci_project is None:
                 raise NoSuchOCIProjectName(spn_name)
             return owner, oci_project, None
diff --git a/lib/lp/code/model/tests/test_gitlookup.py b/lib/lp/code/model/tests/test_gitlookup.py
index 8914d4c..266c7b3 100644
--- a/lib/lp/code/model/tests/test_gitlookup.py
+++ b/lib/lp/code/model/tests/test_gitlookup.py
@@ -6,6 +6,7 @@
 from lazr.uri import URI
 from zope.component import getUtility
 
+from lp.app.enums import InformationType
 from lp.code.errors import (
     InvalidNamespace,
     NoSuchGitRepository,
@@ -384,6 +385,33 @@ class TestGitTraverser(TestCaseWithFactory):
                 dsp.distribution.name, dsp.sourcepackagename.name,
                 repository.name))
 
+    def test_visible_package_in_private_distribution(self):
+        # `traverse_path` resolves 'distro/+source/package' to the
+        # distribution source package even if the distribution is private.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            distro = self.factory.makeDistribution(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            dsp = self.factory.makeDistributionSourcePackage(
+                distribution=distro)
+            path = "%s/+source/%s" % (
+                dsp.distribution.name, dsp.sourcepackagename.name)
+            self.assertTraverses(path, None, dsp)
+
+    def test_invisible_package_in_private_distribution(self):
+        # `traverse_path` raises `NoSuchSourcePackageName` (denying
+        # existence in order to avoid disclosing whether the object exists)
+        # if it cannot traverse to a package due to the distribution being
+        # private.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            distro = self.factory.makeDistribution(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            dsp = self.factory.makeDistributionSourcePackage(
+                distribution=distro)
+            path = "%s/+source/%s" % (
+                dsp.distribution.name, dsp.sourcepackagename.name)
+        self.assertRaises(
+            NoSuchSourcePackageName, self.traverser.traverse_path, path)
+
     def test_missing_ociprojectname(self):
         # `traverse_path` raises `InvalidNamespace` if there are no segments
         # after '+oci'.
@@ -426,6 +454,52 @@ class TestGitTraverser(TestCaseWithFactory):
             "%s/+oci/%s/+git/%s" % (
                 oci_project.pillar.name, oci_project.name, repository.name))
 
+    def test_visible_ociproject_in_private_distribution(self):
+        # `traverse_path` resolves 'distro/+oci/ociproject' to the OCI
+        # project even if the distribution is private.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            distro = self.factory.makeDistribution(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            oci_project = self.factory.makeOCIProject(pillar=distro)
+            path = "%s/+oci/%s" % (oci_project.pillar.name, oci_project.name)
+            self.assertTraverses(path, None, oci_project)
+
+    def test_invisible_ociproject_in_private_distribution(self):
+        # `traverse_path` raises `NoSuchOCIProjectName` (denying existence
+        # in order to avoid disclosing whether the object exists) if it
+        # cannot traverse to an OCI project due to the distribution being
+        # private.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            distro = self.factory.makeDistribution(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            oci_project = self.factory.makeOCIProject(pillar=distro)
+            path = "%s/+oci/%s" % (oci_project.pillar.name, oci_project.name)
+        self.assertRaises(
+            NoSuchOCIProjectName, self.traverser.traverse_path, path)
+
+    def test_visible_ociproject_in_private_project(self):
+        # `traverse_path` resolves 'project/+oci/ociproject' to the OCI
+        # project even if the project is private.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            project = self.factory.makeProduct(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            oci_project = self.factory.makeOCIProject(pillar=project)
+            path = "%s/+oci/%s" % (oci_project.pillar.name, oci_project.name)
+            self.assertTraverses(path, None, oci_project)
+
+    def test_invisible_ociproject_in_private_project(self):
+        # `traverse_path` raises `NoSuchOCIProjectName` (denying existence
+        # in order to avoid disclosing whether the object exists) if it
+        # cannot traverse to an OCI project due to the project being
+        # private.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            project = self.factory.makeProduct(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            oci_project = self.factory.makeOCIProject(pillar=project)
+            path = "%s/+oci/%s" % (oci_project.pillar.name, oci_project.name)
+        self.assertRaises(
+            NoSuchOCIProjectName, self.traverser.traverse_path, path)
+
     def test_nonexistent_person(self):
         # `traverse_path` raises `NoSuchPerson` when resolving a path of
         # '~person/project' if the person doesn't exist.
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 9dd57c1..baad855 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -514,6 +514,20 @@ class TestGitAPIMixin:
         self.assertGitRepositoryNotFound(None, path, can_authenticate=False)
         self.assertUnauthorized(None, path, can_authenticate=True)
 
+    def test_translatePath_anonymous_cannot_see_private_distro_repository(
+            self):
+        with person_logged_in(self.factory.makePerson()) as owner:
+            distro = self.factory.makeDistribution(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            dsp = self.factory.makeDistributionSourcePackage(
+                distribution=distro)
+            repository = removeSecurityProxy(
+                self.factory.makeGitRepository(
+                    registrant=owner, owner=owner, target=dsp))
+            path = "/%s" % repository.unique_name
+        self.assertGitRepositoryNotFound(None, path, can_authenticate=False)
+        self.assertUnauthorized(None, path, can_authenticate=True)
+
     def test_translatePath_team_unowned(self):
         requester = self.factory.makePerson()
         team = self.factory.makeTeam(self.factory.makePerson())