launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
Fix determination of bug target roles

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1875398 in Launchpad itself: "Can't edit attachments on Ubuntu bugs anymore"
  https://bugs.launchpad.net/launchpad/+bug/1875398

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

When checking whether a user can edit a bug or a bug attachment, the interesting roles (owner, driver, bug supervisor) exist mainly on the bug target pillar rather than on the bug target (compare DeleteBugTask), and should be checked there.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-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 8301459..bd5f970 100644
--- a/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
+++ b/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
@@ -39,6 +39,11 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
 
         login_person(self.bug_owner)
         self.bug = self.factory.makeBug(owner=self.bug_owner)
+        # Reassign the bug's default bug task to a target such that the
+        # pillar is different from the immediate target.  This can make a
+        # difference for some security checks.
+        self.bug.default_bugtask.transitionToTarget(
+            self.factory.makeDistributionSourcePackage(), self.bug_owner)
         self.bugattachment = self.factory.makeBugAttachment(
             bug=self.bug, filename='foo.diff', data=b'file content',
             description='attachment description', content_type='text/plain',
diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py
index 381f93c..da8bd30 100644
--- a/lib/lp/bugs/security.py
+++ b/lib/lp/bugs/security.py
@@ -122,12 +122,13 @@ 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."""
+def _has_any_bug_role(user, tasks):
+    """Return True if the user has any role on any of these bug tasks."""
     # 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.
+    targets = [task.pillar for task in tasks]
     for target in targets:
         roles = []
         if IHasOwner.providedBy(target):
@@ -174,8 +175,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
-            _has_any_bug_role(
-                user, [task.target for task in self.obj.bugtasks]))
+            _has_any_bug_role(user, self.obj.bugtasks))
 
     def checkUnauthenticated(self):
         """Never allow unauthenticated users to edit a bug."""
@@ -224,8 +224,7 @@ class EditBugAttachment(AuthorizationBase):
         return (user.in_admin or
                 user.in_registry_experts or
                 user.inTeam(self.obj.message.owner) or
-                _has_any_bug_role(
-                    user, [task.target for task in self.obj.bug.bugtasks]))
+                _has_any_bug_role(user, self.obj.bug.bugtasks))
 
     def checkUnauthenticated(self):
         return False