Bug 274724: The 'Edit Attachment' link is now available even if a user does not have 'editbugs' privs - Patch by Frédéric Buclin <LpSolit@gmail.com> r=myk a=myk

3 files changed, 53 insertions, 76 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 1a1246d86..4d223d633 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -64,34 +64,28 @@ sub new {
 sub query
 {
   # Retrieves and returns an array of attachment records for a given bug. 
-  # This data should be given to attachment/list.atml in an
+  # This data should be given to attachment/list.html.tmpl in an
   # "attachments" variable.
   my ($bugid) = @_;
 
   my $dbh = Bugzilla->dbh;
 
-  my $in_editbugs = UserInGroup("editbugs");
-  &::SendSQL("SELECT product_id
-           FROM bugs 
-           WHERE bug_id = $bugid");
-  my $productid = &::FetchOneColumn();
-  my $caneditproduct = &::CanEditProductId($productid);
-
   # Retrieve a list of attachments for this bug and write them into an array
   # of hashes in which each hash represents a single attachment.
-  &::SendSQL("SELECT attach_id, " .
-             $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') .
-             ", mimetype, description, ispatch, isobsolete, isprivate, 
-              submitter_id, LENGTH(thedata)
-              FROM attachments WHERE bug_id = $bugid ORDER BY attach_id
-            ");
+  my $list = $dbh->selectall_arrayref("SELECT attach_id, " .
+                                      $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') .
+                                      ", mimetype, description, ispatch,
+                                      isobsolete, isprivate, LENGTH(thedata)
+                                      FROM attachments
+                                      WHERE bug_id = ? ORDER BY attach_id",
+                                      undef, $bugid);
+
   my @attachments = ();
-  while (&::MoreSQLData()) {
+  foreach my $row (@$list) {
     my %a;
-    my $submitter_id;
-    ($a{'attachid'}, $a{'date'}, $a{'contenttype'}, $a{'description'},
-     $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'}, $submitter_id, 
-     $a{'datasize'}) = &::FetchSQLData();
+    ($a{'attachid'}, $a{'date'}, $a{'contenttype'},
+     $a{'description'}, $a{'ispatch'}, $a{'isobsolete'},
+     $a{'isprivate'}, $a{'datasize'}) = @$row;
 
     # Retrieve a list of flags for this attachment.
     $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'},
@@ -107,16 +101,9 @@ sub query
             close(AH);
         }
     }
-    
-    # We will display the edit link if the user can edit the attachment;
-    # ie the are the submitter, or they have canedit.
-    # Also show the link if the user is not logged in - in that cae,
-    # They'll be prompted later
-    $a{'canedit'} = ($::userid == 0 || (($submitter_id == $::userid ||
-                     $in_editbugs) && $caneditproduct));
     push @attachments, \%a;
   }
-  
+
   return \@attachments;  
 }
 
diff --git a/attachment.cgi b/attachment.cgi
index 2b119e7ff..8b9bdaafd 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -197,13 +197,6 @@ sub validateCanEdit
 {
     my ($attach_id) = (@_);
 
-    # If the user is not logged in, claim that they can edit. This allows
-    # the edit screen to be displayed to people who aren't logged in.
-    # People not logged in can't actually commit changes, because that code
-    # calls Bugzilla->login with LOGIN_REQUIRED, not with LOGIN_NORMAL,
-    # before calling this sub
-    return unless Bugzilla->user;
-
     # People in editbugs can edit all attachments
     return if UserInGroup("editbugs");
 
@@ -1057,16 +1050,14 @@ sub insert
     || ThrowTemplateError($template->error());
 }
 
-# Edit an attachment record.  Users with "editbugs" privileges, (or the
-# original attachment's submitter) can edit the attachment's description,
-# content type, ispatch and isobsolete flags, and statuses, and they can
-# also submit a comment that appears in the bug.
-# Users cannot edit the content of the attachment itself.
+# Displays a form for editing attachment properties.
+# Any user is allowed to access this page, unless the attachment
+# is private and the user does not belong to the insider group.
+# Validations are done later when the user submits changes.
 sub edit
 {
   # Retrieve and validate parameters
   my ($attach_id) = validateID();
-  validateCanEdit($attach_id);
 
   # Retrieve the attachment from the database.
   SendSQL("SELECT description, mimetype, filename, bug_id, ispatch, isobsolete, isprivate, LENGTH(thedata)
@@ -1124,7 +1115,11 @@ sub edit
     || ThrowTemplateError($template->error());
 }
 
-# Updates an attachment record.
+# Updates an attachment record. Users with "editbugs" privileges, (or the
+# original attachment's submitter) can edit the attachment's description,
+# content type, ispatch and isobsolete flags, and statuses, and they can
+# also submit a comment that appears in the bug.
+# Users cannot edit the content of the attachment itself.
 sub update
 {
   my $dbh = Bugzilla->dbh;
diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl
index 41115ab9c..5840139a3 100644
--- a/template/en/default/attachment/list.html.tmpl
+++ b/template/en/default/attachment/list.html.tmpl
@@ -33,29 +33,29 @@
   </tr>
   [% canseeprivate = !Param("insidergroup") || UserInGroup(Param("insidergroup")) %]
   [% FOREACH attachment = attachments %]
-        [% IF !attachment.isprivate || canseeprivate %]
-    <tr [% "class=\"bz_private\"" IF attachment.isprivate %]>
-      <td valign="top">
-        <a href="attachment.cgi?id=[% attachment.attachid %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a>
-      </td>
-
-      <td valign="top">
-        [% IF attachment.ispatch %]
-          <i>patch</i>
-        [% ELSE %]
-          [% attachment.contenttype FILTER html %]
-        [% END %]
-      </td>
-
-      <td valign="top">[% attachment.date FILTER time %]</td>
-      <td valign="top">[% attachment.datasize FILTER unitconvert %]</td>
+    [% IF !attachment.isprivate || canseeprivate %]
+      <tr [% "class=\"bz_private\"" IF attachment.isprivate %]>
+        <td valign="top">
+          <a href="attachment.cgi?id=[% attachment.attachid %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a>
+        </td>
 
-      [% IF show_attachment_flags %]
         <td valign="top">
-          [% IF attachment.flags.size == 0 %]
-            <i>none</i>
+          [% IF attachment.ispatch %]
+            <i>patch</i>
           [% ELSE %]
-            [% FOREACH flag = attachment.flags %]
+            [% attachment.contenttype FILTER html %]
+          [% END %]
+        </td>
+
+        <td valign="top">[% attachment.date FILTER time %]</td>
+        <td valign="top">[% attachment.datasize FILTER unitconvert %]</td>
+
+        [% IF show_attachment_flags %]
+          <td valign="top">
+            [% IF attachment.flags.size == 0 %]
+              <i>none</i>
+            [% ELSE %]
+              [% FOREACH flag = attachment.flags %]
                 [% IF flag.setter %]
                   [% flag.setter.nick FILTER html %]:
                 [% END %]
@@ -63,24 +63,19 @@
                 [%+ IF flag.status == "?" && flag.requestee %]
                   ([% flag.requestee.nick FILTER html %])
                 [% END %]<br>
+              [% END %]
             [% END %]
-          [% END %]
-        </td>
-      [% END %]
-      
-      <td valign="top">
-        [% IF attachment.canedit %]
-          <a href="attachment.cgi?id=[% attachment.attachid %]&amp;action=edit">Edit</a>
+          </td>
         [% END %]
-        [% IF attachment.ispatch && patchviewerinstalled %]
-          [% IF attachment.canedit %]
-            |
+
+        <td valign="top">
+          <a href="attachment.cgi?id=[% attachment.attachid %]&amp;action=edit">Edit</a>
+          [% IF attachment.ispatch && patchviewerinstalled %]
+            | <a href="attachment.cgi?id=[% attachment.attachid %]&amp;action=diff">Diff</a>
           [% END %]
-          <a href="attachment.cgi?id=[% attachment.attachid %]&amp;action=diff">Diff</a>
-        [% END %]
-      </td>
-    </tr>
-        [% END %]
+        </td>
+      </tr>
+    [% END %]
   [% END %]
 
   <tr>