Last Comment Bug 44595 - interface for administrator to delete attachments
: interface for administrator to delete attachments
Status: RESOLVED FIXED
[content:attachments]
: selenium
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: unspecified
: All All
: -- enhancement with 3 votes (vote)
: Bugzilla 3.0
Assigned To: Frédéric Buclin
: default-qa
Mentors:
: 228477 339743 (view as bug list)
Depends on: 84338
Blocks: 334387 481952
  Show dependency treegraph
 
Reported: 2000-07-05 15:18 PDT by Jason Eager
Modified: 2011-02-05 13:28 PST (History)
18 users (show)
justdave: approval+
LpSolit: documentation+
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (13.45 KB, patch)
2006-03-11 20:26 PST, Frédéric Buclin
wicked: review-
Details | Diff | Splinter Review
patch, v2 (14.99 KB, patch)
2006-03-27 14:16 PST, Frédéric Buclin
wicked: review+
justdave: review+
Details | Diff | Splinter Review
docs patch for tip (761 bytes, patch)
2006-05-22 01:38 PDT, victory <never@receive.bug.mails.i.hate.spammer>
mozilla+bmo: review-
Details | Diff | Splinter Review
docs patch for tip v2 (769 bytes, patch)
2006-05-27 08:00 PDT, victory <never@receive.bug.mails.i.hate.spammer>
mozilla+bmo: review-
Details | Diff | Splinter Review
docs patch for tip v3 ;'( (771 bytes, patch)
2006-05-27 08:14 PDT, victory <never@receive.bug.mails.i.hate.spammer>
goobix: review-
Details | Diff | Splinter Review
'a irreversible way' -> 'an irreversible way' (785 bytes, patch)
2006-06-08 05:25 PDT, Vlad Dascalu
no flags Details | Diff | Splinter Review
doc patch, v. N+1 (1.64 KB, patch)
2007-02-11 13:00 PST, Frédéric Buclin
mozilla+bmo: review+
Details | Diff | Splinter Review
selenium test, v1 (2.80 KB, patch)
2011-02-05 13:28 PST, Frédéric Buclin
LpSolit: review+
Details | Diff | Splinter Review

Description Jason Eager 2000-07-05 15:18:51 PDT
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 2000-08-26 00:37:57 PDT
submitters of attachments should be able to revoke them. although it is useful 
to have a history of everything...
Comment 2 Jesse Ruderman 2000-10-10 00:58:33 PDT
Is this about attachments or comments?
Comment 3 Andreas Franke (gone) 2001-02-19 21:34:46 PST
According to the terminology used here, this bug is about attachments. 
Bug 540 is about comments.
Comment 4 Andreas Franke (gone) 2001-04-08 14:37:31 PDT
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 2001-05-18 19:24:54 PDT
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.
Comment 6 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-08-31 11:42:22 PDT
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) 2001-09-01 14:46:20 PDT
-> Bugzilla product, Changing Bugs component, reassigning.
Comment 8 R.K.Aa. 2002-01-02 09:54:31 PST
is this a dup of bug 2920?
Comment 9 Myk Melez [:myk] [@mykmelez] 2002-01-02 10:05:17 PST
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 2002-02-27 13:41:49 PST
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 2002-06-27 05:11:56 PDT
*** Bug 121728 has been marked as a duplicate of this bug. ***
Comment 12 Alex Vincent [:WeirdAl] 2002-11-16 15:37:53 PST
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 2003-07-01 09:33:16 PDT
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 2003-07-01 15:33:02 PDT
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 2003-07-01 15:34:21 PDT
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] (justdave@bugzilla.org) 2003-12-14 12:14:21 PST
*** Bug 228477 has been marked as a duplicate of this bug. ***
Comment 17 Jerry Baker 2003-12-14 13:46:17 PST
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 2005-05-26 03:15:15 PDT
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 2005-07-21 05:47:26 PDT
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] 2005-07-23 01:16:10 PDT
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 2005-11-29 13:59:32 PST
I will add a 'allow_attachment_deletion' parameter.
Comment 22 Frédéric Buclin 2006-03-11 20:26:32 PST
Created attachment 214809 [details] [diff] [review]
patch, v1

The content of attachments can be removed. Only users with admin privs can do that.
Comment 23 Teemu Mannermaa (:wicked) 2006-03-26 17:44:07 PST
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 2006-03-27 13:20:32 PST
(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 2006-03-27 14:16:05 PST
Created attachment 216459 [details] [diff] [review]
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) 2006-04-15 12:38:31 PDT
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] (justdave@bugzilla.org) 2006-04-17 12:38:36 PDT
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 2006-04-17 13:20:50 PDT
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 <never@receive.bug.mails.i.hate.spammer> 2006-05-22 01:38:58 PDT
Created attachment 222830 [details] [diff] [review]
docs patch for tip
Comment 30 Colin Ogilvie [:cso] 2006-05-27 07:47:51 PDT
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 <never@receive.bug.mails.i.hate.spammer> 2006-05-27 08:00:16 PDT
Created attachment 223545 [details] [diff] [review]
docs patch for tip v2
Comment 32 Colin Ogilvie [:cso] 2006-05-27 08:09:45 PDT
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 <never@receive.bug.mails.i.hate.spammer> 2006-05-27 08:14:20 PDT
Created attachment 223547 [details] [diff] [review]
docs patch for tip v3 ;'(
Comment 34 Frédéric Buclin 2006-05-30 15:54:55 PDT
*** Bug 339743 has been marked as a duplicate of this bug. ***
Comment 35 Vlad Dascalu 2006-06-02 11:36:32 PDT
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 2006-06-08 05:25:14 PDT
Created attachment 224852 [details] [diff] [review]
'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 <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 37 Vlad Dascalu 2006-06-08 05:30:09 PDT
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 2006-06-08 05:34:47 PDT
>> -> [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 2006-09-07 17:54:14 PDT
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 <never@receive.bug.mails.i.hate.spammer> 2006-12-09 04:11:35 PST
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 2007-02-11 13:00:41 PST
Created attachment 254750 [details] [diff] [review]
doc patch, v. N+1
Comment 42 Colin Ogilvie [:cso] 2007-02-11 13:11:39 PST
Comment on attachment 254750 [details] [diff] [review]
doc patch, v. N+1

r=me; looks OK to me
Comment 43 Frédéric Buclin 2007-02-11 13:15:14 PST
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 2011-02-05 13:28:09 PST
Created attachment 510047 [details] [diff] [review]
selenium test, v1

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.

Note You need to log in before you can comment on or make changes to this bug.