launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/registry-delete-archive into lp:launchpad.

Commit message:
Allow registry experts to delete non-main archives.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/registry-delete-archive/+merge/358964

This makes it easier to deal with PPA description spam.  Registry experts already have extensive powers along these lines; suspending accounts is normally sufficient right now, but there are some awkward edge cases where straightforward deletion would be easier.

I also granted the ability for registry experts to see public but disabled archives, as otherwise they'd lose access to the object immediately after calling the delete method.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/registry-delete-archive into lp:launchpad.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2018-10-16 11:58:39 +0000
+++ lib/lp/security.py	2018-11-18 18:44:49 +0000
@@ -2686,6 +2686,12 @@
         if user.in_admin or user.in_commercial_admin:
             return True
 
+        # Registry experts are allowed to view public but disabled archives
+        # (since they are allowed to disable archives).
+        if (not self.obj.private and not self.obj.enabled and
+                user.in_registry_experts):
+            return True
+
         # Owners can view the PPA.
         if user.inTeam(self.obj.owner):
             return True
@@ -2748,6 +2754,21 @@
         return user.isOwner(self.obj) or user.in_admin
 
 
+class DeleteArchive(EditArchive):
+    """Restrict archive deletion operations.
+
+    People who can edit an archive can delete it.  In addition, registry
+    experts can delete non-main archives, as a spam control mechanism.
+    """
+    permission = 'launchpad.Delete'
+    usedfor = IArchive
+
+    def checkAuthenticated(self, user):
+        return (
+            super(DeleteArchive, self).checkAuthenticated(user) or
+            (not self.obj.is_main and user.in_registry_experts))
+
+
 class AppendArchive(AuthorizationBase):
     """Restrict appending (upload and copy) operations on archives.
 

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2017-07-18 16:22:03 +0000
+++ lib/lp/soyuz/configure.zcml	2018-11-18 18:44:49 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -330,6 +330,9 @@
             set_attributes="build_debug_symbols description displayname
                             publish publish_debug_symbols status
                             suppress_subscription_notifications"/>
+        <require
+            permission="launchpad.Delete"
+            interface="lp.soyuz.interfaces.archive.IArchiveDelete"/>
         <!--
            NOTE: The 'private' permission controls who can turn a public
            archive into a private one, and vice versa. The logic that

=== removed file 'lib/lp/soyuz/doc/archive-deletion.txt'
--- lib/lp/soyuz/doc/archive-deletion.txt	2018-05-27 18:32:33 +0000
+++ lib/lp/soyuz/doc/archive-deletion.txt	1970-01-01 00:00:00 +0000
@@ -1,71 +0,0 @@
-= Deleting an archive =
-
-When deleting an archive, the user calls IArchive.delete(), passing in
-the IPerson who is requesting the deletion. The archive is disabled and
-the status set to DELETING.
-
-This status tells the publisher to then set the publications to DELETED
-and delete the repository area. Once it completes that task the status
-of the archive itself is set to DELETED.
-
-    >>> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-    >>> login("admin@xxxxxxxxxxxxx")
-    >>> stp = SoyuzTestPublisher()
-    >>> stp.prepareBreezyAutotest()
-    >>> owner = factory.makePerson(name='archive-owner')
-    >>> archive = factory.makeArchive(owner=owner)
-
-The archive is currently active:
-
-    >>> print(archive.enabled)
-    True
-
-    >>> print(archive.status.name)
-    ACTIVE
-
-We can create some packages in it using the test publisher:
-
-    >>> from lp.soyuz.enums import PackagePublishingStatus
-    >>> ignore = stp.getPubBinaries(
-    ...     archive=archive, binaryname="foo-bin1",
-    ...     status=PackagePublishingStatus.PENDING)
-    >>> ignore = stp.getPubBinaries(
-    ...     archive=archive, binaryname="foo-bin2",
-    ...     status=PackagePublishingStatus.PUBLISHED)
-    >>> from storm.store import Store
-    >>> Store.of(archive).flush()
-
-Calling delete() will now do the deletion.  It is only callable by someone
-with launchpad.Edit permission on the archive.  Here, "duderino" who is
-some random dude is refused:
-
-    >>> person = factory.makePerson(name="duderino")
-    >>> ignored = login_person(person)
-    >>> archive.delete(person)
-    Traceback (most recent call last):
-    ...
-    Unauthorized:...
-
-However we can delete it using the owner of the archive:
-
-    >>> ignored = login_person(archive.owner)
-    >>> archive.delete(archive.owner)
-
-Now the archive is disabled and the status is DELETING to tell the
-publisher to remove the publications and the repository:
-
-    >>> print(archive.enabled)
-    False
-
-    >>> print(archive.status.name)
-    DELETING
-
-Once deleted the archive can't be reenabled.
-
-    >>> archive.enable()
-    Traceback (most recent call last):
-    ...
-    AssertionError: Deleted archives can't be enabled.
-
-    >>> print(archive.enabled)
-    False

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2018-08-03 12:16:16 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2018-11-18 18:44:49 +0000
@@ -2046,22 +2046,6 @@
     def disable():
         """Disable the archive."""
 
-    @export_destructor_operation()
-    @call_with(deleted_by=REQUEST_USER)
-    @operation_for_version('devel')
-    def delete(deleted_by):
-        """Delete this archive.
-
-        :param deleted_by: The `IPerson` requesting the deletion.
-
-        The ArchiveStatus will be set to DELETING and any published
-        packages will be marked as DELETED by deleted_by.
-
-        The publisher is responsible for deleting the repository area
-        when it sees the status change and sets it to DELETED once
-        processed.
-        """
-
     def addArchiveDependency(dependency, pocket, component=None):
         """Record an archive dependency record for the context archive.
 
@@ -2242,6 +2226,26 @@
         """
 
 
+class IArchiveDelete(Interface):
+    """Archive interface for operations restricted by delete privilege."""
+
+    @export_destructor_operation()
+    @call_with(deleted_by=REQUEST_USER)
+    @operation_for_version('devel')
+    def delete(deleted_by):
+        """Delete this archive.
+
+        :param deleted_by: The `IPerson` requesting the deletion.
+
+        The ArchiveStatus will be set to DELETING and any published
+        packages will be marked as DELETED by deleted_by.
+
+        The publisher is responsible for deleting the repository area
+        when it sees the status change and sets it to DELETED once
+        processed.
+        """
+
+
 class IArchiveAdmin(Interface):
     """Archive interface for operations restricted by commercial."""
 
@@ -2269,7 +2273,7 @@
             "with a higher score will build sooner.")))
 
 
-class IArchive(IArchivePublic, IArchiveAppend, IArchiveEdit,
+class IArchive(IArchivePublic, IArchiveAppend, IArchiveEdit, IArchiveDelete,
                IArchiveSubscriberView, IArchiveView, IArchiveAdmin,
                IArchiveRestricted):
     """Main Archive interface."""

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2018-08-03 12:16:16 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2018-11-18 18:44:49 +0000
@@ -17,6 +17,7 @@
 from pytz import UTC
 import responses
 import six
+from storm.store import Store
 from testtools.matchers import (
     AllMatch,
     DocTestMatches,
@@ -1619,29 +1620,78 @@
 
 
 class TestArchiveDelete(TestCaseWithFactory):
-    """Edge-case tests for PPA deletion.
-
-    PPA deletion is also documented in lp/soyuz/doc/archive-deletion.txt.
-    """
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        """Create a test archive and login as the owner."""
-        super(TestArchiveDelete, self).setUp()
-        self.archive = self.factory.makeArchive()
-        login_person(self.archive.owner)
-
-    def test_delete(self):
-        # Sanity check for the unit-test.
-        self.archive.delete(deleted_by=self.archive.owner)
-        self.assertEqual(ArchiveStatus.DELETING, self.archive.status)
+    """Test PPA deletion."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def makePopulatedArchive(self):
+        archive = self.factory.makeArchive()
+        self.assertActive(archive)
+        publisher = SoyuzTestPublisher()
+        with admin_logged_in():
+            publisher.prepareBreezyAutotest()
+            publisher.getPubBinaries(
+                archive=archive, binaryname="foo-bin1",
+                status=PackagePublishingStatus.PENDING)
+            publisher.getPubBinaries(
+                archive=archive, binaryname="foo-bin2",
+                status=PackagePublishingStatus.PUBLISHED)
+        Store.of(archive).flush()
+        return archive
+
+    def assertActive(self, archive):
+        self.assertTrue(archive.enabled)
+        self.assertEqual(ArchiveStatus.ACTIVE, archive.status)
+
+    def assertDeleting(self, archive):
+        # Deleting an archive sets the status to DELETING.  This tells the
+        # publisher to set the publications to DELETED and delete the
+        # published archive from disk, after which the status of the archive
+        # itself is set to DELETED.
+        self.assertFalse(archive.enabled)
+        self.assertEqual(ArchiveStatus.DELETING, archive.status)
+
+    def test_delete_unprivileged(self):
+        # An unprivileged user cannot delete an archive.
+        archive = self.factory.makeArchive()
+        self.assertActive(archive)
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            self.assertRaises(Unauthorized, getattr, archive, 'delete')
+            self.assertActive(archive)
+
+    def test_delete_archive_owner(self):
+        # The owner of an archive can delete it.
+        archive = self.makePopulatedArchive()
+        with person_logged_in(archive.owner):
+            archive.delete(deleted_by=archive.owner)
+            self.assertDeleting(archive)
+
+    def test_delete_registry_expert(self):
+        # A registry expert can delete an archive.
+        archive = self.makePopulatedArchive()
+        with celebrity_logged_in("registry_experts"):
+            archive.delete(deleted_by=archive.owner)
+            self.assertDeleting(archive)
 
     def test_delete_when_disabled(self):
         # A disabled archive can also be deleted (bug 574246).
-        self.archive.disable()
-        self.archive.delete(deleted_by=self.archive.owner)
-        self.assertEqual(ArchiveStatus.DELETING, self.archive.status)
+        archive = self.makePopulatedArchive()
+        with person_logged_in(archive.owner):
+            archive.disable()
+            archive.delete(deleted_by=archive.owner)
+            self.assertDeleting(archive)
+
+    def test_cannot_reenable(self):
+        # A deleted archive cannot be re-enabled.
+        archive = self.factory.makeArchive()
+        with person_logged_in(archive.owner):
+            archive.delete(deleted_by=archive.owner)
+            self.assertDeleting(archive)
+            self.assertRaisesWithContent(
+                AssertionError, "Deleted archives can't be enabled.",
+                archive.enable)
+            self.assertDeleting(archive)
 
 
 class TestSuppressSubscription(TestCaseWithFactory):