Bug 544332 - New bug_check_can_change_field hook for Bugzilla/Bug.pm

r/a=mkanat

5 files changed, 143 insertions, 16 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 9282bc3d2..c7c168125 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -49,7 +49,7 @@ use Bugzilla::Group;
 use Bugzilla::Status;
 use Bugzilla::Comment;
 
-use List::Util qw(min);
+use List::Util qw(min first);
 use Storable qw(dclone);
 use URI;
 use URI::QueryParam;
@@ -3315,6 +3315,20 @@ sub check_can_change_field {
         return 1;
     }
 
+    my @priv_results;
+    Bugzilla::Hook::process('bug_check_can_change_field',
+        { bug => $self, field => $field, 
+          new_value => $newvalue, old_value => $oldvalue, 
+          priv_results => \@priv_results });
+    if (my $priv_required = first { $_ > 0 } @priv_results) {
+        $$PrivilegesRequired = $priv_required;
+        return 0;
+    }
+    my $allow_found = first { $_ == 0 } @priv_results;
+    if (defined $allow_found) {
+        return 1;
+    }
+
     # Allow anyone to change comments.
     if ($field =~ /^longdesc/) {
         return 1;
@@ -3324,15 +3338,15 @@ sub check_can_change_field {
     # 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.
+    # $PrivilegesRequired = PRIVILEGES_REQUIRED_NONE : no privileges required;
+    # $PrivilegesRequired = PRIVILEGES_REQUIRED_REPORTER : the reporter, assignee or an empowered user;
+    # $PrivilegesRequired = PRIVILEGES_REQUIRED_ASSIGNEE : the assignee or an empowered user;
+    # $PrivilegesRequired = PRIVILEGES_REQUIRED_EMPOWERED : an empowered user.
     
     # Only users in the time-tracking group can change time-tracking fields.
     if ( grep($_ eq $field, qw(deadline estimated_time remaining_time)) ) {
         if (!$user->is_timetracker) {
-            $$PrivilegesRequired = 3;
+            $$PrivilegesRequired = PRIVILEGES_REQUIRED_EMPOWERED;
             return 0;
         }
     }
@@ -3344,7 +3358,7 @@ sub check_can_change_field {
 
     # *Only* users with (product-specific) "canconfirm" privs can confirm bugs.
     if ($self->_changes_everconfirmed($field, $oldvalue, $newvalue)) {
-        $$PrivilegesRequired = 3;
+        $$PrivilegesRequired = PRIVILEGES_REQUIRED_EMPOWERED;
         return $user->in_group('canconfirm', $self->{'product_id'});
     }
 
@@ -3375,36 +3389,36 @@ sub check_can_change_field {
     #   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;
+        $$PrivilegesRequired = PRIVILEGES_REQUIRED_ASSIGNEE;
         return 0;
     }
     # - change the QA contact
     if ($field eq 'qa_contact') {
-        $$PrivilegesRequired = 2;
+        $$PrivilegesRequired = PRIVILEGES_REQUIRED_ASSIGNEE;
         return 0;
     }
     # - change the target milestone
     if ($field eq 'target_milestone') {
-        $$PrivilegesRequired = 2;
+        $$PrivilegesRequired = PRIVILEGES_REQUIRED_ASSIGNEE;
         return 0;
     }
     # - change the priority (unless he could have set it originally)
     if ($field eq 'priority'
         && !Bugzilla->params->{'letsubmitterchoosepriority'})
     {
-        $$PrivilegesRequired = 2;
+        $$PrivilegesRequired = PRIVILEGES_REQUIRED_ASSIGNEE;
         return 0;
     }
     # - unconfirm bugs (confirming them is handled above)
     if ($field eq 'everconfirmed') {
-        $$PrivilegesRequired = 2;
+        $$PrivilegesRequired = PRIVILEGES_REQUIRED_ASSIGNEE;
         return 0;
     }
     # - change the status from one open state to another
     if ($field eq 'bug_status'
         && is_open_state($oldvalue) && is_open_state($newvalue)) 
     {
-       $$PrivilegesRequired = 2;
+       $$PrivilegesRequired = PRIVILEGES_REQUIRED_ASSIGNEE;
        return 0;
     }
 
@@ -3415,7 +3429,7 @@ sub check_can_change_field {
 
     # If we haven't returned by this point, then the user doesn't
     # have the necessary permissions to change this field.
-    $$PrivilegesRequired = 1;
+    $$PrivilegesRequired = PRIVILEGES_REQUIRED_REPORTER;
     return 0;
 }
 
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index d38f123fd..87210ffd4 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -175,6 +175,11 @@ use File::Basename;
     PASSWORD_SALT_LENGTH
     
     CGI_URI_LIMIT
+
+    PRIVILEGES_REQUIRED_NONE
+    PRIVILEGES_REQUIRED_REPORTER
+    PRIVILEGES_REQUIRED_ASSIGNEE
+    PRIVILEGES_REQUIRED_EMPOWERED
 );
 
 @Bugzilla::Constants::EXPORT_OK = qw(contenttypes);
@@ -522,6 +527,15 @@ use constant PASSWORD_SALT_LENGTH => 8;
 # can be safely done or not based on the web server's URI length setting.
 use constant CGI_URI_LIMIT => 8000;
 
+# 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.
+
+use constant PRIVILEGES_REQUIRED_NONE      => 0;
+use constant PRIVILEGES_REQUIRED_REPORTER  => 1;
+use constant PRIVILEGES_REQUIRED_ASSIGNEE  => 2;
+use constant PRIVILEGES_REQUIRED_EMPOWERED => 3;
+
 sub bz_locations {
     # We know that Bugzilla/Constants.pm must be in %INC at this point.
     # So the only question is, what's the name of the directory
diff --git a/Bugzilla/Hook.pm b/Bugzilla/Hook.pm
index a5be1d38d..af73814ce 100644
--- a/Bugzilla/Hook.pm
+++ b/Bugzilla/Hook.pm
@@ -249,6 +249,66 @@ C<$changes-E<gt>{field} = [old, new]>
 
 =back
 
+=head2 bug_check_can_change_field
+
+This hook controls what fields users are allowed to change. You can add code here for 
+site-specific policy changes and other customizations. This hook is only
+executed if the field's new and old values differ. Any denies take priority over any allows. 
+So, if another extension denies a change but yours allows the change, the other extension's 
+deny will override your extension's allow.
+
+Params:
+
+=over
+
+=item C<bug>
+
+L<Bugzilla::Bug> - The current bug object that this field is changing on.
+
+=item C<field>
+
+The name (from the C<fielddefs> table) of the field that we are checking.
+
+=item C<new_value>
+
+The new value that the field is being changed to.
+
+=item C<old_value>
+
+The old value that the field is being changed from.
+
+=item C<priv_results>
+
+C<array> - This is how you explicitly allow or deny a change. You should only
+push something into this array if you want to explicitly allow or explicitly
+deny the change, and thus skip all other permission checks that would otherwise
+happen after this hook is called. If you don't care about the field change,
+then don't push anything into the array.
+
+The pushed value should be a choice from the following constants:
+
+=over
+
+=item C<PRIVILEGES_REQUIRED_NONE>
+
+No privileges required. This explicitly B<allows> a change.
+
+=item C<PRIVILEGES_REQUIRED_REPORTER>
+
+User is not the reporter, assignee or an empowered user, so B<deny>.
+
+=item C<PRIVILEGES_REQUIRED_ASSIGNEE>
+
+User is not the assignee or an empowered user, so B<deny>.
+
+=item C<PRIVILEGES_REQUIRED_EMPOWERED>
+
+User is not a sufficiently empowered user, so B<deny>.
+
+=back
+
+=back
+
 =head2 bug_fields
 
 Allows the addition of database fields from the bugs table to the standard
diff --git a/extensions/Example/Extension.pm b/extensions/Example/Extension.pm
index 6acf3e135..398ddbd56 100644
--- a/extensions/Example/Extension.pm
+++ b/extensions/Example/Extension.pm
@@ -29,6 +29,7 @@ use Bugzilla::Error;
 use Bugzilla::Group;
 use Bugzilla::User;
 use Bugzilla::Util qw(diff_arrays html_quote);
+use Bugzilla::Status qw(is_open_state);
 
 # This is extensions/Example/lib/Util.pm. I can load this here in my
 # Extension.pm only because I have a Config.pm.
@@ -587,6 +588,44 @@ sub template_before_process {
     }
 }
 
+sub bug_check_can_change_field {
+    my ($self, $args) = @_;
+
+    my ($bug, $field, $new_value, $old_value, $priv_results)
+        = @$args{qw(bug field new_value old_value priv_results)};
+
+    my $user = Bugzilla->user;
+
+    # Disallow a bug from being reopened if currently closed unless user 
+    # is in 'admin' group
+    if ($field eq 'bug_status' && $bug->product_obj->name eq 'Example') {
+        if (!is_open_state($old_value) && is_open_state($new_value) 
+            && !$user->in_group('admin')) 
+        {
+            push(@$priv_results, PRIVILEGES_REQUIRED_EMPOWERED);
+            return;
+        }
+    }
+
+    # Disallow a bug's keywords from being edited unless user is the
+    # reporter of the bug 
+    if ($field eq 'keywords' && $bug->product_obj->name eq 'Example' 
+        && $user->login ne $bug->reporter->login) 
+    {
+        push(@$priv_results, PRIVILEGES_REQUIRED_REPORTER);
+        return;
+    }
+
+    # Allow updating of priority even if user cannot normally edit the bug 
+    # and they are in group 'engineering'
+    if ($field eq 'priority' && $bug->product_obj->name eq 'Example'
+        && $user->in_group('engineering')) 
+    {
+        push(@$priv_results, PRIVILEGES_REQUIRED_NONE);
+        return;
+    }
+}
+
 sub webservice {
     my ($self, $args) = @_;
 
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index d9872f1b0..6bf904a94 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -775,9 +775,9 @@
       to <em>[% newvalue.join(', ') FILTER html %]</em>
     [% END %]
     , but only
-    [% IF privs < 3 %]
+    [% IF privs < constants.PRIVILEGES_REQUIRED_EMPOWERED %]
       the assignee
-      [% IF privs < 2 %] or reporter [% END %]
+      [% IF privs < constants.PRIVILEGES_REQUIRED_ASSIGNEE %] or reporter [% END %]
       of the [% terms.bug %], or
     [% END %]
     a user with the required permissions may change that field.