Merge ~pappacena/launchpad:bug-attachment-removal-restrictions into launchpad:master

[Thiago F. Pappacena]

Restricting bug attachments removal so that only the attachment owner, admin users or registry experts 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.

[Ioana Lasc (ilasc)]

Code looks good to me but I would have the same question as Thiago typed himself inline: is this list what we want: attachments can be removed by admin users, launchpad developers, bug owner or by the user who uploaded the attachment?

Unit Tests also look good, one question: this test case might be in there and I might be missing this but if answer to above is yes, should we also test for LP developers being able delete ?

[Colin Watson (cjwatson)]

I agree with Ioana that we should add an extra test here, though (per my comments) for a registry expert rather than for an LP developer.

review: Needs Fixing

3581fef... by Thiago F. Pappacena on 2020-04-02

Using zope permission system for bug attachment edit permission

d48828a... by Thiago F. Pappacena on 2020-04-02

Minor refactoring

77ce41b... by Thiago F. Pappacena on 2020-04-02

Removing unused exception class

ec057d2... by Thiago F. Pappacena on 2020-04-02

Adjusting some tests

d7d73cd... by Thiago F. Pappacena on 2020-04-02

Minor refactoring and comment

[Thiago F. Pappacena (pappacena)]

Changed the way we check permission to use Zope's machinery for both change and delete.

Also, changed the list of who can change/delete an attachment:
- Admin
- Registry expert
- The owner of the message with the attachment

51e3f3c... by Thiago F. Pappacena on 2020-04-14

Merge branch 'master' into bug-attachment-removal-restrictions

[Colin Watson (cjwatson)]

Looks good now, thanks! Just a few nits.

Also, please update the commit message of this MP, which is now out of date with the security policy you're applying.

review: Approve

0aa3b0b... by Thiago F. Pappacena on 2020-04-17

Formatting import and coding style changes

[Thiago F. Pappacena (pappacena)]

Thanks for the review, cjwatson. The requested changes were pushed. I'll land this in some minutes.

