From 6d11d68f570d4d921f078a67ce1928703df4bf2e Mon Sep 17 00:00:00 2001
From: =?utf8?q?Fr=C3=A9d=C3=A9ric=20Buclin?= <LpSolit@gmail.com>
Date: Mon, 1 Feb 2010 13:19:31 -0800
Subject: [PATCH] =?utf8?q?Bug=20532493:=20[SECURITY]=20Restricting=20a=20b?=
 =?utf8?q?ug=20to=20a=20group=20while=20moving=20it=20to=20another=20produ?=
 =?utf8?q?ct=20has=20no=20effect=20if=20the=20group=20is=20not=20used=20by?=
 =?utf8?q?=20both=20products=20Patch=20by=20Fr=C3=A9d=C3=A9ric=20Buclin=20?=
 =?utf8?q?<LpSolit@gmail.com>=20r=3Dmkanat=20a=3DLpSolit?=
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

---
 Bugzilla/Bug.pm | 27 ---------------------------
 process_bug.cgi | 37 ++++++++++++++++++++-----------------
 2 files changed, 20 insertions(+), 44 deletions(-)

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 1eaafb6983..fd28b5b82c 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -643,33 +643,6 @@ sub run_create_validators {
     return $params;
 }
 
-sub set_all {
-    my ($self, $args) = @_;
-
-    # For security purposes, and because lots of other checks depend on it,
-    # we set the product first before anything else.
-    my $product_change = 0;
-    if ($args->{product}) {
-        my $changed = $self->set_product($args->{product},
-                                        { component => $args->{component},
-                                          version   => $args->{version},
-                                          target_milestone => $args->{target_milestone},
-                                          change_confirmed => $args->{confirm_product_change},
-                                          other_bugs => $args->{other_bugs},
-                                        });
-        # that will be used later to check strict isolation
-        $product_change = $changed;
-    }
-
-    # add/remove groups
-    $self->remove_group($_) foreach @{$args->{remove_group}};
-    $self->add_group($_) foreach @{$args->{add_group}};
-
-    # this is temporary until all related code is moved from
-    # process_bug.cgi to set_all
-    return $product_change;
-}
-
 sub update {
     my $self = shift;
 
diff --git a/process_bug.cgi b/process_bug.cgi
index 6c9baa3d3d..237588ebd6 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -236,36 +236,39 @@ foreach my $bug (@bug_objects) {
     }
 }
 
-my $product_change;
-foreach my $bug (@bug_objects) {
-    my $args;
-    if (should_set('product')) {
-        $args->{product} = scalar $cgi->param('product');
-        $args->{component} = scalar $cgi->param('component');
-        $args->{version} = scalar $cgi->param('version');
-        $args->{target_milestone} = scalar $cgi->param('target_milestone');
-        $args->{confirm_product_change} =  scalar $cgi->param('confirm_product_change');
-        $args->{other_bugs} = \@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;
     }
+}
 
-    foreach my $group (@{$bug->product_obj->groups_valid}) {
+# strict_isolation checks mean that we should set the groups
+# immediately after changing the product.
+foreach my $b (@bug_objects) {
+    foreach my $group (@{$b->product_obj->groups_valid}) {
         my $gid = $group->id;
         if (should_set("bit-$gid", 1)) {
             # Check ! first to avoid having to check defined below.
             if (!$cgi->param("bit-$gid")) {
-                push (@{$args->{remove_group}}, $gid);
+                $b->remove_group($gid);
             }
             # "== 1" is important because mass-change uses -1 to mean
             # "don't change this restriction"
             elsif ($cgi->param("bit-$gid") == 1) {
-               push (@{$args->{add_group}}, $gid);
+                $b->add_group($gid);
             }
         }
     }
-
-    # this will be deleted later when code moves to $bug->set_all
-    my $changed = $bug->set_all($args);
-    $product_change ||= $changed;
 }
 
 # Flags should be set AFTER the bug has been moved into another product/component.
-- 
2.47.2