launchpad-reviewers team mailing list archive

[Colin Watson <mp+431535@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:archive-translate-path-private-urls into launchpad:master.

Commit message:
Include tokens in URLs returned by ArchiveAPI.translatePath

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Files in the restricted librarian can only be accessed with some kind of authentication token, either a `TimeLimitedToken` or a macaroon; without adding something like that to the URLs returned by `ArchiveAPI.translatePath`, there's no way to use it to access files in private PPAs.  Macaroons don't offer any advantages in this case since we can't constrain them in any interesting way, but a simple `TimeLimitedToken` is fine.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:archive-translate-path-private-urls into launchpad:master.

diff --git a/lib/lp/soyuz/xmlrpc/archive.py b/lib/lp/soyuz/xmlrpc/archive.py
index a23b7fa..c0e7b25 100644
--- a/lib/lp/soyuz/xmlrpc/archive.py
+++ b/lib/lp/soyuz/xmlrpc/archive.py
@@ -144,7 +144,7 @@ class ArchiveAPI(LaunchpadXMLRPCView):
             path.as_posix(),
             archive_file.library_file.id,
         )
-        return archive_file.library_file.getURL()
+        return archive_file.library_file.getURL(include_token=True)
 
     def _translatePathNonPool(
         self, archive_reference: str, archive, path: PurePath
@@ -163,7 +163,7 @@ class ArchiveAPI(LaunchpadXMLRPCView):
             path.as_posix(),
             archive_file.library_file.id,
         )
-        return archive_file.library_file.getURL()
+        return archive_file.library_file.getURL(include_token=True)
 
     def _translatePathPool(
         self, archive_reference: str, archive, path: PurePath
@@ -178,7 +178,7 @@ class ArchiveAPI(LaunchpadXMLRPCView):
             path.as_posix(),
             lfa.id,
         )
-        return lfa.getURL()
+        return lfa.getURL(include_token=True)
 
     @return_fault
     def _translatePath(self, archive_reference: str, path: PurePath) -> str:
diff --git a/lib/lp/soyuz/xmlrpc/tests/test_archive.py b/lib/lp/soyuz/xmlrpc/tests/test_archive.py
index cdb7d7d..c8ba612 100644
--- a/lib/lp/soyuz/xmlrpc/tests/test_archive.py
+++ b/lib/lp/soyuz/xmlrpc/tests/test_archive.py
@@ -14,7 +14,7 @@ from lp.services.macaroons.interfaces import IMacaroonIssuer
 from lp.soyuz.enums import ArchiveRepositoryFormat, PackagePublishingStatus
 from lp.soyuz.interfaces.archive import NAMED_AUTH_TOKEN_FEATURE_FLAG
 from lp.soyuz.xmlrpc.archive import ArchiveAPI
-from lp.testing import TestCaseWithFactory
+from lp.testing import TestCaseWithFactory, person_logged_in
 from lp.testing.layers import LaunchpadFunctionalLayer
 from lp.xmlrpc import faults
 
@@ -324,6 +324,31 @@ class TestArchiveAPI(TestCaseWithFactory):
             % (archive.reference, path, archive_file.library_file.id)
         )
 
+    def test_translatePath_by_hash_checksum_found_private(self):
+        archive = removeSecurityProxy(self.factory.makeArchive(private=True))
+        self.factory.makeArchiveFile(
+            archive=archive,
+            container="release:jammy",
+            path="dists/jammy/InRelease",
+        )
+        archive_file = self.factory.makeArchiveFile(
+            archive=archive,
+            container="release:jammy",
+            path="dists/jammy/InRelease",
+        )
+        path = (
+            "dists/jammy/by-hash/SHA256/%s"
+            % archive_file.library_file.content.sha256
+        )
+        self.assertStartsWith(
+            archive_file.library_file.getURL() + "?token=",
+            self.archive_api.translatePath(archive.reference, path),
+        )
+        self.assertLogs(
+            "%s: %s (by-hash) -> LFA %d"
+            % (archive.reference, path, archive_file.library_file.id)
+        )
+
     def test_translatePath_non_pool_not_found(self):
         archive = removeSecurityProxy(self.factory.makeArchive())
         self.factory.makeArchiveFile(archive=archive)
@@ -354,6 +379,25 @@ class TestArchiveAPI(TestCaseWithFactory):
             )
         )
 
