Bug 556407: Move the code for setting product and checking strict_isolation

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;
 }
 

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");