Merge lp:~cjwatson/launchpad/uefi-copy-no-auto-approve into lp:launchpad

[Colin Watson]

Explicitly select the target archive when creating CustomUploadCopiers, rather than trying to infer it and thereby refusing to copy out of PPAs.

== Summary ==

Bug 1036616: we can't stage custom uploads in PPAs, because the custom uploads copier doesn't work quite right. This is about to become very important to Ubuntu Engineering in terms of how we handle UEFI secure boot: we'll need it for security updates post-release, and it appears that we may need it very soon pre-release as well for signed kernels because the workflow for uploading kernels to the development series involves staging them in a PPA.

== Proposed fix ==

As described in the bug report, I first had to pre-emptively secure against the ability to bypass the forced UNAPPROVED logic for UEFI custom uploads to the primary archive by way of copying them from a PPA.

After that, the problem was that CustomUploadCopier.getTargetArchive was just wrong. It was trying to infer the target archive based on the series and the original archive, always selecting one with the same purpose. Obviously that won't work for copying from a PPA to the primary archive. Furthermore, this convoluted logic was unnecessary anyway. There are exactly two non-test callers of CustomUploadCopier, one in lib/lp/archivepublisher/scripts/publish_ftpmaster.py (which is always copying between RELEASE pockets of two series in the same distribution, and thus always within the same archive) and the other in lib/lp/soyuz/scripts/packagecopier.py which already knows the target archive.

== Tests ==

bin/test -vvct lp.archivepublisher.tests.test_publish_ftpmaster -t lp.soyuz.scripts.tests.test_copypackage -t lp.soyuz.scripts.tests.test_custom_uploads_copier

== Demo and Q/A ==

On dogfood, attempt to copy an efilinux upload from https://dogfood.launchpad.net/~cjwatson/+archive/ppa/+packages to another PPA and to the primary archive. Both should work and the custom upload should be copied. In the case of copying to the primary archive, it should land in the UNAPPROVED queue.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/soyuz/scripts/custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/custom_uploads_copier.py	2012-07-03 17:20:05 +0000
+++ lib/lp/soyuz/scripts/custom_uploads_copier.py	2012-09-24 23:55:27 +0000
@@ -14,8 +14,6 @@
 
 from operator import attrgetter
 
-from zope.component import getUtility
-
 from lp.archivepublisher.ddtp_tarball import DdtpTarballUpload
 from lp.archivepublisher.debian_installer import DebianInstallerUpload
 from lp.archivepublisher.dist_upgrader import DistUpgraderUpload
@@ -23,10 +21,6 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.database.bulk import load_referencing
 from lp.soyuz.enums import PackageUploadCustomFormat
-from lp.soyuz.interfaces.archive import (
-    IArchiveSet,
-    MAIN_ARCHIVE_PURPOSES,
-    )
 from lp.soyuz.model.queue import PackageUploadCustom
 
 
@@ -47,14 +41,27 @@
         }
 
     def __init__(self, target_series,
-                 target_pocket=PackagePublishingPocket.RELEASE):
+                 target_pocket=PackagePublishingPocket.RELEASE,
+                 target_archive=None):
         self.target_series = target_series
         self.target_pocket = target_pocket
+        self.target_archive = target_archive
 
     def isCopyable(self, upload):
         """Is `upload` the kind of `PackageUploadCustom` that we can copy?"""
         return upload.customformat in self.copyable_types
 
+    def autoApprove(self, custom):
+        """Can `custom` be automatically approved?"""
+        # XXX cjwatson 2012-08-16: This more or less duplicates
+        # BuildDaemonUploadPolicy.autoApprove/CustomUploadFile.autoApprove.
+        if custom.packageupload.archive.is_ppa:
+            return True
+        # UEFI uploads are signed, and must therefore be approved by a human.
+        if custom.customformat == PackageUploadCustomFormat.UEFI:
+            return False
+        return True
+
     def getCandidateUploads(self, source_series,
                             source_pocket=PackagePublishingPocket.RELEASE):
         """Find custom uploads that may need copying."""
@@ -99,19 +106,6 @@
                 latest_uploads.setdefault(key, upload)
         return latest_uploads
 
