44595 - interface for administrator to delete attachments

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Jason Eager]

I don't know if "GOD" users have this capability, but it would be nice
if users could at least edit and delete their attachments. That way, when
an attachment mistake is made (like I made with bug 44437), I don't
have to tell people to ignore the bad attachments.

Comment 1

[Chris Yeh]

submitters of attachments should be able to revoke them. although it is useful 
to have a history of everything...

Comment 2

[Jesse Ruderman]

Is this about attachments or comments?

Comment 3

[Andreas Franke (gone)]

According to the terminology used here, this bug is about attachments. 
Bug 540 is about comments.

Comment 4

[Andreas Franke (gone)]

See bug 75176 (marking attachments as "obsolete") for a similiar approach that
avoids the "would be nice to have a history of everything" problem. 
edit/delete could still be implemented later.

Comment 5

[phoenixreads]

I was able to delete an attachment manually out of an account mail file and it
simply disappeared - the message remained; this is what I wanted.  Therefore I
don't see the difficulty in adding a delete attachment option.  This will
prevent data files from growing to unmanagable sizes and solve
[e-mail] initial problem.

Comment 6

[Dave Miller [:justdave]]

the patch checked in for bug 84338 added the ability to mark an attachment as
obsolete, and also to edit attributes of the attachment (although  not to
actually delete or replace the file itself).  Does that adequately solve this,
or do we need to do more here still?

Comment 7

[Andreas Franke (gone)]

-> Bugzilla product, Changing Bugs component, reassigning.

Comment 8

[R.K.Aa.]

is this a dup of bug 2920?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

No, bug 2920 is about not saving attachments to emails in Mozilla Mail/News. 
This bug is about editing/deleting attachments to Bugzilla bugs.

Comment 10

[Brian 'netdragon' Bober]

When you remove or change an attachment, the "evidence" shouldn't be removed
from the system. It should still hold a copy of all deleted attachments and also
a record in the bug that it was deleted. As in - hide the attachment, but keep
it on the system. 

- Only the person who attached it, and ones with the proper bits (i.e. only Dawn
and Asa maybe a couple others) would be able to look at it. This would stop
people from attaching things like porn for a few days and deleting it before
they get in trouble. 

- A record of the hiding should be kept in the Bug's Activity list.

- Misuse of this will cause people's comments attachments to refer to
attachments that were hidden. Therefore, this shouldn't be used to remove
established attachments. How can we prevent this? An attachment that is hidden
should be still accessable from the Bug's Activity List. Only a few people
should be able to fully hide an attachment for reasons of racial prejudice,
obsene images, etc (Asa, Endico).

- Attachments should be marked as hidden by a field. There would be three
settings: Active, hidden, blocked.

- I don't think that allowing you to edit an attachment would be a good idea.
This might get abused by people who keep editing their patch's attachment
instead of submitting new patches. Also, it would have to reset all the settings
on the attachment, would require the old copies to be stored in the database in
a new field, and would require a lot of extra work that isn't really necessary. 

- If a bug has hidden attachments, it should provied a link to see all hidden
attachments.

Comment 11

[Daniel Steinberger]

*** Bug 121728 has been marked as a duplicate of this bug. ***

Comment 12

[Alex Vincent [:WeirdAl]]

Given my more interesting difficulties with Bugzilla, I *need* this to avoid
gallons and gallons of wasted attachments (bug 180504 is a perfect example).

Deleting attachments is probably a bad idea.

Re comment 10:  well, could we use CVS and diff to tell us if they're abusing
it?  Say, if they're changing more than x number of lines.  For trivial changes
that involve only one line, or a total of only five lines mutated from the
original version, I would think we would be fine.

Sure would be nice to tie our Bugzilla's attachment management system to our
CVS.  Kill two birds with one stone, and if diff reports changes too big for
Bugzilla to like, it simply doesn't check it in to the CVS, but requires the
user to add comments and file as a new attachment.

Comment 13

[Jon Wilmoth]

