Merge lp:~cjwatson/launchpad/fix-check-copy-permissions into lp:launchpad

[Colin Watson]

Always check copy permissions based on the component in the target archive, not the source archive.

== Summary ==

William Grant pointed out that my change in https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557 has incorrect semantics, because it checks the component of the source rather than of the target in the "series is None" (i.e. copy to same series as source) case.

== Proposed fix ==

Check the target component instead. This requires using archive.getPublishedSources; with some care, we can consolidate duplicate code again.

== LOC Rationale ==

+32. Same rationale as https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557 - removing delayed copies, which this is part of the road to, is worth at least 1100 lines.

== Tests ==

bin/test -vvct test_archive.TestSyncSource

== Demo and Q/A ==

As https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557, but copy into a distribution archive with various permissions. PPA publications are always in main, so the ability to copy a new version of a package in universe with only universe component upload permissions is a good indicator that this is working.

== Lint ==

''Paste output from `make lint` here - if there's lint related to your code, fix it so that you don't have to paste it here!''

[William Grant (wgrant)]

Thanks.

review: Approve (code)

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2012-06-26 12:25:42 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2012-07-01 17:22:21 +0000
@@ -953,7 +953,7 @@
             sourcepackagerelease = self.changes.dsc.storeInDatabase(build)
             package_upload_source = self.queue_root.addSource(
                 sourcepackagerelease)
-            ancestry = package_upload_source.getSourceAncestry()
+            ancestry = package_upload_source.getSourceAncestryForDiffs()
             if ancestry is not None:
                 to_sourcepackagerelease = ancestry.sourcepackagerelease
                 diff = to_sourcepackagerelease.requestDiffTo(

=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py	2012-05-30 08:50:50 +0000
+++ lib/lp/soyuz/interfaces/queue.py	2012-07-01 17:22:21 +0000
@@ -426,7 +426,7 @@
             readonly=False,
             )
 
