Merge ~lgp171188/launchpad:enforce-stricter-permission-check-when-a-bug-is-locked-down into launchpad:master
- Git
- lp:~lgp171188/launchpad
- enforce-stricter-permission-check-when-a-bug-is-locked-down
- Merge into master
Proposed by
Guruprasad
Status: | Merged |
---|---|
Approved by: | Guruprasad |
Approved revision: | 68dbc5ae78abe0bc1565e38bb2292faab277b5f6 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~lgp171188/launchpad:enforce-stricter-permission-check-when-a-bug-is-locked-down |
Merge into: | launchpad:master |
Diff against target: |
1381 lines (+988/-23) 12 files modified
lib/lp/bugs/adapters/bugchange.py (+90/-2) lib/lp/bugs/browser/bugtask.py (+26/-2) lib/lp/bugs/browser/tests/bug-views.txt (+53/-2) lib/lp/bugs/browser/tests/test_bugtask.py (+90/-2) lib/lp/bugs/configure.zcml (+5/-1) lib/lp/bugs/enums.py (+30/-1) lib/lp/bugs/interfaces/bug.py (+67/-3) lib/lp/bugs/model/bug.py (+95/-2) lib/lp/bugs/security.py (+40/-6) lib/lp/bugs/tests/test_bug.py (+438/-2) lib/lp/scripts/garbo.py (+38/-0) lib/lp/scripts/tests/test_garbo.py (+16/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Review via email: mp+414097@code.launchpad.net |
Commit message
* Add lock_status, lock_reason fields and lock, unlock methods to Bug
* Add a ModerateBug security adapter for IBugModerate
This adapter uses the 'launchpad.
to lock and unlock a bug.
* Create bug change entries when locking and unlocking a bug
* Allow only the relevant roles to edit a locked bug
* Add unit tests for the previous changes related to bug (un)locking
* Add the updated sample data with the new fields
* Add the garbo job for backfilling lock_status
Description of the change
To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) : | # |
Revision history for this message
Colin Watson (cjwatson) : | # |
Revision history for this message
Guruprasad (lgp171188) : | # |
Revision history for this message
Guruprasad (lgp171188) : | # |
Revision history for this message
Guruprasad (lgp171188) : | # |
Revision history for this message
Colin Watson (cjwatson) wrote : | # |
Thanks for these updates. There are a few more things to fix (especially issues around the garbo job), but I think it's nearly there now.
review:
Approve
Revision history for this message
Guruprasad (lgp171188) : | # |
Revision history for this message
Guruprasad (lgp171188) : | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/lp/bugs/adapters/bugchange.py b/lib/lp/bugs/adapters/bugchange.py |
2 | index ba0e99a..234cc0e 100644 |
3 | --- a/lib/lp/bugs/adapters/bugchange.py |
4 | +++ b/lib/lp/bugs/adapters/bugchange.py |
5 | @@ -1,4 +1,4 @@ |
6 | -# Copyright 2009-2016 Canonical Ltd. This software is licensed under the |
7 | +# Copyright 2009-2022 Canonical Ltd. This software is licensed under the |
8 | # GNU Affero General Public License version 3 (see the file LICENSE). |
9 | |
10 | """Implementations for bug changes.""" |
11 | @@ -17,6 +17,8 @@ __all__ = [ |
12 | 'BugDescriptionChange', |
13 | 'BugDuplicateChange', |
14 | 'BugInformationTypeChange', |
15 | + 'BugLocked', |
16 | + 'BugLockReasonSet', |
17 | 'BugTagsChange', |
18 | 'BugTaskAdded', |
19 | 'BugTaskAssigneeChange', |
20 | @@ -27,6 +29,7 @@ __all__ = [ |
21 | 'BugTaskStatusChange', |
22 | 'BugTaskTargetChange', |
23 | 'BugTitleChange', |
24 | + 'BugUnlocked', |
25 | 'BugWatchAdded', |
26 | 'BugWatchRemoved', |
27 | 'CHANGED_DUPLICATE_MARKER', |
28 | @@ -52,7 +55,10 @@ from textwrap import dedent |
29 | from zope.interface import implementer |
30 | from zope.security.proxy import isinstance as zope_isinstance |
31 | |
32 | -from lp.bugs.enums import BugNotificationLevel |
33 | +from lp.bugs.enums import ( |
34 | + BugLockStatus, |
35 | + BugNotificationLevel, |
36 | + ) |
37 | from lp.bugs.interfaces.bugchange import IBugChange |
38 | from lp.bugs.interfaces.bugtask import ( |
39 | IBugTask, |
40 | @@ -735,6 +741,88 @@ class CveUnlinkedFromBug(BugChangeBase): |
41 | return {'text': "** CVE removed: %s" % self.cve.url} |
42 | |
43 | |
44 | +class BugLocked(BugChangeBase): |
45 | + """Used to represent the locking of a bug.""" |
46 | + |
47 | + def __init__(self, when, person, old_status, |
48 | + new_status, reason, **kwargs): |
49 | + super().__init__(when, person) |
50 | + self.old_status = old_status |
51 | + self.new_status = new_status |
52 | + self.reason = reason |
53 | + |
54 | + def getBugActivity(self): |
55 | + """See `IBugChange'.""" |
56 | + activity = dict( |
57 | + whatchanged='lock status', |
58 | + oldvalue=str(self.old_status), |
59 | + newvalue=str(self.new_status), |
60 | + ) |
61 | + if self.reason: |
62 | + activity['message'] = self.reason |
63 | + |
64 | + return activity |
65 | + |
66 | + def getBugNotification(self): |
67 | + """See `IBugChange`.""" |
68 | + text = "** Bug metadata locked and limited to project staff" |
69 | + if self.reason: |
70 | + text = "{}: {}".format(text, self.reason) |
71 | + |
72 | + return { |
73 | + 'text': text |
74 | + } |
75 | + |
76 | +class BugUnlocked(BugChangeBase): |
77 | + """Used to represent the unlocking of a bug.""" |
78 | + |
79 | + def __init__(self, when, person, old_status, **kwargs): |
80 | + super().__init__(when, person) |
81 | + self.old_status = old_status |
82 | + |
83 | + def getBugActivity(self): |
84 | + """See `IBugChange'.""" |
85 | + return dict( |
86 | + whatchanged='lock status', |
87 | + newvalue=str(BugLockStatus.UNLOCKED), |
88 | + oldvalue=str(self.old_status), |
89 | + ) |
90 | + |
91 | + def getBugNotification(self): |
92 | + """See `IBugChange`.""" |
93 | + return { |
94 | + 'text': "** Bug unlocked" |
95 | + } |
96 | + |
97 | + |
98 | +class BugLockReasonSet(BugChangeBase): |
99 | + """Used to represent setting of the Bug.lock_reason field.""" |
100 | + |
101 | + def __init__(self, when, person, old_reason, new_reason, **kwargs): |
102 | + super().__init__(when, person) |
103 | + self.old_reason = old_reason |
104 | + self.new_reason = new_reason |
105 | + |
106 | + def getBugActivity(self): |
107 | + """See `IBugChange`.""" |
108 | + return dict( |
109 | + whatchanged='lock reason', |
110 | + oldvalue=self.old_reason if self.old_reason else 'unset', |
111 | + newvalue=self.new_reason if self.new_reason else 'unset', |
112 | + ) |
113 | + |
114 | + def getBugNotification(self): |
115 | + """See `IBugChange`.""" |
116 | + if self.new_reason is None: |
117 | + text = "Bug lock reason unset" |
118 | + else: |
119 | + text = '** Bug lock reason changed: {}'.format(self.new_reason) |
120 | + |
121 | + return { |
122 | + 'text': text |
123 | + } |
124 | + |
125 | + |
126 | class BugTaskAttributeChange(AttributeChange): |
127 | """Used to represent a change in a BugTask's attributes. |
128 | |
129 | diff --git a/lib/lp/bugs/browser/bugtask.py b/lib/lp/bugs/browser/bugtask.py |
130 | index 1ae7e7c..76e1bb8 100644 |
131 | --- a/lib/lp/bugs/browser/bugtask.py |
132 | +++ b/lib/lp/bugs/browser/bugtask.py |
133 | @@ -1,4 +1,4 @@ |
134 | -# Copyright 2009-2021 Canonical Ltd. This software is licensed under the |
135 | +# Copyright 2009-2022 Canonical Ltd. This software is licensed under the |
136 | # GNU Affero General Public License version 3 (see the file LICENSE). |
137 | |
138 | """IBugTask-related browser views.""" |
139 | @@ -110,6 +110,7 @@ from lp.bugs.browser.widgets.bugtask import ( |
140 | BugTaskTargetWidget, |
141 | DBItemDisplayWidget, |
142 | ) |
143 | +from lp.bugs.enums import BugLockStatus |
144 | from lp.bugs.interfaces.bug import ( |
145 | IBug, |
146 | IBugSet, |
147 | @@ -591,7 +592,7 @@ class BugTaskView(LaunchpadView, BugViewMixin, FeedsMixin): |
148 | activity = self.context.bug.activity |
149 | bug_change_re = ( |
150 | 'affects|description|security vulnerability|information type|' |
151 | - 'summary|tags|visibility|bug task deleted') |
152 | + 'summary|tags|visibility|bug task deleted|lock status|lock reason') |
153 | bugtask_change_re = ( |
154 | r'[a-z0-9][a-z0-9\+\.\-]+( \([A-Za-z0-9\s]+\))?: ' |
155 | r'(assignee|importance|milestone|status)') |
156 | @@ -2540,6 +2541,29 @@ class BugActivityItem: |
157 | else: |
158 | return_dict[key] = html_escape(return_dict[key]) |
159 | |
160 | + elif attribute == 'lock status': |
161 | + if self.newvalue == str(BugLockStatus.COMMENT_ONLY): |
162 | + reason = " " |
163 | + detail = ( |
164 | + 'Metadata changes locked{}and ' |
165 | + 'limited to project staff' |
166 | + ) |
167 | + if self.message: |
168 | + reason = " ({}) ".format(html_escape(self.message)) |
169 | + return detail.format(reason) |
170 | + else: |
171 | + return 'Metadata changes unlocked' |
172 | + elif attribute == 'lock reason': |
173 | + if self.newvalue != "unset" and self.oldvalue != "unset": |
174 | + # return a proper message with old and new values |
175 | + return "{} → {}".format( |
176 | + html_escape(self.oldvalue), html_escape(self.newvalue) |
177 | + ) |
178 | + pass |
179 | + elif self.newvalue != "unset": |
180 | + return "{}".format(self.newvalue) |
181 | + else: |
182 | + return "Unset" |
183 | elif attribute == 'milestone': |
184 | for key in return_dict: |
185 | if return_dict[key] is None: |
186 | diff --git a/lib/lp/bugs/browser/tests/bug-views.txt b/lib/lp/bugs/browser/tests/bug-views.txt |
187 | index 705ab03..ca716c2 100644 |
188 | --- a/lib/lp/bugs/browser/tests/bug-views.txt |
189 | +++ b/lib/lp/bugs/browser/tests/bug-views.txt |
190 | @@ -692,8 +692,14 @@ for a bug, ordered by date. |
191 | First, some set-up. |
192 | |
193 | >>> import pytz |
194 | - >>> from datetime import datetime |
195 | - >>> from lp.bugs.adapters.bugchange import BugTitleChange |
196 | + >>> from datetime import datetime, timedelta |
197 | + >>> from lp.bugs.adapters.bugchange import ( |
198 | + ... BugLocked, |
199 | + ... BugLockReasonSet, |
200 | + ... BugTitleChange, |
201 | + ... BugUnlocked, |
202 | + ... ) |
203 | + >>> from lp.bugs.enums import BugLockStatus |
204 | >>> nowish = datetime( |
205 | ... 2009, 3, 26, 21, 37, 45, tzinfo=pytz.timezone('UTC')) |
206 | |
207 | @@ -705,6 +711,27 @@ First, some set-up. |
208 | ... old_value=bug.title, new_value="A new bug title") |
209 | >>> bug.addChange(title_change) |
210 | |
211 | + >>> nowish = nowish + timedelta(days=1) |
212 | + >>> locked = BugLocked( |
213 | + ... when=nowish, |
214 | + ... person=foo_bar, old_status=BugLockStatus.UNLOCKED, |
215 | + ... new_status=BugLockStatus.COMMENT_ONLY, reason='too hot') |
216 | + >>> bug.addChange(locked) |
217 | + >>> nowish = nowish + timedelta(days=1) |
218 | + >>> lock_reason_updated = BugLockReasonSet( |
219 | + ... when=nowish, person=foo_bar, old_reason='too hot', |
220 | + ... new_reason='too hot!') |
221 | + >>> bug.addChange(lock_reason_updated) |
222 | + >>> nowish = nowish + timedelta(days=1) |
223 | + >>> lock_reason_unset = BugLockReasonSet( |
224 | + ... when=nowish, person=foo_bar, old_reason='too hot!', |
225 | + ... new_reason=None) |
226 | + >>> bug.addChange(lock_reason_unset) |
227 | + >>> nowish = nowish + timedelta(days=1) |
228 | + >>> unlocked = BugUnlocked( |
229 | + ... when=nowish, person=foo_bar, |
230 | + ... old_status=BugLockStatus.COMMENT_ONLY) |
231 | + >>> bug.addChange(unlocked) |
232 | >>> request = LaunchpadTestRequest( |
233 | ... method="POST", |
234 | ... form={'field.subject': bug.title, |
235 | @@ -775,6 +802,18 @@ order, the comments and activity that have taken place on a bug. |
236 | Changed: |
237 | * summary: A bug title => A new bug title |
238 | -- name16 -- |
239 | + Changed: |
240 | + * lock status: Unlocked => Comment-only |
241 | + -- name16 -- |
242 | + Changed: |
243 | + * lock reason: too hot => too hot! |
244 | + -- name16 -- |
245 | + Changed: |
246 | + * lock reason: too hot! => unset |
247 | + -- name16 -- |
248 | + Changed: |
249 | + * lock status: Comment-only => Unlocked |
250 | + -- name16 -- |
251 | A comment, for the reading of. |
252 | Changed in testproduct: |
253 | * status: New => Confirmed |
254 | @@ -809,6 +848,18 @@ grouped with that comment. |
255 | Changed: |
256 | * summary: A bug title => A new bug title |
257 | -- name16 -- |
258 | + Changed: |
259 | + * lock status: Unlocked => Comment-only |
260 | + -- name16 -- |
261 | + Changed: |
262 | + * lock reason: too hot => too hot! |
263 | + -- name16 -- |
264 | + Changed: |
265 | + * lock reason: too hot! => unset |
266 | + -- name16 -- |
267 | + Changed: |
268 | + * lock status: Comment-only => Unlocked |
269 | + -- name16 -- |
270 | A comment, for the reading of. |
271 | -- name16 -- |
272 | I triaged it. |
273 | diff --git a/lib/lp/bugs/browser/tests/test_bugtask.py b/lib/lp/bugs/browser/tests/test_bugtask.py |
274 | index d53dd1b..f47860e 100644 |
275 | --- a/lib/lp/bugs/browser/tests/test_bugtask.py |
276 | +++ b/lib/lp/bugs/browser/tests/test_bugtask.py |
277 | @@ -1,4 +1,4 @@ |
278 | -# Copyright 2009-2021 Canonical Ltd. This software is licensed under the |
279 | +# Copyright 2009-2022 Canonical Ltd. This software is licensed under the |
280 | # GNU Affero General Public License version 3 (see the file LICENSE). |
281 | |
282 | from datetime import ( |
283 | @@ -42,7 +42,10 @@ from lp.bugs.browser.bugtask import ( |
284 | BugTasksNominationsView, |
285 | BugTasksTableView, |
286 | ) |
287 | -from lp.bugs.enums import BugNotificationLevel |
288 | +from lp.bugs.enums import ( |
289 | + BugLockStatus, |
290 | + BugNotificationLevel, |
291 | + ) |
292 | from lp.bugs.feed.bug import PersonBugsFeed |
293 | from lp.bugs.interfaces.bugactivity import IBugActivitySet |
294 | from lp.bugs.interfaces.bugnomination import IBugNomination |
295 | @@ -1886,6 +1889,91 @@ class TestBugActivityItem(TestCaseWithFactory): |
296 | "- foo<br />+ bar &<>", |
297 | BugActivityItem(bug.activity[-1]).change_details) |
298 | |
299 | + def test_change_details_bug_locked_with_reason(self): |
300 | + target = self.factory.makeProduct() |
301 | + bug = self.factory.makeBug(owner=target.owner, target=target) |
302 | + with person_logged_in(target.owner): |
303 | + bug.lock( |
304 | + status=BugLockStatus.COMMENT_ONLY, |
305 | + who=target.owner, |
306 | + reason='too hot' |
307 | + ) |
308 | + self.assertEqual( |
309 | + 'Metadata changes locked (too hot) and limited to project staff', |
310 | + BugActivityItem(bug.activity[-1]).change_details |
311 | + ) |
312 | + |
313 | + def test_change_details_bug_locked_without_reason(self): |
314 | + target = self.factory.makeProduct() |
315 | + bug = self.factory.makeBug(owner=target.owner, target=target) |
316 | + with person_logged_in(target.owner): |
317 | + bug.lock( |
318 | + status=BugLockStatus.COMMENT_ONLY, |
319 | + who=target.owner, |
320 | + ) |
321 | + self.assertEqual( |
322 | + 'Metadata changes locked and limited to project staff', |
323 | + BugActivityItem(bug.activity[-1]).change_details |
324 | + ) |
325 | + |
326 | + def test_change_details_bug_unlocked(self): |
327 | + target = self.factory.makeProduct() |
328 | + bug = self.factory.makeBug(owner=target.owner, target=target) |
329 | + with person_logged_in(target.owner): |
330 | + bug.lock( |
331 | + status=BugLockStatus.COMMENT_ONLY, |
332 | + who=target.owner, |
333 | + ) |
334 | + bug.unlock(who=target.owner) |
335 | + self.assertEqual( |
336 | + 'Metadata changes unlocked', |
337 | + BugActivityItem(bug.activity[-1]).change_details |
338 | + ) |
339 | + |
340 | + def test_change_details_bug_lock_reason_set(self): |
341 | + target = self.factory.makeProduct() |
342 | + bug = self.factory.makeBug(owner=target.owner, target=target) |
343 | + with person_logged_in(target.owner): |
344 | + bug.lock( |
345 | + status=BugLockStatus.COMMENT_ONLY, |
346 | + who=target.owner, |
347 | + ) |
348 | + bug.setLockReason('too hot', who=target.owner) |
349 | + self.assertEqual( |
350 | + 'too hot', |
351 | + BugActivityItem(bug.activity[-1]).change_details |
352 | + ) |
353 | + |
354 | + def test_change_details_bug_lock_reason_updated(self): |
355 | + target = self.factory.makeProduct() |
356 | + bug = self.factory.makeBug(owner=target.owner, target=target) |
357 | + with person_logged_in(target.owner): |
358 | + bug.lock( |
359 | + status=BugLockStatus.COMMENT_ONLY, |
360 | + who=target.owner, |
361 | + reason='too hot', |
362 | + ) |
363 | + bug.setLockReason('too hot!', who=target.owner) |
364 | + self.assertEqual( |
365 | + 'too hot → too hot!', |
366 | + BugActivityItem(bug.activity[-1]).change_details |
367 | + ) |
368 | + |
369 | + def test_change_details_bug_lock_reason_unset(self): |
370 | + target = self.factory.makeProduct() |
371 | + bug = self.factory.makeBug(owner=target.owner, target=target) |
372 | + with person_logged_in(target.owner): |
373 | + bug.lock( |
374 | + status=BugLockStatus.COMMENT_ONLY, |
375 | + who=target.owner, |
376 | + reason='too hot', |
377 | + ) |
378 | + bug.setLockReason(None, who=target.owner) |
379 | + self.assertEqual( |
380 | + 'Unset', |
381 | + BugActivityItem(bug.activity[-1]).change_details |
382 | + ) |
383 | + |
384 | |
385 | class TestCommentCollapseVisibility(TestCaseWithFactory): |
386 | """Test for the conditions around display of collapsed/hidden comments.""" |
387 | diff --git a/lib/lp/bugs/configure.zcml b/lib/lp/bugs/configure.zcml |
388 | index 5b46af7..33f6685 100644 |
389 | --- a/lib/lp/bugs/configure.zcml |
390 | +++ b/lib/lp/bugs/configure.zcml |
391 | @@ -1,4 +1,4 @@ |
392 | -<!-- Copyright 2009-2021 Canonical Ltd. This software is licensed under the |
393 | +<!-- Copyright 2009-2022 Canonical Ltd. This software is licensed under the |
394 | GNU Affero General Public License version 3 (see the file LICENSE). |
395 | --> |
396 | |
397 | @@ -740,6 +740,10 @@ |
398 | tags |
399 | title |
400 | description"/> |
401 | + <require |
402 | + permission="launchpad.Moderate" |
403 | + interface="lp.bugs.interfaces.bug.IBugModerate" |
404 | + set_attributes="lock_reason"/> |
405 | </class> |
406 | <adapter |
407 | for="lp.bugs.interfaces.bug.IBug" |
408 | diff --git a/lib/lp/bugs/enums.py b/lib/lp/bugs/enums.py |
409 | index 6290763..913e1bf 100644 |
410 | --- a/lib/lp/bugs/enums.py |
411 | +++ b/lib/lp/bugs/enums.py |
412 | @@ -1,9 +1,11 @@ |
413 | -# Copyright 2010-2012 Canonical Ltd. This software is licensed under the |
414 | +# Copyright 2010-2022 Canonical Ltd. This software is licensed under the |
415 | # GNU Affero General Public License version 3 (see the file LICENSE). |
416 | |
417 | """Enums for the Bugs app.""" |
418 | |
419 | __all__ = [ |
420 | + 'BugLockedStatus', |
421 | + 'BugLockStatus', |
422 | 'BugNotificationLevel', |
423 | 'BugNotificationStatus', |
424 | ] |
425 | @@ -11,6 +13,7 @@ __all__ = [ |
426 | from lazr.enum import ( |
427 | DBEnumeratedType, |
428 | DBItem, |
429 | + use_template, |
430 | ) |
431 | |
432 | |
433 | @@ -74,3 +77,29 @@ class BugNotificationStatus(DBEnumeratedType): |
434 | The notification is deferred. The recipient list was not calculated |
435 | at creation time but is done when processed. |
436 | """) |
437 | + |
438 | + |
439 | +class BugLockStatus(DBEnumeratedType): |
440 | + """ |
441 | + The lock status of a bug. |
442 | + """ |
443 | + |
444 | + UNLOCKED = DBItem(0, """ |
445 | + Unlocked |
446 | + |
447 | + The bug is unlocked and the usual permissions apply. |
448 | + """) |
449 | + |
450 | + COMMENT_ONLY = DBItem(1, """ |
451 | + Comment-only |
452 | + |
453 | + The bug allows those without a relevant role to comment, |
454 | + but not to edit metadata. |
455 | + """) |
456 | + |
457 | + |
458 | +class BugLockedStatus(DBEnumeratedType): |
459 | + """ |
460 | + The various locked status values of a bug. |
461 | + """ |
462 | + use_template(BugLockStatus, exclude=('UNLOCKED',)) |
463 | diff --git a/lib/lp/bugs/interfaces/bug.py b/lib/lp/bugs/interfaces/bug.py |
464 | index db94b63..7ffc619 100644 |
465 | --- a/lib/lp/bugs/interfaces/bug.py |
466 | +++ b/lib/lp/bugs/interfaces/bug.py |
467 | @@ -1,4 +1,4 @@ |
468 | -# Copyright 2009-2016 Canonical Ltd. This software is licensed under the |
469 | +# Copyright 2009-2022 Canonical Ltd. This software is licensed under the |
470 | # GNU Affero General Public License version 3 (see the file LICENSE). |
471 | |
472 | """Interfaces related to bugs.""" |
473 | @@ -66,7 +66,11 @@ from lp.app.errors import NotFoundError |
474 | from lp.app.interfaces.launchpad import IPrivacy |
475 | from lp.app.validators.attachment import attachment_size_constraint |
476 | from lp.app.validators.name import bug_name_validator |
477 | -from lp.bugs.enums import BugNotificationLevel |
478 | +from lp.bugs.enums import ( |
479 | + BugLockedStatus, |
480 | + BugLockStatus, |
481 | + BugNotificationLevel, |
482 | + ) |
483 | from lp.bugs.interfaces.bugactivity import IBugActivity |
484 | from lp.bugs.interfaces.bugattachment import IBugAttachment |
485 | from lp.bugs.interfaces.bugbranch import IBugBranch |
486 | @@ -382,6 +386,21 @@ class IBugView(Interface): |
487 | readonly=True, |
488 | value_type=Reference(schema=IMessage)), |
489 | exported_as='messages')) |
490 | + lock_status = exported( |
491 | + Choice( |
492 | + title=_('Lock Status'), vocabulary=BugLockStatus, |
493 | + default=BugLockStatus.UNLOCKED, |
494 | + description=_("The lock status of this bug."), |
495 | + readonly=True, |
496 | + required=False), |
497 | + as_of="devel") |
498 | + lock_reason = exported( |
499 | + Text( |
500 | + title=_('Lock Reason'), |
501 | + description=_('The reason for locking this bug.'), |
502 | + readonly=True, |
503 | + required=False), |
504 | + as_of="devel") |
505 | |
506 | def getSpecifications(user): |
507 | """List of related specifications that the user can view.""" |
508 | @@ -978,9 +997,54 @@ class IBugAppend(Interface): |
509 | Return None if no bugtask was edited. |
510 | """ |
511 | |
512 | +class IBugModerate(Interface): |
513 | + """IBug attributes that require the launchpad.Moderate permission.""" |
514 | + |
515 | + @operation_parameters( |
516 | + status=copy_field( |
517 | + IBugView['lock_status'], |
518 | + vocabulary=BugLockedStatus, |
519 | + ), |
520 | + reason=copy_field(IBugView['lock_reason']) |
521 | + ) |
522 | + @export_write_operation() |
523 | + @call_with(who=REQUEST_USER) |
524 | + @operation_for_version("devel") |
525 | + def lock(who, status, reason=None): |
526 | + """ |
527 | + Lock the bug metadata edits to the relevant roles. |
528 | + |
529 | + :param status: The lock status of the bug - one of the values |
530 | + in the BugLockStatus enum. |
531 | + :param reason: The reason for locking this bug. |
532 | + :param who: The IPerson who is making the change. |
533 | + """ |
534 | + |
535 | + @export_write_operation() |
536 | + @call_with(who=REQUEST_USER) |
537 | + @operation_for_version("devel") |
538 | + def unlock(who): |
539 | + """ |
540 | + Unlock the bug metadata edits to the default roles. |
541 | + |
542 | + :param who: The IPerson who is making the change. |
543 | + """ |
544 | + |
545 | + @mutator_for(IBugView['lock_reason']) |
546 | + @operation_parameters(reason=copy_field(IBugView['lock_reason'])) |
547 | + @export_write_operation() |
548 | + @call_with(who=REQUEST_USER) |
549 | + @operation_for_version("devel") |
550 | + def setLockReason(reason, who): |
551 | + """Set the lock reason. |
552 | + |
553 | + :reason: The reason for locking the bug. |
554 | + :who: The IPerson who is making the change. |
555 | + """ |
556 | + |
557 | |
558 | @exported_as_webservice_entry() |
559 | -class IBug(IBugPublic, IBugView, IBugAppend, IHasLinkedBranches): |
560 | +class IBug(IBugPublic, IBugView, IBugAppend, IBugModerate, IHasLinkedBranches): |
561 | """The core bug entry.""" |
562 | |
563 | linked_bugbranches = exported( |
564 | diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py |
565 | index f7c588c..6cb9825 100644 |
566 | --- a/lib/lp/bugs/model/bug.py |
567 | +++ b/lib/lp/bugs/model/bug.py |
568 | @@ -1,4 +1,4 @@ |
569 | -# Copyright 2009-2021 Canonical Ltd. This software is licensed under the |
570 | +# Copyright 2009-2022 Canonical Ltd. This software is licensed under the |
571 | # GNU Affero General Public License version 3 (see the file LICENSE). |
572 | |
573 | """Launchpad bug-related database table classes.""" |
574 | @@ -10,6 +10,7 @@ __all__ = [ |
575 | 'BugMute', |
576 | 'BugSet', |
577 | 'BugTag', |
578 | + 'CannotSetLockReason', |
579 | 'FileBugData', |
580 | 'generate_subscription_with', |
581 | 'get_also_notified_subscribers', |
582 | @@ -19,6 +20,7 @@ __all__ = [ |
583 | |
584 | from email.utils import make_msgid |
585 | from functools import wraps |
586 | +import http.client |
587 | from io import BytesIO |
588 | from itertools import chain |
589 | import operator |
590 | @@ -26,6 +28,7 @@ import re |
591 | |
592 | from lazr.lifecycle.event import ObjectCreatedEvent |
593 | from lazr.lifecycle.snapshot import Snapshot |
594 | +from lazr.restful.declarations import error_status |
595 | import pytz |
596 | from six.moves.collections_abc import ( |
597 | Iterable, |
598 | @@ -95,12 +98,18 @@ from lp.bugs.adapters.bugchange import ( |
599 | BranchUnlinkedFromBug, |
600 | BugConvertedToQuestion, |
601 | BugDuplicateChange, |
602 | + BugLocked, |
603 | + BugLockReasonSet, |
604 | + BugUnlocked, |
605 | BugWatchAdded, |
606 | BugWatchRemoved, |
607 | SeriesNominated, |
608 | UnsubscribedFromBug, |
609 | ) |
610 | -from lp.bugs.enums import BugNotificationLevel |
611 | +from lp.bugs.enums import ( |
612 | + BugLockStatus, |
613 | + BugNotificationLevel, |
614 | + ) |
615 | from lp.bugs.errors import InvalidDuplicateValue |
616 | from lp.bugs.interfaces.bug import ( |
617 | IBug, |
618 | @@ -237,6 +246,18 @@ from lp.services.webapp.snapshot import notify_modified |
619 | from lp.services.xref.interfaces import IXRefSet |
620 | |
621 | |
622 | +@error_status(http.client.BAD_REQUEST) |
623 | +class CannotSetLockReason(Exception): |
624 | + """Raised when someone tries to set lock_reason for an unlocked bug.""" |
625 | + |
626 | + |
627 | +@error_status(http.client.BAD_REQUEST) |
628 | +class CannotLockBug(Exception): |
629 | + """ |
630 | + Raised when someone tries to lock a bug already locked |
631 | + with the same reason. |
632 | + """ |
633 | + |
634 | def snapshot_bug_params(bug_params): |
635 | """Return a snapshot of a `CreateBugParams` object.""" |
636 | return Snapshot( |
637 | @@ -387,6 +408,21 @@ class Bug(SQLBase, InformationTypeMixin): |
638 | heat = IntCol(notNull=True, default=0) |
639 | heat_last_updated = UtcDateTimeCol(default=None) |
640 | latest_patch_uploaded = UtcDateTimeCol(default=None) |
641 | + _lock_status = DBEnum( |
642 | + name='lock_status', enum=BugLockStatus, |
643 | + allow_none=True, default=BugLockStatus.UNLOCKED) |
644 | + lock_reason = StringCol(notNull=False, default=None) |
645 | + |
646 | + @property |
647 | + def lock_status(self): |
648 | + return ( |
649 | + BugLockStatus.UNLOCKED if self._lock_status is None |
650 | + else self._lock_status |
651 | + ) |
652 | + |
653 | + @lock_status.setter |
654 | + def lock_status(self, value): |
655 | + self._lock_status = value |
656 | |
657 | @property |
658 | def linked_branches(self): |
659 | @@ -2198,6 +2234,63 @@ class Bug(SQLBase, InformationTypeMixin): |
660 | BugActivity.datechanged <= end_date) |
661 | return activity_in_range |
662 | |
663 | + def lock(self, who, status, reason=None): |
664 | + """See `IBug`.""" |
665 | + if status == self.lock_status: |
666 | + raise CannotLockBug( |
667 | + "This bug is already locked " |
668 | + "with the lock status '{}'.".format( |
669 | + status |
670 | + ) |
671 | + ) |
672 | + else: |
673 | + old_lock_status = self.lock_status |
674 | + self.lock_status = BugLockStatus.items[status.value] |
675 | + if reason: |
676 | + self.lock_reason = reason |
677 | + |
678 | + self.addChange( |
679 | + BugLocked( |
680 | + when=UTC_NOW, |
681 | + person=who, |
682 | + old_status=old_lock_status, |
683 | + new_status=self.lock_status, |
684 | + reason=reason |
685 | + ) |
686 | + ) |
687 | + |
688 | + def unlock(self, who): |
689 | + """See `IBug`.""" |
690 | + if self.lock_status != BugLockStatus.UNLOCKED: |
691 | + old_lock_status = self.lock_status |
692 | + self.lock_status = BugLockStatus.UNLOCKED |
693 | + self.lock_reason = None |
694 | + |
695 | + self.addChange( |
696 | + BugUnlocked( |
697 | + when=UTC_NOW, |
698 | + person=who, |
699 | + old_status=old_lock_status |
700 | + ) |
701 | + ) |
702 | + |
703 | + def setLockReason(self, reason, who): |
704 | + """See `IBug`.""" |
705 | + if self.lock_status == BugLockStatus.UNLOCKED: |
706 | + raise CannotSetLockReason( |
707 | + "Lock reason cannot be set for an unlocked bug." |
708 | + ) |
709 | + if self.lock_reason != reason: |
710 | + old_reason = self.lock_reason |
711 | + self.lock_reason = reason |
712 | + self.addChange( |
713 | + BugLockReasonSet( |
714 | + when=UTC_NOW, |
715 | + person=who, |
716 | + old_reason=old_reason, |
717 | + new_reason=reason |
718 | + ) |
719 | + ) |
720 | |
721 | @ProxyFactory |
722 | def get_also_notified_subscribers( |
723 | diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py |
724 | index b60b426..beba8af 100644 |
725 | --- a/lib/lp/bugs/security.py |
726 | +++ b/lib/lp/bugs/security.py |
727 | @@ -1,4 +1,4 @@ |
728 | -# Copyright 2010-2020 Canonical Ltd. This software is licensed under the |
729 | +# Copyright 2010-2022 Canonical Ltd. This software is licensed under the |
730 | # GNU Affero General Public License version 3 (see the file LICENSE). |
731 | |
732 | """Security adapters for the bugs module.""" |
733 | @@ -10,6 +10,7 @@ from lp.app.security import ( |
734 | AuthorizationBase, |
735 | DelegatedAuthorization, |
736 | ) |
737 | +from lp.bugs.enums import BugLockStatus |
738 | from lp.bugs.interfaces.bug import IBug |
739 | from lp.bugs.interfaces.bugactivity import IBugActivity |
740 | from lp.bugs.interfaces.bugattachment import IBugAttachment |
741 | @@ -166,6 +167,19 @@ class EditBug(AuthorizationBase): |
742 | user, permission='launchpad.Append'): |
743 | # The user cannot even see the bug. |
744 | return False |
745 | + |
746 | + def in_allowed_roles(): |
747 | + return ( |
748 | + # Users with relevant roles can edit the bug. |
749 | + user.in_admin or |
750 | + user.in_commercial_admin or |
751 | + user.in_registry_experts or |
752 | + _has_any_bug_role(user, self.obj.bugtasks) |
753 | + ) |
754 | + |
755 | + if self.obj.lock_status == BugLockStatus.COMMENT_ONLY: |
756 | + return in_allowed_roles() |
757 | + |
758 | return ( |
759 | # If the bug is private, then we don't need more elaborate |
760 | # checks as they must have been explicitly subscribed. |
761 | @@ -173,18 +187,38 @@ class EditBug(AuthorizationBase): |
762 | # If the user seems generally legitimate, let them through. |
763 | self.forwardCheckAuthenticated( |
764 | user, permission='launchpad.AnyLegitimatePerson') or |
765 | - # The bug reporter can always edit their own bug. |
766 | + # The bug reporter can edit their own bug if it is unlocked. |
767 | user.inTeam(self.obj.owner) or |
768 | - # Users with relevant roles can edit the bug. |
769 | - user.in_admin or user.in_commercial_admin or |
770 | - user.in_registry_experts or |
771 | - _has_any_bug_role(user, self.obj.bugtasks)) |
772 | + in_allowed_roles()) |
773 | |
774 | def checkUnauthenticated(self): |
775 | """Never allow unauthenticated users to edit a bug.""" |
776 | return False |
777 | |
778 | |
779 | +class ModerateBug(AuthorizationBase): |
780 | + """Security adapter for moderating bugs. |
781 | + |
782 | + This is used for operations like locking and unlocking a bug to the |
783 | + relevant roles. |
784 | + """ |
785 | + permission = 'launchpad.Moderate' |
786 | + usedfor = IBug |
787 | + |
788 | + def checkAuthenticated(self, user): |
789 | + if not self.forwardCheckAuthenticated( |
790 | + user, permission='launchpad.Append'): |
791 | + # The user cannot even see the bug. |
792 | + return False |
793 | + |
794 | + return ( |
795 | + user.in_admin or |
796 | + user.in_commercial_admin or |
797 | + user.in_registry_experts or |
798 | + _has_any_bug_role(user, self.obj.bugtasks) |
799 | + ) |
800 | + |
801 | + |
802 | class PublicToAllOrPrivateToExplicitSubscribersForBug(AuthorizationBase): |
803 | permission = 'launchpad.View' |
804 | usedfor = IBug |
805 | diff --git a/lib/lp/bugs/tests/test_bug.py b/lib/lp/bugs/tests/test_bug.py |
806 | index 0c1e24b..ebe1e3b 100644 |
807 | --- a/lib/lp/bugs/tests/test_bug.py |
808 | +++ b/lib/lp/bugs/tests/test_bug.py |
809 | @@ -1,17 +1,22 @@ |
810 | -# Copyright 2009-2020 Canonical Ltd. This software is licensed under the |
811 | +# Copyright 2009-2022 Canonical Ltd. This software is licensed under the |
812 | # GNU Affero General Public License version 3 (see the file LICENSE). |
813 | |
814 | """Tests for lp.bugs.model.Bug.""" |
815 | |
816 | from datetime import timedelta |
817 | +import json |
818 | |
819 | from lazr.lifecycle.snapshot import Snapshot |
820 | +from testtools.matchers import MatchesStructure |
821 | from zope.component import getUtility |
822 | from zope.interface import providedBy |
823 | from zope.security.management import checkPermission |
824 | from zope.security.proxy import removeSecurityProxy |
825 | |
826 | -from lp.bugs.enums import BugNotificationLevel |
827 | +from lp.bugs.enums import ( |
828 | + BugLockStatus, |
829 | + BugNotificationLevel, |
830 | + ) |
831 | from lp.bugs.interfaces.bug import ( |
832 | CreateBugParams, |
833 | IBugSet, |
834 | @@ -24,15 +29,23 @@ from lp.bugs.interfaces.bugtask import ( |
835 | UserCannotEditBugTaskImportance, |
836 | UserCannotEditBugTaskMilestone, |
837 | ) |
838 | +from lp.bugs.model.bug import ( |
839 | + CannotLockBug, |
840 | + CannotSetLockReason, |
841 | + ) |
842 | from lp.registry.tests.test_person import KarmaTestMixin |
843 | +from lp.services.webapp.interfaces import OAuthPermission |
844 | from lp.testing import ( |
845 | admin_logged_in, |
846 | + ANONYMOUS, |
847 | + api_url, |
848 | celebrity_logged_in, |
849 | person_logged_in, |
850 | StormStatementRecorder, |
851 | TestCaseWithFactory, |
852 | ) |
853 | from lp.testing.layers import DatabaseFunctionalLayer |
854 | +from lp.testing.pages import webservice_for_person |
855 | |
856 | |
857 | class TestBug(TestCaseWithFactory): |
858 | @@ -410,3 +423,426 @@ class TestBugPermissions(TestCaseWithFactory, KarmaTestMixin): |
859 | person) |
860 | with person_logged_in(person): |
861 | self.assertTrue(checkPermission('launchpad.Edit', self.bug)) |
862 | + |
863 | + |
864 | +class TestBugLocking(TestCaseWithFactory): |
865 | + """ |
866 | + Tests for the bug locking functionality. |
867 | + """ |
868 | + |
869 | + layer = DatabaseFunctionalLayer |
870 | + |
871 | + def setUp(self): |
872 | + super().setUp() |
873 | + self.person = self.factory.makePerson() |
874 | + self.target = self.factory.makeProduct() |
875 | + |
876 | + def test_bug_lock_status_lock_reason_default_values(self): |
877 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
878 | + self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status) |
879 | + self.assertIsNone(bug.lock_reason) |
880 | + |
881 | + def test_bug_locking_when_bug_already_locked(self): |
882 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
883 | + with person_logged_in(self.target.owner): |
884 | + bug.lock(who=self.target.owner, status=BugLockStatus.COMMENT_ONLY) |
885 | + self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status) |
886 | + self.assertRaises( |
887 | + CannotLockBug, |
888 | + bug.lock, |
889 | + who=self.target.owner, |
890 | + status=BugLockStatus.COMMENT_ONLY |
891 | + ) |
892 | + self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status) |
893 | + |
894 | + def test_bug_locking_with_a_reason_works(self): |
895 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
896 | + with person_logged_in(self.target.owner): |
897 | + bug.lock( |
898 | + who=self.person, |
899 | + status=BugLockStatus.COMMENT_ONLY, |
900 | + reason='too hot', |
901 | + ) |
902 | + self.assertEqual('too hot', bug.lock_reason) |
903 | + |
904 | + def test_updating_bug_lock_reason_not_set_before(self): |
905 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
906 | + with person_logged_in(self.target.owner): |
907 | + bug.lock(who=self.person, status=BugLockStatus.COMMENT_ONLY) |
908 | + self.assertIsNone(bug.lock_reason) |
909 | + bug.setLockReason('too hot', who=self.target.owner) |
910 | + self.assertEqual('too hot', bug.lock_reason) |
911 | + |
912 | + def test_updating_existing_bug_lock_reason_to_none(self): |
913 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
914 | + with person_logged_in(self.target.owner): |
915 | + bug.lock( |
916 | + who=self.person, |
917 | + status=BugLockStatus.COMMENT_ONLY, |
918 | + reason='too hot', |
919 | + ) |
920 | + self.assertEqual('too hot', bug.lock_reason) |
921 | + bug.setLockReason(None, who=self.target.owner) |
922 | + self.assertIsNone(bug.lock_reason) |
923 | + |
924 | + def test_bug_unlocking_clears_the_reason(self): |
925 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
926 | + with person_logged_in(self.target.owner): |
927 | + bug.lock( |
928 | + who=self.person, |
929 | + status=BugLockStatus.COMMENT_ONLY, |
930 | + reason='too hot', |
931 | + ) |
932 | + bug.unlock(who=self.person) |
933 | + self.assertIsNone(bug.lock_reason) |
934 | + |
935 | + def test_bug_locking_unlocking_adds_bug_activity_entries(self): |
936 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
937 | + self.assertEqual(1, bug.activity.count()) |
938 | + with person_logged_in(self.target.owner): |
939 | + bug.lock( |
940 | + who=self.target.owner, |
941 | + status=BugLockStatus.COMMENT_ONLY, |
942 | + reason='too hot' |
943 | + ) |
944 | + self.assertEqual(2, bug.activity.count()) |
945 | + self.assertThat( |
946 | + bug.activity[1], |
947 | + MatchesStructure.byEquality( |
948 | + person=self.target.owner, |
949 | + whatchanged='lock status', |
950 | + oldvalue=str(BugLockStatus.UNLOCKED), |
951 | + newvalue=str(BugLockStatus.COMMENT_ONLY), |
952 | + ) |
953 | + ) |
954 | + bug.unlock(who=self.target.owner) |
955 | + self.assertEqual(3, bug.activity.count()) |
956 | + self.assertThat( |
957 | + bug.activity[2], |
958 | + MatchesStructure.byEquality( |
959 | + person=self.target.owner, |
960 | + whatchanged='lock status', |
961 | + oldvalue=str(BugLockStatus.COMMENT_ONLY), |
962 | + newvalue=str(BugLockStatus.UNLOCKED), |
963 | + ) |
964 | + ) |
965 | + |
966 | + def test_cannot_set_lock_reason_for_an_unlocked_bug(self): |
967 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
968 | + with person_logged_in(self.target.owner): |
969 | + self.assertRaises( |
970 | + CannotSetLockReason, |
971 | + bug.setLockReason, |
972 | + 'too hot', |
973 | + who=self.target.owner |
974 | + ) |
975 | + |
976 | + def test_edit_permission_restrictions_when_a_bug_is_locked(self): |
977 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
978 | + another_person = self.factory.makePerson() |
979 | + |
980 | + with person_logged_in(self.target.owner): |
981 | + bug.lock(who=self.target.owner, status=BugLockStatus.COMMENT_ONLY) |
982 | + |
983 | + self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status) |
984 | + |
985 | + # A user without the relevant role cannot edit a locked bug. |
986 | + with person_logged_in(another_person): |
987 | + self.assertFalse(checkPermission('launchpad.Edit', bug)) |
988 | + |
989 | + # The bug reporter cannot edit a locked bug. |
990 | + with person_logged_in(self.person): |
991 | + self.assertFalse(checkPermission('launchpad.Edit', bug)) |
992 | + |
993 | + # Target driver can edit a locked bug. |
994 | + new_person = self.factory.makePerson() |
995 | + removeSecurityProxy(bug.default_bugtask.target).driver = new_person |
996 | + with person_logged_in(new_person): |
997 | + self.assertTrue(checkPermission('launchpad.Edit', bug)) |
998 | + |
999 | + # Admins can edit a locked bug. |
1000 | + with admin_logged_in(): |
1001 | + self.assertTrue(checkPermission('launchpad.Edit', bug)) |
1002 | + |
1003 | + # Commercial admins can edit a locked bug. |
1004 | + with celebrity_logged_in('commercial_admin'): |
1005 | + self.assertTrue(checkPermission('launchpad.Edit', bug)) |
1006 | + |
1007 | + # Registry experts can edit a locked bug. |
1008 | + with celebrity_logged_in('registry_experts'): |
1009 | + self.assertTrue(checkPermission('launchpad.Edit', bug)) |
1010 | + |
1011 | + |
1012 | + def test_only_those_with_moderate_permission_can_lock_unlock_a_bug(self): |
1013 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1014 | + another_person = self.factory.makePerson() |
1015 | + |
1016 | + # Unauthenticated person cannot moderate a bug. |
1017 | + self.assertFalse(checkPermission('launchpad.Moderate', bug)) |
1018 | + |
1019 | + # A user without the relevant role cannot moderate a bug. |
1020 | + with person_logged_in(another_person): |
1021 | + self.assertFalse(checkPermission('launchpad.Moderate', bug)) |
1022 | + |
1023 | + # The bug reporter cannot moderate a bug. |
1024 | + with person_logged_in(self.person): |
1025 | + self.assertFalse(checkPermission('launchpad.Moderate', bug)) |
1026 | + |
1027 | + # Admins can moderate a bug. |
1028 | + with admin_logged_in(): |
1029 | + self.assertTrue(checkPermission('launchpad.Moderate', bug)) |
1030 | + |
1031 | + # Commercial admins can moderate a bug. |
1032 | + with celebrity_logged_in('commercial_admin'): |
1033 | + self.assertTrue(checkPermission('launchpad.Moderate', bug)) |
1034 | + |
1035 | + # Registry experts can moderate a bug. |
1036 | + with celebrity_logged_in('registry_experts'): |
1037 | + self.assertTrue(checkPermission('launchpad.Moderate', bug)) |
1038 | + |
1039 | + # Target owner can moderate a bug. |
1040 | + with person_logged_in( |
1041 | + removeSecurityProxy(bug.default_bugtask.target).owner |
1042 | + ): |
1043 | + self.assertTrue(checkPermission('launchpad.Moderate', bug)) |
1044 | + |
1045 | + # Target driver can moderate a bug. |
1046 | + new_person = self.factory.makePerson() |
1047 | + removeSecurityProxy(bug.default_bugtask.target).driver = new_person |
1048 | + with person_logged_in(new_person): |
1049 | + self.assertTrue(checkPermission('launchpad.Moderate', bug)) |
1050 | + |
1051 | + yet_another_person = self.factory.makePerson() |
1052 | + removeSecurityProxy( |
1053 | + bug.default_bugtask.target |
1054 | + ).bug_supervisor = yet_another_person |
1055 | + with person_logged_in(yet_another_person): |
1056 | + self.assertTrue(checkPermission('launchpad.Moderate', bug)) |
1057 | + |
1058 | + |
1059 | +class TestBugLockingWebService(TestCaseWithFactory): |
1060 | + """Tests for the bug locking and unlocking web service methods.""" |
1061 | + |
1062 | + layer = DatabaseFunctionalLayer |
1063 | + |
1064 | + def setUp(self): |
1065 | + super().setUp() |
1066 | + self.person = self.factory.makePerson() |
1067 | + self.target = self.factory.makeProduct() |
1068 | + |
1069 | + def test_bug_lock_status_invalid_values(self): |
1070 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1071 | + invalid_values_list = ["test", 1.23, 123, "Unlocked"] |
1072 | + webservice = webservice_for_person( |
1073 | + self.target.owner, |
1074 | + permission=OAuthPermission.WRITE_PRIVATE |
1075 | + ) |
1076 | + bug_url = api_url(bug) |
1077 | + webservice.default_api_version = "devel" |
1078 | + for invalid_value in invalid_values_list: |
1079 | + response = webservice.named_post( |
1080 | + bug_url, 'lock', status=invalid_value |
1081 | + ) |
1082 | + self.assertEqual(400, response.status) |
1083 | + |
1084 | + def test_who_value_for_lock_is_correctly_set(self): |
1085 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1086 | + self.assertEqual(bug.activity.count(), 1) |
1087 | + self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status) |
1088 | + |
1089 | + bug_url = api_url(bug) |
1090 | + webservice = webservice_for_person( |
1091 | + self.target.owner, |
1092 | + permission=OAuthPermission.WRITE_PRIVATE |
1093 | + ) |
1094 | + webservice.default_api_version = "devel" |
1095 | + |
1096 | + response = webservice.named_post( |
1097 | + bug_url, 'lock', status="Comment-only", |
1098 | + ) |
1099 | + self.assertEqual(200, response.status) |
1100 | + |
1101 | + with person_logged_in(ANONYMOUS): |
1102 | + self.assertEqual(2, bug.activity.count()) |
1103 | + self.assertThat( |
1104 | + bug.activity[1], |
1105 | + MatchesStructure.byEquality( |
1106 | + person=self.target.owner, |
1107 | + whatchanged='lock status', |
1108 | + oldvalue=str(BugLockStatus.UNLOCKED), |
1109 | + newvalue=str(BugLockStatus.COMMENT_ONLY) |
1110 | + ) |
1111 | + ) |
1112 | + |
1113 | + def test_who_value_for_unlock_is_correctly_set(self): |
1114 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1115 | + self.assertEqual(1, bug.activity.count()) |
1116 | + with person_logged_in(self.target.owner): |
1117 | + bug.lock(who=self.target.owner, status=BugLockStatus.COMMENT_ONLY) |
1118 | + self.assertEqual(2, bug.activity.count()) |
1119 | + bug_url = api_url(bug) |
1120 | + webservice = webservice_for_person( |
1121 | + self.target.owner, |
1122 | + permission=OAuthPermission.WRITE_PRIVATE |
1123 | + ) |
1124 | + webservice.default_api_version = "devel" |
1125 | + |
1126 | + response = webservice.named_post( |
1127 | + bug_url, 'unlock' |
1128 | + ) |
1129 | + self.assertEqual(200, response.status) |
1130 | + with person_logged_in(ANONYMOUS): |
1131 | + self.assertEqual(3, bug.activity.count()) |
1132 | + self.assertThat( |
1133 | + bug.activity[2], |
1134 | + MatchesStructure.byEquality( |
1135 | + person=self.target.owner, |
1136 | + whatchanged='lock status', |
1137 | + oldvalue=str(BugLockStatus.COMMENT_ONLY), |
1138 | + newvalue=str(BugLockStatus.UNLOCKED), |
1139 | + ) |
1140 | + ) |
1141 | + |
1142 | + def test_lock_status_lock_reason_values_unlocked_bug(self): |
1143 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1144 | + bug_url = api_url(bug) |
1145 | + |
1146 | + webservice = webservice_for_person( |
1147 | + self.target.owner, |
1148 | + permission=OAuthPermission.WRITE_PRIVATE |
1149 | + ) |
1150 | + webservice.default_api_version = 'devel' |
1151 | + |
1152 | + response = webservice.get(bug_url) |
1153 | + self.assertEqual(200, response.status) |
1154 | + response_json = response.jsonBody() |
1155 | + self.assertEqual( |
1156 | + str(BugLockStatus.UNLOCKED), |
1157 | + response_json['lock_status'] |
1158 | + ) |
1159 | + self.assertIsNone(response_json['lock_reason']) |
1160 | + |
1161 | + def test_lock_status_lock_reason_values_after_locking(self): |
1162 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1163 | + bug_url = api_url(bug) |
1164 | + |
1165 | + with person_logged_in(self.target.owner): |
1166 | + bug.lock( |
1167 | + who=self.target.owner, |
1168 | + status=BugLockStatus.COMMENT_ONLY, |
1169 | + reason='too hot' |
1170 | + ) |
1171 | + |
1172 | + webservice = webservice_for_person( |
1173 | + self.target.owner, |
1174 | + permission=OAuthPermission.WRITE_PRIVATE |
1175 | + ) |
1176 | + webservice.default_api_version = 'devel' |
1177 | + |
1178 | + response = webservice.get(bug_url) |
1179 | + self.assertEqual(200, response.status) |
1180 | + response_json = response.jsonBody() |
1181 | + self.assertEqual( |
1182 | + str(BugLockStatus.COMMENT_ONLY), |
1183 | + response_json['lock_status'] |
1184 | + ) |
1185 | + self.assertEqual( |
1186 | + response_json['lock_reason'], |
1187 | + 'too hot' |
1188 | + ) |
1189 | + |
1190 | + def test_setting_lock_reason_for_an_unlocked_bug(self): |
1191 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1192 | + bug_url = api_url(bug) |
1193 | + webservice = webservice_for_person( |
1194 | + self.target.owner, |
1195 | + permission=OAuthPermission.WRITE_PRIVATE |
1196 | + ) |
1197 | + webservice.default_api_version = 'devel' |
1198 | + |
1199 | + response = webservice.patch( |
1200 | + bug_url, "application/json", |
1201 | + json.dumps({'lock_reason': 'too hot'}) |
1202 | + ) |
1203 | + self.assertThat( |
1204 | + response, |
1205 | + MatchesStructure.byEquality( |
1206 | + status=400, |
1207 | + body=b'Lock reason cannot be set for an unlocked bug.' |
1208 | + ) |
1209 | + ) |
1210 | + |
1211 | + def test_setting_lock_reason_for_a_locked_bug_without_a_reason(self): |
1212 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1213 | + with person_logged_in(self.target.owner): |
1214 | + bug.lock(who=self.target.owner, status=BugLockStatus.COMMENT_ONLY) |
1215 | + |
1216 | + self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status) |
1217 | + self.assertIsNone(bug.lock_reason) |
1218 | + |
1219 | + bug_url = api_url(bug) |
1220 | + webservice = webservice_for_person( |
1221 | + self.target.owner, |
1222 | + permission=OAuthPermission.WRITE_PRIVATE |
1223 | + ) |
1224 | + webservice.default_api_version = 'devel' |
1225 | + |
1226 | + response = webservice.patch( |
1227 | + bug_url, "application/json", |
1228 | + json.dumps({'lock_reason': 'too hot'}) |
1229 | + ) |
1230 | + self.assertEqual(209, response.status) |
1231 | + self.assertEqual('too hot', response.jsonBody()['lock_reason']) |
1232 | + |
1233 | + |
1234 | + def test_setting_lock_reason_for_a_locked_bug_with_a_reason(self): |
1235 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1236 | + with person_logged_in(self.target.owner): |
1237 | + bug.lock( |
1238 | + who=self.target.owner, |
1239 | + status=BugLockStatus.COMMENT_ONLY, |
1240 | + reason='too hot' |
1241 | + ) |
1242 | + |
1243 | + self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status) |
1244 | + self.assertEqual('too hot', bug.lock_reason) |
1245 | + |
1246 | + bug_url = api_url(bug) |
1247 | + webservice = webservice_for_person( |
1248 | + self.target.owner, |
1249 | + permission=OAuthPermission.WRITE_PRIVATE |
1250 | + ) |
1251 | + webservice.default_api_version = 'devel' |
1252 | + |
1253 | + response = webservice.patch( |
1254 | + bug_url, "application/json", |
1255 | + json.dumps({'lock_reason': 'too hot!'}) |
1256 | + ) |
1257 | + self.assertEqual(209, response.status) |
1258 | + self.assertEqual('too hot!', response.jsonBody()['lock_reason']) |
1259 | + |
1260 | + def test_removing_lock_reason_for_a_locked_bug_with_a_reason(self): |
1261 | + bug = self.factory.makeBug(owner=self.person, target=self.target) |
1262 | + with person_logged_in(self.target.owner): |
1263 | + bug.lock( |
1264 | + who=self.target.owner, |
1265 | + status=BugLockStatus.COMMENT_ONLY, |
1266 | + reason='too hot' |
1267 | + ) |
1268 | + |
1269 | + self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status) |
1270 | + self.assertEqual('too hot', bug.lock_reason) |
1271 | + |
1272 | + bug_url = api_url(bug) |
1273 | + webservice = webservice_for_person( |
1274 | + self.target.owner, |
1275 | + permission=OAuthPermission.WRITE_PRIVATE |
1276 | + ) |
1277 | + webservice.default_api_version = 'devel' |
1278 | + |
1279 | + response = webservice.patch( |
1280 | + bug_url, "application/json", |
1281 | + json.dumps({'lock_reason': None}) |
1282 | + ) |
1283 | + self.assertEqual(209, response.status) |
1284 | + self.assertEqual(None, response.jsonBody()['lock_reason']) |
1285 | diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py |
1286 | index 5eabeb6..da3f2f7 100644 |
1287 | --- a/lib/lp/scripts/garbo.py |
1288 | +++ b/lib/lp/scripts/garbo.py |
1289 | @@ -51,6 +51,7 @@ from zope.component import getUtility |
1290 | from zope.security.proxy import removeSecurityProxy |
1291 | |
1292 | from lp.answers.model.answercontact import AnswerContact |
1293 | +from lp.bugs.enums import BugLockStatus |
1294 | from lp.bugs.interfaces.bug import IBugSet |
1295 | from lp.bugs.model.bug import Bug |
1296 | from lp.bugs.model.bugattachment import BugAttachment |
1297 | @@ -1762,6 +1763,42 @@ class PopulateSnapBuildStoreRevision(TunableLoop): |
1298 | transaction.commit() |
1299 | |
1300 | |
1301 | +class PopulateBugLockStatusDefaultUnlocked(TunableLoop): |
1302 | + """ |
1303 | + Populates Bug.lock_status to BugLockStatus.UNLOCKED if not set |
1304 | + """ |
1305 | + |
1306 | + maximum_chunk_size = 5000 |
1307 | + |
1308 | + def __init__(self, log, abort_time=None): |
1309 | + super().__init__(log, abort_time) |
1310 | + self.store = IMasterStore(Bug) |
1311 | + self.start_at = 1 |
1312 | + |
1313 | + def findBugsLockStatusNone(self): |
1314 | + return self.store.find( |
1315 | + Bug.id, |
1316 | + Bug.id >= self.start_at, |
1317 | + Bug._lock_status == None, |
1318 | + ).order_by(Bug.id) |
1319 | + |
1320 | + def setBugLockStatusUnlocked(self, bug_ids): |
1321 | + self.store.find(Bug, Bug.id.is_in(bug_ids)).set( |
1322 | + _lock_status=BugLockStatus.UNLOCKED |
1323 | + ) |
1324 | + |
1325 | + def isDone(self): |
1326 | + return self.findBugsLockStatusNone().is_empty() |
1327 | + |
1328 | + def __call__(self, chunk_size): |
1329 | + bug_ids = [ |
1330 | + bug_id for bug_id in self.findBugsLockStatusNone()[:chunk_size] |
1331 | + ] |
1332 | + self.setBugLockStatusUnlocked(bug_ids) |
1333 | + self.start_at = bug_ids[-1] + 1 |
1334 | + transaction.commit() |
1335 | + |
1336 | + |
1337 | class BaseDatabaseGarbageCollector(LaunchpadCronScript): |
1338 | """Abstract base class to run a collection of TunableLoops.""" |
1339 | script_name = None # Script name for locking and database user. Override. |
1340 | @@ -2053,6 +2090,7 @@ class DailyDatabaseGarbageCollector(BaseDatabaseGarbageCollector): |
1341 | OCIFilePruner, |
1342 | ObsoleteBugAttachmentPruner, |
1343 | OldTimeLimitedTokenDeleter, |
1344 | + PopulateBugLockStatusDefaultUnlocked, |
1345 | PopulateSnapBuildStoreRevision, |
1346 | POTranslationPruner, |
1347 | PreviewDiffPruner, |
1348 | diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py |
1349 | index 36c64e6..94c2c2d 100644 |
1350 | --- a/lib/lp/scripts/tests/test_garbo.py |
1351 | +++ b/lib/lp/scripts/tests/test_garbo.py |
1352 | @@ -96,6 +96,7 @@ from lp.scripts.garbo import ( |
1353 | load_garbo_job_state, |
1354 | LoginTokenPruner, |
1355 | OpenIDConsumerAssociationPruner, |
1356 | + PopulateBugLockStatusDefaultUnlocked, |
1357 | PopulateSnapBuildStoreRevision, |
1358 | ProductVCSPopulator, |
1359 | save_garbo_job_state, |
1360 | @@ -2045,6 +2046,21 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory): |
1361 | switch_dbuser('testadmin') |
1362 | self.assertEqual(build1._store_upload_revision, 1) |
1363 | |
1364 | + def test_PopulateBugLockStatusDefaultUnlocked(self): |
1365 | + switch_dbuser('testadmin') |
1366 | + populator = PopulateBugLockStatusDefaultUnlocked(log=None) |
1367 | + for _ in range(5): |
1368 | + bug = self.factory.makeBug() |
1369 | + removeSecurityProxy(bug).lock_status = None |
1370 | + |
1371 | + result_set = populator.findBugsLockStatusNone() |
1372 | + self.assertGreater(result_set.count(), 0) |
1373 | + |
1374 | + self.runDaily() |
1375 | + |
1376 | + result_set = populator.findBugsLockStatusNone() |
1377 | + self.assertTrue(result_set.is_empty()) |
1378 | + |
1379 | |
1380 | class TestGarboTasks(TestCaseWithFactory): |
1381 | layer = LaunchpadZopelessLayer |
Thanks for working on this!
Having read this, I think I'd in fact prefer you to revert the sampledata changes from this MP. We need to make sure that we run tests on a database that has some rows with NULL lock_status, in order to avoid transitional mistakes. Sampledata can be updated once garbo has run.
Could you add screenshots of what (a) the ordinary bug log (i.e. what you see if you just load the bug's default web page) and the +activity page for the bug look like for a bug that has been locked? There are some fiddly interactions here, and I'd like to make sure that both of them come out looking reasonable.