diff --git a/lib/lp/bugs/browser/bugattachment.py b/lib/lp/bugs/browser/bugattachment.py
index b239c45..e96e9fe 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."""
@@ -42,6 +42,7 @@ from lp.services.webapp import (
     GetitemNavigation,
     Navigation,
     )
+from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.escaping import structured
 from lp.services.webapp.interfaces import (
     ICanonicalUrlData,
@@ -135,7 +136,10 @@ class BugAttachmentEditView(LaunchpadFormView, BugAttachmentContentCheck):
             patch=attachment.type == BugAttachmentType.PATCH,
             contenttype=attachment.libraryfile.mimetype)
 
-    @action('Change', name='change')
+    def canEditAttachment(self, action):
+        return check_permission('launchpad.Edit', self.context)
+
+    @action('Change', name='change', condition=canEditAttachment)
     def change_action(self, action, data):
         if data['patch']:
             new_type = BugAttachmentType.PATCH
@@ -169,7 +173,7 @@ class BugAttachmentEditView(LaunchpadFormView, BugAttachmentContentCheck):
                 ILibraryFileAliasWithParent)
             lfa_with_parent.mimetype = data['contenttype']
 
-    @action('Delete Attachment', name='delete')
+    @action('Delete Attachment', name='delete', condition=canEditAttachment)
     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..34d9a03 100644
--- a/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
+++ b/lib/lp/bugs/browser/tests/test_bugattachment_edit_view.py
@@ -1,11 +1,12 @@
-# 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
 
 import transaction
-from zope.security.interfaces import Unauthorized
+from zope.component import getUtility
 
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.testing import (
     login_person,
     person_logged_in,
@@ -30,6 +31,11 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
     def setUp(self):
         super(TestBugAttachmentEditView, self).setUp()
         self.bug_owner = self.factory.makePerson()
+        self.registry_expert = self.factory.makePerson()
+        registry = getUtility(ILaunchpadCelebrities).registry_experts
+        with person_logged_in(registry.teamowner):
+            registry.addMember(self.registry_expert, registry.teamowner)
+
         login_person(self.bug_owner)
         self.bug = self.factory.makeBug(owner=self.bug_owner)
         self.bugattachment = self.factory.makeBugAttachment(
@@ -40,11 +46,8 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
         # we start the tests.
         transaction.commit()
 
-    def test_change_action_public_bug(self):
-        # Properties of attachments for public bugs can be
-        # changed by every user.
-        user = self.factory.makePerson()
-        login_person(user)
+    def test_user_changes_their_own_attachment(self):
+        login_person(self.bugattachment.message.owner)
         create_initialized_view(
             self.bugattachment, name='+edit', form=self.CHANGE_FORM_DATA)
         self.assertEqual('new description', self.bugattachment.title)
@@ -52,14 +55,9 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
         self.assertEqual(
             'application/whatever', self.bugattachment.libraryfile.mimetype)
 
-    def test_change_action_private_bug(self):
-        # Subscribers of a private bug can edit 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)
-        transaction.commit()
-        login_person(user)
+    def test_admin_changes_any_attachment(self):
+        admin = self.factory.makeAdministrator()
+        login_person(admin)
         create_initialized_view(
             self.bugattachment, name='+edit', form=self.CHANGE_FORM_DATA)
         self.assertEqual('new description', self.bugattachment.title)
@@ -67,44 +65,57 @@ class TestBugAttachmentEditView(TestCaseWithFactory):
         self.assertEqual(
             'application/whatever', self.bugattachment.libraryfile.mimetype)
 
-    def test_change_action_private_bug_unauthorized(self):
-        # Other users cannot edit attachments of private bugs.
-        user = self.factory.makePerson()
-        self.bug.setPrivate(True, self.bug_owner)
-        transaction.commit()
-        login_person(user)
-        self.assertRaises(
-            Unauthorized, create_initialized_view, self.bugattachment,
-            name='+edit', form=self.CHANGE_FORM_DATA)
+    def test_registry_expert_changes_any_attachment(self):
+        login_person(self.registry_expert)
+        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)
+        create_initialized_view(
+            self.bugattachment, name='+edit', form=self.CHANGE_FORM_DATA)
+        self.assertEqual('attachment description', self.bugattachment.title)
+        self.assertFalse(self.bugattachment.is_patch)
+        self.assertEqual('text/plain', self.bugattachment.libraryfile.mimetype)
 
     DELETE_FORM_DATA = {
         '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(1, self.bug.attachments.count())
+
+    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(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_registry_expert_can_delete_any_attachment(self):
+        login_person(self.registry_expert)
         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_their_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())
diff --git a/lib/lp/bugs/browser/tests/test_bugattachment_file_access.py b/lib/lp/bugs/browser/tests/test_bugattachment_file_access.py
index 2f68b38..8a967c8 100644
--- a/lib/lp/bugs/browser/tests/test_bugattachment_file_access.py
+++ b/lib/lp/bugs/browser/tests/test_bugattachment_file_access.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).
 
 __metaclass__ = type
@@ -53,7 +53,8 @@ class TestAccessToBugAttachmentFiles(TestCaseWithFactory):
         login_person(self.bug_owner)
         self.bug = self.factory.makeBug(owner=self.bug_owner)
         self.bugattachment = self.factory.makeBugAttachment(
-            bug=self.bug, filename='foo.txt', data=b'file content')
+            owner=self.bug_owner, bug=self.bug, filename='foo.txt',
+            data=b'file content')
 
     def test_traversal_to_lfa_of_bug_attachment(self):
         # Traversing to the URL provided by a ProxiedLibraryFileAlias of a
diff --git a/lib/lp/bugs/configure.zcml b/lib/lp/bugs/configure.zcml
index f410aa8..7edae44 100644
--- a/lib/lp/bugs/configure.zcml
+++ b/lib/lp/bugs/configure.zcml
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2019 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).
 -->
 
@@ -338,10 +338,11 @@
         class="lp.bugs.model.bugattachment.BugAttachment">
         <require
             permission="launchpad.View"
-            interface="lp.bugs.interfaces.bugattachment.IBugAttachment"/>
+            interface="lp.bugs.interfaces.bugattachment.IBugAttachmentView"/>
         <require
             permission="launchpad.Edit"
-            set_schema="lp.bugs.interfaces.bugattachment.IBugAttachment"/>
+            interface="lp.bugs.interfaces.bugattachment.IBugAttachmentEdit"
+            set_schema="lp.bugs.interfaces.bugattachment.IBugAttachmentView" />
     </class>
 
     <!-- BugAttachmentSet -->
diff --git a/lib/lp/bugs/doc/bugattachments.txt b/lib/lp/bugs/doc/bugattachments.txt
index 0bac3b7..977c944 100644
--- a/lib/lp/bugs/doc/bugattachments.txt
+++ b/lib/lp/bugs/doc/bugattachments.txt
@@ -315,29 +315,25 @@ anonymous can read the attachment's attributes, but they can't set them:
     >>> import transaction
     >>> transaction.abort()
 
-Both Sample Person and Foo Bar can access and set the attributes, though:
-
-    >>> login('[e-mail]')
-    >>> attachment.title
-    u'this fixes the bug'
-    >>> attachment.title = 'Better Title'
+Attachment owner can access and set the attributes, though:
 
     >>> login('[e-mail]')
     >>> attachment.title
-    u'Better Title'
+    u'this fixes the bug'
     >>> attachment.title = 'Even Better Title'
 
 Now let's make the bug private instead:
 
     >>> bug_four.setPrivate(True, getUtility(ILaunchBag).user)
     True
+    >>> logout()
 
 Foo Bar isn't explicitly subscribed to the bug, BUT they are an admin, so
-they can access and set the attachment's attributes:
+they can access the attachment's attributes:
 
+    >>> login('[e-mail]')
     >>> attachment.title
     u'Even Better Title'
-    >>> attachment.title = 'Even Better Title'
 
 Mr. No Privs, who is not subscribed to bug_four, cannot access or set the
 attachments attributes:
@@ -365,13 +361,11 @@ Of course, anonymous is also not allowed to access or set them:
     ...
     Unauthorized: (..., 'title',...
 
-Sample Person is explicitly subscribed, so they can both access and set
-the attributes:
+Sample Person is explicitly subscribed, so they can access the attributes:
 
     >>> login('[e-mail]')
     >>> attachment.title
     u'Even Better Title'
-    >>> attachment.title = 'Better Title'
 
 
 Let's make the bug public again:
@@ -423,7 +417,7 @@ There are no patches attached to any bugs:
 Let's make our attachment a patch and search again:
 
     >>> from lp.services.database.sqlbase import flush_database_updates
-    >>> login('[e-mail]')
+    >>> login('[e-mail]')
     >>> attachment.type = BugAttachmentType.PATCH
     >>> flush_database_updates()
     >>> attachmenttype = BugAttachmentType.PATCH
diff --git a/lib/lp/bugs/interfaces/bugattachment.py b/lib/lp/bugs/interfaces/bugattachment.py
index bbfa055..7616fd6 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."""
@@ -64,36 +64,8 @@ class BugAttachmentType(DBEnumeratedType):
         """)
 
 
-class IBugAttachment(IHasBug):
-    """A file attachment to an IBug.
-
-    Launchpadlib example of accessing content of an attachment::
-
-        for attachment in bug.attachments:
-            buffer = attachment.data.open()
-            for line in buffer:
-                print line
-            buffer.close()
-
-    Launchpadlib example of accessing metadata about an attachment::
-
-        attachment = bug.attachments[0]
-        print "title:", attachment.title
-        print "ispatch:", attachment.type
-
-    For information about the file-like object returned by
-    attachment.data.open() see lazr.restfulclient's documentation of the
-    HostedFile object.
-
-    Details about the message associated with an attachment can be found on
-    the "message" attribute::
-
-        message = attachment.message
-        print "subject:", message.subject.encode('utf-8')
-        print "owner:", message.owner.display_name.encode('utf-8')
-        print "created:", message.date_created
-    """
-    export_as_webservice_entry()
+class IBugAttachmentView(IHasBug):
+    """Interface for BugAttachment that requires launchpad.View permission."""
 
     id = Int(title=_('ID'), required=True, readonly=True)
     bug = exported(
@@ -126,10 +98,16 @@ class IBugAttachment(IHasBug):
         description=_('Is this attachment a patch?'),
         readonly=True)
 
-    @call_with(user=REQUEST_USER)
-    @export_write_operation()
-    def removeFromBug(user):
-        """Remove the attachment from the bug."""
+    def getFileByName(filename):
+        """Return the `ILibraryFileAlias` for the given file name.
+
+        NotFoundError is raised if the given filename does not match
+        libraryfile.filename.
+        """
+
+
+class IBugAttachmentEdit(Interface):
+    """Interface for BugAttachment that requires launchpad.Edit permission."""
 
     def destroySelf():
         """Delete this record.