I recently received a request from one of our users to be able to delete an 
attachment made in error.  Is it enough to simply expose the "Mark as 
obsolete" function on the main bug detail page instead of deleting the 
attachment?  This would prevent the linkify errors discussed earlier and 
preserve the history for the security focus folks.  As for editing an 
attachment I think the mark as obsolete with a replacement attachment 
alleviates this need.

Comment 14

[Brian 'netdragon' Bober]

I mark misplaced attachments as obsolete. I don't think deleting would be a good
idea since it could be done accidentally.

Comment 15

[Brian 'netdragon' Bober]

What would be good though is to mark it as invalid so that it wouldn't appear in
the list, but maybe have a link in the bottom of the table to edit ones marked
invalid in case an attachment was marked invalid by accident.

Comment 16

[Dave Miller [:justdave]]

*** Bug 228477 has been marked as a duplicate of this bug. ***

Comment 17

[Jerry Baker]

In my case the content NEEDS to be removed from Bugzilla. It is not sufficient
to "hide" it. If you do not trust users with these permissions, perhaps the
problem is being to liberal with your permissions and not with the ability to
delete per se.

Comment 18

[Andrew Chick]

This issue has been dead for a while - but I'll add to it. I have a user who has
attached the wrong document to a bug (company confidential material) and I want
to remove it.  I can understand why 'normal' users should probably not delete
attachments - but why not bugzilla administrators?

Comment 19

[Onno Garms]

One good reason to want to delete attachments is comment #17.
Another one is that on open bugzilla systems people might upload illegal stuff
which the administrator is legally obliged to remove.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

It shouldn't be possible to edit attachments, and it shouldn't be possible for
general users to delete attachments, but it makes sense to make it easier for
administrators to do it by providing an interface for the task rather than
making them submit raw SQL queries.  Retargeting bug to that request.

Comment 21

[Frédéric Buclin]

I will add a 'allow_attachment_deletion' parameter.

Comment 22

[Frédéric Buclin]

Attachment: patch, v1

The content of attachments can be removed. Only users with admin privs can do that.

Comment 23

[Teemu Mannermaa (:wicked)]

Comment on attachment 214809 [details] [diff] [review]
patch, v1

Please, fix the comment in Bugzilla/Attachment.pm datasize sub, an attachment with zero size on the DB is not necessarily stored locally (can also be deleteted). Luckily the code deals with this fact but the code will try to open a local file for all deleted attachment.

