launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:bug-attachment-removal-restrictions into launchpad:master.

Commit message:
Restricting bug attachments removal so that only the bug owner, the attachment owner, admin users or launchpad developers can remove attachments. 

This is done by double checking permission before removing at the backend, and removing the "Delete Attachment" button from the interface for users without permission to do so.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/381537
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:bug-attachment-removal-restrictions into launchpad:master.

diff --git a/lib/lp/bugs/browser/bugattachment.py b/lib/lp/bugs/browser/bugattachment.py
index b239c45..a009e77 100644
--- a/lib/lp/bugs/browser/bugattachment.py
+++ b/lib/lp/bugs/browser/bugattachment.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2018 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).
 
 """Bug attachment views."""
@@ -169,7 +169,10 @@ class BugAttachmentEditView(LaunchpadFormView, BugAttachmentContentCheck):
                 ILibraryFileAliasWithParent)
             lfa_with_parent.mimetype = data['contenttype']
 
-    @action('Delete Attachment', name='delete')
+    def canRemoveFromBug(self, action):
+        return self.context.canRemoveFromBug(self.user)
+
+    @action('Delete Attachment', name='delete', condition=canRemoveFromBug)
     def delete_action(self, action, data):
         libraryfile_url = ProxiedLibraryFileAlias(
             self.context.libraryfile, self.context).http_url
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 2710d05..f1e1448 100644
--- a/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
+++ b/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
@@ -1,4 +1,4 @@
-# Copyright 2010 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).
 
 __metaclass__ = type
@@ -81,30 +81,55 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
         'field.actions.delete': 'Delete Attachment',
         }
 
-    def test_delete_action_public_bug(self):
-        # Bug attachments can be removed from a bug.
+    def test_delete_cannot_be_performed_by_other_users(self):
         user = self.factory.makePerson()
         login_person(user)
         create_initialized_view(
             self.bugattachment, name='+edit', form=self.DELETE_FORM_DATA)
-        self.assertEqual(0, self.bug.attachments.count())
+        self.assertEqual(1, self.bug.attachments.count())
 
-    def test_delete_action_private_bug(self):
-        # Subscribers of a private bug can delete attachments.
-        user = self.factory.makePerson()
-        self.bug.setPrivate(True, self.bug_owner)
-        with person_logged_in(self.bug_owner):
-            self.bug.subscribe(user, self.bug_owner)
-        login_person(user)
+    def test_admin_can_delete_any_attachment(self):
+        admin = self.factory.makeAdministrator()
+        login_person(admin)
         create_initialized_view(
             self.bugattachment, name='+edit', form=self.DELETE_FORM_DATA)
         self.assertEqual(0, self.bug.attachments.count())
 
