launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:bug-attachment-removal-roles into launchpad:master.

Commit message:
Allow people with bug target roles to edit attachments

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #117752 in Launchpad itself: "Any logged in user can delete any attachments"
  https://bugs.launchpad.net/launchpad/+bug/117752

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

In particular, this ensures that bug supervisors can edit attachments on bugs with corresponding tasks, which is needed by Ubuntu's retracers.

The implementation of this is borrowed from EditBug, and, as in that case, isn't particularly fast for bugs with many tasks.  However, checking whether we can edit a bug attachment isn't a hot operation (in the UI, I don't believe we check this when viewing bugs, only on the page to edit an attachment), so we can continue to get away with this for the time being.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:bug-attachment-removal-roles into launchpad:master.

diff --git a/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py b/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
index 34d9a03..8301459 100644
--- a/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
+++ b/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
@@ -8,6 +8,7 @@ from zope.component import getUtility
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.testing import (
+    login_admin,
     login_person,
     person_logged_in,
     TestCaseWithFactory,
@@ -56,8 +57,7 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
             'application/whatever', self.bugattachment.libraryfile.mimetype)
 
     def test_admin_changes_any_attachment(self):
-        admin = self.factory.makeAdministrator()
-        login_person(admin)
+        login_admin()
         create_initialized_view(
             self.bugattachment, name='+edit', form=self.CHANGE_FORM_DATA)
         self.assertEqual('new description', self.bugattachment.title)
@@ -74,6 +74,18 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
         self.assertEqual(
             'application/whatever', self.bugattachment.libraryfile.mimetype)
 
+    def test_pillar_bug_supervisor_changes_any_attachment(self):
+        login_admin()
+        bug_supervisor = self.factory.makePerson()
+        self.bug.default_bugtask.pillar.bug_supervisor = bug_supervisor
+        login_person(bug_supervisor)
+        create_initialized_view(
+            self.bugattachment, name='+edit', form=self.CHANGE_FORM_DATA)
+        self.assertEqual('new description', self.bugattachment.title)
+        self.assertTrue(self.bugattachment.is_patch)
+        self.assertEqual(
+            'application/whatever', self.bugattachment.libraryfile.mimetype)
+
     def test_other_user_changes_attachment_fails(self):
         random_user = self.factory.makePerson()
         login_person(random_user)
@@ -95,8 +107,7 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
         self.assertEqual(1, self.bug.attachments.count())
 
     def test_admin_can_delete_any_attachment(self):
-        admin = self.factory.makeAdministrator()
-        login_person(admin)
+        login_admin()
         create_initialized_view(
             self.bugattachment, name='+edit', form=self.DELETE_FORM_DATA)
         self.assertEqual(0, self.bug.attachments.count())
@@ -107,6 +118,15 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
             self.bugattachment, name='+edit', form=self.DELETE_FORM_DATA)
         self.assertEqual(0, self.bug.attachments.count())
 
+    def test_pillar_bug_supervisor_can_delete_any_attachment(self):
+        login_admin()
+        bug_supervisor = self.factory.makePerson()
+        self.bug.default_bugtask.pillar.bug_supervisor = bug_supervisor
+        login_person(bug_supervisor)
+        create_initialized_view(
+            self.bugattachment, name='+edit', form=self.DELETE_FORM_DATA)
+        self.assertEqual(0, self.bug.attachments.count())
+
     def test_attachment_owner_can_delete_their_own_attachment(self):
         bug = self.factory.makeBug(owner=self.bug_owner)
         another_user = self.factory.makePerson()
diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py
index 573f85a..381f93c 100644
--- a/lib/lp/bugs/security.py
+++ b/lib/lp/bugs/security.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Security adapters for the bugs module."""
@@ -122,6 +122,25 @@ class AppendBug(AuthorizationBase):
         return False
 
 
+def _has_any_bug_role(user, targets):
+    """Return True if the user has any role on any of these bug targets."""
+    # XXX cjwatson 2019-03-26: This is inefficient for bugs with many
+    # targets.  However, we only get here if we can't easily establish that
+    # the user seems legitimate, so it shouldn't be a big problem in
+    # practice.  We can optimise this further if it turns out to matter.
+    for target in targets:
+        roles = []
+        if IHasOwner.providedBy(target):
+            roles.append('owner')
+        if IHasAppointedDriver.providedBy(target):
+            roles.append('driver')
+        if IHasBugSupervisor.providedBy(target):
+            roles.append('bug_supervisor')
+        if user.isOneOf(target, roles):
+            return True
+    return False
+
+
 class EditBug(AuthorizationBase):
     """Security adapter for editing bugs.
 
@@ -132,25 +151,6 @@ class EditBug(AuthorizationBase):
     permission = 'launchpad.Edit'
     usedfor = IBug
 
-    def _hasAnyRole(self, user, targets):
-        """Return True if the user has any role on any of these bug targets."""
-        # XXX cjwatson 2019-03-26: This is inefficient for bugs with many
-        # targets.  However, we only get here if we can't easily establish
-        # that the user seems legitimate, so it shouldn't be a big problem
-        # in practice.  We can optimise this further if it turns out to
-        # matter.
-        for target in targets:
-            roles = []
-            if IHasOwner.providedBy(target):
-                roles.append('owner')
-            if IHasAppointedDriver.providedBy(target):
-                roles.append('driver')
-            if IHasBugSupervisor.providedBy(target):
-                roles.append('bug_supervisor')
-            if user.isOneOf(target, roles):
-                return True
-        return False
-
     def checkAuthenticated(self, user):
         """Allow sufficiently-trusted users to edit bugs.
 
@@ -174,7 +174,7 @@ class EditBug(AuthorizationBase):
             # Users with relevant roles can edit the bug.
             user.in_admin or user.in_commercial_admin or
             user.in_registry_experts or
-            self._hasAnyRole(
+            _has_any_bug_role(
                 user, [task.target for task in self.obj.bugtasks]))
 
     def checkUnauthenticated(self):
@@ -221,11 +221,11 @@ class EditBugAttachment(AuthorizationBase):
     usedfor = IBugAttachment
 
     def checkAuthenticated(self, user):
-        # XXX: pappacena 2020-04-02: Maybe for bug tasks we should also allow
-        # pillar's bug supervisor to edit the attachments.
         return (user.in_admin or
                 user.in_registry_experts or
-                user.inTeam(self.obj.message.owner))
+                user.inTeam(self.obj.message.owner) or
+                _has_any_bug_role(
+                    user, [task.target for task in self.obj.bug.bugtasks]))
 
     def checkUnauthenticated(self):
         return False