diff options
| -rw-r--r-- | Bugzilla/Bug.pm | 33 | ||||
| -rwxr-xr-x | process_bug.cgi | 34 |
2 files changed, 30 insertions, 37 deletions
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 939829cc5..6f0563465 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -1890,6 +1890,13 @@ sub set_all { my $self = shift; my ($params) = @_; + # For security purposes, and because lots of other checks depend on it, + # we set the product first before anything else. + my $product_changed; # Used only for strict_isolation checks. + if (exists $params->{'product'}) { + $product_changed = $self->_set_product($params->{'product'}, $params); + } + # strict_isolation checks mean that we should set the groups # immediately after changing the product. $self->_add_remove($params, 'groups'); @@ -1960,6 +1967,14 @@ sub set_all { } $self->_add_remove($params, 'cc'); + + # Theoretically you could move a product without ever specifying + # a new assignee or qa_contact, or adding/removing any CCs. So, + # we have to check that the current assignee, qa, and CCs are still + # valid if we've switched products, under strict_isolation. We can only + # do that here, because if they *did* change the assignee, qa, or CC, + # then we don't want to check the original ones, only the new ones. + $self->_check_strict_isolation() if $product_changed; } # Helper for set_all that helps with fields that have an "add/remove" @@ -2089,7 +2104,9 @@ sub set_flags { sub set_op_sys { $_[0]->set('op_sys', $_[1]); } sub set_platform { $_[0]->set('rep_platform', $_[1]); } sub set_priority { $_[0]->set('priority', $_[1]); } -sub set_product { +# For security reasons, you have to use set_all to change the product. +# See the strict_isolation check in set_all for an explanation. +sub _set_product { my ($self, $name, $params) = @_; my $old_product = $self->product_obj; my $product = $self->_check_product($name); @@ -2107,9 +2124,10 @@ sub set_product { } $params ||= {}; - my $comp_name = $params->{component} || $self->component; - my $vers_name = $params->{version} || $self->version; - my $tm_name = $params->{target_milestone}; + # We delete these so that they're not set again later in set_all. + my $comp_name = delete $params->{component} || $self->component; + my $vers_name = delete $params->{version} || $self->version; + my $tm_name = delete $params->{target_milestone}; # This way, if usetargetmilestone is off and we've changed products, # set_target_milestone will reset our target_milestone to # $product->default_milestone. But if we haven't changed products, @@ -2141,7 +2159,7 @@ sub set_product { undef $@; Bugzilla->error_mode($old_error_mode); - my $verified = $params->{change_confirmed}; + my $verified = $params->{product_change_confirmed}; my %vars; if (!$verified || !$component_ok || !$version_ok || !$milestone_ok) { $vars{defaults} = { @@ -2195,7 +2213,9 @@ sub set_product { # just die if any of these are invalid. $self->set_component($comp_name); $self->set_version($vers_name); - if ($product_changed && !$self->check_can_change_field('target_milestone', 0, 1)) { + if ($product_changed + and !$self->check_can_change_field('target_milestone', 0, 1)) + { # Have to set this directly to bypass the validators. $self->{target_milestone} = $product->default_milestone; } @@ -2223,7 +2243,6 @@ sub set_product { } } - # XXX This is temporary until all of process_bug uses update(); return $product_changed; } diff --git a/process_bug.cgi b/process_bug.cgi index d2ba976a8..3c67f15c9 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -239,22 +239,6 @@ foreach my $bug (@bug_objects) { } } -# For security purposes, and because lots of other checks depend on it, -# we set the product first before anything else. -my $product_change; # Used only for strict_isolation checks, right now. -if (should_set('product')) { - foreach my $b (@bug_objects) { - my $changed = $b->set_product(scalar $cgi->param('product'), - { component => scalar $cgi->param('component'), - version => scalar $cgi->param('version'), - target_milestone => scalar $cgi->param('target_milestone'), - change_confirmed => scalar $cgi->param('confirm_product_change'), - other_bugs => \@bug_objects, - }); - $product_change ||= $changed; - } -} - # Component, target_milestone, and version are in here just in case # the 'product' field wasn't defined in the CGI. It doesn't hurt to set # them twice. @@ -264,7 +248,8 @@ my @set_fields = qw(op_sys rep_platform priority bug_severity deadline remaining_time estimated_time work_time set_default_assignee set_default_qa_contact keywords keywordaction - cclist_accessible reporter_accessible); + cclist_accessible reporter_accessible + product confirm_product_change); push(@set_fields, 'assigned_to') if !$cgi->param('set_default_assignee'); push(@set_fields, 'qa_contact') if !$cgi->param('set_default_qa_contact'); my %field_translation = ( @@ -275,9 +260,10 @@ my %field_translation = ( set_default_assignee => 'reset_assigned_to', set_default_qa_contact => 'reset_qa_contact', keywordaction => 'keywords_action', + confirm_product_change => 'product_change_confirmed', ); -my %set_all_fields; +my %set_all_fields = ( other_bugs => \@bug_objects ); foreach my $field_name (@set_fields) { if (should_set($field_name, 1)) { my $param_name = $field_translation{$field_name} || $field_name; @@ -402,18 +388,6 @@ if (defined $cgi->param('id')) { $first_bug->set_flags($flags, $new_flags); } -foreach my $b (@bug_objects) { - # Theoretically you could move a product without ever specifying - # a new assignee or qa_contact, or adding/removing any CCs. So, - # we have to check that the current assignee, qa, and CCs are still - # valid if we've switched products, under strict_isolation. We can only - # do that here. There ought to be some better way to do this, - # architecturally, but I haven't come up with it. - if ($product_change) { - $b->_check_strict_isolation(); - } -} - my $move_action = $cgi->param('action') || ''; if ($move_action eq Bugzilla->params->{'move-button-text'}) { Bugzilla->params->{'move-enabled'} || ThrowUserError("move_bugs_disabled"); |