diff options
| -rw-r--r-- | Bugzilla/Attachment.pm | 76 | ||||
| -rw-r--r-- | Bugzilla/Attachment/PatchReader.pm | 36 | ||||
| -rw-r--r-- | Bugzilla/User.pm | 16 | ||||
| -rwxr-xr-x | attachment.cgi | 102 | ||||
| -rwxr-xr-x | request.cgi | 17 | ||||
| -rw-r--r-- | template/en/default/attachment/list.html.tmpl | 3 | ||||
| -rw-r--r-- | template/en/default/attachment/show-multiple.html.tmpl | 12 | ||||
| -rw-r--r-- | template/en/default/filterexceptions.pl | 2 |
8 files changed, 143 insertions, 121 deletions
diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm index ab2562521..ebf8c8de8 100644 --- a/Bugzilla/Attachment.pm +++ b/Bugzilla/Attachment.pm @@ -20,6 +20,7 @@ # Contributor(s): Terry Weissman <terry@mozilla.org> # Myk Melez <myk@mozilla.org> # Marc Schumann <wurblzap@gmail.com> +# Frédéric Buclin <LpSolit@gmail.com> use strict; @@ -475,7 +476,8 @@ sub _validate_data { =item C<get_attachments_by_bug($bug_id)> -Description: retrieves and returns the attachments for the given bug. +Description: retrieves and returns the attachments the currently logged in + user can view for the given bug. Params: C<$bug_id> - integer - the ID of the bug for which to retrieve and return attachments. @@ -486,10 +488,22 @@ Returns: a reference to an array of attachment objects. sub get_attachments_by_bug { my ($class, $bug_id) = @_; - my $attach_ids = Bugzilla->dbh->selectcol_arrayref("SELECT attach_id - FROM attachments - WHERE bug_id = ?", - undef, $bug_id); + my $user = Bugzilla->user; + my $dbh = Bugzilla->dbh; + + # By default, private attachments are not accessible, unless the user + # is in the insider group or submitted the attachment. + my $and_restriction = ''; + my @values = ($bug_id); + + unless ($user->is_insider) { + $and_restriction = 'AND (isprivate = 0 OR submitter_id = ?)'; + push(@values, $user->id); + } + + my $attach_ids = $dbh->selectcol_arrayref("SELECT attach_id FROM attachments + WHERE bug_id = ? $and_restriction", + undef, @values); my $attachments = Bugzilla::Attachment->get_list($attach_ids); return $attachments; } @@ -597,33 +611,41 @@ sub validate_content_type { =item C<validate_can_edit()> -Description: validates if the user is allowed to edit the attachment. +Description: validates if the user is allowed to view and edit the attachment. Only the submitter or someone with editbugs privs can edit it. + Only the submitter and users in the insider group can view + private attachments. Returns: 1 on success. Else an error is thrown. =cut sub validate_can_edit { - my ($class, $attach_id) = @_; + my $attachment = shift; my $dbh = Bugzilla->dbh; my $user = Bugzilla->user; - # People in editbugs can edit all attachments - return if $user->in_group('editbugs'); + # Bug 97729 - the submitter can edit their attachments. + return if ($attachment->attacher->id == $user->id); - # Bug 97729 - the submitter can edit their attachments - my ($ref) = $dbh->selectrow_array('SELECT attach_id FROM attachments - WHERE attach_id = ? AND submitter_id = ?', - undef, ($attach_id, $user->id)); + # Only users in the insider group can view private attachments. + if ($attachment->isprivate && !$user->is_insider) { + ThrowUserError('illegal_attachment_edit', {attach_id => $attachment->id}); + } - $ref || ThrowUserError('illegal_attachment_edit', { attach_id => $attach_id }); + # Users with editbugs privs can edit all attachments. + return if $user->in_group('editbugs'); + + # If we come here, then this attachment cannot be seen by the user. + ThrowUserError('illegal_attachment_edit', { attach_id => $attachment->id }); } =item C<validate_obsolete($bug_id)> Description: validates if attachments the user wants to mark as obsolete really belong to the given bug and are not already obsolete. + Moreover, a user cannot mark an attachment as obsolete if + he cannot view it (due to restrictions on it). Params: $bug_id - The bug ID obsolete attachments should belong to. @@ -636,7 +658,8 @@ sub validate_obsolete { my $cgi = Bugzilla->cgi; # Make sure the attachment id is valid and the user has permissions to view - # the bug to which it is attached. + # the bug to which it is attached. Make sure also that the user can view + # the attachment itself. my @obsolete_attachments; foreach my $attachid ($cgi->param('obsolete')) { my $vars = {}; @@ -650,6 +673,9 @@ sub validate_obsolete { # Make sure the attachment exists in the database. ThrowUserError('invalid_attach_id', $vars) unless $attachment; + # Check that the user can view and edit this attachment. + $attachment->validate_can_edit; + $vars->{'description'} = $attachment->description; if ($attachment->bug_id != $bug_id) { @@ -662,8 +688,6 @@ sub validate_obsolete { ThrowCodeError('attachment_already_obsolete', $vars); } - # Check that the user can modify this attachment. - $class->validate_can_edit($attachid); push(@obsolete_attachments, $attachment); } return @obsolete_attachments; @@ -704,8 +728,10 @@ sub insert_attachment_for_bug { $class->validate_is_patch($throw_error) || return 0; $class->validate_description($throw_error) || return 0; - if (($attachurl =~ /^(http|https|ftp):\/\/\S+/) - && !(defined $cgi->upload('data'))) { + if (Bugzilla->params->{'allow_attach_url'} + && ($attachurl =~ /^(http|https|ftp):\/\/\S+/) + && !defined $cgi->upload('data')) + { $filename = ''; $data = $attachurl; $isurl = 1; @@ -729,6 +755,12 @@ sub insert_attachment_for_bug { $isurl = 0; } + # Check attachments the user tries to mark as obsolete. + my @obsolete_attachments; + if ($cgi->param('obsolete')) { + @obsolete_attachments = $class->validate_obsolete($bug->bug_id); + } + # The order of these function calls is important, as Flag::validate # assumes User::match_field has ensured that the # values in the requestee fields are legitimate user email addresses. @@ -801,12 +833,6 @@ sub insert_attachment_for_bug { close $fh; } - # Now handle flags. - my @obsolete_attachments; - if ($cgi->param('obsolete')) { - @obsolete_attachments = $class->validate_obsolete($bug->bug_id); - } - # Make existing attachments obsolete. my $fieldid = get_field_id('attachments.isobsolete'); diff --git a/Bugzilla/Attachment/PatchReader.pm b/Bugzilla/Attachment/PatchReader.pm index 8543d6e22..00623dcf2 100644 --- a/Bugzilla/Attachment/PatchReader.pm +++ b/Bugzilla/Attachment/PatchReader.pm @@ -20,6 +20,7 @@ use strict; package Bugzilla::Attachment::PatchReader; use Bugzilla::Error; +use Bugzilla::Attachment; sub process_diff { @@ -41,32 +42,28 @@ sub process_diff { $reader->iterate_string('Attachment ' . $attachment->id, $attachment->data); } else { - $vars->{'other_patches'} = []; + my @other_patches = (); if ($lc->{interdiffbin} && $lc->{diffpath}) { - # Get list of attachments on this bug. + # Get the list of attachments that the user can view in this bug. + my @attachments = + @{Bugzilla::Attachment->get_attachments_by_bug($attachment->bug_id)}; + # Extract patches only. + @attachments = grep {$_->ispatch == 1} @attachments; + # We want them sorted from newer to older. + @attachments = sort { $b->id <=> $a->id } @attachments; + # Ignore the current patch, but select the one right before it # chronologically. - my $attachment_list = - $dbh->selectall_arrayref('SELECT attach_id, description - FROM attachments - WHERE bug_id = ? - AND ispatch = 1 - ORDER BY creation_ts DESC', - undef, $attachment->bug_id); - my $select_next_patch = 0; - foreach (@$attachment_list) { - my ($other_id, $other_desc) = @$_; - if ($other_id == $attachment->id) { + foreach my $attach (@attachments) { + if ($attach->id == $attachment->id) { $select_next_patch = 1; } else { - push(@{$vars->{'other_patches'}}, {'id' => $other_id, - 'desc' => $other_desc, - 'selected' => $select_next_patch}); - if ($select_next_patch) { - $select_next_patch = 0; - } + push(@other_patches, { 'id' => $attach->id, + 'desc' => $attach->description, + 'selected' => $select_next_patch }); + $select_next_patch = 0; } } } @@ -74,6 +71,7 @@ sub process_diff { $vars->{'bugid'} = $attachment->bug_id; $vars->{'attachid'} = $attachment->id; $vars->{'description'} = $attachment->description; + $vars->{'other_patches'} = \@other_patches; setup_template_patch_reader($last_reader, $format, $context, $vars); # Actually print out the patch. diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 19a72b9e7..02f17b85d 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -1348,6 +1348,17 @@ sub is_mover { return $self->{'is_mover'}; } +sub is_insider { + my $self = shift; + + if (!defined $self->{'is_insider'}) { + my $insider_group = Bugzilla->params->{'insidergroup'}; + $self->{'is_insider'} = + ($insider_group && $self->in_group($insider_group)) ? 1 : 0; + } + return $self->{'is_insider'}; +} + sub get_userlist { my $self = shift; @@ -1886,6 +1897,11 @@ Returns true if the user is in the list of users allowed to move bugs to another database. Note that this method doesn't check whether bug moving is enabled. +=item C<is_insider> + +Returns true if the user can access private comments and attachments, +i.e. if the 'insidergroup' parameter is set and the user belongs to this group. + =back =head1 CLASS FUNCTIONS diff --git a/attachment.cgi b/attachment.cgi index 70cad865c..431db444e 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -137,7 +137,8 @@ sub validateID { my $param = @_ ? $_[0] : 'id'; my $dbh = Bugzilla->dbh; - + my $user = Bugzilla->user; + # If we're not doing interdiffs, check if id wasn't specified and # prompt them with a page that allows them to choose an attachment. # Happens when calling plain attachment.cgi from the urlbar directly @@ -158,8 +159,8 @@ sub validateID || ThrowUserError("invalid_attach_id", { attach_id => $cgi->param($param) }); # Make sure the attachment exists in the database. - my ($bugid, $isprivate) = $dbh->selectrow_array( - "SELECT bug_id, isprivate + my ($bugid, $isprivate, $submitter_id) = $dbh->selectrow_array( + "SELECT bug_id, isprivate, submitter_id FROM attachments WHERE attach_id = ?", undef, $attach_id); @@ -167,15 +168,13 @@ sub validateID unless $bugid; # Make sure the user is authorized to access this attachment's bug. - ValidateBugID($bugid); - if ($isprivate && Bugzilla->params->{"insidergroup"}) { - Bugzilla->user->in_group(Bugzilla->params->{"insidergroup"}) - || ThrowUserError("auth_failure", {action => "access", - object => "attachment"}); + if ($isprivate && $user->id != $submitter_id && !$user->is_insider) { + ThrowUserError('auth_failure', {action => 'access', + object => 'attachment'}); } - return ($attach_id,$bugid); + return ($attach_id, $bugid); } # Validates format of a diff/interdiff. Takes a list as an parameter, which @@ -386,56 +385,27 @@ sub diff { # Display all attachments for a given bug in a series of IFRAMEs within one # HTML page. -sub viewall -{ +sub viewall { # Retrieve and validate parameters my $bugid = $cgi->param('bugid'); ValidateBugID($bugid); + my $bug = new Bugzilla::Bug($bugid); - # Retrieve the attachments from the database and write them into an array - # of hashes where each hash represents one attachment. - my $privacy = ""; - my $dbh = Bugzilla->dbh; + my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid); - if ( Bugzilla->params->{"insidergroup"} - && !Bugzilla->user->in_group(Bugzilla->params->{"insidergroup"}) ) - { - $privacy = "AND isprivate < 1 "; + foreach my $a (@$attachments) { + $a->{'isviewable'} = isViewable($a->contenttype); } - my $attachments = $dbh->selectall_arrayref( - "SELECT attach_id AS attachid, " . - $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . " AS date, - mimetype AS contenttype, description, ispatch, isobsolete, isprivate, - LENGTH(thedata) AS datasize - FROM attachments - INNER JOIN attach_data - ON attach_id = id - WHERE bug_id = ? $privacy - ORDER BY attach_id", {'Slice'=>{}}, $bugid); - - foreach my $a (@{$attachments}) - { - - $a->{'isviewable'} = isViewable($a->{'contenttype'}); - $a->{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a->{'attachid'} }); - } - # Retrieve the bug summary (for displaying on screen) and assignee. - my ($bugsummary, $assignee_id) = $dbh->selectrow_array( - "SELECT short_desc, assigned_to FROM bugs " . - "WHERE bug_id = ?", undef, $bugid); + # Define the variables and functions that will be passed to the UI template. + $vars->{'bug'} = $bug; + $vars->{'attachments'} = $attachments; - # Define the variables and functions that will be passed to the UI template. - $vars->{'bugid'} = $bugid; - $vars->{'attachments'} = $attachments; - $vars->{'bugassignee_id'} = $assignee_id; - $vars->{'bugsummary'} = $bugsummary; - - print $cgi->header(); + print $cgi->header(); - # Generate and return the UI (HTML page) from the appropriate template. - $template->process("attachment/show-multiple.html.tmpl", $vars) - || ThrowTemplateError($template->error()); + # Generate and return the UI (HTML page) from the appropriate template. + $template->process("attachment/show-multiple.html.tmpl", $vars) + || ThrowTemplateError($template->error()); } # Display a form for entering a new attachment. @@ -586,9 +556,9 @@ sub edit { # Retrieve a list of attachments for this bug as well as a summary of the bug # to use in a navigation bar across the top of the screen. my $bugattachments = - $dbh->selectcol_arrayref('SELECT attach_id FROM attachments - WHERE bug_id = ? ORDER BY attach_id', - undef, $attachment->bug_id); + Bugzilla::Attachment->get_attachments_by_bug($attachment->bug_id); + # We only want attachment IDs. + @$bugattachments = map { $_->id } @$bugattachments; my ($bugsummary, $product_id, $component_id) = $dbh->selectrow_array('SELECT short_desc, product_id, component_id @@ -629,19 +599,34 @@ sub edit { # Users cannot edit the content of the attachment itself. sub update { - my $userid = Bugzilla->user->id; + my $user = Bugzilla->user; + my $userid = $user->id; + my $dbh = Bugzilla->dbh; # Retrieve and validate parameters ValidateComment(scalar $cgi->param('comment')); my ($attach_id, $bugid) = validateID(); - Bugzilla::Attachment->validate_can_edit($attach_id); + my $attachment = Bugzilla::Attachment->get($attach_id); + $attachment->validate_can_edit; validateCanChangeAttachment($attach_id); Bugzilla::Attachment->validate_description(THROW_ERROR); Bugzilla::Attachment->validate_is_patch(THROW_ERROR); Bugzilla::Attachment->validate_content_type(THROW_ERROR) unless $cgi->param('ispatch'); validateIsObsolete(); validatePrivate(); - my $dbh = Bugzilla->dbh; + + # If the submitter of the attachment is not in the insidergroup, + # be sure that he cannot overwrite the private bit. + # This check must be done before calling Bugzilla::Flag*::validate(), + # because they will look at the private bit when checking permissions. + # XXX - This is a ugly hack. Ideally, we shouldn't have to look at the + # old private bit twice (first here, and then below again), but this is + # the less risky change. + unless ($user->is_insider) { + my $oldisprivate = $dbh->selectrow_array('SELECT isprivate FROM attachments + WHERE attach_id = ?', undef, $attach_id); + $cgi->param('isprivate', $oldisprivate); + } # The order of these function calls is important, as Flag::validate # assumes User::match_field has ensured that the values in the @@ -688,7 +673,6 @@ sub update # to attachments so that we can delete pending requests if the user # is obsoleting this attachment without deleting any requests # the user submits at the same time. - my $attachment = Bugzilla::Attachment->get($attach_id); Bugzilla::Flag::process($bug, $attachment, $timestamp, $cgi); # Update the attachment record in the database. @@ -799,10 +783,10 @@ sub delete_attachment { # Make sure the administrator is allowed to edit this attachment. my ($attach_id, $bug_id) = validateID(); - Bugzilla::Attachment->validate_can_edit($attach_id); + my $attachment = Bugzilla::Attachment->get($attach_id); + $attachment->validate_can_edit; validateCanChangeAttachment($attach_id); - my $attachment = Bugzilla::Attachment->get($attach_id); $attachment->datasize || ThrowUserError('attachment_removed'); # We don't want to let a malicious URL accidentally delete an attachment. diff --git a/request.cgi b/request.cgi index 4b2adb6b5..8d514347a 100755 --- a/request.cgi +++ b/request.cgi @@ -78,13 +78,6 @@ sub queue { my $status = validateStatus($cgi->param('status')); my $form_group = validateGroup($cgi->param('group')); - my $attach_join_clause = "flags.attach_id = attachments.attach_id"; - if (Bugzilla->params->{"insidergroup"} - && !Bugzilla->user->in_group(Bugzilla->params->{"insidergroup"})) - { - $attach_join_clause .= " AND attachments.isprivate < 1"; - } - my $query = # Select columns describing each flag, the bug/attachment on which # it has been set, who set it, and of whom they are requesting it. @@ -105,7 +98,7 @@ sub queue { " FROM flags LEFT JOIN attachments - ON ($attach_join_clause) + ON flags.attach_id = attachments.attach_id INNER JOIN flagtypes ON flags.type_id = flagtypes.id INNER JOIN profiles AS requesters @@ -134,7 +127,13 @@ sub queue { (bugs.assigned_to = $userid) " . (Bugzilla->params->{'useqacontact'} ? "OR (bugs.qa_contact = $userid))" : ")"); - + + unless ($user->is_insider) { + $query .= " AND (attachments.attach_id IS NULL + OR attachments.isprivate = 0 + OR attachments.submitter_id = $userid)"; + } + # Limit query to pending requests. $query .= " AND flags.status = '?' " unless $status; diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl index adb927e1a..a0445b16a 100644 --- a/template/en/default/attachment/list.html.tmpl +++ b/template/en/default/attachment/list.html.tmpl @@ -32,11 +32,10 @@ [% END %] <th bgcolor="#cccccc" align="left">Actions</th> </tr> - [% canseeprivate = !Param("insidergroup") || user.in_group(Param("insidergroup")) %] [% count = 0 %] [% FOREACH attachment = attachments %] [% count = count + 1 %] - [% IF !attachment.isprivate || canseeprivate %] + [% IF !attachment.isprivate || user.is_insider || attachment.attacher.id == user.id %] <tr [% "class=\"bz_private\"" IF attachment.isprivate %]> <td valign="top"> <a name="a[% count %]" href="attachment.cgi?id=[% attachment.id %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a> diff --git a/template/en/default/attachment/show-multiple.html.tmpl b/template/en/default/attachment/show-multiple.html.tmpl index ca2690c6e..ad0dfbafd 100644 --- a/template/en/default/attachment/show-multiple.html.tmpl +++ b/template/en/default/attachment/show-multiple.html.tmpl @@ -41,7 +41,7 @@ <table class="attachment_info" cellspacing="0" cellpadding="4" border="1" width="75%"> <tr> <td valign="top" bgcolor="#cccccc" colspan="6"> - <big><b>Attachment #[% a.attachid %]</b></big> + <big><b>Attachment #[% a.id %]</b></big> </td> </tr> <tr> @@ -57,7 +57,7 @@ [% END %] </td> - <td valign="top">[% a.date FILTER time %]</td> + <td valign="top">[% a.attached FILTER time %]</td> <td valign="top">[% a.datasize FILTER unitconvert %]</td> <td valign="top"> @@ -76,20 +76,20 @@ </td> <td valign="top"> - <a href="attachment.cgi?id=[% a.attachid %]&action=edit">Details</a> + <a href="attachment.cgi?id=[% a.id %]&action=edit">Details</a> </td> </tr> </table> [% IF a.isviewable %] - <iframe src="attachment.cgi?id=[% a.attachid %]" width="75%" height="350"> + <iframe src="attachment.cgi?id=[% a.id %]" width="75%" height="350"> <b>You cannot view the attachment on this page because your browser does not support IFRAMEs. - <a href="attachment.cgi?id=[% a.attachid %]">View the attachment on a separate page</a>.</b> + <a href="attachment.cgi?id=[% a.id %]">View the attachment on a separate page</a>.</b> </iframe> [% ELSE %] <p><b> Attachment cannot be viewed because its MIME type is not text/*, image/*, or application/vnd.mozilla.*. - <a href="attachment.cgi?id=[% a.attachid %]">Download the attachment instead</a>. + <a href="attachment.cgi?id=[% a.id %]">Download the attachment instead</a>. </b></p> [% END %] </div> diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl index e80c758cd..d9a3e1913 100644 --- a/template/en/default/filterexceptions.pl +++ b/template/en/default/filterexceptions.pl @@ -436,7 +436,7 @@ ], 'attachment/show-multiple.html.tmpl' => [ - 'a.attachid', + 'a.id', 'flag.status' ], |