interface for administrator to delete attachments

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Attachments & Requests
--
enhancement
RESOLVED FIXED
17 years ago
7 years ago

People

(Reporter: Jason Eager, Assigned: Frédéric Buclin)

Tracking

({selenium})

unspecified
Bugzilla 3.0
selenium
Dependency tree / graph
Bug Flags:
approval +
documentation +
testcase +

Details

(Whiteboard: [content:attachments])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

17 years ago
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

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

Comment 2

17 years ago
Is this about attachments or comments?

Comment 3

17 years ago
According to the terminology used here, this bug is about attachments. 
Bug 540 is about comments.
Target Milestone: --- → Future

Comment 4

17 years ago
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

17 years ago
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.
Depends on: 84338
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

16 years ago
-> Bugzilla product, Changing Bugs component, reassigning.
Assignee: tara → myk
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Whiteboard: [content:attachments]
Version: other → unspecified

Comment 8

16 years ago
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.




Comment 11

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

Updated

15 years ago
Component: Creating/Changing Bugs → attachment and request management
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

14 years ago
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.

Updated

14 years ago
Summary: [RFE] Need ability to edit/delete attachments → Need ability to edit/delete attachments
*** Bug 228477 has been marked as a duplicate of this bug. ***

Comment 17

14 years ago
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

12 years ago
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

12 years ago
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.
Summary: Need ability to edit/delete attachments → interface for administrator to delete attachments
(Assignee)

Updated

12 years ago
Assignee: myk → LpSolit
Priority: P3 → --
QA Contact: mattyt-bugzilla → default-qa
Target Milestone: Future → Bugzilla 2.24
(Assignee)

Comment 21

12 years ago
I will add a 'allow_attachment_deletion' parameter.
Status: NEW → ASSIGNED
(Assignee)

Comment 22

12 years ago
Created attachment 214809 [details] [diff] [review]
patch, v1

The content of attachments can be removed. Only users with admin privs can do that.
Attachment #214809 - Flags: review?(myk)
(Assignee)

Updated

12 years ago
Attachment #214809 - Flags: review?(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/
Attachment #214809 - Flags: review?(wicked+bz)
Attachment #214809 - Flags: review?(myk)
Attachment #214809 - Flags: review-
(Assignee)

Comment 24

12 years ago
(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.
(Assignee)

Comment 25

12 years ago
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.
Attachment #214809 - Attachment is obsolete: true
Attachment #216459 - Flags: review?(wicked+bz)
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.
Attachment #216459 - Flags: review?(wicked+bz) → review+
Flags: approval?
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.
Attachment #216459 - Flags: review+
Flags: approval? → approval+
(Assignee)

Comment 28

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: relnote
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 334387
Created attachment 222830 [details] [diff] [review]
docs patch for tip
Attachment #222830 - Flags: review?(documentation)
Attachment #222830 - Flags: review?(documentation) → review?(bmo)

Comment 30

11 years ago
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
Attachment #222830 - Flags: review?(bmo) → review-
Created attachment 223545 [details] [diff] [review]
docs patch for tip v2
Attachment #222830 - Attachment is obsolete: true
Attachment #223545 - Flags: review?(colin.ogilvie)

Comment 32

11 years ago
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."
Attachment #223545 - Flags: review?(colin.ogilvie) → review-
Created attachment 223547 [details] [diff] [review]
docs patch for tip v3 ;'(
Attachment #223545 - Attachment is obsolete: true
Attachment #223547 - Flags: review?(colin.ogilvie)
(Assignee)

Comment 34

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

Comment 35

11 years ago
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

11 years ago
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

11 years ago
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.
Attachment #223547 - Flags: review?(colin.ogilvie) → review-

Comment 38

11 years ago
>> -> [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

11 years ago
Added to relnotes in bug 349423. Please let me know if I missed any critical information about this bug in the release notes.
Keywords: relnote
(Assignee)

Updated

11 years ago
Flags: testcase?
(Assignee)

Updated

11 years ago
Flags: documentation?
To native English speakers:
i don't update docs patch for this bug.
feel free to take this docs.
Attachment #223547 - Attachment is obsolete: true
(Assignee)

Comment 41

11 years ago
Created attachment 254750 [details] [diff] [review]
doc patch, v. N+1
Attachment #224852 - Attachment is obsolete: true
Attachment #254750 - Flags: review?(colin.ogilvie)

Comment 42

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

r=me; looks OK to me
Attachment #254750 - Flags: review?(colin.ogilvie) → review+
(Assignee)

Comment 43

11 years ago
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
Flags: documentation? → documentation+
(Assignee)

Updated

9 years ago
Blocks: 481952
(Assignee)

Comment 44

7 years ago
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.
Attachment #510047 - Flags: review+
(Assignee)

Updated

7 years ago
Flags: testcase? → testcase+
Keywords: selenium
You need to log in before you can comment on or make changes to this bug.