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.

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

Is this about attachments or comments?

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

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.

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 matty@chariot.net.au initial problem.

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?

-> Bugzilla product, Changing Bugs component, reassigning.

is this a dup of bug 2920?

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

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.

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

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.

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.

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

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.

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

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.

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?

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.

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.

I will add a 'allow_attachment_deletion' parameter.

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

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/

(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.

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 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 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.

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 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 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."

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

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.

Follow-up spelling patch for bug 44595: replace 'a irreversible way' with 'an irreversible way'; patch by Vlad Dascalu <vladd@bugzilla.org>. 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 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.

>> -> [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.

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

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

Comment on attachment 254750 [details] [diff] [review] doc patch, v. N+1 r=me; looks OK to me

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

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/ modified t/test_private_attachments.t Committed revision 174. Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/3.6/ modified t/test_private_attachments.t Committed revision 145. Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/3.4/ modified t/test_private_attachments.t Committed revision 118.