Bug 340278: Move CheckCanChangeField() from process_bug.cgi to Bug.pm - Patch by…

Bugzilla/Bug.pm

@@ -100,7 +100,7 @@ sub initBug  {
   my ($bug_id, $user_id) = (@_);
   my $dbh = Bugzilla->dbh;
 
-  $bug_id = trim($bug_id);
+  $bug_id = trim($bug_id || 0);
 
   my $old_bug_id = $bug_id;
 
@@ -1166,6 +1166,138 @@ sub CheckIfVotedConfirmed {
     return $ret;
 }
 
+################################################################################
+# check_can_change_field() defines what users are allowed to change. You
+# can add code here for site-specific policy changes, according to the
+# instructions given in the Bugzilla Guide and below. Note that you may also
+# have to update the Bugzilla::Bug::user() function to give people access to the
+# options that they are permitted to change.
+#
+# check_can_change_field() returns true if the user is allowed to change this
+# field, and false if they are not.
+#
+# The parameters to this method are as follows:
+# $field    - name of the field in the bugs table the user is trying to change
+# $oldvalue - what they are changing it from
+# $newvalue - what they are changing it to
+# $PrivilegesRequired - return the reason of the failure, if any
+# $data     - hash containing relevant parameters, e.g. from the CGI object
+################################################################################
+sub check_can_change_field {
+    my $self = shift;
+    my ($field, $oldvalue, $newvalue, $PrivilegesRequired, $data) = (@_);
+    my $user = $self->{'who'};
+
+    $oldvalue = defined($oldvalue) ? $oldvalue : '';
+    $newvalue = defined($newvalue) ? $newvalue : '';
+
+    # Return true if they haven't changed this field at all.
+    if ($oldvalue eq $newvalue) {
+        return 1;
+    } elsif (trim($oldvalue) eq trim($newvalue)) {
+        return 1;
+    # numeric fields need to be compared using ==
+    } elsif (($field eq 'estimated_time' || $field eq 'remaining_time')
+             && $newvalue ne $data->{'dontchange'}
+             && $oldvalue == $newvalue)
+    {
+        return 1;
+    }
+
+    # Allow anyone to change comments.
+    if ($field =~ /^longdesc/) {
+        return 1;
+    }
+
+    # Ignore the assigned_to field if the bug is not being reassigned
+    if ($field eq 'assigned_to'
+        && $data->{'knob'} ne 'reassignbycomponent'
+        && $data->{'knob'} ne 'reassign')
+    {
+        return 1;
+    }
+
+    # If the user isn't allowed to change a field, we must tell him who can.
+    # We store the required permission set into the $PrivilegesRequired
+    # variable which gets passed to the error template.
+    #
+    # $PrivilegesRequired = 0 : no privileges required;
+    # $PrivilegesRequired = 1 : the reporter, assignee or an empowered user;
+    # $PrivilegesRequired = 2 : the assignee or an empowered user;
+    # $PrivilegesRequired = 3 : an empowered user.
+
+    # Allow anyone with "editbugs" privs to change anything.
+    if ($user->in_group('editbugs')) {
+        return 1;
+    }
+
+    # *Only* users with "canconfirm" privs can confirm bugs.
+    if ($field eq 'canconfirm'
+        || ($field eq 'bug_status'
+            && $oldvalue eq 'UNCONFIRMED'
+            && is_open_state($newvalue)))
+    {
+        $PrivilegesRequired = 3;
+        return $user->in_group('canconfirm');
+    }
+
+    # Make sure that a valid bug ID has been given.
+    if (!$self->{'error'}) {
+        # Allow the assignee to change anything else.
+        if ($self->{'assigned_to_id'} == $user->id) {
+            return 1;
+        }
+
+        # Allow the QA contact to change anything else.
+        if (Bugzilla->params->{'useqacontact'}
+            && $self->{'qa_contact_id'}
+            && ($self->{'qa_contact_id'} == $user->id))
+        {
+            return 1;
+        }
+    }
+
+    # At this point, the user is either the reporter or an
+    # unprivileged user. We first check for fields the reporter
+    # is not allowed to change.
+
+    # The reporter may not:
+    # - reassign bugs, unless the bugs are assigned to him;
+    #   in that case we will have already returned 1 above
+    #   when checking for the assignee of the bug.
+    if ($field eq 'assigned_to') {
+        $PrivilegesRequired = 2;
+        return 0;
+    }
+    # - change the QA contact
+    if ($field eq 'qa_contact') {
+        $PrivilegesRequired = 2;
+        return 0;
+    }
+    # - change the target milestone
+    if ($field eq 'target_milestone') {
+        $PrivilegesRequired = 2;
+        return 0;
+    }
+    # - change the priority (unless he could have set it originally)
+    if ($field eq 'priority'
+        && !Param('letsubmitterchoosepriority'))
+    {
+        $PrivilegesRequired = 2;
+        return 0;
+    }
+
+    # The reporter is allowed to change anything else.
+    if (!$self->{'error'} && $self->{'reporter_id'} == $user->id) {
+        return 1;
+    }
+
+    # If we haven't returned by this point, then the user doesn't
+    # have the necessary permissions to change this field.
+    $PrivilegesRequired = 1;
+    return 0;
+}
+
 #
 # Field Validation
 #

docs/xml/customization.xml

@@ -673,11 +673,11 @@
     <para>
       For maximum flexibility, customizing this means editing Bugzilla's Perl 
       code. This gives the administrator complete control over exactly who is
-      allowed to do what. The relevant function is called 
-      <filename>CheckCanChangeField()</filename>,
-      and is found in <filename>process_bug.cgi</filename> in your 
-      Bugzilla directory. If you open that file and search for 
-      <quote>sub CheckCanChangeField</quote>, you'll find it.
+      allowed to do what. The relevant method is called
+      <filename>check_can_change_field()</filename>,
+      and is found in <filename>Bug.pm</filename> in your
+      Bugzilla/ directory. If you open that file and search for
+      <quote>sub check_can_change_field</quote>, you'll find it.
     </para>
     
     <para>

process_bug.cgi

@@ -41,11 +41,6 @@
 
 use strict;
 
-my $UserInEditGroupSet = -1;
-my $UserInCanConfirmGroupSet = -1;
-my $PrivilegesRequired = 0;
-my $lastbugid = 0;
-
 use lib qw(.);
 
 use Bugzilla;
@@ -78,6 +73,7 @@ $vars->{'use_keywords'} = 1 if Bugzilla::Keyword::keyword_count();
 my @editable_bug_fields = editable_bug_fields();
 
 my $requiremilestone = 0;
+my $PrivilegesRequired = 0;
 
 ######################################################################
 # Subroutines
@@ -143,6 +139,11 @@ if (defined $cgi->param('id')) {
 # Make sure there are bugs to process.
 scalar(@idlist) || ThrowUserError("no_bugs_chosen");
 
+# Build a bug object using $cgi->param('id') as ID.
+# If there are more than one bug changed at once, the bug object will be
+# empty, which doesn't matter.
+my $bug = new Bugzilla::Bug(scalar $cgi->param('id'), $whoid);
+
 # Make sure form param 'dontchange' is defined so it can be compared to easily.
 $cgi->param('dontchange','') unless defined $cgi->param('dontchange');
 
@@ -176,7 +177,6 @@ ValidateComment(scalar $cgi->param('comment'));
 # during validation.
 foreach my $field ("dependson", "blocked") {
     if ($cgi->param('id')) {
-        my $bug = new Bugzilla::Bug($cgi->param('id'), $user->id);
         my @old = @{$bug->$field};
         my @new;
         foreach my $id (split(/[\s,]+/, $cgi->param($field))) {
@@ -199,7 +199,8 @@ foreach my $field ("dependson", "blocked") {
             }
         }
         if ((@$added || @$removed)
-            && (!CheckCanChangeField($field, $bug->bug_id, 0, 1))) {
+            && !$bug->check_can_change_field($field, 0, 1, \$PrivilegesRequired))
+        {
             $vars->{'privs'} = $PrivilegesRequired;
             $vars->{'field'} = $field;
             ThrowUserError("illegal_change", $vars);
@@ -307,8 +308,9 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
     && CheckonComment( "reassignbycomponent" ))
 {
     # Check to make sure they actually have the right to change the product
-    if (!CheckCanChangeField('product', scalar $cgi->param('id'), $oldproduct,
-                              $cgi->param('product'))) {
+    if (!$bug->check_can_change_field('product', $oldproduct, $cgi->param('product'),
+                                      \$PrivilegesRequired))
+    {
         $vars->{'oldvalue'} = $oldproduct;
         $vars->{'newvalue'} = $cgi->param('product');
         $vars->{'field'} = 'product';
@@ -410,169 +412,6 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
     }
 }
 
-
-# Checks that the user is allowed to change the given field.  Actually, right
-# now, the rules are pretty simple, and don't look at the field itself very
-# much, but that could be enhanced.
-
-my $ownerid;
-my $reporterid;
-my $qacontactid;
-
-################################################################################
-# CheckCanChangeField() defines what users are allowed to change what bugs. You
-# can add code here for site-specific policy changes, according to the 
-# instructions given in the Bugzilla Guide and below. Note that you may also
-# have to update the Bugzilla::Bug::user() function to give people access to the
-# options that they are permitted to change.
-#
-# CheckCanChangeField() should return true if the user is allowed to change this
-# field, and false if they are not.
-#
-# The parameters to this function are as follows:
-# $field    - name of the field in the bugs table the user is trying to change
-# $bugid    - the ID of the bug they are changing
-# $oldvalue - what they are changing it from
-# $newvalue - what they are changing it to
-#
-# Note that this function is currently not called for dependency changes 
-# (bug 141593) or CC changes, which means anyone can change those fields.
-#
-# Do not change the sections between START DO_NOT_CHANGE and END DO_NOT_CHANGE.
-################################################################################
-sub CheckCanChangeField {
-    # START DO_NOT_CHANGE
-    my ($field, $bugid, $oldvalue, $newvalue) = (@_);
-
-    $oldvalue = defined($oldvalue) ? $oldvalue : '';
-    $newvalue = defined($newvalue) ? $newvalue : '';
-
-    # Return true if they haven't changed this field at all.
-    if ($oldvalue eq $newvalue) {
-        return 1;
-    } elsif (trim($oldvalue) eq trim($newvalue)) {
-        return 1;
-    # numeric fields need to be compared using == 
-    } elsif (($field eq "estimated_time" || $field eq "remaining_time")
-             && $newvalue ne $cgi->param('dontchange')
-             && $oldvalue == $newvalue)
-    {
-        return 1;
-    }
-    # END DO_NOT_CHANGE
-
-    # Allow anyone to change comments.
-    if ($field =~ /^longdesc/) {
-        return 1;
-    }
-
-    # Ignore the assigned_to field if the bug is not being reassigned
-    if ($field eq "assigned_to"
-        && $cgi->param('knob') ne "reassignbycomponent"
-        && $cgi->param('knob') ne "reassign")
-    {
-        return 1;
-    }
-
-    # START DO_NOT_CHANGE
-    # Find out whether the user is a member of the "editbugs" and/or
-    # "canconfirm" groups. $UserIn*GroupSet are caches of the return value of 
-    # the UserInGroup calls.
-    if ($UserInEditGroupSet < 0) {
-        $UserInEditGroupSet = UserInGroup("editbugs");
-    }
-    
-    if ($UserInCanConfirmGroupSet < 0) {
-        $UserInCanConfirmGroupSet = UserInGroup("canconfirm");
-    }
-    # END DO_NOT_CHANGE
-
-    # If the user isn't allowed to change a field, we must tell him who can.
-    # We store the required permission set into the $PrivilegesRequired
-    # variable which gets passed to the error template.
-    #
-    # $PrivilegesRequired = 0 : no privileges required;
-    # $PrivilegesRequired = 1 : the reporter, assignee or an empowered user;
-    # $PrivilegesRequired = 2 : the assignee or an empowered user;
-    # $PrivilegesRequired = 3 : an empowered user.
-
-    # Allow anyone with "editbugs" privs to change anything.
-    if ($UserInEditGroupSet) {
-        return 1;
-    }
-
-    # *Only* users with "canconfirm" privs can confirm bugs.
-    if ($field eq "canconfirm"
-        || ($field eq "bug_status"
-            && $oldvalue eq 'UNCONFIRMED'
-            && is_open_state($newvalue)))
-    {
-        $PrivilegesRequired = 3;
-        return $UserInCanConfirmGroupSet;
-    }
-
-    # START DO_NOT_CHANGE
-    # $reporterid, $ownerid and $qacontactid are caches of the results of
-    # the call to find out the assignee, reporter and qacontact of the current bug.
-    if ($lastbugid != $bugid) {
-        ($reporterid, $ownerid, $qacontactid) = $dbh->selectrow_array(
-            q{SELECT reporter, assigned_to, qa_contact FROM bugs
-            WHERE bug_id = ? }, undef, $bugid);
-        $lastbugid = $bugid;
-    }
-    # END DO_NOT_CHANGE
-
-    # Allow the assignee to change anything else.
-    if ($ownerid == $whoid) {
-        return 1;
-    }
-    
-    # Allow the QA contact to change anything else.
-    if (Param('useqacontact') && $qacontactid && ($qacontactid == $whoid)) {
-        return 1;
-    }
-    
-    # At this point, the user is either the reporter or an
-    # unprivileged user. We first check for fields the reporter
-    # is not allowed to change.
-
-    # The reporter may not:
-    # - reassign bugs, unless the bugs are assigned to him;
-    #   in that case we will have already returned 1 above
-    #   when checking for the assignee of the bug.
-    if ($field eq "assigned_to") {
-        $PrivilegesRequired = 2;
-        return 0;
-    }
-    # - change the QA contact
-    if ($field eq "qa_contact") {
-        $PrivilegesRequired = 2;
-        return 0;
-    }
-    # - change the target milestone
-    if ($field eq "target_milestone") {
-        $PrivilegesRequired = 2;
-        return 0;
-    }
-    # - change the priority (unless he could have set it originally)
-    if ($field eq "priority"
-        && !Param('letsubmitterchoosepriority'))
-    {
-        $PrivilegesRequired = 2;
-        return 0;
-    }
-
-    # The reporter is allowed to change anything else.
-    if ($reporterid == $whoid) {
-        return 1;
-    }
-
-    # If we haven't returned by this point, then the user doesn't
-    # have the necessary permissions to change this field.
-    $PrivilegesRequired = 1;
-    return 0;
-}
-
 # Confirm that the reporter of the current bug can access the bug we are duping to.
 sub DuplicateUserConfirm {
     # if we've already been through here, then exit
@@ -775,7 +614,7 @@ sub DoComma {
 # confirming the bug or not.
 my $everconfirmed;
 sub DoConfirm {
-    if (CheckCanChangeField("canconfirm", scalar $cgi->param('id'), 0, 1)) {
+    if ($bug->check_can_change_field("canconfirm", 0, 1, \$PrivilegesRequired)) {
         DoComma();
         $::query .= "everconfirmed = 1";
         $everconfirmed = 1;
@@ -860,7 +699,9 @@ sub ChangeResolution {
                 $dbh->selectrow_array('SELECT resolution FROM bugs WHERE bug_id = ?',
                                        undef, $bug_id);
         }
-        unless (CheckCanChangeField('resolution', $bug_id, $old_resolution, $str)) {
+        unless ($bug->check_can_change_field('resolution', $old_resolution, $str,
+                                             \$PrivilegesRequired))
+        {
             $vars->{'oldvalue'} = $old_resolution;
             $vars->{'newvalue'} = $str;
             $vars->{'field'} = 'resolution';
@@ -873,7 +714,7 @@ sub ChangeResolution {
         trick_taint($str);
         push(@values, $str);
         # We define this variable here so that customized installations
-        # may set rules based on the resolution in CheckCanChangeField.
+        # may set rules based on the resolution in Bug::check_can_change_field().
         $cgi->param('resolution', $str);
     }
 }
@@ -1541,7 +1382,8 @@ foreach my $id (@idlist) {
         $oldvalues[$i] = defined($oldvalues[$i]) ? $oldvalues[$i] : '';
         # Convert the deadline taken from the DB into the YYYY-MM-DD format
         # for consistency with the deadline provided by the user, if any.
-        # Else CheckCanChangeField() would see them as different in all cases.
+        # Else Bug::check_can_change_field() would see them as different
+        # in all cases.
         if ($col eq 'deadline') {
             $oldvalues[$i] = format_time($oldvalues[$i], "%Y-%m-%d");
         }
@@ -1562,12 +1404,18 @@ foreach my $id (@idlist) {
             $formhash{'bug_status'} = $oldhash{'bug_status'};
         }
     }
+    # This hash is required by Bug::check_can_change_field().
+    my $cgi_hash = {
+        'dontchange' => scalar $cgi->param('dontchange'),
+        'knob'       => scalar $cgi->param('knob')
+    };
     foreach my $col (@editable_bug_fields) {
         # The 'resolution' field is checked by ChangeResolution(),
         # i.e. only if we effectively use it.
         next if ($col eq 'resolution');
         if (exists $formhash{$col}
-            && !CheckCanChangeField($col, $id, $oldhash{$col}, $formhash{$col}))
+            && !$old_bug_obj->check_can_change_field($col, $oldhash{$col}, $formhash{$col},
+                                                     \$PrivilegesRequired, $cgi_hash))
         {
             my $vars;
             if ($col eq 'component_id') {
@@ -1592,14 +1440,15 @@ foreach my $id (@idlist) {
     
     # 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,
-    # CheckCanChangeField above will fail to check permissions because it thinks
-    # the list hasn't changed.  To fix that, we have to call CheckCanChangeField
+    # Bug::check_can_change_field will fail to check permissions because it thinks
+    # the list hasn't changed. To fix that, we have to call Bug::check_can_change_field
     # again with old!=new if the keyword action is "delete" and old=new.
     if ($keywordaction eq "delete"
         && defined $cgi->param('keywords')
         && length(@keywordlist) > 0
         && $cgi->param('keywords') eq $oldhash{keywords}
-        && !CheckCanChangeField("keywords", $id, "old is not", "equal to new"))
+        && !$old_bug_obj->check_can_change_field("keywords", "old is not", "equal to new",
+                                                 \$PrivilegesRequired))
     {
         $vars->{'oldvalue'} = $oldhash{keywords};
         $vars->{'newvalue'} = "no keywords";