Bug 1317965 - Flag permission checks broken by bug 1257662 allowing unauthorized flag modification

2 files changed, 51 insertions, 17 deletions

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 3e7aa6819..6383ef364 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -768,23 +768,11 @@ sub _check_setter {
     # to the new flag status.
     my $status = $self->status;
 
-    # Make sure the user is authorized to modify flags, see bug 180879:
-    # - The flag exists and is unchanged.
-    # - The flag setter can unset flag.
-    # - Users in the request_group can clear pending requests
-    # - Users in the grant_group can set/cleari/request flags, including "+" and "-".
-    unless (($status eq $self->{_old_status})
-            || ($status eq 'X' && $setter->id == Bugzilla->user->id)
-            || (($status eq 'X' || $status eq '?')
-                && $setter->can_request_flag($self->type))
-            || $setter->can_unset_flag($self->type, $self->{_old_status})
-            || $setter->can_set_flag($self->type))
-    {
-        ThrowUserError('flag_update_denied',
-                        { name       => $self->type->name,
-                          status     => $status,
-                          old_status => $self->{_old_status} });
-    }
+    ThrowUserError('flag_update_denied',
+                   { name       => $self->type->name,
+                     status     => $status,
+                     old_status => $self->{_old_status} })
+        unless $setter->can_change_flag($self->type, $self->{_old_status} || 'X', $status);
 
     # If the request is being retargetted, we don't update
     # the setter, so that the setter gets the notification.
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 447c33c21..0eb9587eb 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -1583,6 +1583,52 @@ sub check_can_admin_flagtype {
     return wantarray ? ($flagtype, $can_fully_edit) : $flagtype;
 }
 
+sub can_change_flag {
+    my ($self, $flag_type, $old_status, $new_status) = @_;
+
+    # "old_status:new_status" => [OR conditions
+    state $flag_transitions = {
+        'X:-' => ['grant_group'],
+        'X:+' => ['grant_group'],
+        'X:?' => ['request_group'],
+
+        '?:X' => ['request_group', 'is_setter'],
+        '?:-' => ['grant_group'],
+        '?:+' => ['grant_group'],
+
+        '+:X' => ['grant_group'],
+        '+:-' => ['grant_group'],
+        '+:?' => ['grant_group'],
+
+        '-:X' => ['grant_group'],
+        '-:+' => ['grant_group'],
+        '-:?' => ['grant_group'],
+    };
+
+    return 1 if $new_status eq $old_status;
+
+    my $action = "$old_status:$new_status";
+    my %bool = (
+        request_group => $self->can_request_flag($flag_type),
+        grant_group   => $self->can_set_flag($flag_type),
+        is_setter     => $self->id == Bugzilla->user->id,
+    );
+
+    my $cond = $flag_transitions->{$action};
+    if ($cond) {
+        if (any { $bool{ $_ } } @$cond) {
+            return 1;
+        }
+        else {
+            return 0;
+        }
+    }
+    else {
+        warn "unknown flag transition blocked: $action";
+        return 0;
+    }
+}
+
 sub can_request_flag {
     my ($self, $flag_type) = @_;