>Index: attachment.cgi
>===================================================================
>+        $dbh->bz_lock_tables('attachments WRITE', 'attach_data WRITE', 'flags WRITE');
>+        $dbh->do('DELETE FROM attach_data WHERE id = ?', undef, $attach_id);
>+        $dbh->do('UPDATE attachments SET mimetype = ?, ispatch = ?, isurl = ?
>+                  WHERE attach_id = ?', undef, ('text/plain', 0, 0, $attach_id));
>+        $dbh->do('DELETE FROM flags WHERE attach_id = ?', undef, $attach_id);
>+        $dbh->bz_unlock_tables;

If this attachment is stored as a file, you need to delete it. Currently this says it deleted a local file attachment but the attachment remains undeleted.

This code will also happily try to delete an already deleted attachment if the token generation block has been run before attachment is deleted.

>Index: template/en/default/attachment/list.html.tmpl
>===================================================================

Nit: Why not add the Delete to actions column?

>Index: template/en/default/attachment/confirm-delete.html.tmpl
>===================================================================
>+  # a: object; attachment the user wants to delete.

Nit: s/object/attachment object/ to make it clear what kind of object this is.

>+[% title = BLOCK %]
>+  Delete Attachment [% a.id FILTER html %] of
>+  [%+ "$terms.Bug " _ a.bug_id FILTER bug_link(a.bug_id) FILTER none %]
>+[% END %]

Nope, this won't work because title can't contain HTML. This makes page title to show raw HTML.

>+    <td valign="top">Creation Date:</td>
>+    <td valign="top">[% a.attached FILTER html %]</td>

Use FILTER time to get correct formatting for the time.

>+[% IF Param("allow_attachment_deletion") %]

This test is useless here because this template won't even be shown if param is disabled.

>+        The content of this attachment will be deleted in a <blink><b>irreversible</b></blink> way.

Yuck! Blink is not nice attribute. It's not even standard, which makes this page fail validation. Just don't use it.

>Index: template/en/default/attachment/delete_reason.txt.tmpl
>===================================================================

Nit: INTERFACE documentation missing.

>+The token used to delete this attachment was generated at [% date %].

Need to format date here too.

Too bad the reason can't be shown in place of the deleted attachment.

>Index: template/en/default/global/user-error.html.tmpl
>===================================================================
>+    Attachment deletion has been disabled on this installation.

Nit: Hmm, maybe s/has been/is/

Comment 24

[Frédéric Buclin]

(In reply to comment #23)
> This code will also happily try to delete an already deleted attachment if the
> token generation block has been run before attachment is deleted.

I know. But unless we have a perf issue here, I think it's fine to have this behavior about deleted attachments. We can open a separate bug if we want to add a 'deleted' attribute.


> Nit: Why not add the Delete to actions column?

Because deleting attachments is dangerous enough (read: irreversible + dataloss) to not make this feature too visible. I really want the administrator to do an additional step before being able to delete attachments, so that he is "sure" of what he is doing.


> Nope, this won't work because title can't contain HTML. This makes page title
> to show raw HTML.

I opened bug 330555 about this problem already. For obvious reasons, I won't comment here. All I can say here is that I prefer to have it escaped too many times than not escaped at all. :) The problem you are mentionning will be fixed on a more general way in bug 330555. Some other templates are affected by this problem already.

Comment 25

[Frédéric Buclin]

Attachment: patch, v2

I finally fixed the problem about looking everytime for a local file when the attachment is deleted. If $self->{datasize} == 0, attachment->datasize will return before looking for the local file again, because $self->{datasize} can be 0, but exists in all cases, and so we check for the existence of this attribute.

Comment 26

[Teemu Mannermaa (:wicked)]

Comment on attachment 216459 [details] [diff] [review]
patch, v2

With this patch and a deleted file, attachment.cgi spews two uninitialized value errors from lines 509 and 510 when the attachment is viewed. This is because INNER JOIN to attach_data failes and so no data is returned even from attachment table. It seems this is only place affected by this as most other places either don't use INNER JOIN or detect this condition otherwise before using uninitialized variables.

I'm not sure why line 509 is there, it could be later. And line 510 needs protection from undefined $thedata. Or you could simply not delete the whole entry from attach_data but only remove thedata contents. This is how bigfile upload gets away with this. Either way, I'm sure this can be fixed in other bug or before commit.

>Index: attachment.cgi
>===================================================================
>+        # If the attachment is stored locally, remove it.
>+        if (-e $attachment->_get_local_filename) {
>+            unlink $attachment->_get_local_filename;
>+        }

Nit: Routines that start with _ are meant to be internal only so you really shouldn't call that here. Looks like there's need for this elsewhere too so this can be fixed in a new bug. I filed bug 334152 for this.

Comment 27

[Dave Miller [:justdave]]

Comment on attachment 216459 [details] [diff] [review]
patch, v2

>Index: attachment.cgi
>+    # We don't want to let a malicious URL accidentally delete an attachment.
>+    my $token = trim($cgi->param('token'));

Is trim() undef safe these days?  The arguments to a function are processed in list format unless the function provides a prototype that says otherwise.  $cgi->param() will return a list in that context.  If no token was passed, that means you're passing an empty list to trim().  Actually, even if it was returning a scalar, it would still be undef if it wasn't passed. I guess if trim() is undef-safe these days this would be okay, but it used to throw a warning to apache when you tried this (and we don't want to produce garbage in the error_log)

>+            ThrowUserError('token_inexistent');

The dictionary tells me that "inexistent" is actually a word, but I've never heard it in my life in day-to-day usage.  Normally one says "nonexistent"

The rest of this looks pretty good.  I really like the idea of generating a token on the confirmation screen.  Nice touch.

Comment 28

[Frédéric Buclin]

justdave, yes, trim() knows what to do with undefined values. ;)


Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.108; previous revision: 1.107
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/Config/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/confirm-delete.html.tmpl,v
done
Checking in template/en/default/attachment/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/delete_reason.txt.tmpl,v
done
Checking in template/en/default/attachment/delete_reason.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/delete_reason.txt.tmpl,v  <--  delete_reason.txt.tmpl
initial revision: 1.1
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.34; previous revision: 1.33
done
Checking in template/en/default/attachment/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.28; previous revision: 1.27
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.164; previous revision: 1.163
done

Comment 29

[victory <[e-mail]>]

Attachment: docs patch for tip

Comment 30

[Colin Ogilvie [:cso]]

Comment on attachment 222830 [details] [diff] [review]
docs patch for tip

>Index: docs/xml/using.xml
>+      <para>
>+        If <quote>allow_attachment_deletion</quote> parameter is on and
>+        you have admin privilege, you can delete attachments on the
>+        <quote>Details</quote> page of the attachments.
>+      </para>

If *the* <quote>allow_attch....
and you have *the* admin privilege

Comment 31

[victory <[e-mail]>]

Attachment: docs patch for tip v2

Comment 32

[Colin Ogilvie [:cso]]

Comment on attachment 223545 [details] [diff] [review]
docs patch for tip v2

>Index: docs/xml/using.xml
>+
>+      <para>
>+        If the <quote>allow_attachment_deletion</quote> parameter is
>+        on and you have the admin privilege, you can delete attachments
>+        on the <quote>Details</quote> page of the attachments.
>+      </para>

This is probably a nit (the patch is fine) but there is just something I don't like about the sentence :(

How about "you can delete an attachment from the <quote>Details</quote> page for the attachment."

Comment 33

[victory <[e-mail]>]

Attachment: docs patch for tip v3 ;'(

Comment 34

[Frédéric Buclin]

*** Bug 339743 has been marked as a duplicate of this bug. ***

Comment 35

[Vlad Dascalu]

Comment on attachment 223547 [details] [diff] [review]
docs patch for tip v3 ;'(

> you can delete attachments from the <quote>Details</quote> page of the attachments.

Maybe "you can delete attachments from their <quote>Details</quote> page."? Just asking, I don't know if it sounds better or not.

Comment 36

[Vlad Dascalu]

Attachment: 'a irreversible way' -> 'an irreversible way'

Follow-up spelling patch for bug 44595: replace 'a irreversible way' with 'an irreversible way'; patch by Vlad Dascalu <[e-mail]>.

Checking in template/en/default/attachment/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.2; previous revision: 1.1
done

Comment 37

[Vlad Dascalu]

Comment on attachment 223547 [details] [diff] [review]
docs patch for tip v3 ;'(

In addition to comment #35, after trying this feature, I must say that the documentation is not complete:

-> we should specify that the deletion will prompt for a reason
-> we should specify that only the content of the attachment will be deleted; the attachment timestamp, author etc (its metadata) will remain available in the bug.
-> [nit] we could probably specify that the allow_attachment_deletion parameter is located in the Attachments param section.
-> [nit] we should probably specify how to view the deletion log.

Comment 38

[Vlad Dascalu]

>> -> [nit] we should probably specify how to view the deletion log.

I mean that we should specify the fact that a "delete log" will be available under the form of a general comment in that bug.

Comment 39

[Max Kanat-Alexander]

Added to relnotes in bug 349423. Please let me know if I missed any critical information about this bug in the release notes.

Comment 40

[victory <[e-mail]>]

To native English speakers:
i don't update docs patch for this bug.
feel free to take this docs.

Comment 41

[Frédéric Buclin]

Attachment: doc patch, v. N+1

Comment 42

[Colin Ogilvie [:cso]]

Comment on attachment 254750 [details] [diff] [review]
doc patch, v. N+1

r=me; looks OK to me

Comment 43

[Frédéric Buclin]

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.63; previous revision: 1.62
done

Comment 44

[Frédéric Buclin]

Attachment: selenium test, v1

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
modified t/test_private_attachments.t
Committed revision 174.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/3.6/
modified t/test_private_attachments.t
Committed revision 145.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/3.4/
modified t/test_private_attachments.t
Committed revision 118.