+    def test_translatePath_non_pool_found_private(self):
+        archive = removeSecurityProxy(self.factory.makeArchive(private=True))
+        self.factory.makeArchiveFile(archive=archive)
+        archive_file = self.factory.makeArchiveFile(archive=archive)
+        self.assertStartsWith(
+            archive_file.library_file.getURL() + "?token=",
+            self.archive_api.translatePath(
+                archive.reference, archive_file.path
+            ),
+        )
+        self.assertLogs(
+            "%s: %s (non-pool) -> LFA %d"
+            % (
+                archive.reference,
+                archive_file.path,
+                archive_file.library_file.id,
+            )
+        )
+
     def test_translatePath_pool_bad_file_name(self):
         archive = removeSecurityProxy(self.factory.makeArchive())
         path = "pool/nonexistent"
@@ -413,6 +457,38 @@ class TestArchiveAPI(TestCaseWithFactory):
             % (archive.reference, path, sprf.libraryfile.id)
         )
 
+    def test_translatePath_pool_source_found_private(self):
+        archive = removeSecurityProxy(self.factory.makeArchive(private=True))
+        with person_logged_in(archive.owner):
+            spph = self.factory.makeSourcePackagePublishingHistory(
+                archive=archive,
+                status=PackagePublishingStatus.PUBLISHED,
+                sourcepackagename="test-package",
+                component="main",
+            )
+            sprf = self.factory.makeSourcePackageReleaseFile(
+                sourcepackagerelease=spph.sourcepackagerelease,
+                library_file=self.factory.makeLibraryFileAlias(
+                    filename="test-package_1.dsc", db_only=True
+                ),
+            )
+            self.factory.makeSourcePackageReleaseFile(
+                sourcepackagerelease=spph.sourcepackagerelease,
+                library_file=self.factory.makeLibraryFileAlias(
+                    filename="test-package_1.tar.xz", db_only=True
+                ),
+            )
+            IStore(sprf).flush()
+        path = "pool/main/t/test-package/test-package_1.dsc"
+        self.assertStartsWith(
+            sprf.libraryfile.getURL() + "?token=",
+            self.archive_api.translatePath(archive.reference, path),
+        )
+        self.assertLogs(
+            "%s: %s (pool) -> LFA %d"
+            % (archive.reference, path, sprf.libraryfile.id)
+        )
+
     def test_translatePath_pool_binary_not_found(self):
         archive = removeSecurityProxy(self.factory.makeArchive())
         self.factory.makeBinaryPackagePublishingHistory(
@@ -466,3 +542,41 @@ class TestArchiveAPI(TestCaseWithFactory):
             "%s: %s (pool) -> LFA %d"
             % (archive.reference, path, bpf.libraryfile.id)
         )
+
+    def test_translatePath_pool_binary_found_private(self):
+        archive = removeSecurityProxy(self.factory.makeArchive(private=True))
+        with person_logged_in(archive.owner):
+            bpph = self.factory.makeBinaryPackagePublishingHistory(
+                archive=archive,
+                status=PackagePublishingStatus.PUBLISHED,
+                sourcepackagename="test-package",
+                component="main",
+            )
+            bpf = self.factory.makeBinaryPackageFile(
+                binarypackagerelease=bpph.binarypackagerelease,
+                library_file=self.factory.makeLibraryFileAlias(
+                    filename="test-package_1_amd64.deb", db_only=True
+                ),
+            )
+            bpph2 = self.factory.makeBinaryPackagePublishingHistory(
+                archive=archive,
+                status=PackagePublishingStatus.PUBLISHED,
+                sourcepackagename="test-package",
+                component="main",
+            )
+            self.factory.makeBinaryPackageFile(
+                binarypackagerelease=bpph2.binarypackagerelease,
+                library_file=self.factory.makeLibraryFileAlias(
+                    filename="test-package_1_i386.deb", db_only=True
+                ),
+            )
+            IStore(bpf).flush()
+        path = "pool/main/t/test-package/test-package_1_amd64.deb"
+        self.assertStartsWith(
+            bpf.libraryfile.getURL() + "?token=",
+            self.archive_api.translatePath(archive.reference, path),
+        )
+        self.assertLogs(
+            "%s: %s (pool) -> LFA %d"
+            % (archive.reference, path, bpf.libraryfile.id)
+        )