summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
-rw-r--r--Bugzilla/Bug.pm33
-rwxr-xr-xprocess_bug.cgi34
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");