-    def getSourceAncestry():
+    def getSourceAncestryForDiffs():
         """Return a suitable ancestry publication for this context.
 
         The possible ancestries locations for a give source upload, assuming

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2012-06-19 03:26:57 +0000
+++ lib/lp/soyuz/model/queue.py	2012-07-01 17:22:21 +0000
@@ -1091,7 +1091,7 @@
         dbName='sourcepackagerelease',
         foreignKey='SourcePackageRelease')
 
-    def getSourceAncestry(self):
+    def getSourceAncestryForDiffs(self):
         """See `IPackageUploadSource`."""
         primary_archive = self.packageupload.distroseries.main_archive
         release_pocket = PackagePublishingPocket.RELEASE
@@ -1103,18 +1103,17 @@
             (primary_archive, None, release_pocket),
             ]
 
-        ancestry = None
         for archive, distroseries, pocket in ancestry_locations:
             ancestries = archive.getPublishedSources(
                 name=self.sourcepackagerelease.name,
                 distroseries=distroseries, pocket=pocket,
                 exact_match=True)
             try:
-                ancestry = ancestries[0]
+                return ancestries[0]
             except IndexError:
-                continue
-            break
-        return ancestry
+                pass
+
+        return None
 
     def verifyBeforeAccept(self):
         """See `IPackageUploadSource`."""

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2012-06-29 01:03:10 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2012-07-01 17:22:21 +0000
@@ -16,6 +16,7 @@
     'update_files_privacy',
     ]
 
+from itertools import repeat
 from operator import attrgetter
 import os
 import tempfile
@@ -231,34 +232,41 @@
     # the destination (archive, component, pocket). This check is done
     # here rather than in the security adapter because it requires more
     # info than is available in the security adapter.
+    sourcepackagenames = [
+        source.sourcepackagerelease.sourcepackagename for source in sources]
     if series is None:
         # Use each source's series as the destination for that source.
-        for source in sources:
-            reason = archive.checkUpload(
-                person, source.distroseries,
-                source.sourcepackagerelease.sourcepackagename,
-                source.component, pocket, strict_component=True)
-
-            if reason is not None:
-                raise CannotCopy(reason)
+        series_iter = map(attrgetter("distroseries"), sources)
     else:
-        sourcepackagenames = [
-            source.sourcepackagerelease.sourcepackagename
-            for source in sources]
-        for spn in set(sourcepackagenames):
-            package = series.getSourcePackage(spn)
-            destination_component = package.latest_published_component
-
-            # If destination_component is not None, make sure the person
-            # has upload permission for this component.  Otherwise, any
-            # upload permission on this archive will do.
-            strict_component = destination_component is not None
-            reason = archive.checkUpload(
-                person, series, spn, destination_component, pocket,
-                strict_component=strict_component)
-
-            if reason is not None:
-                raise CannotCopy(reason)
+        series_iter = repeat(series)
+    for spn, dest_series in set(zip(sourcepackagenames, series_iter)):
+        # XXX cjwatson 20120630: We should do a proper ancestry check
+        # instead of simply querying for publications in any pocket.
+        # Unfortunately there are currently at least three different
+        # implementations of ancestry lookup:
+        # NascentUpload.getSourceAncestry,
+        # PackageUploadSource.getSourceAncestryForDiffs, and
+        # PublishingSet.getNearestAncestor, none of which is obviously
+        # correct here.  Instead of adding a fourth, we should consolidate
+        # these.
+        ancestries = archive.getPublishedSources(
+            name=spn.name, exact_match=True, status=active_publishing_status,
+            distroseries=dest_series)
+        try:
+            destination_component = ancestries[0].component
+        except IndexError:
+            destination_component = None
+
+        # If destination_component is not None, make sure the person
+        # has upload permission for this component.  Otherwise, any
+        # upload permission on this archive will do.
+        strict_component = destination_component is not None
+        reason = archive.checkUpload(
+            person, dest_series, spn, destination_component, pocket,
+            strict_component=strict_component)
+
+        if reason is not None:
+            raise CannotCopy(reason)
 
 
 class CopyChecker:

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-06-28 12:21:01 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-07-01 17:22:21 +0000
@@ -2548,6 +2548,39 @@
             [source.distroseries for source in sources],
             [copy_job.target_distroseries for copy_job in copy_jobs])
 
+    def test_copyPackages_with_default_distroseries_and_override(self):
+        # If to_series is None, copyPackages checks permissions based on the
+        # component in the target archive, not the component in the source
+        # archive.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data(
+            target_purpose=ArchivePurpose.PRIMARY)
+        sources = [source]
+        uploader = self.factory.makePerson()
+        main = self.factory.makeComponent(name="main")
+        universe = self.factory.makeComponent(name="universe")
+        ComponentSelection(distroseries=to_series, component=main)
+        ComponentSelection(distroseries=to_series, component=universe)
+        with person_logged_in(target_archive.owner):
+            target_archive.newComponentUploader(uploader, universe)
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=source.distroseries, archive=target_archive,
+            pocket=to_pocket, status=PackagePublishingStatus.PUBLISHED,
+            sourcepackagename=source_name, version="%s~1" % version,
+            component=universe)
+        names = [source.sourcepackagerelease.sourcepackagename.name
+                 for source in sources]
+
+        with person_logged_in(uploader):
+            target_archive.copyPackages(
+                names, source_archive, to_pocket.name, include_binaries=False,
+                person=uploader)
+
+        # There should be a copy job with the correct target series.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_job = job_source.getActiveJobs(target_archive).one()
+        self.assertEqual(source.distroseries, copy_job.target_distroseries)
+
 
 class TestgetAllPublishedBinaries(TestCaseWithFactory):
 

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-06-28 12:21:01 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-07-01 17:22:21 +0000
@@ -1030,19 +1030,20 @@
         spr.changelog = self.factory.makeLibraryFileAlias(content=changelog)
         spr.changelog_entry = "dummy"
         self.layer.txn.commit()  # Librarian.
+
+        # Now put the same named package in the target archive at the
+        # oldest version in the changelog.
+        target_source_pub = self.publisher.getPubSource(
+            distroseries=self.distroseries, sourcename="libc",
+            version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
+            archive=target_archive)
+
         bugtask280 = self.factory.makeBugTask(
-            target=spr.sourcepackage, bug=bug280)
+            target=spr.sourcepackage, bug=bug280, publish=False)
         bugtask281 = self.factory.makeBugTask(
-            target=spr.sourcepackage, bug=bug281)
+            target=spr.sourcepackage, bug=bug281, publish=False)
         bugtask282 = self.factory.makeBugTask(
-            target=spr.sourcepackage, bug=bug282)
-
-        # Now put the same named package in the target archive at the
-        # oldest version in the changelog.
-        self.publisher.getPubSource(
-            distroseries=self.distroseries, sourcename="libc",
-            version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
-            archive=target_archive)
+            target=spr.sourcepackage, bug=bug282, publish=False)
 
         # Run the copy job.
         requester = self.factory.makePerson()