-    def test_delete_action_private_bug_unautorized(self):
-        # Other users cannot delete private bug attachments.
-        user = self.factory.makePerson()
-        self.bug.setPrivate(True, self.bug_owner)
-        login_person(user)
-        self.assertRaises(
-            Unauthorized, create_initialized_view, self.bugattachment,
-            name='+edit', form=self.DELETE_FORM_DATA)
+    def test_attachment_owner_can_delete_his_own_attachment(self):
+        bug = self.factory.makeBug(owner=self.bug_owner)
+        another_user = self.factory.makePerson()
+        attachment = self.factory.makeBugAttachment(
+            bug=bug, owner=another_user, filename='foo.diff',
+            data=b'the file content', description='some file',
+            content_type='text/plain', is_patch=False)
+
+        login_person(another_user)
+        create_initialized_view(
+            attachment, name='+edit', form=self.DELETE_FORM_DATA)
+        self.assertEqual(0, bug.attachments.count())
+
+    def test_bug_owner_can_delete_any_attachment(self):
+        bug = self.factory.makeBug(owner=self.bug_owner)
+        # Attachment from bug owner.
+        attachment1 = self.factory.makeBugAttachment(
+            bug=bug, owner=self.bug_owner, filename='foo.diff',
+            data=b'the file content', description='some file',
+            content_type='text/plain', is_patch=False)
+
+        # Attachment from another user.
+        another_user = self.factory.makePerson()
+        attachment2 = self.factory.makeBugAttachment(
+            bug=bug, owner=another_user, filename='foo.diff',
+            data=b'the file content', description='some file',
+            content_type='text/plain', is_patch=False)
+
+        login_person(self.bug_owner)
+        # Bug owner can remove his own attachment.
+        create_initialized_view(
+            attachment1, name='+edit', form=self.DELETE_FORM_DATA)
+        self.assertEqual(1, bug.attachments.count())
+
+        # Bug owner can remove someone else's attachment.
+        create_initialized_view(
+            attachment2, name='+edit', form=self.DELETE_FORM_DATA)
+        self.assertEqual(0, bug.attachments.count())
diff --git a/lib/lp/bugs/interfaces/bugattachment.py b/lib/lp/bugs/interfaces/bugattachment.py
index bbfa055..798b340 100644
--- a/lib/lp/bugs/interfaces/bugattachment.py
+++ b/lib/lp/bugs/interfaces/bugattachment.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Bug attachment interfaces."""
@@ -126,6 +126,14 @@ class IBugAttachment(IHasBug):
         description=_('Is this attachment a patch?'),
         readonly=True)
 
+    def canRemoveFromBug(user):
+        """Checks if this attachment can be removed from bug by the given
+        user.
+
+        An attachment can only be removed by admin users, launchpad
+        developers, bug owner or by the user who uploaded the attachment.
+        """
+
     @call_with(user=REQUEST_USER)
     @export_write_operation()
     def removeFromBug(user):
diff --git a/lib/lp/bugs/model/bugattachment.py b/lib/lp/bugs/model/bugattachment.py
index d514e31..6cf0ee0 100644
--- a/lib/lp/bugs/model/bugattachment.py
+++ b/lib/lp/bugs/model/bugattachment.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -16,6 +16,7 @@ from sqlobject import (
 from storm.store import Store
 from zope.event import notify
 from zope.interface import implementer
+from zope.security.interfaces import Unauthorized
 
 from lp.app.errors import NotFoundError
 from lp.bugs.interfaces.bugattachment import (
@@ -23,11 +24,19 @@ from lp.bugs.interfaces.bugattachment import (
     IBugAttachment,
     IBugAttachmentSet,
     )
+from lp.registry.interfaces.role import IPersonRoles
 from lp.services.database.enumcol import EnumCol
 from lp.services.database.sqlbase import SQLBase
 from lp.services.propertycache import cachedproperty
 
 
+class BugAttachmentPermissionError(Unauthorized):
+    def __init__(self, user):
+        super(BugAttachmentPermissionError, self).__init__(
+            "{} doesn't have permission to perform this action".format(
+                user.name))
+
+
 @implementer(IBugAttachment)
 class BugAttachment(SQLBase):
     """A bug attachment."""
@@ -62,8 +71,17 @@ class BugAttachment(SQLBase):
         """See IBugAttachment."""
         return self.type == BugAttachmentType.PATCH
 
+    def canRemoveFromBug(self, user):
+        """See IBugAttachment."""
+        person_roles = IPersonRoles(user)
+        if person_roles.in_admin or person_roles.in_launchpad_developers:
+            return True
+        return user.inTeam(self.message.owner) or user.inTeam(self.bug.owner)
+
     def removeFromBug(self, user):
         """See IBugAttachment."""
+        if not self.canRemoveFromBug(user):
+            raise BugAttachmentPermissionError(user)
         notify(ObjectDeletedEvent(self, user))
         self.destroySelf()
 
diff --git a/lib/lp/bugs/stories/bugattachments/xx-delete-bug-attachment.txt b/lib/lp/bugs/stories/bugattachments/xx-delete-bug-attachment.txt
index 53ab490..0f725a9 100644
--- a/lib/lp/bugs/stories/bugattachments/xx-delete-bug-attachment.txt
+++ b/lib/lp/bugs/stories/bugattachments/xx-delete-bug-attachment.txt
@@ -42,6 +42,26 @@ attachment.
     >>> user_browser.getControl('Delete Attachment') is not None
     True
 
+But this delete option should not be shown for other users.
+
+    >>> from lp.testing.factory import LaunchpadObjectFactory
+    >>> from lp.testing.pages import setupBrowserForUser
+    >>> login('foo.bar@xxxxxxxxxxxxx')
+    >>> another_user = LaunchpadObjectFactory().makePerson()
+    >>> another_browser = setupBrowserForUser(another_user)
+    >>> logout()
+
+    >>> another_browser.open('http://launchpad.test/bugs/2')
+    >>> another_browser.getLink(url=re.compile(r'.*/\+attachment/\d+$')).click()
+    >>> print(another_browser.title)
+    Bug #2...
+    >>> try:
+    ...     another_browser.getControl('Delete Attachment')
+    ...     raise ValueError("'Delete Attachment' button shouldn't be here!")
+    ... except LookupError:
+    ...     print(True)
+    True
+
 If the button is pressed, the attachment will be deleted, which means
 that it won't show up in the attachments portlet anymore. Since there
 arent' any other attachments, the portlet won't show up at all.