launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:archive-append-mark-suite-dirty into launchpad:master.

Commit message:
Move markSuiteDirty to IArchiveAppend

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1874859 in Launchpad itself: "Ubuntu archive admins can't mark suites dirty"
  https://bugs.launchpad.net/launchpad/+bug/1874859

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

`markSuiteDirty` allows forcing a suite in an archive to be republished as if by an upload without actually uploading to it.  It was previously in `IArchiveEdit`, which requires the distribution owner in the case of primary archives and the archive owner in all other cases.  This turns out to be inconvenient in practice for Ubuntu, where the archive owner is ~ubuntu-archive and handles day-to-day archive administration work, and the distribution owner is ~techboard which doesn't.

There isn't an obvious perfect fit here, but `IArchiveAppend` seems a lot closer: it requires the archive owner or somebody with upload permission in the case of PPAs, and the archive owner in all other cases.  The only significant downside is that Launchpad administrators won't be able to use it any more, but I think this is tolerable.  If it becomes a problem, we can probably press the `launchpad.Owner` permission into service, since it's currently unused for archives.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:archive-append-mark-suite-dirty into launchpad:master.

diff --git a/lib/lp/soyuz/interfaces/archive.py b/lib/lp/soyuz/interfaces/archive.py
index e4929b7..1752ba3 100644
--- a/lib/lp/soyuz/interfaces/archive.py
+++ b/lib/lp/soyuz/interfaces/archive.py
@@ -1831,6 +1831,26 @@ class IArchiveAppend(Interface):
         :param job_id: The ID of the `PlainPackageCopyJob` to be removed.
         """
 
+    @operation_parameters(
+        distroseries=Reference(
+            IDistroSeries, title=_("Distro series"), required=True),
+        pocket=Choice(
+            title=_("Pocket"),
+            vocabulary=PackagePublishingPocket,
+            required=True),
+        )
+    @export_write_operation()
+    @operation_for_version("devel")
+    def markSuiteDirty(distroseries, pocket):
+        """Mark a suite as dirty in this archive.
+
+        The next publisher run will publish this suite regardless of whether
+        it has any pending publications.
+
+        :param distroseries: An `IDistroSeries`.
+        :param pocket: A `PackagePublishingPocket`.
+        """
+
 
 class IArchiveEdit(Interface):
     """Archive interface for operations restricted by edit privilege."""
@@ -2219,26 +2239,6 @@ class IArchiveEdit(Interface):
         :param names: A list of token names.
         """
 
-    @operation_parameters(
-        distroseries=Reference(
-            IDistroSeries, title=_("Distro series"), required=True),
-        pocket=Choice(
-            title=_("Pocket"),
-            vocabulary=PackagePublishingPocket,
-            required=True),
-        )
-    @export_write_operation()
-    @operation_for_version("devel")
-    def markSuiteDirty(distroseries, pocket):
-        """Mark a suite as dirty in this archive.
-
-        The next publisher run will publish this suite regardless of whether
-        it has any pending publications.
-
-        :param distroseries: An `IDistroSeries`.
-        :param pocket: A `PackagePublishingPocket`.
-        """
-
 
 class IArchiveDelete(Interface):
     """Archive interface for operations restricted by delete privilege."""
diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
index 425355b..c600cde 100644
--- a/lib/lp/soyuz/tests/test_archive.py
+++ b/lib/lp/soyuz/tests/test_archive.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test Archive features."""
@@ -4584,10 +4584,36 @@ class TestMarkSuiteDirty(TestCaseWithFactory):
         archive = self.factory.makeArchive()
         self.assertIsNone(archive.dirty_suites)
 
-    def test_requires_owner(self):
+    def test_unprivileged_disallowed(self):
         archive = self.factory.makeArchive()
         self.assertRaises(Unauthorized, getattr, archive, "markSuiteDirty")
 
+    def test_primary_archive_uploader_disallowed(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+        person = self.factory.makePerson()
+        with person_logged_in(archive.distribution.owner):
+            archive.newComponentUploader(person, "main")
+        with person_logged_in(person):
+            self.assertRaises(Unauthorized, getattr, archive, "markSuiteDirty")
+
+    def test_primary_archive_owner_allowed(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+        # Simulate Ubuntu's situation where the primary archive is owned by
+        # an archive admin team while the distribution is owned by the
+        # technical board, allowing us to tell the difference between
+        # permissions granted to the archive owner vs. the distribution
+        # owner.
+        with person_logged_in(archive.distribution.owner):
+            archive.distribution.owner = (
+                getUtility(ILaunchpadCelebrities).ubuntu_techboard)
+        distroseries = self.factory.makeDistroSeries(
+            distribution=archive.distribution)
+        with person_logged_in(archive.owner):
+            archive.markSuiteDirty(
+                distroseries, PackagePublishingPocket.UPDATES)
+        self.assertEqual(
+            ["%s-updates" % distroseries.name], archive.dirty_suites)
+
     def test_first_suite(self):
         archive = self.factory.makeArchive()
         distroseries = self.factory.makeDistroSeries(