launchpad-reviewers team mailing list archive

[Colin Watson <mp+426399@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:ci-build-upload-require-package into launchpad:master with ~cjwatson/launchpad:refactor-spr-creation as a prerequisite.

Commit message:
Only allow uploading CI builds for package namespaces

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

`Archive.uploadCIBuild` previously allowed uploads of CI builds from Git repositories in any namespace.  Constrain this to only allowing uploads from distribution source package namespaces.  This has the benefit that it means we always know the source package name, which allows checking finer-grained archive permissions and will shortly make it easier to upload source package files as well.

I don't believe that this will be a problem for any of the relatively small number of ways in which people are currently using this API on production.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:ci-build-upload-require-package into launchpad:master.

diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index 2a6d750..be93f27 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -80,6 +80,9 @@ from lp.registry.enums import (
     PersonVisibility,
     )
 from lp.registry.errors import NoSuchDistroSeries
+from lp.registry.interfaces.distributionsourcepackage import (
+    IDistributionSourcePackage,
+    )
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.distroseriesparent import IDistroSeriesParentSet
 from lp.registry.interfaces.gpg import IGPGKeySet
@@ -2018,18 +2021,20 @@ class Archive(SQLBase):
             raise CannotCopy(
                 "CI builds may only be uploaded to archives published using "
                 "Artifactory.")
+        dsp = ci_build.git_repository.target
+        if not IDistributionSourcePackage.providedBy(dsp):
+            raise CannotCopy(
+                "Only CI builds for repositories in package namespaces may be "
+                "uploaded to archives.")
         if ci_build.status != BuildStatus.FULLYBUILT:
             raise CannotCopy(
                 "%r has status '%s', not '%s'." %
                 (ci_build, ci_build.status.title, BuildStatus.FULLYBUILT))
-        # Check upload permissions.  We don't know the package name until we
-        # actually run the job; however, per-package upload permissions are
-        # by source package name, so don't necessarily make sense for CI
-        # builds anyway.  For now, just ignore per-package upload
-        # permissions.
+        # Check upload permissions.
         reason = self.checkUpload(
-            person=person, distroseries=series, sourcepackagename=None,
-            component=None, pocket=pocket)
+            person=person, distroseries=series,
+            sourcepackagename=dsp.sourcepackagename, component=None,
+            pocket=pocket)
         if reason is not None:
             raise CannotCopy(reason)
         getUtility(ICIBuildUploadJobSource).create(
diff --git a/lib/lp/soyuz/model/archivejob.py b/lib/lp/soyuz/model/archivejob.py
index 58a4356..e2bf402 100644
--- a/lib/lp/soyuz/model/archivejob.py
+++ b/lib/lp/soyuz/model/archivejob.py
@@ -28,6 +28,9 @@ import zstandard
 from lp.code.enums import RevisionStatusArtifactType
 from lp.code.interfaces.cibuild import ICIBuildSet
 from lp.code.interfaces.revisionstatus import IRevisionStatusArtifactSet
+from lp.registry.interfaces.distributionsourcepackage import (
+    IDistributionSourcePackage,
+    )
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.config import config
@@ -63,6 +66,9 @@ from lp.soyuz.interfaces.queue import IPackageUploadSet
 from lp.soyuz.model.archive import Archive
 
 
+logger = logging.getLogger(__name__)
+
+
 @implementer(IArchiveJob)
 class ArchiveJob(StormBase):
     """Base class for jobs related to Archives."""
@@ -187,7 +193,6 @@ class PackageUploadNotificationJob(ArchiveJobDerived):
             changes_file_object = None
         else:
             changes_file_object = io.BytesIO(packageupload.changesfile.read())
-        logger = logging.getLogger()
         packageupload.notify(
             status=self.packageupload_status, summary_text=self.summary_text,
             changes_file_object=changes_file_object, logger=logger)
@@ -372,7 +377,14 @@ class CIBuildUploadJob(ArchiveJobDerived):
 
     def run(self):
         """See `IRunnableJob`."""
-        logger = logging.getLogger()
+        build_target = self.ci_build.git_repository.target
+        if not IDistributionSourcePackage.providedBy(build_target):
+            # This should be caught by `Archive.uploadCIBuild`, but check it
+            # here as well just in case.
+            logger.warning(
+                "Source CI build is for %s, which is not a package",
+                repr(build_target))
+            return
         with tempfile.TemporaryDirectory(prefix="ci-build-copy-job") as tmpdir:
             releases = {
                 (release.binarypackagename, release.binpackageformat): release
diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
index ae21161..6554af0 100644
--- a/lib/lp/soyuz/tests/test_archive.py
+++ b/lib/lp/soyuz/tests/test_archive.py
@@ -3452,6 +3452,14 @@ class TestUploadCIBuild(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
+    def makeCIBuild(self, distribution, **kwargs):
+        # CIBuilds must be in a package namespace in order to be uploaded to
+        # an archive.
+        dsp = self.factory.makeDistributionSourcePackage(
+            distribution=distribution)
+        repository = self.factory.makeGitRepository(target=dsp)
+        return self.factory.makeCIBuild(git_repository=repository, **kwargs)
+
     def test_creates_job(self):
         # The uploadCIBuild method creates a CIBuildUploadJob with the
         # appropriate parameters.
@@ -3459,7 +3467,8 @@ class TestUploadCIBuild(TestCaseWithFactory):
             publishing_method=ArchivePublishingMethod.ARTIFACTORY)
         series = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild(status=BuildStatus.FULLYBUILT)
+        build = self.makeCIBuild(
+            archive.distribution, status=BuildStatus.FULLYBUILT)
         with person_logged_in(archive.owner):
             archive.uploadCIBuild(
                 build, archive.owner, series.name, "Release",
@@ -3477,7 +3486,8 @@ class TestUploadCIBuild(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         series = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild(status=BuildStatus.FULLYBUILT)
+        build = self.makeCIBuild(
+            archive.distribution, status=BuildStatus.FULLYBUILT)
         with person_logged_in(archive.owner):
             self.assertRaisesWithContent(
                 CannotCopy,
@@ -3486,13 +3496,30 @@ class TestUploadCIBuild(TestCaseWithFactory):
                 archive.uploadCIBuild,
                 build, archive.owner, series.name, "Release")
 
+    def test_disallows_non_package_namespace(self):
+        # Only CI builds for repositories in package namespaces may be
+        # copied into archives.
+        archive = self.factory.makeArchive(
+            publishing_method=ArchivePublishingMethod.ARTIFACTORY)
+        series = self.factory.makeDistroSeries(
+            distribution=archive.distribution)
+        build = self.factory.makeCIBuild(status=BuildStatus.FULLYBUILT)
+        with person_logged_in(archive.owner):
+            self.assertRaisesWithContent(
+                CannotCopy,
+                "Only CI builds for repositories in package namespaces may be "
+                "uploaded to archives.",
+                archive.uploadCIBuild,
+                build, archive.owner, series.name, "Release")
+
     def test_disallows_incomplete_builds(self):
         # CI builds with statuses other than FULLYBUILT may not be copied.
         archive = self.factory.makeArchive(
             publishing_method=ArchivePublishingMethod.ARTIFACTORY)
         series = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild(status=BuildStatus.FAILEDTOBUILD)
+        build = self.makeCIBuild(
+            archive.distribution, status=BuildStatus.FAILEDTOBUILD)
         person = self.factory.makePerson()
         self.assertRaisesWithContent(
             CannotCopy,
@@ -3506,7 +3533,8 @@ class TestUploadCIBuild(TestCaseWithFactory):
             publishing_method=ArchivePublishingMethod.ARTIFACTORY)
         series = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild(status=BuildStatus.FULLYBUILT)
+        build = self.makeCIBuild(
+            archive.distribution, status=BuildStatus.FULLYBUILT)
         person = self.factory.makePerson()
         self.assertRaisesWithContent(
             CannotCopy, "Signer has no upload rights to this PPA.",
diff --git a/lib/lp/soyuz/tests/test_archivejob.py b/lib/lp/soyuz/tests/test_archivejob.py
index 5696a0c..e0df1c8 100644
--- a/lib/lp/soyuz/tests/test_archivejob.py
+++ b/lib/lp/soyuz/tests/test_archivejob.py
@@ -156,11 +156,19 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
 
+    def makeCIBuild(self, distribution, **kwargs):
+        # CIBuilds must be in a package namespace in order to be uploaded to
+        # an archive.
+        dsp = self.factory.makeDistributionSourcePackage(
+            distribution=distribution)
+        repository = self.factory.makeGitRepository(target=dsp)
+        return self.factory.makeCIBuild(git_repository=repository, **kwargs)
+
     def test_repr_no_channel(self):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE)
@@ -173,7 +181,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -186,7 +194,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -206,7 +214,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -226,7 +234,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -246,7 +254,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -266,7 +274,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -287,7 +295,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -308,7 +316,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -329,7 +337,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
-        build = self.factory.makeCIBuild()
+        build = self.makeCIBuild(archive.distribution)
         job = CIBuildUploadJob.create(
             build, build.git_repository.owner, archive, distroseries,
             PackagePublishingPocket.RELEASE, target_channel="edge")
@@ -354,7 +362,8 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         dases = [
             self.factory.makeDistroArchSeries(distroseries=distroseries)
             for _ in range(2)]
-        build = self.factory.makeCIBuild(distro_arch_series=dases[0])
+        build = self.makeCIBuild(
+            archive.distribution, distro_arch_series=dases[0])
         report = build.getOrCreateRevisionStatusReport("build:0")
         report.setLog(b"log data")
         path = "wheel-indep/dist/wheel_indep-0.0.1-py3-none-any.whl"
@@ -402,7 +411,8 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         dases = [
             self.factory.makeDistroArchSeries(distroseries=distroseries)
             for _ in range(2)]
-        build = self.factory.makeCIBuild(distro_arch_series=dases[0])
+        build = self.makeCIBuild(
+            archive.distribution, distro_arch_series=dases[0])
         report = build.getOrCreateRevisionStatusReport("build:0")
         report.setLog(b"log data")
         path = "wheel-arch/dist/wheel_arch-0.0.1-cp310-cp310-linux_x86_64.whl"
@@ -449,7 +459,8 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         dases = [
             self.factory.makeDistroArchSeries(distroseries=distroseries)
             for _ in range(2)]
-        build = self.factory.makeCIBuild(distro_arch_series=dases[0])
+        build = self.makeCIBuild(
+            archive.distribution, distro_arch_series=dases[0])
         report = build.getOrCreateRevisionStatusReport("build:0")
         report.setLog(b"log data")
         path = "conda-arch/dist/linux-64/conda-arch-0.1-0.tar.bz2"
@@ -496,7 +507,8 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         dases = [
             self.factory.makeDistroArchSeries(distroseries=distroseries)
             for _ in range(2)]
-        build = self.factory.makeCIBuild(distro_arch_series=dases[0])
+        build = self.makeCIBuild(
+            archive.distribution, distro_arch_series=dases[0])
         report = build.getOrCreateRevisionStatusReport("build:0")
         report.setLog(b"log data")
         path = "conda-v2-arch/dist/linux-64/conda-v2-arch-0.1-0.conda"
@@ -544,7 +556,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
         das = self.factory.makeDistroArchSeries(distroseries=distroseries)
-        build = self.factory.makeCIBuild(distro_arch_series=das)
+        build = self.makeCIBuild(archive.distribution, distro_arch_series=das)
         report = build.getOrCreateRevisionStatusReport("build:0")
         path = "wheel-indep/dist/wheel_indep-0.0.1-py3-none-any.whl"
         with open(datadir(path), mode="rb") as f:
@@ -596,7 +608,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
         das = self.factory.makeDistroArchSeries(distroseries=distroseries)
-        build = self.factory.makeCIBuild(distro_arch_series=das)
+        build = self.makeCIBuild(archive.distribution, distro_arch_series=das)
         wheel_report = build.getOrCreateRevisionStatusReport("build-wheel:0")
         wheel_path = "wheel-indep/dist/wheel_indep-0.0.1-py3-none-any.whl"
         conda_path = "conda-v2-arch/dist/linux-64/conda-v2-arch-0.1-0.conda"
@@ -636,7 +648,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
         das = self.factory.makeDistroArchSeries(distroseries=distroseries)
-        build = self.factory.makeCIBuild(distro_arch_series=das)
+        build = self.makeCIBuild(archive.distribution, distro_arch_series=das)
         report = build.getOrCreateRevisionStatusReport("build:0")
         path = "wheel-indep/dist/wheel_indep-0.0.1-py3-none-any.whl"
         with open(datadir(path), mode="rb") as f:
@@ -671,7 +683,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
         das = self.factory.makeDistroArchSeries(distroseries=distroseries)
-        build = self.factory.makeCIBuild(distro_arch_series=das)
+        build = self.makeCIBuild(archive.distribution, distro_arch_series=das)
         report = build.getOrCreateRevisionStatusReport("build:0")
         path = "wheel-indep/dist/wheel_indep-0.0.1-py3-none-any.whl"
         with open(datadir(path), mode="rb") as f:
@@ -736,7 +748,11 @@ class TestViaCelery(TestCaseWithFactory):
         distroseries = self.factory.makeDistroSeries(
             distribution=archive.distribution)
         das = self.factory.makeDistroArchSeries(distroseries=distroseries)
-        build = self.factory.makeCIBuild(distro_arch_series=das)
+        dsp = self.factory.makeDistributionSourcePackage(
+            distribution=archive.distribution)
+        repository = self.factory.makeGitRepository(target=dsp)
+        build = self.factory.makeCIBuild(
+            git_repository=repository, distro_arch_series=das)
         report = build.getOrCreateRevisionStatusReport("build:0")
         path = "wheel-indep/dist/wheel_indep-0.0.1-py3-none-any.whl"
         with open(datadir(path), mode="rb") as f: