Merge lp:~cjwatson/launchpad/registry-delete-archive into lp:launchpad

Proposed by Colin Watson on 2018-11-18
Status: Needs review
Proposed branch: lp:~cjwatson/launchpad/registry-delete-archive
Merge into: lp:launchpad
Diff against target: 308 lines (+116/-109)
5 files modified
lib/lp/security.py (+21/-0)
lib/lp/soyuz/configure.zcml (+4/-1)
lib/lp/soyuz/doc/archive-deletion.txt (+0/-71)
lib/lp/soyuz/interfaces/archive.py (+21/-17)
lib/lp/soyuz/tests/test_archive.py (+70/-20)
To merge this branch: bzr merge lp:~cjwatson/launchpad/registry-delete-archive
Reviewer Review Type Date Requested Status
Launchpad code reviewers 2018-11-18 Pending
Review via email: mp+358964@code.launchpad.net

Commit message

Allow registry experts to delete non-main archives.

Description of the change

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.

To post a comment you must log in.

Unmerged revisions

18824. By Colin Watson on 2018-11-18

Allow registry experts to delete non-main archives.

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/security.py'
2--- lib/lp/security.py 2018-10-16 11:58:39 +0000
3+++ lib/lp/security.py 2018-11-18 18:44:49 +0000
4@@ -2686,6 +2686,12 @@
5 if user.in_admin or user.in_commercial_admin:
6 return True
7
8+ # Registry experts are allowed to view public but disabled archives
9+ # (since they are allowed to disable archives).
10+ if (not self.obj.private and not self.obj.enabled and
11+ user.in_registry_experts):
12+ return True
13+
14 # Owners can view the PPA.
15 if user.inTeam(self.obj.owner):
16 return True
17@@ -2748,6 +2754,21 @@
18 return user.isOwner(self.obj) or user.in_admin
19
20
21+class DeleteArchive(EditArchive):
22+ """Restrict archive deletion operations.
23+
24+ People who can edit an archive can delete it. In addition, registry
25+ experts can delete non-main archives, as a spam control mechanism.
26+ """
27+ permission = 'launchpad.Delete'
28+ usedfor = IArchive
29+
30+ def checkAuthenticated(self, user):
31+ return (
32+ super(DeleteArchive, self).checkAuthenticated(user) or
33+ (not self.obj.is_main and user.in_registry_experts))
34+
35+
36 class AppendArchive(AuthorizationBase):
37 """Restrict appending (upload and copy) operations on archives.
38
39
40=== modified file 'lib/lp/soyuz/configure.zcml'
41--- lib/lp/soyuz/configure.zcml 2017-07-18 16:22:03 +0000
42+++ lib/lp/soyuz/configure.zcml 2018-11-18 18:44:49 +0000
43@@ -1,4 +1,4 @@
44-<!-- Copyright 2009-2017 Canonical Ltd. This software is licensed under the
45+<!-- Copyright 2009-2018 Canonical Ltd. This software is licensed under the
46 GNU Affero General Public License version 3 (see the file LICENSE).
47 -->
48
49@@ -330,6 +330,9 @@
50 set_attributes="build_debug_symbols description displayname
51 publish publish_debug_symbols status
52 suppress_subscription_notifications"/>
53+ <require
54+ permission="launchpad.Delete"
55+ interface="lp.soyuz.interfaces.archive.IArchiveDelete"/>
56 <!--
57 NOTE: The 'private' permission controls who can turn a public
58 archive into a private one, and vice versa. The logic that
59
60=== removed file 'lib/lp/soyuz/doc/archive-deletion.txt'
61--- lib/lp/soyuz/doc/archive-deletion.txt 2018-05-27 18:32:33 +0000
62+++ lib/lp/soyuz/doc/archive-deletion.txt 1970-01-01 00:00:00 +0000
63@@ -1,71 +0,0 @@
64-= Deleting an archive =
65-
66-When deleting an archive, the user calls IArchive.delete(), passing in
67-the IPerson who is requesting the deletion. The archive is disabled and
68-the status set to DELETING.
69-
70-This status tells the publisher to then set the publications to DELETED
71-and delete the repository area. Once it completes that task the status
72-of the archive itself is set to DELETED.
73-
74- >>> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
75- >>> login("admin@canonical.com")
76- >>> stp = SoyuzTestPublisher()
77- >>> stp.prepareBreezyAutotest()
78- >>> owner = factory.makePerson(name='archive-owner')
79- >>> archive = factory.makeArchive(owner=owner)
80-
81-The archive is currently active:
82-
83- >>> print(archive.enabled)
84- True
85-
86- >>> print(archive.status.name)
87- ACTIVE
88-
89-We can create some packages in it using the test publisher:
90-
91- >>> from lp.soyuz.enums import PackagePublishingStatus
92- >>> ignore = stp.getPubBinaries(
93- ... archive=archive, binaryname="foo-bin1",
94- ... status=PackagePublishingStatus.PENDING)
95- >>> ignore = stp.getPubBinaries(
96- ... archive=archive, binaryname="foo-bin2",
97- ... status=PackagePublishingStatus.PUBLISHED)
98- >>> from storm.store import Store
99- >>> Store.of(archive).flush()
100-
101-Calling delete() will now do the deletion. It is only callable by someone
102-with launchpad.Edit permission on the archive. Here, "duderino" who is
103-some random dude is refused:
104-
105- >>> person = factory.makePerson(name="duderino")
106- >>> ignored = login_person(person)
107- >>> archive.delete(person)
108- Traceback (most recent call last):
109- ...
110- Unauthorized:...
111-
112-However we can delete it using the owner of the archive:
113-
114- >>> ignored = login_person(archive.owner)
115- >>> archive.delete(archive.owner)
116-
117-Now the archive is disabled and the status is DELETING to tell the
118-publisher to remove the publications and the repository:
119-
120- >>> print(archive.enabled)
121- False
122-
123- >>> print(archive.status.name)
124- DELETING
125-
126-Once deleted the archive can't be reenabled.
127-
128- >>> archive.enable()
129- Traceback (most recent call last):
130- ...
131- AssertionError: Deleted archives can't be enabled.
132-
133- >>> print(archive.enabled)
134- False
135
136=== modified file 'lib/lp/soyuz/interfaces/archive.py'
137--- lib/lp/soyuz/interfaces/archive.py 2018-08-03 12:16:16 +0000
138+++ lib/lp/soyuz/interfaces/archive.py 2018-11-18 18:44:49 +0000
139@@ -2046,22 +2046,6 @@
140 def disable():
141 """Disable the archive."""
142
143- @export_destructor_operation()
144- @call_with(deleted_by=REQUEST_USER)
145- @operation_for_version('devel')
146- def delete(deleted_by):
147- """Delete this archive.
148-
149- :param deleted_by: The `IPerson` requesting the deletion.
150-
151- The ArchiveStatus will be set to DELETING and any published
152- packages will be marked as DELETED by deleted_by.
153-
154- The publisher is responsible for deleting the repository area
155- when it sees the status change and sets it to DELETED once
156- processed.
157- """
158-
159 def addArchiveDependency(dependency, pocket, component=None):
160 """Record an archive dependency record for the context archive.
161
162@@ -2242,6 +2226,26 @@
163 """
164
165
166+class IArchiveDelete(Interface):
167+ """Archive interface for operations restricted by delete privilege."""
168+
169+ @export_destructor_operation()
170+ @call_with(deleted_by=REQUEST_USER)
171+ @operation_for_version('devel')
172+ def delete(deleted_by):
173+ """Delete this archive.
174+
175+ :param deleted_by: The `IPerson` requesting the deletion.
176+
177+ The ArchiveStatus will be set to DELETING and any published
178+ packages will be marked as DELETED by deleted_by.
179+
180+ The publisher is responsible for deleting the repository area
181+ when it sees the status change and sets it to DELETED once
182+ processed.
183+ """
184+
185+
186 class IArchiveAdmin(Interface):
187 """Archive interface for operations restricted by commercial."""
188
189@@ -2269,7 +2273,7 @@
190 "with a higher score will build sooner.")))
191
192
193-class IArchive(IArchivePublic, IArchiveAppend, IArchiveEdit,
194+class IArchive(IArchivePublic, IArchiveAppend, IArchiveEdit, IArchiveDelete,
195 IArchiveSubscriberView, IArchiveView, IArchiveAdmin,
196 IArchiveRestricted):
197 """Main Archive interface."""
198
199=== modified file 'lib/lp/soyuz/tests/test_archive.py'
200--- lib/lp/soyuz/tests/test_archive.py 2018-08-03 12:16:16 +0000
201+++ lib/lp/soyuz/tests/test_archive.py 2018-11-18 18:44:49 +0000
202@@ -17,6 +17,7 @@
203 from pytz import UTC
204 import responses
205 import six
206+from storm.store import Store
207 from testtools.matchers import (
208 AllMatch,
209 DocTestMatches,
210@@ -1619,29 +1620,78 @@
211
212
213 class TestArchiveDelete(TestCaseWithFactory):
214- """Edge-case tests for PPA deletion.
215-
216- PPA deletion is also documented in lp/soyuz/doc/archive-deletion.txt.
217- """
218-
219- layer = DatabaseFunctionalLayer
220-
221- def setUp(self):
222- """Create a test archive and login as the owner."""
223- super(TestArchiveDelete, self).setUp()
224- self.archive = self.factory.makeArchive()
225- login_person(self.archive.owner)
226-
227- def test_delete(self):
228- # Sanity check for the unit-test.
229- self.archive.delete(deleted_by=self.archive.owner)
230- self.assertEqual(ArchiveStatus.DELETING, self.archive.status)
231+ """Test PPA deletion."""
232+
233+ layer = LaunchpadFunctionalLayer
234+
235+ def makePopulatedArchive(self):
236+ archive = self.factory.makeArchive()
237+ self.assertActive(archive)
238+ publisher = SoyuzTestPublisher()
239+ with admin_logged_in():
240+ publisher.prepareBreezyAutotest()
241+ publisher.getPubBinaries(
242+ archive=archive, binaryname="foo-bin1",
243+ status=PackagePublishingStatus.PENDING)
244+ publisher.getPubBinaries(
245+ archive=archive, binaryname="foo-bin2",
246+ status=PackagePublishingStatus.PUBLISHED)
247+ Store.of(archive).flush()
248+ return archive
249+
250+ def assertActive(self, archive):
251+ self.assertTrue(archive.enabled)
252+ self.assertEqual(ArchiveStatus.ACTIVE, archive.status)
253+
254+ def assertDeleting(self, archive):
255+ # Deleting an archive sets the status to DELETING. This tells the
256+ # publisher to set the publications to DELETED and delete the
257+ # published archive from disk, after which the status of the archive
258+ # itself is set to DELETED.
259+ self.assertFalse(archive.enabled)
260+ self.assertEqual(ArchiveStatus.DELETING, archive.status)
261+
262+ def test_delete_unprivileged(self):
263+ # An unprivileged user cannot delete an archive.
264+ archive = self.factory.makeArchive()
265+ self.assertActive(archive)
266+ person = self.factory.makePerson()
267+ with person_logged_in(person):
268+ self.assertRaises(Unauthorized, getattr, archive, 'delete')
269+ self.assertActive(archive)
270+
271+ def test_delete_archive_owner(self):
272+ # The owner of an archive can delete it.
273+ archive = self.makePopulatedArchive()
274+ with person_logged_in(archive.owner):
275+ archive.delete(deleted_by=archive.owner)
276+ self.assertDeleting(archive)
277+
278+ def test_delete_registry_expert(self):
279+ # A registry expert can delete an archive.
280+ archive = self.makePopulatedArchive()
281+ with celebrity_logged_in("registry_experts"):
282+ archive.delete(deleted_by=archive.owner)
283+ self.assertDeleting(archive)
284
285 def test_delete_when_disabled(self):
286 # A disabled archive can also be deleted (bug 574246).
287- self.archive.disable()
288- self.archive.delete(deleted_by=self.archive.owner)
289- self.assertEqual(ArchiveStatus.DELETING, self.archive.status)
290+ archive = self.makePopulatedArchive()
291+ with person_logged_in(archive.owner):
292+ archive.disable()
293+ archive.delete(deleted_by=archive.owner)
294+ self.assertDeleting(archive)
295+
296+ def test_cannot_reenable(self):
297+ # A deleted archive cannot be re-enabled.
298+ archive = self.factory.makeArchive()
299+ with person_logged_in(archive.owner):
300+ archive.delete(deleted_by=archive.owner)
301+ self.assertDeleting(archive)
302+ self.assertRaisesWithContent(
303+ AssertionError, "Deleted archives can't be enabled.",
304+ archive.enable)
305+ self.assertDeleting(archive)
306
307
308 class TestSuppressSubscription(TestCaseWithFactory):