Bug 315332 fix several bugs in process_bug.cgi when 'strict_isolation' is on

Patch by Joel Peshkin <bugreport@peshkin.net>
r=lpsolit, a=justdave

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 6a6c87d378bf0c93ec0e444422f33fa7b083c911..61798f7cb017ad8d4e6484d2b56c5bd45dc5142b 100755 (executable)
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -1299,17 +1299,6 @@ sub ValidateDependencies {
     return %deps;
 }
 
-#Verify if the new assignee belongs to the group of 
-#the product that the bug(s) is in.
-sub can_add_user_to_bug {
-    my ($prod_id, $id, $uid) = @_;
-    my $user = new Bugzilla::User($uid);
-    if (!$user->can_edit_product($prod_id)) {
-        ThrowUserError("invalid_user_group", { 'user' =>
-        $user->login, bug_id => $id });
-   } 
-}
-
 sub AUTOLOAD {
   use vars qw($AUTOLOAD);
   my $attr = $AUTOLOAD;

diff --git a/process_bug.cgi b/process_bug.cgi
index af0283d6ceb5cf739c27c562629a40cbd492fb58..e56793fcd63f14e25c44e17d4c126252556e624f 100755 (executable)
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -164,19 +164,20 @@ foreach my $field ("dependson", "blocked") {
             # 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 (!CheckCanChangeField($field, $bug->bug_id, 0, 1)) {
-                $vars->{'privs'} = $PrivilegesRequired;
-                $vars->{'field'} = $field;
-                ThrowUserError("illegal_change", $vars);
-            }
             if (Param("strict_isolation")) {
-                my $deltabug = new Bugzilla::Bug($id, $user);
+                my $deltabug = new Bugzilla::Bug($id, $user->id);
                 if (!$user->can_edit_product($deltabug->{'product_id'})) {
                     $vars->{'field'} = $field;
                     ThrowUserError("illegal_change_deps", $vars);
                 }
             }
         }
