From f65619b0235e24eb5df01fe82c90a45b2ae985e6 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Tue, 15 May 2007 00:57:35 +0000 Subject: Bug 373660: Move dependency checking from process_bug into Bugzilla::Bug Patch By Max Kanat-Alexander r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 72 +++++++++++++++++++++++++++++++++++++++++---------------- process_bug.cgi | 46 ++++++++---------------------------- 2 files changed, 62 insertions(+), 56 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 62be49564..e523621e0 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -411,7 +411,8 @@ sub run_create_validators { $params->{assigned_to}, $params->{qa_contact}); ($params->{dependson}, $params->{blocked}) = - $class->_check_dependencies($product, $params->{dependson}, $params->{blocked}); + $class->_check_dependencies($params->{dependson}, $params->{blocked}, + $product); # You can't set these fields on bug creation (or sometimes ever). delete $params->{resolution}; @@ -701,28 +702,59 @@ sub _check_deadline { # Takes two comma/space-separated strings and returns arrayrefs # of valid bug IDs. sub _check_dependencies { - my ($invocant, $product, $depends_on, $blocks) = @_; + my ($invocant, $depends_on, $blocks, $product) = @_; - # Only editbugs users can set dependencies on bug entry. - return ([], []) unless Bugzilla->user->in_group('editbugs', $product->id); - - $depends_on ||= ''; - $blocks ||= ''; + if (!ref $invocant) { + # Only editbugs users can set dependencies on bug entry. + return ([], []) unless Bugzilla->user->in_group('editbugs', + $product->id); + } + + my %deps_in = (dependson => $depends_on || '', blocked => $blocks || ''); + + foreach my $type qw(dependson blocked) { + my @bug_ids = split(/[\s,]+/, $deps_in{$type}); + # Eliminate nulls. + @bug_ids = grep {$_} @bug_ids; + # We do Validate up here to make sure all aliases are converted to IDs. + ValidateBugID($_, $type) foreach @bug_ids; + + my @check_access = @bug_ids; + # When we're updating a bug, only added or removed bug_ids are + # checked for whether or not we can see/edit those bugs. + if (ref $invocant) { + my $old = $invocant->$type; + my ($removed, $added) = diff_arrays($old, \@bug_ids); + @check_access = (@$added, @$removed); + + # Check field permissions if we've changed anything. + if (@check_access) { + my $privs; + if (!$invocant->check_can_change_field($type, 0, 1, \$privs)) { + ThrowUserError('illegal_change', { field => $type, + privs => $privs }); + } + } + } - # Make sure all the bug_ids are valid. - my @results; - foreach my $string ($depends_on, $blocks) { - my @array = split(/[\s,]+/, $string); - # Eliminate nulls - @array = grep($_, @array); - # $field is not passed to ValidateBugID to prevent adding new - # dependencies on inaccessible bugs. - ValidateBugID($_) foreach (@array); - push(@results, \@array); + my $user = Bugzilla->user; + foreach my $modified_id (@check_access) { + ValidateBugID($modified_id); + # Under strict isolation, you can't modify a bug if you can't + # edit it, even if you can see it. + if (Bugzilla->params->{"strict_isolation"}) { + my $delta_bug = new Bugzilla::Bug($modified_id); + if (!$user->can_edit_product($delta_bug->{'product_id'})) { + ThrowUserError("illegal_change_deps", {field => $type}); + } + } + } + + $deps_in{$type} = \@bug_ids; } - - # dependson blocks - my %deps = ValidateDependencies($results[0], $results[1]); + + # And finally, check for dependency loops. + my %deps = ValidateDependencies($deps_in{dependson}, $deps_in{blocked}); return ($deps{'dependson'}, $deps{'blocked'}); } diff --git a/process_bug.cgi b/process_bug.cgi index 8be6cb48f..a28efa02f 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -185,42 +185,16 @@ $cgi->param('comment', $bug->_check_comment($cgi->param('comment'))); # instead of throwing an error, f.e. when one or more of the values # is a bug alias that gets converted to its corresponding bug ID # during validation. -foreach my $field ("dependson", "blocked") { - if ($cgi->param('id')) { - my @old = @{$bug->$field}; - my @new; - foreach my $id (split(/[\s,]+/, $cgi->param($field))) { - next unless $id; - ValidateBugID($id, $field); - push @new, $id; - } - $cgi->param($field, join(",", @new)); - my ($added, $removed) = Bugzilla::Util::diff_arrays(\@old, \@new); - foreach my $id (@$added , @$removed) { - # ValidateBugID is called without $field here so that it will - # throw an error if any of the changed bugs are not visible. - ValidateBugID($id); - if (Bugzilla->params->{"strict_isolation"}) { - my $deltabug = new Bugzilla::Bug($id); - if (!$user->can_edit_product($deltabug->{'product_id'})) { - $vars->{'field'} = $field; - ThrowUserError("illegal_change_deps", $vars); - } - } - } - if ((@$added || @$removed) - && !$bug->check_can_change_field($field, 0, 1, \$PrivilegesRequired)) - { - $vars->{'privs'} = $PrivilegesRequired; - $vars->{'field'} = $field; - ThrowUserError("illegal_change", $vars); - } - } else { - # Bugzilla does not support mass-change of dependencies so they - # are not validated. To prevent a URL-hacking risk, the dependencies - # are deleted for mass-changes. - $cgi->delete($field); - } +if ($cgi->param('id')) { + my ($dependson, $blocks) = $bug->_check_dependencies( + scalar $cgi->param('dependson'), scalar $cgi->param('blocked')); + $cgi->param('dependson', $dependson); + $cgi->param('blocked', $blocks); +} +# Right now, you can't modify dependencies on a mass change. +else { + $cgi->delete('dependson'); + $cgi->delete('blocked'); } # do a match on the fields if applicable -- cgit v1.2.3-24-g4f1b