launchpad-reviewers team mailing list archive

[Colin Watson <mp+437678@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:artifactory-ppa-privatize into launchpad:master.

Commit message:
Allow making non-empty Artifactory PPAs private

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

The reasons why we can't change the privacy of non-empty locally-published PPAs don't quite apply in the Artifactory case.  There are still some reasons to be careful, but we can at least give ourselves the ability to fix the failure mode where we've forgotten to make an archive private.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:artifactory-ppa-privatize into launchpad:master.

diff --git a/lib/lp/soyuz/browser/archive.py b/lib/lp/soyuz/browser/archive.py
index 6bbca05..c47b057 100644
--- a/lib/lp/soyuz/browser/archive.py
+++ b/lib/lp/soyuz/browser/archive.py
@@ -114,6 +114,7 @@ from lp.soyuz.browser.sourceslist import SourcesListEntriesWidget
 from lp.soyuz.browser.widgets.archive import PPANameWidget
 from lp.soyuz.enums import (
     ArchivePermissionType,
+    ArchivePublishingMethod,
     ArchiveStatus,
     PackageCopyPolicy,
     PackagePublishingStatus,
@@ -2394,14 +2395,40 @@ class ArchiveAdminView(BaseArchiveEditView, EnableProcessorsMixin):
         """Validate the save action on ArchiveAdminView."""
         super().validate_save(action, data)
 
+        # XXX cjwatson 2023-02-22: This check duplicates
+        # Archive._validate_archive_privacy.  Can we avoid this and just
+        # catch exceptions somewhere?
         if data.get("private") != self.context.private:
             # The privacy is being switched.
             if not self.context.getPublishedSources().is_empty():
-                self.setFieldError(
-                    "private",
-                    "This archive already has published sources. It is "
-                    "not possible to switch the privacy.",
-                )
+                if (
+                    # For local publishing, we can't switch privacy after
+                    # anything has been published to it, because the publisher
+                    # uses different paths on disk for public and private
+                    # archives.
+                    self.context.publishing_method
+                    == ArchivePublishingMethod.LOCAL
+                    # Refuse to switch from private to public even for
+                    # non-local publishing, partly as a safety measure and
+                    # partly because the files in the archive would have to be
+                    # unrestricted and we don't have code to do that yet.
+                    #
+                    # Switching an archive from public to private should
+                    # ideally also restrict any files published only in that
+                    # archive.  However, that's quite complex because we'd
+                    # have to check whether the files are published anywhere
+                    # else, and nothing breaks if we leave the files alone.
+                    # It's not ideal that we might have files reachable from
+                    # the public librarian that are now only published in a
+                    # private archive, but since Launchpad won't give out any
+                    # links to those, it could be worse.
+                    or self.context.private
+                ):
+                    self.setFieldError(
+                        "private",
+                        "This archive already has published sources. It is "
+                        "not possible to switch the privacy.",
+                    )
 
         if self.owner_is_private_team and not data["private"]:
             self.setFieldError(
diff --git a/lib/lp/soyuz/browser/tests/test_archive_admin_view.py b/lib/lp/soyuz/browser/tests/test_archive_admin_view.py
index d31966c..fe9cd31 100644
--- a/lib/lp/soyuz/browser/tests/test_archive_admin_view.py
+++ b/lib/lp/soyuz/browser/tests/test_archive_admin_view.py
@@ -81,8 +81,8 @@ class TestArchiveAdminView(TestCaseWithFactory):
         self.assertEqual(0, len(view.errors))
         self.assertFalse(view.context.private)
 
-    def test_set_private_with_packages(self):
-        # A PPA that does have packages cannot be privatised.
+    def test_set_private_with_packages_local(self):
+        # A local PPA that does have packages cannot be made private.
         self.publish_to_ppa(self.ppa)
         view = self.initialize_admin_view(self.ppa, {"field.private": "on"})
         self.assertEqual(1, len(view.errors))
@@ -92,9 +92,32 @@ class TestArchiveAdminView(TestCaseWithFactory):
             view.errors[0],
         )
 
-    def test_set_public_with_packages(self):
-        # A PPA that does have (or had) packages published is presented
-        # with a disabled 'private' field.
+    def test_set_public_with_packages_local(self):
+        # A local PPA that does have (or had) packages published cannot be
+        # made public.
+        self.ppa.private = True
+        self.publish_to_ppa(self.ppa)
+
+        view = self.initialize_admin_view(self.ppa, {"field.private": "off"})
+        self.assertEqual(1, len(view.errors))
+        self.assertEqual(
+            "This archive already has published sources. "
+            "It is not possible to switch the privacy.",
+            view.errors[0],
+        )
+
+    def test_set_private_with_packages_artifactory(self):
+        # An Artifactory PPA that does have packages can be made private.
+        self.ppa.publishing_method = ArchivePublishingMethod.ARTIFACTORY
+        self.publish_to_ppa(self.ppa)
+        view = self.initialize_admin_view(self.ppa, {"field.private": "on"})
+        self.assertEqual(0, len(view.errors))
+        self.assertTrue(view.context.private)
+
+    def test_set_public_with_packages_artifactory(self):
+        # An Artifactory PPA that does have (or had) packages published
+        # cannot be made public.
+        self.ppa.publishing_method = ArchivePublishingMethod.ARTIFACTORY
         self.ppa.private = True
         self.publish_to_ppa(self.ppa)
 
diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index 32b23dd..e5ed691 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -243,13 +243,33 @@ class Archive(SQLBase):
                 self.owner.visibility != PersonVisibility.PRIVATE
             ), "Private teams may not have public PPAs."
 
-        # If the privacy is being changed ensure there are no sources
-        # published.
         if not self.getPublishedSources().is_empty():
-            raise CannotSwitchPrivacy(
-                "This archive has had sources published and therefore "
-                "cannot have its privacy switched."
-            )
+            if (
+                # For local publishing, we can't switch privacy after
+                # anything has been published to it, because the publisher
+                # uses different paths on disk for public and private
+                # archives.
+                self.publishing_method == ArchivePublishingMethod.LOCAL
+                # Refuse to switch from private to public even for non-local
+                # publishing, partly as a safety measure and partly because
+                # the files in the archive would have to be unrestricted and
+                # we don't have code to do that yet.
+                #
+                # Switching an archive from public to private should ideally
+                # also restrict any files published only in that archive.
+                # However, that's quite complex because we'd have to check
+                # whether the files are published anywhere else, and nothing
+                # breaks if we leave the files alone.  It's not ideal that
+                # we might have files reachable from the public librarian
+                # that are now only published in a private archive, but
+                # since Launchpad won't give out any links to those, it
+                # could be worse.
+                or not value
+            ):
+                raise CannotSwitchPrivacy(
+                    "This archive has had sources published and therefore "
+                    "cannot have its privacy switched."
+                )
 
         return value
 
diff --git a/lib/lp/soyuz/tests/test_archive_privacy.py b/lib/lp/soyuz/tests/test_archive_privacy.py
index 6e06174..581bf64 100644
--- a/lib/lp/soyuz/tests/test_archive_privacy.py
+++ b/lib/lp/soyuz/tests/test_archive_privacy.py
@@ -5,6 +5,7 @@
 
 from zope.security.interfaces import Unauthorized
 
+from lp.soyuz.enums import ArchivePublishingMethod
 from lp.soyuz.interfaces.archive import CannotSwitchPrivacy
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
@@ -77,7 +78,7 @@ class TestPrivacySwitching(TestCaseWithFactory):
         private_ppa.private = False
         self.assertFalse(private_ppa.private)
 
-    def test_switch_privacy_with_pubs_fails(self):
+    def test_switch_privacy_with_pubs_fails_local(self):
         # Changing the privacy is not possible when the archive already
         # has published sources.
         public_ppa = self.factory.makeArchive(private=False)
@@ -95,3 +96,31 @@ class TestPrivacySwitching(TestCaseWithFactory):
         self.assertRaises(
             CannotSwitchPrivacy, setattr, private_ppa, "private", False
         )
+
+    def test_make_private_with_pubs_succeeds_artifactory(self):
+        # Making a public Artifactory archive private is fine even if the
+        # archive already has published sources.
+        public_ppa = self.factory.makeArchive(
+            private=False,
+            publishing_method=ArchivePublishingMethod.ARTIFACTORY,
+        )
+        publisher = SoyuzTestPublisher()
+        publisher.prepareBreezyAutotest()
+        publisher.getPubSource(archive=public_ppa)
+
+        public_ppa.private = True
+        self.assertTrue(public_ppa.private)
+
+    def test_make_public_with_pubs_fails_artifactory(self):
+        # Making a public Artifactory archive private fails if the archive
+        # already has published sources.
+        private_ppa = self.factory.makeArchive(
+            private=True, publishing_method=ArchivePublishingMethod.ARTIFACTORY
+        )
+        publisher = SoyuzTestPublisher()
+        publisher.prepareBreezyAutotest()
+        publisher.getPubSource(archive=private_ppa)
+
+        self.assertRaises(
+            CannotSwitchPrivacy, setattr, private_ppa, "private", False
+        )