+        if ((@$added  || @$removed)
+            && (!CheckCanChangeField($field, $bug->bug_id, 0, 1))) {
+            $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
@@ -855,6 +856,8 @@ foreach my $field ("rep_platform", "priority", "bug_severity",
 }
 
 my $prod_id;
+my $prod_changed;
+my @newprod_ids;
 if ($cgi->param('product') ne $cgi->param('dontchange')) {
     $prod_id = get_product_id($cgi->param('product'));
     $prod_id ||
@@ -862,17 +865,23 @@ if ($cgi->param('product') ne $cgi->param('dontchange')) {
                      {product => $cgi->param('product')});
       
     DoComma();
+    @newprod_ids = ($prod_id);
+    $prod_changed = 1;
     $::query .= "product_id = $prod_id";
 } else {
-    SendSQL("SELECT DISTINCT product_id FROM bugs WHERE bug_id IN (" .
-            join(',', @idlist) . ") " . $dbh->sql_limit(2));
-    $prod_id = FetchOneColumn();
-    $prod_id = undef if (FetchOneColumn());
+    @newprod_ids = @{$dbh->selectcol_arrayref("SELECT DISTINCT product_id
+                                               FROM bugs 
+                                               WHERE bug_id IN (" .
+                                                   join(',', @idlist) . 
+                                               ")")};
+    if (scalar(@newprod_ids) == 1) {
+        ($prod_id) = @newprod_ids;
+    }
 }
 
 my $comp_id;
 if ($cgi->param('component') ne $cgi->param('dontchange')) {
-    if (!defined $prod_id) {
+    if (scalar(@newprod_ids) > 1) {
         ThrowUserError("no_component_change_for_multiple_products");
     }
     $comp_id = get_component_id($prod_id,
@@ -1005,9 +1014,16 @@ if (defined $cgi->param('newcc')
 # only way to keep these informations when bugs are reassigned by
 # component as $cgi->param('assigned_to') and $cgi->param('qa_contact')
 # are not the right fields to look at.
+# If the assignee or qacontact is changed, the new one is checked when
+# changed information is validated.  If not, then the unchanged assignee
+# or qacontact may have to be validated later.
 
 my $assignee;
 my $qacontact;
+my $qacontact_checked = 0;
+my $assignee_checked = 0;
+
+my %usercache = ();
 
 if (defined $cgi->param('qa_contact')
     && $cgi->param('knob') ne "reassignbycomponent")
@@ -1016,11 +1032,22 @@ if (defined $cgi->param('qa_contact')
     # The QA contact cannot be deleted from show_bug.cgi for a single bug!
     if ($name ne $cgi->param('dontchange')) {
         $qacontact = DBNameToIdAndCheck($name) if ($name ne "");
-        if (Param("strict_isolation")) {
-                my $product_id = get_product_id($cgi->param('product'));
-                Bugzilla::Bug::can_add_user_to_bug(
-                    $product_id, $cgi->param('id'), $qacontact);
+        if ($qacontact && Param("strict_isolation")) {
+                $usercache{$qacontact} ||= Bugzilla::User->new($qacontact);
+                my $qa_user = $usercache{$qacontact};
+                foreach my $product_id (@newprod_ids) {
+                    if (!$qa_user->can_edit_product($product_id)) {
+                        my $product_name = get_product_name($product_id);
+                        ThrowUserError('invalid_user_group',
+                                          {'users'   => $qa_user->login,
+                                           'product' => $product_name,
+                                           'bug_id' => (scalar(@idlist) > 1)
+                                                         ? undef : $idlist[0]
+                                          });
+                    }
+                }
         }
+        $qacontact_checked = 1;
         DoComma();
         if($qacontact) {
             $::query .= "qa_contact = $qacontact";
@@ -1082,18 +1109,28 @@ SWITCH: for ($cgi->param('knob')) {
         }
         ChangeStatus('NEW');
         DoComma();
-        if (defined $cgi->param('assigned_to')) { 
-            my $uid = DBNameToIdAndCheck($cgi->param('assigned_to'));
+        if (defined $cgi->param('assigned_to')
+            && trim($cgi->param('assigned_to')) ne "") { 
+            $assignee = DBNameToIdAndCheck(trim($cgi->param('assigned_to')));
             if (Param("strict_isolation")) {
-                my $product_id = get_product_id($cgi->param('product'));
-                Bugzilla::Bug::can_add_user_to_bug(
-                    $product_id, $cgi->param('id'), $uid);
+                $usercache{$assignee} ||= Bugzilla::User->new($assignee);
+                my $assign_user = $usercache{$assignee};
+                foreach my $product_id (@newprod_ids) {
+                    if (!$assign_user->can_edit_product($product_id)) {
+                        my $product_name = get_product_name($product_id);
+                        ThrowUserError('invalid_user_group',
+                                          {'users'   => $assign_user->login,
+                                           'product' => $product_name,
+                                           'bug_id' => (scalar(@idlist) > 1)
+                                                         ? undef : $idlist[0]
+                                          });
+                    }
+                }
             }
-        } elsif (!defined $cgi->param('assigned_to')
-            || trim($cgi->param('assigned_to')) eq "") {
+        } else {
             ThrowUserError("reassign_to_empty");
         }
-        $assignee = DBNameToIdAndCheck(trim($cgi->param('assigned_to')));
+        $assignee_checked = 1;
         $::query .= "assigned_to = $assignee";
         last SWITCH;
     };
@@ -1290,6 +1327,75 @@ sub LogDependencyActivity {
     return 0;
 }
 
+if (Param("strict_isolation")) {
+    my @blocked_cc = ();
+    foreach my $pid (keys %cc_add) {
+        $usercache{$pid} ||= Bugzilla::User->new($pid);
+        my $cc_user = $usercache{$pid};
+        foreach my $product_id (@newprod_ids) {
+            if (!$cc_user->can_edit_product($product_id)) {
+                push (@blocked_cc, $cc_user->login);
+                last;
+            }
+        }
+    }
+    if (scalar(@blocked_cc)) {
+        ThrowUserError("invalid_user_group", 
+            {'users' => \@blocked_cc,
+             'bug_id' => (scalar(@idlist) > 1) ? undef : $idlist[0]});
+    }
+}
+
+if ($prod_changed && Param("strict_isolation")) {
+    my $sth_cc = $dbh->prepare("SELECT who
+                                FROM cc
+                                WHERE bug_id = ?");
+    my $sth_bug = $dbh->prepare("SELECT assigned_to, qa_contact
+                                 FROM bugs
+                                 WHERE bug_id = ?");
+    my $prod_name = get_product_name($prod_id);
+    foreach my $id (@idlist) {
+        $sth_cc->execute($id);
+        my @blocked_cc = ();
+        while (my ($pid) = $sth_cc->fetchrow_array) {
+            $usercache{$pid} ||= Bugzilla::User->new($pid);
+            my $cc_user = $usercache{$pid};
+            if (!$cc_user->can_edit_product($prod_id)) {
+                push (@blocked_cc, $cc_user->login);
+            }
+        }
+        if (scalar(@blocked_cc)) {
+            ThrowUserError('invalid_user_group',
+                              {'users'   => \@blocked_cc,
+                               'bug_id' => $id,
+                               'product' => $prod_name});
+        }
+        $sth_bug->execute($id);
+        my ($assignee, $qacontact) = $sth_bug->fetchrow_array;
+        if (!$assignee_checked) {
+            $usercache{$assignee} ||= Bugzilla::User->new($assignee);
+            my $assign_user = $usercache{$assignee};
+            if (!$assign_user->can_edit_product($prod_id)) {
+                    ThrowUserError('invalid_user_group',
+                                      {'users'   => $assign_user->login,
+                                       'bug_id' => $id,
+                                       'product' => $prod_name});
+            }
+        }
+        if (!$qacontact_checked && $qacontact) {
+            $usercache{$qacontact} ||= Bugzilla::User->new($qacontact);
+            my $qa_user = $usercache{$qacontact};
+            if (!$qa_user->can_edit_product($prod_id)) {
+                    ThrowUserError('invalid_user_group',
+                                      {'users'   => $qa_user->login,
+                                       'bug_id' => $id,
+                                       'product' => $prod_name});
+            }
+        }
+    }
+}
+
+
 # This loop iterates once for each bug to be processed (i.e. all the
 # bugs selected when this script is called with multiple bugs selected
 # from buglist.cgi, or just the one bug when called from
@@ -1397,15 +1503,6 @@ foreach my $id (@idlist) {
             ThrowUserError("illegal_change", $vars);
         }
     }
-    if ($cgi->param('assigned_to') && Param("strict_isolation")) { 
-        my $uid = DBNameToIdAndCheck($cgi->param('assigned_to'));
-        Bugzilla::Bug::can_add_user_to_bug(
-            $bug_obj->{'product_id'}, $id, $uid);
-    }
-    if ($cgi->param('qa_contact') && Param("strict_isolation")) { 
-        Bugzilla::Bug::can_add_user_to_bug(
-            $bug_obj->{'product_id'}, $id, $qacontact);
-    }
     
     # When editing multiple bugs, users can specify a list of keywords to delete
     # from bugs.  If the list matches the current set of keywords on those bugs,
@@ -1543,7 +1640,7 @@ foreach my $id (@idlist) {
         }
     }
     $query .= " where bug_id = $id";
-    
+
     if ($::comma ne "") {
         SendSQL($query);
     }
@@ -1621,21 +1718,7 @@ foreach my $id (@idlist) {
             $oncc{FetchOneColumn()} = 1;
         }
 
-        my (@added, @removed, @blocked_cc) = ();
-        
-        if (Param("strict_isolation")) {
-            foreach my $pid (keys %cc_add) {
-                my $user = Bugzilla::User->new($pid);
-                if (!$user->can_edit_product($bug_obj->{'product_id'})) {
-                    push (@blocked_cc, $cc_add{$pid});
-                }
-            }
-            if (scalar(@blocked_cc)) {
-                my $blocked_cc = join(", ", @blocked_cc);
-                ThrowUserError("invalid_user_group", 
-                    {'user' => $blocked_cc , bug_id => $id });
-            }
-        }
+        my (@added, @removed) = ();
  
         foreach my $pid (keys %cc_add) {
             # If this person isn't already on the cc list, add them

diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index ac019b681376eb5d5240d7800a6baca985011efd..be86ae5064c193fe174fd3fc3f48ae9adf6a2f93 100644 (file)
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -670,8 +670,19 @@
 
   [% ELSIF error == "invalid_user_group" %]
     [% title = "Invalid User Group" %]
-    User '[% user FILTER html %]' is not able to edit the 
-    [% terms.bug %] '[% bug_id FILTER html %]'.
+    [% IF users.size > 1 %] Users [% ELSE %] User [% END %]
+    '[% users.join(', ') FILTER html %]'
+    [% IF users.size > 1 %] are [% ELSE %] is [% END %]
+    not able to edit the
+    [% IF product %]
+      '[% product FILTER html %]'
+    [% END %]
+    [%+ field_descs.product FILTER html %]
+    [% IF bug_id %]
+      for [% terms.bug %] '[% bug_id FILTER html %]'.
+    [% ELSE %]
+      for at least one [% terms.bug %] being changed.
+    [% END %]
 
   [% ELSIF error == "invalid_username" %]
     [% title = "Invalid Username" %]