Bug 140999: Users without edit permissions for an attachment should still be…

Bugzilla/Attachment.pm

@@ -720,7 +720,7 @@ Description: validates if the user is allowed to view and edit the attachment.
 Params:      $attachment - the attachment object being edited.
              $product_id - the product ID the attachment belongs to.
 
-Returns:     1 on success. Else an error is thrown.
+Returns:     1 on success, 0 otherwise.
 
 =cut
 
@@ -729,12 +729,9 @@ sub validate_can_edit {
     my $user = Bugzilla->user;
 
     # The submitter can edit their attachments.
-    return 1 if ($attachment->attacher->id == $user->id
+    return ($attachment->attacher->id == $user->id
             || ((!$attachment->isprivate || $user->is_insider)
-                      && $user->in_group('editbugs', $product_id)));
-
-    # If we come here, then this attachment cannot be edited by the user.
-    ThrowUserError('illegal_attachment_edit', { attach_id => $attachment->id });
+                 && $user->in_group('editbugs', $product_id))) ? 1 : 0;
 }
 
 =item C<validate_obsolete($bug)>
@@ -769,7 +766,8 @@ sub validate_obsolete {
           || ThrowUserError('invalid_attach_id', $vars);
 
         # Check that the user can view and edit this attachment.
-        $attachment->validate_can_edit($bug->product_id);
+        $attachment->validate_can_edit($bug->product_id)
+          || ThrowUserError('illegal_attachment_edit', { attach_id => $attachment->id });
 
         $vars->{'description'} = $attachment->description;
 

attachment.cgi

@@ -572,8 +572,9 @@ sub update {
     my $attachment = validateID();
     my $bug = $attachment->bug;
     $attachment->_check_bug;
-    $attachment->validate_can_edit($bug->product_id); # FIXME: allow comments anyway.
+    my $can_edit = $attachment->validate_can_edit($bug->product_id);
 
+    if ($can_edit) {
         $attachment->set_description(scalar $cgi->param('description'));
         $attachment->set_is_patch(scalar $cgi->param('ispatch'));
         $attachment->set_content_type(scalar $cgi->param('contenttypeentry'));
@@ -605,6 +606,7 @@ sub update {
                 exit;
             }
         }
+    }
 
     # We couldn't do this check earlier as we first had to validate attachment ID
     # and display the mid-air collision page if modification_time changed.
@@ -622,16 +624,22 @@ sub update {
         $bug->add_comment($comment, { isprivate => $attachment->isprivate });
     }
 
-    my ($flags, $new_flags) = Bugzilla::Flag->extract_flags_from_cgi($bug, $attachment, $vars);
+    if ($can_edit) {
+        my ($flags, $new_flags) =
+          Bugzilla::Flag->extract_flags_from_cgi($bug, $attachment, $vars);
         $attachment->set_flags($flags, $new_flags);
+    }
 
     # Figure out when the changes were made.
     my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
 
+    if ($can_edit) {
         my $changes = $attachment->update($timestamp);
         # If there are changes, we updated delta_ts in the DB. We have to
         # reflect this change in the bug object.
         $bug->{delta_ts} = $timestamp if scalar(keys %$changes);
+    }
+
     # Commit the comment, if any.
     $bug->update($timestamp);