Patch for bug 180879: Implement privs for bug flags modification; patch by Frédéric Buclin <LpSolit@netscape.net>, r=joel, a=justdave.

8 files changed, 154 insertions, 9 deletions

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index a58bc7e3a..0fd4b047f 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -139,6 +139,7 @@ sub count {
 sub validate {
     # Validates fields containing flag modifications.
 
+    my $user = Bugzilla->user;
     my ($data, $bug_id) = @_;
   
     # Get a list of flags to validate.  Uses the "map" function
@@ -213,6 +214,24 @@ sub validate {
                 }
             }
         }
+
+        # Make sure the user is authorized to modify flags, see bug 180879
+        # - The flag is unchanged
+        next if ($status eq $flag->{status});
+
+        # - User can clear flags set by itself
+        next if (($status eq "X") && ($user->id eq $flag->{setter}));
+
+        # - User in the $grant_gid group can set/clear flags,
+        #   including "+" and "-"
+        next if (!$flag->{type}->{grant_gid}
+                 || $user->in_group(&::GroupIdToName($flag->{type}->{grant_gid})));
+
+        # - Any other flag modification is denied
+        ThrowUserError("flag_update_denied",
+                        { name       => $flag->{type}->{name},
+                          status     => $status,
+                          old_status => $flag->{status} });
     }
 }
 
diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm
index 687a01768..5b681dc0c 100644
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -49,7 +49,8 @@ my @base_columns =
   ("1", "flagtypes.id", "flagtypes.name", "flagtypes.description", 
    "flagtypes.cc_list", "flagtypes.target_type", "flagtypes.sortkey", 
    "flagtypes.is_active", "flagtypes.is_requestable", 
-   "flagtypes.is_requesteeble", "flagtypes.is_multiplicable");
+   "flagtypes.is_requesteeble", "flagtypes.is_multiplicable", 
+   "flagtypes.grant_group_id", "flagtypes.request_group_id");
 
 # Note: when adding tables to @base_tables, make sure to include the separator 
 # (i.e. a comma or words like "LEFT OUTER JOIN") before the table name, 
@@ -181,6 +182,7 @@ sub count {
 }
 
 sub validate {
+    my $user = Bugzilla->user;
     my ($data, $bug_id, $attach_id) = @_;
   
     # Get a list of flag types to validate.  Uses the "map" function
@@ -249,6 +251,22 @@ sub validate {
                                  attach_id => $attach_id });
             }
         }
+
+        # Make sure the user is authorized to modify flags, see bug 180879
+        # - User in the $grant_gid group can set flags, including "+" and "-"
+        next if (!$flag_type->{grant_gid}
+                 || $user->in_group(&::GroupIdToName($flag_type->{grant_gid})));
+
+        # - User in the $request_gid group can request flags
+        next if ($status eq '?'
+                 && (!$flag_type->{request_gid}
+                     || $user->in_group(&::GroupIdToName($flag_type->{request_gid}))));
+
+        # - Any other flag modification is denied
+        ThrowUserError("flag_update_denied",
+                        { name       => $flag_type->{name},
+                          status     => $status,
+                          old_status => "X" });
     }
 }
 
@@ -348,6 +366,12 @@ sub sqlify_criteria {
         push(@$columns, "COUNT(flagexclusions.type_id) AS num_exclusions");
         $$having = "num_exclusions = 0";
     }
+    if ($criteria->{group}) {
+        my $gid = $criteria->{group};
+        detaint_natural($gid);
+        push(@criteria, "(flagtypes.grant_group_id = $gid " .
+                        " OR flagtypes.request_group_id = $gid)");
+    }
     
     return @criteria;
 }
@@ -368,7 +392,9 @@ sub perlify_record {
     $type->{'is_requestable'} = $_[8];
     $type->{'is_requesteeble'} = $_[9];
     $type->{'is_multiplicable'} = $_[10];
-    $type->{'flag_count'} = $_[11];
+    $type->{'grant_gid'} = $_[11];
+    $type->{'request_gid'} = $_[12];
+    $type->{'flag_count'} = $_[13];
         
     return $type;
 }