@@ -137,12 +115,42 @@ class IBugAttachment(IHasBug):
         The library file content for this attachment is set to None.
         """
 
-    def getFileByName(filename):
-        """Return the `ILibraryFileAlias for the given file name.
+    @call_with(user=REQUEST_USER)
+    @export_write_operation()
+    def removeFromBug(user):
+        """Remove the attachment from the bug."""
 
-        NotFoundError is raised if the given filename does not match
-        libraryfile.filename.
-        """
+
+class IBugAttachment(IBugAttachmentView, IBugAttachmentEdit):
+    """A file attachment to an IBug.
+
+        Launchpadlib example of accessing content of an attachment::
+
+            for attachment in bug.attachments:
+                buffer = attachment.data.open()
+                for line in buffer:
+                    print line
+                buffer.close()
+
+        Launchpadlib example of accessing metadata about an attachment::
+
+            attachment = bug.attachments[0]
+            print "title:", attachment.title
+            print "ispatch:", attachment.type
+
+        For information about the file-like object returned by
+        attachment.data.open() see lazr.restfulclient's documentation of the
+        HostedFile object.
+
+        Details about the message associated with an attachment can be found on
+        the "message" attribute::
+
+            message = attachment.message
+            print "subject:", message.subject.encode('utf-8')
+            print "owner:", message.owner.display_name.encode('utf-8')
+            print "created:", message.date_created
+    """
+    export_as_webservice_entry()
 
 
 # Need to do this here because of circular imports.
diff --git a/lib/lp/bugs/model/bugattachment.py b/lib/lp/bugs/model/bugattachment.py
index d514e31..e621b52 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
diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py
index 4e22be0..573f85a 100644
--- a/lib/lp/bugs/security.py
+++ b/lib/lp/bugs/security.py
@@ -211,7 +211,7 @@ class ViewBugAttachment(DelegatedAuthorization):
             bugattachment, bugattachment.bug)
 
 
-class EditBugAttachment(DelegatedAuthorization):
+class EditBugAttachment(AuthorizationBase):
     """Security adapter for editing a bug attachment.
 
     If the user is authorized to view the bug, they're allowed to edit the
@@ -220,9 +220,15 @@ class EditBugAttachment(DelegatedAuthorization):
     permission = 'launchpad.Edit'
     usedfor = IBugAttachment
 
-    def __init__(self, bugattachment):
-        super(EditBugAttachment, self).__init__(
-            bugattachment, bugattachment.bug)
+    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))
+
+    def checkUnauthenticated(self):
+        return False
 
 
 class ViewBugActivity(DelegatedAuthorization):
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..3a45f61 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,24 @@ attachment.
     >>> user_browser.getControl('Delete Attachment') is not None
     True
 
+But this delete option should not be shown for other users.
+
+    >>> from lp.testing.pages import setupBrowserForUser
+    >>> login('[e-mail]')
+    >>> another_user = factory.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:
+    ...     pass
+
 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.