-    def getTargetArchive(self, original_archive):
-        """Find counterpart of `original_archive` in `self.target_series`.
-
-        :param original_archive: The `Archive` that the original upload went
-            into.  If this is not a primary, partner, or debug archive,
-            None is returned.
-        :return: The `Archive` of the same purpose for `self.target_series`.
-        """
-        if original_archive.purpose not in MAIN_ARCHIVE_PURPOSES:
-            return None
-        return getUtility(IArchiveSet).getByDistroPurpose(
-            self.target_series.distribution, original_archive.purpose)
-
     def isObsolete(self, upload, target_uploads):
         """Is `upload` superseded by one that the target series already has?
 
@@ -123,16 +117,20 @@
 
     def copyUpload(self, original_upload):
         """Copy `original_upload` into `self.target_series`."""
-        target_archive = self.getTargetArchive(
-            original_upload.packageupload.archive)
-        if target_archive is None:
-            return None
+        if self.target_archive is None:
+            # Copy within the same archive.
+            target_archive = original_upload.packageupload.archive
+        else:
+            target_archive = self.target_archive
         package_upload = self.target_series.createQueueEntry(
             self.target_pocket, target_archive,
             changes_file_alias=original_upload.packageupload.changesfile)
         custom = package_upload.addCustom(
             original_upload.libraryfilealias, original_upload.customformat)
-        package_upload.setAccepted()
+        if self.autoApprove(custom):
+            package_upload.setAccepted()
+        else:
+            package_upload.setUnapproved()
         return custom
 
     def copy(self, source_series,

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2012-08-08 16:34:39 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2012-09-24 23:55:27 +0000
@@ -45,6 +45,7 @@
     )
 from lp.soyuz.scripts.custom_uploads_copier import CustomUploadsCopier
 
+
 # XXX cprov 2009-06-12: this function should be incorporated in
 # IPublishing.
 def update_files_privacy(pub_record):
@@ -785,7 +786,8 @@
     if custom_files:
         # Custom uploads aren't modelled as publication history records, so
         # we have to send these through the upload queue.
-        custom_copier = CustomUploadsCopier(series, target_pocket=pocket)
+        custom_copier = CustomUploadsCopier(
+            series, target_pocket=pocket, target_archive=archive)
         for custom in custom_files:
             if custom_copier.isCopyable(custom):
                 custom_copier.copyUpload(custom)

=== modified file 'lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-07-03 17:20:05 +0000
+++ lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-09-24 23:55:27 +0000
@@ -8,11 +8,9 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.soyuz.enums import (
-    ArchivePurpose,
     PackageUploadCustomFormat,
     PackageUploadStatus,
     )
-from lp.soyuz.interfaces.archive import MAIN_ARCHIVE_PURPOSES
 from lp.soyuz.scripts.custom_uploads_copier import CustomUploadsCopier
 from lp.testing import TestCaseWithFactory
 from lp.testing.fakemethod import FakeMethod
@@ -96,8 +94,7 @@
         # If extractSeriesKey returns None, getKey also returns None.
         copier = CustomUploadsCopier(FakeDistroSeries())
         copier.extractSeriesKey = FakeMethod()
-        self.assertIs(
-            None,
+        self.assertIsNone(
             copier.getKey(FakeUpload(
                 PackageUploadCustomFormat.DEBIAN_INSTALLER,
                 "bad-filename.tar")))
@@ -109,7 +106,7 @@
     # Alas, PackageUploadCustom relies on the Librarian.
     layer = LaunchpadZopelessLayer
 
-    def makeUpload(self, distroseries=None, pocket=None,
+    def makeUpload(self, distroseries=None, archive=None, pocket=None,
                    custom_type=PackageUploadCustomFormat.DEBIAN_INSTALLER,
                    version=None, arch=None, component=None):
         """Create a `PackageUploadCustom`."""
@@ -128,8 +125,8 @@
                 arch = self.factory.getUniqueString()
             filename = "%s.tar.gz" % "_".join([package_name, version, arch])
         package_upload = self.factory.makeCustomPackageUpload(
-            distroseries=distroseries, pocket=pocket, custom_type=custom_type,
-            filename=filename)
+            distroseries=distroseries, archive=archive, pocket=pocket,
+            custom_type=custom_type, filename=filename)
         return package_upload.customfiles[0]
 
     def test_copies_custom_upload(self):
@@ -241,10 +238,7 @@
             source_series, custom_type=PackageUploadCustomFormat.DIST_UPGRADER,
             arch='mips')
         copier = CustomUploadsCopier(FakeDistroSeries())
-        expected_key = (
-            PackageUploadCustomFormat.DIST_UPGRADER,
-            'mips',
-            )
+        expected_key = (PackageUploadCustomFormat.DIST_UPGRADER, 'mips')
         self.assertEqual(expected_key, copier.getKey(upload))
 
     def test_getKey_ddtp_includes_format_and_component(self):
@@ -255,10 +249,7 @@
             source_series, custom_type=PackageUploadCustomFormat.DDTP_TARBALL,
             component='restricted')
         copier = CustomUploadsCopier(FakeDistroSeries())
-        expected_key = (
-            PackageUploadCustomFormat.DDTP_TARBALL,
-            'restricted',
-            )
+        expected_key = (PackageUploadCustomFormat.DDTP_TARBALL, 'restricted')
         self.assertEqual(expected_key, copier.getKey(upload))
 
     def test_getLatestUploads_indexes_uploads_by_key(self):
@@ -298,60 +289,6 @@
         self.assertContentEqual(
             uploads[-1:], copier.getLatestUploads(source_series).values())
 
-    def test_getTargetArchive_on_same_distro_is_same_archive(self):
-        # When copying within the same distribution, getTargetArchive
-        # always returns the same archive you feed it.
-        distro = self.factory.makeDistribution()
-        archives = [
-            self.factory.makeArchive(distribution=distro, purpose=purpose)
-            for purpose in MAIN_ARCHIVE_PURPOSES]
-        copier = CustomUploadsCopier(self.factory.makeDistroSeries(distro))
-        self.assertEqual(
-            archives,
-            [copier.getTargetArchive(archive) for archive in archives])
-
-    def test_getTargetArchive_returns_None_if_not_distribution_archive(self):
-        # getTargetArchive returns None for any archive that is not a
-        # distribution archive, regardless of whether the target series
-        # has an equivalent.
-        distro = self.factory.makeDistribution()
-        archives = [
-            self.factory.makeArchive(distribution=distro, purpose=purpose)
-            for purpose in ArchivePurpose.items
-                if purpose not in MAIN_ARCHIVE_PURPOSES]
-        copier = CustomUploadsCopier(self.factory.makeDistroSeries(distro))
-        self.assertEqual(
-            [None] * len(archives),
-            [copier.getTargetArchive(archive) for archive in archives])
-
-    def test_getTargetArchive_finds_matching_archive(self):
-        # When copying across archives, getTargetArchive looks for an
-        # archive for the target series with the same purpose as the
-        # original archive.
-        source_series = self.factory.makeDistroSeries()
-        source_archive = self.factory.makeArchive(
-            distribution=source_series.distribution,
-            purpose=ArchivePurpose.PARTNER)
-        target_series = self.factory.makeDistroSeries()
-        target_archive = self.factory.makeArchive(
-            distribution=target_series.distribution,
-            purpose=ArchivePurpose.PARTNER)
-
-        copier = CustomUploadsCopier(target_series)
-        self.assertEqual(
-            target_archive, copier.getTargetArchive(source_archive))
-
-    def test_getTargetArchive_returns_None_if_no_archive_matches(self):
-        # If the target series has no archive to match the archive that
-        # the original upload was far, it returns None.
-        source_series = self.factory.makeDistroSeries()
-        source_archive = self.factory.makeArchive(
-            distribution=source_series.distribution,
-            purpose=ArchivePurpose.PARTNER)
-        target_series = self.factory.makeDistroSeries()
-        copier = CustomUploadsCopier(target_series)
-        self.assertIs(None, copier.getTargetArchive(source_archive))
-
     def test_isObsolete_returns_False_if_no_equivalent_in_target(self):
         # isObsolete returns False if the upload in question has no
         # equivalent in the target series.
@@ -393,7 +330,8 @@
         # original, but for the target series.
         original_upload = self.makeUpload()
         target_series = self.factory.makeDistroSeries()
-        copier = CustomUploadsCopier(target_series)
+        copier = CustomUploadsCopier(
+            target_series, target_archive=target_series.main_archive)
         copied_upload = copier.copyUpload(original_upload)
         self.assertEqual([copied_upload], list_custom_uploads(target_series))
         self.assertNotEqual(
@@ -443,13 +381,68 @@
         self.assertEqual(
             PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
 
-    def test_copyUpload_does_not_copy_if_no_archive_matches(self):
-        # If getTargetArchive does not find an appropriate target
-        # archive, copyUpload does nothing.
-        source_series = self.factory.makeDistroSeries()
-        upload = self.makeUpload(distroseries=source_series)
-        target_series = self.factory.makeDistroSeries()
-        copier = CustomUploadsCopier(target_series)
-        copier.getTargetArchive = FakeMethod(result=None)
-        self.assertIs(None, copier.copyUpload(upload))
-        self.assertEqual([], list_custom_uploads(target_series))
+    def test_copyUpload_unapproves_uefi(self):
+        # Copies of UEFI custom uploads to a primary archive are set to
+        # UNAPPROVED, since they will normally end up being signed.
+        original_upload = self.makeUpload(
+            custom_type=PackageUploadCustomFormat.UEFI)
+        target_series = self.factory.makeDistroSeries()
+        copier = CustomUploadsCopier(target_series)
+        copied_upload = copier.copyUpload(original_upload)
+        self.assertEqual(
+            PackageUploadStatus.UNAPPROVED, copied_upload.packageupload.status)
+
+    def test_copyUpload_approves_uefi_to_ppa(self):
+        # Copies of UEFI custom uploads to a PPA are automatically accepted,
+        # since PPAs have much more limited upload permissions than the main
+        # archive, and in any case PPAs do not have an upload approval
+        # workflow.
+        original_upload = self.makeUpload(
+            custom_type=PackageUploadCustomFormat.UEFI)
+        target_series = self.factory.makeDistroSeries()
+        target_archive = self.factory.makeArchive(
+            distribution=target_series.distribution)
+        copier = CustomUploadsCopier(
+            target_series, target_archive=target_archive)
+        copied_upload = copier.copyUpload(original_upload)
+        self.assertEqual(
+            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
+
+    def test_copyUpload_archive_None_copies_within_archive(self):
+        # If CustomUploadsCopier was created with no target archive,
+        # copyUpload copies an upload to the same archive as the original
+        # upload.
+        original_upload = self.makeUpload()
+        target_series = self.factory.makeDistroSeries()
+        copier = CustomUploadsCopier(target_series)
+        copied_upload = copier.copyUpload(original_upload)
+        self.assertEqual(
+            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
+        self.assertEqual(
+            original_upload.packageupload.archive,
+            copied_upload.packageupload.archive)
+
+    def test_copyUpload_to_specified_archive(self):
+        # If CustomUploadsCopier was created with a target archive,
+        # copyUpload copies an upload to that archive.
+        series = self.factory.makeDistroSeries()
+        original_upload = self.makeUpload(distroseries=series)
+        archive = self.factory.makeArchive(distribution=series.distribution)
+        copier = CustomUploadsCopier(series, target_archive=archive)
+        copied_upload = copier.copyUpload(original_upload)
+        self.assertEqual(
+            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
+        self.assertEqual(archive, copied_upload.packageupload.archive)
+
+    def test_copyUpload_from_ppa_to_main_archive(self):
+        # copyUpload can copy uploads from a PPA to the main archive.
+        series = self.factory.makeDistroSeries()
+        archive = self.factory.makeArchive(distribution=series.distribution)
+        original_upload = self.makeUpload(distroseries=series, archive=archive)
+        copier = CustomUploadsCopier(
+            series, target_archive=series.main_archive)
+        copied_upload = copier.copyUpload(original_upload)
+        self.assertEqual(
+            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
+        self.assertEqual(
+            series.main_archive, copied_upload.packageupload.archive)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-09-20 15:36:33 +0000
+++ lib/lp/testing/factory.py	2012-09-24 23:55:27 +0000
@@ -3553,16 +3553,17 @@
             component=component)
         return upload
 
-    def makeCustomPackageUpload(self, distroseries=None, pocket=None,
-                                custom_type=None, filename=None):
+    def makeCustomPackageUpload(self, distroseries=None, archive=None,
+                                pocket=None, custom_type=None, filename=None):
         """Make a `PackageUpload` with a `PackageUploadCustom` attached."""
         if distroseries is None:
             distroseries = self.makeDistroSeries()
+        if archive is None:
+            archive = distroseries.main_archive
         if custom_type is None:
             custom_type = PackageUploadCustomFormat.DEBIAN_INSTALLER
         upload = self.makePackageUpload(
-            distroseries=distroseries, archive=distroseries.main_archive,
-            pocket=pocket)
+            distroseries=distroseries, archive=archive, pocket=pocket)
         file_alias = self.makeLibraryFileAlias(filename=filename)
         upload.addCustom(file_alias, custom_type)
         return upload

=== modified file 'lib/lp/testing/tests/test_factory.py'
--- lib/lp/testing/tests/test_factory.py	2012-07-03 10:29:53 +0000
+++ lib/lp/testing/tests/test_factory.py	2012-09-24 23:55:27 +0000
@@ -807,14 +807,16 @@
 
     def test_makeCustomPackageUpload_passes_on_args(self):
         distroseries = self.factory.makeDistroSeries()
+        archive = self.factory.makeArchive()
         custom_type = PackageUploadCustomFormat.ROSETTA_TRANSLATIONS
         filename = self.factory.getUniqueString()
         pu = self.factory.makeCustomPackageUpload(
-            distroseries=distroseries, pocket=PackagePublishingPocket.PROPOSED,
-            custom_type=custom_type, filename=filename)
+            distroseries=distroseries, archive=archive,
+            pocket=PackagePublishingPocket.PROPOSED, custom_type=custom_type,
+            filename=filename)
         custom = list(pu.customfiles)[0]
         self.assertEqual(distroseries, pu.distroseries)
-        self.assertEqual(distroseries.distribution, pu.archive.distribution)
+        self.assertEqual(archive, pu.archive)
         self.assertEqual(PackagePublishingPocket.PROPOSED, pu.pocket)
         self.assertEqual(custom_type, custom.customformat)
         self.assertEqual(filename, custom.libraryfilealias.filename)