diff --git a/checksetup.pl b/checksetup.pl
index 710140191..c8af512ba 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -1685,7 +1685,9 @@ $table{flagtypes} =
     is_requesteeble     TINYINT       NOT NULL  DEFAULT 0 , 
     is_multiplicable    TINYINT       NOT NULL  DEFAULT 0 , 
     
-    sortkey             SMALLINT      NOT NULL  DEFAULT 0 
+    sortkey             SMALLINT      NOT NULL  DEFAULT 0 , 
+    grant_group_id      MEDIUMINT     NULL , 
+    request_group_id    MEDIUMINT     NULL
    ';
 
 $table{flaginclusions} =
@@ -4124,6 +4126,11 @@ if (GetFieldDef("group_group_map", "isbless")) {
 # login data source
 AddField("profiles", "extern_id", "varchar(64)");
 
+# 2004-11-20 - LpSolit@netscape.net - Bug 180879
+# Add grant and request groups for flags
+AddField('flagtypes', 'grant_group_id', 'mediumint null');
+AddField('flagtypes', 'request_group_id', 'mediumint null');
+
 # If you had to change the --TABLE-- definition in any way, then add your
 # differential change code *** A B O V E *** this comment.
 #
diff --git a/editflagtypes.cgi b/editflagtypes.cgi
index a14f75680..8b51be326 100755
--- a/editflagtypes.cgi
+++ b/editflagtypes.cgi
@@ -90,9 +90,12 @@ exit;
 
 sub list {
     # Define the variables and functions that will be passed to the UI template.
-    $vars->{'bug_types'} = Bugzilla::FlagType::match({ 'target_type' => 'bug' }, 1);
+    $vars->{'bug_types'} = 
+      Bugzilla::FlagType::match({ 'target_type' => 'bug', 
+                                  'group' => $::FORM{'group'} }, 1);
     $vars->{'attachment_types'} = 
-      Bugzilla::FlagType::match({ 'target_type' => 'attachment' }, 1);
+      Bugzilla::FlagType::match({ 'target_type' => 'attachment', 
+                                  'group' => $::FORM{'group'} }, 1);
 
     # Return the appropriate HTTP response headers.
     print Bugzilla->cgi->header();
@@ -129,6 +132,13 @@ sub edit {
         $vars->{'type'} = Bugzilla::FlagType::get($::FORM{'id'});
         $vars->{'type'}->{'inclusions'} = Bugzilla::FlagType::get_inclusions($::FORM{'id'});
         $vars->{'type'}->{'exclusions'} = Bugzilla::FlagType::get_exclusions($::FORM{'id'});
+        # Users want to see group names, not IDs
+        foreach my $group ("grant_gid", "request_gid") {
+            my $gid = $vars->{'type'}->{$group};
+            next if (!$gid);
+            SendSQL("SELECT name FROM groups WHERE id = $gid");
+            $vars->{'type'}->{$group} = FetchOneColumn();
+        }
     }
     # Otherwise set the target type (the minimal information about the type
     # that the template needs to know) from the URL parameter and default
@@ -208,6 +218,7 @@ sub insert {
     validateIsRequestable();
     validateIsRequesteeble();
     validateAllowMultiple();
+    validateGroups();
     
     my $name = SqlQuote($::FORM{'name'});
     my $description = SqlQuote($::FORM{'description'});
@@ -224,11 +235,13 @@ sub insert {
     # Insert a record for the new flag type into the database.
     SendSQL("INSERT INTO flagtypes (id, name, description, cc_list, 
                  target_type, sortkey, is_active, is_requestable, 
-                 is_requesteeble, is_multiplicable) 
+                 is_requesteeble, is_multiplicable, 
+                 grant_group_id, request_group_id) 
              VALUES ($id, $name, $description, $cc_list, '$target_type', 
                  $::FORM{'sortkey'}, $::FORM{'is_active'}, 
                  $::FORM{'is_requestable'}, $::FORM{'is_requesteeble'}, 
-                 $::FORM{'is_multiplicable'})");
+                 $::FORM{'is_multiplicable'}, $::FORM{'grant_gid'}, 
+                 $::FORM{'request_gid'})");
     
     # Populate the list of inclusions/exclusions for this flag type.
     foreach my $category_type ("inclusions", "exclusions") {
@@ -267,6 +280,7 @@ sub update {
     validateIsRequestable();
     validateIsRequesteeble();
     validateAllowMultiple();
+    validateGroups();
     
     my $name = SqlQuote($::FORM{'name'});
     my $description = SqlQuote($::FORM{'description'});
@@ -282,7 +296,9 @@ sub update {
                      is_active = $::FORM{'is_active'} , 
                      is_requestable = $::FORM{'is_requestable'} , 
                      is_requesteeble = $::FORM{'is_requesteeble'} , 
-                     is_multiplicable = $::FORM{'is_multiplicable'} 
+                     is_multiplicable = $::FORM{'is_multiplicable'} , 
+                     grant_group_id = $::FORM{'grant_gid'} , 
+                     request_group_id = $::FORM{'request_gid'} 
               WHERE  id = $::FORM{'id'}");
     
     # Update the list of inclusions/exclusions for this flag type.
@@ -499,3 +515,16 @@ sub validateAllowMultiple {
     $::FORM{'is_multiplicable'} = $::FORM{'is_multiplicable'} ? 1 : 0;
 }
 
+sub validateGroups {
+    # Convert group names to group IDs
+    foreach my $col ("grant_gid", "request_gid") {
+      my $name = $::FORM{$col};
+      $::FORM{$col} ||= "NULL";
+      next if (!$name);
+      SendSQL("SELECT id FROM groups WHERE name = " . SqlQuote($name));
+      $::FORM{$col} = FetchOneColumn();
+      if (!$::FORM{$col}) {
+        ThrowUserError("group_unknown", { name => $name });
+      }
+    }
+}
diff --git a/editgroups.cgi b/editgroups.cgi
index b3b6135e4..bc22d518e 100755
--- a/editgroups.cgi
+++ b/editgroups.cgi
@@ -317,12 +317,20 @@ if ($action eq 'del') {
         $hasproduct = 1;
     }
 
+    my $hasflags = 0;
+    SendSQL("SELECT id FROM flagtypes 
+             WHERE grant_group_id = $gid OR request_group_id = $gid");
+    if (FetchOneColumn()) {
+        $hasflags = 1;
+    }
+
     $vars->{'gid'}         = $gid;
     $vars->{'name'}        = $name;
     $vars->{'description'} = $desc;
     $vars->{'hasusers'}    = $hasusers;
     $vars->{'hasbugs'}     = $hasbugs;
     $vars->{'hasproduct'}  = $hasproduct;
+    $vars->{'hasflags'}    = $hasflags;
     $vars->{'buglist'}     = $buglist;
 
     print Bugzilla->cgi->header();
@@ -365,8 +373,19 @@ if ($action eq 'delete') {
             $cantdelete = 1;
         }
     }
+    SendSQL("SELECT id FROM flagtypes 
+             WHERE grant_group_id = $gid OR request_group_id = $gid");
+    if (FetchOneColumn()) {
+        if (!defined $cgi->param('removeflags')) {
+            $cantdelete = 1;
+        }
+    }
 
     if (!$cantdelete) {
+        SendSQL("UPDATE flagtypes SET grant_group_id = NULL 
+                 WHERE grant_group_id = $gid");
+        SendSQL("UPDATE flagtypes SET request_group_id = NULL 
+                 WHERE request_group_id = $gid");
         SendSQL("DELETE FROM user_group_map WHERE group_id = $gid");
         SendSQL("DELETE FROM group_group_map WHERE grantor_id = $gid");
         SendSQL("DELETE FROM bug_group_map WHERE group_id = $gid");
diff --git a/template/en/default/admin/flag-type/edit.html.tmpl b/template/en/default/admin/flag-type/edit.html.tmpl
index 1faaaf3b8..253a310ac 100644
--- a/template/en/default/admin/flag-type/edit.html.tmpl
+++ b/template/en/default/admin/flag-type/edit.html.tmpl
@@ -188,6 +188,25 @@
     </tr>
 
     <tr>
+      <th>Grant Group:</th>
+      <td>
+        the group allowed to grant/deny flags of this type
+        (to allow all users to grant/deny these flags, leave this empty)<br>
+        <input type="text" name="grant_gid" value="[% type.grant_gid FILTER html %]" size="50" maxlength="255">
+      </td>
+    </tr>
+
+    <tr>
+      <th>Request Group:</th>
+      <td>
+        if flags of this type are requestable, the group allowed to request them
+        (to allow all users to request these flags, leave this empty)<br>
+        Note that the request group alone has no effect if the grant group is not defined!<br>
+        <input type="text" name="request_gid" value="[% type.request_gid FILTER html %]" size="50" maxlength="255">
+      </td>
+    </tr>
+
+    <tr>
       <th></th>
       <td>
         <input type="submit" value="        
diff --git a/template/en/default/admin/groups/delete.html.tmpl b/template/en/default/admin/groups/delete.html.tmpl
index 905f68cf3..842e2c6f1 100644
--- a/template/en/default/admin/groups/delete.html.tmpl
+++ b/template/en/default/admin/groups/delete.html.tmpl
@@ -29,6 +29,7 @@
   # hasusers: boolean int. True if the group includes users in it.
   # hasbugs: boolean int. True if the group includes bugs in it.
   # hasproduct: boolean int. True if the group is binded to a product.
+  # hasflags: boolean int. True if the group is used by a flag type.
   # buglist: string. The list of bugs included in this group.
   #%]
 
@@ -81,11 +82,20 @@
     <br><input type="checkbox" name="unbind">Delete this group anyway,
     and make the <U>[% name FILTER html %]</U> publicly visible.</p>
   [% END %]
+  
+  [% IF hasflags %]
+    <p><b>This group restricts who can make changes to flags of certain types.
+    You cannot delete this group while there are flag types using it.</b>
+
+    <br><a href="editflagtypes.cgi?action=list&group=[% gid FILTER html %]">Show
+    me which types</a> - <input type="checkbox" name="removeflags">Remove all
+    flag types from this group for me.</p>
+  [% END %]
 
   <h2>Confirmation</h2>
 
   <p>Do you really want to delete this group?</p>
-  [% IF (hasusers || hasbugs || hasproduct) %]
+  [% IF (hasusers || hasbugs || hasproduct || hasflags) %]
     <p><b>You must check all of the above boxes or correct the
           indicated problems first before you can proceed.</b></p>
   [% END %]
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 93c50b85a..c92824ad3 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -344,6 +344,17 @@
     [% title = "Flag Type Name Invalid" %]
     The name <em>[% name FILTER html %]</em> must be 1-50 characters long.
 
+  [% ELSIF error == "flag_update_denied" %]
+    [% title = "Flag Modification Denied" %]
+    You tried to [% IF status == "+" %] grant [% ELSIF status == "-" %] deny
+    [% ELSIF status == "X" %] clear [% ELSE %] request [% END %]
+    <code>[% name FILTER html %]</code>
+    [% IF status == "?" && old_status != "X" %], but this flag is already
+    set[% END %].
+    Only a sufficiently empowered user [% IF status == "X" %] or the user who
+    set <code>[% name FILTER html %][% old_status FILTER html %]</code> in
+    the first place [% END %] can make this change.
+
   [% ELSIF error == "format_not_found" %]
     [% title = "Format Not Found" %]
     The requested format <em>[% format FILTER html %]</em> does not exist with
@@ -362,6 +373,11 @@
     [% title = "Group not specified" %]
     No group was specified.
 
+  [% ELSIF error == "group_unknown" %]
+    [% title = "Unknown Group" %]
+    The group [% name FILTER html %] does not exist. Please specify
+    a valid group name. Create it first if necessary!
+
   [% ELSIF error == "illegal_at_least_x_votes" %]
     [% title = "Your Search Makes No Sense" %]
     The <em>At least ___ votes</em> field must be a simple number.