diff options
| -rwxr-xr-x | editgroups.cgi | 254 | ||||
| -rw-r--r-- | template/en/default/admin/groups/edit.html.tmpl | 6 | ||||
| -rw-r--r-- | template/en/default/global/user-error.html.tmpl | 9 |
3 files changed, 162 insertions, 107 deletions
diff --git a/editgroups.cgi b/editgroups.cgi index 3eca512f9..818997114 100755 --- a/editgroups.cgi +++ b/editgroups.cgi @@ -22,6 +22,7 @@ # Joel Peshkin <bugreport@peshkin.net> # Jacob Steenhagen <jake@bugzilla.org> # Vlad Dascalu <jocuri@softhome.net> +# Frédéric Buclin <LpSolit@gmail.com> # Code derived from editowners.cgi and editusers.cgi @@ -33,6 +34,7 @@ use Bugzilla::Constants; require "CGI.pl"; my $cgi = Bugzilla->cgi; +my $dbh = Bugzilla->dbh; use vars qw($template $vars); @@ -71,21 +73,70 @@ sub RederiveRegexp ($$) } } -# TestGroup: check if the group name exists -sub TestGroup ($) -{ - my $group = shift; +# CheckGroupID checks that a positive integer is given and is +# actually a valid group ID. If all tests are successful, the +# trimmed group ID is returned. + +sub CheckGroupID { + my ($group_id) = @_; + $group_id = trim($group_id || 0); + ThrowUserError("group_not_specified") unless $group_id; + (detaint_natural($group_id) + && Bugzilla->dbh->selectrow_array("SELECT id FROM groups WHERE id = ?", + undef, $group_id)) + || ThrowUserError("invalid_group_ID"); + return $group_id; +} - # does the group exist? - SendSQL("SELECT name - FROM groups - WHERE name=" . SqlQuote($group)); - return FetchOneColumn(); +# This subroutine is called when: +# - a new group is created. CheckGroupName checks that its name +# is not empty and is not already used by any existing group. +# - an existing group is edited. CheckGroupName checks that its +# name has not been deleted or renamed to another existing +# group name (whose group ID is different from $group_id). +# In both cases, an error message is returned to the user if any +# test fails! Else, the trimmed group name is returned. + +sub CheckGroupName { + my ($name, $group_id) = @_; + $name = trim($name || ''); + trick_taint($name); + ThrowUserError("empty_group_name") unless $name; + my $excludeself = (defined $group_id) ? " AND id != $group_id" : ""; + my $name_exists = Bugzilla->dbh->selectrow_array("SELECT name FROM groups " . + "WHERE name = ? $excludeself", + undef, $name); + if ($name_exists) { + ThrowUserError("group_exists", { name => $name }); + } + return $name; } -# -# action='' -> No action specified, get a list. -# +# CheckGroupDesc checks that a non empty description is given. The +# trimmed description is returned. + +sub CheckGroupDesc { + my ($desc) = @_; + $desc = trim($desc || ''); + trick_taint($desc); + ThrowUserError("empty_group_description") unless $desc; + return $desc; +} + +# CheckGroupRegexp checks that the regular expression is valid +# (the regular expression being optional, the test is successful +# if none is given, as expected). The trimmed regular expression +# is returned. + +sub CheckGroupRegexp { + my ($regexp) = @_; + $regexp = trim($regexp || ''); + trick_taint($regexp); + ThrowUserError("invalid_regexp") unless (eval {qr/$regexp/}); + return $regexp; +} + +# If no action is specified, get a list of all groups available. unless ($action) { my @groups; @@ -124,14 +175,12 @@ unless ($action) { # if ($action eq 'changeform') { - my $gid = trim($cgi->param('group') || ''); - ThrowUserError("group_not_specified") unless ($gid); - detaint_natural($gid); - - SendSQL("SELECT id, name, description, userregexp, isactive, isbuggroup - FROM groups WHERE id = $gid"); - my ($group_id, $name, $description, $rexp, $isactive, $isbuggroup) - = FetchSQLData(); + # Check that an existing group ID is given + my $group_id = CheckGroupID($cgi->param('group')); + my ($name, $description, $regexp, $isactive, $isbuggroup) = + $dbh->selectrow_array("SELECT name, description, userregexp, " . + "isactive, isbuggroup " . + "FROM groups WHERE id = ?", undef, $group_id); # For each group, we use left joins to establish the existence of # a record making that group a member of this group @@ -175,7 +224,7 @@ if ($action eq 'changeform') { $vars->{'group_id'} = $group_id; $vars->{'name'} = $name; $vars->{'description'} = $description; - $vars->{'rexp'} = $rexp; + $vars->{'regexp'} = $regexp; $vars->{'isactive'} = $isactive; $vars->{'isbuggroup'} = $isbuggroup; $vars->{'groups'} = \@groups; @@ -208,33 +257,14 @@ if ($action eq 'add') { # if ($action eq 'new') { - # Cleanups and valididy checks - my $name = trim($cgi->param('name') || ''); - my $desc = trim($cgi->param('desc') || ''); - my $regexp = trim($cgi->param('regexp') || ''); - # convert an undefined value in the inactive field to zero - # (this occurs when the inactive checkbox is not checked - # and the browser does not send the field to the server) + # Check that a not already used group name is given, that + # a description is also given and check if the regular + # expression is valid (if any). + my $name = CheckGroupName($cgi->param('name')); + my $desc = CheckGroupDesc($cgi->param('desc')); + my $regexp = CheckGroupRegexp($cgi->param('regexp')); my $isactive = $cgi->param('isactive') ? 1 : 0; - # At this point $isactive is either 0 or 1 so we can mark it safe - trick_taint($isactive); - - ThrowUserError("empty_group_name") unless $name; - ThrowUserError("empty_group_description") unless $desc; - - if (TestGroup($name)) { - ThrowUserError("group_exists", { name => $name }); - } - - ThrowUserError("invalid_regexp") unless (eval {qr/$regexp/}); - - # We use SqlQuote and FILTER html on name, description and regexp. - # So they are safe to be detaint - trick_taint($name); - trick_taint($desc); - trick_taint($regexp); - # Add the new group SendSQL("INSERT INTO groups ( " . "name, description, isbuggroup, userregexp, isactive, last_changed " . @@ -280,18 +310,16 @@ if ($action eq 'new') { # if ($action eq 'del') { - my $gid = trim($cgi->param('group') || ''); - ThrowUserError("group_not_specified") unless ($gid); - detaint_natural($gid); - - SendSQL("SELECT id FROM groups WHERE id=$gid"); - ThrowUserError("invalid_group_ID") unless FetchOneColumn(); - - SendSQL("SELECT name,description " . - "FROM groups " . - "WHERE id = $gid"); - - my ($name, $desc) = FetchSQLData(); + # Check that an existing group ID is given + my $gid = CheckGroupID($cgi->param('group')); + my ($name, $desc, $isbuggroup) = + $dbh->selectrow_array("SELECT name, description, isbuggroup " . + "FROM groups WHERE id = ?", undef, $gid); + + # System groups cannot be deleted! + if (!$isbuggroup) { + ThrowUserError("system_group_not_deletable", { name => $name }); + } my $hasusers = 0; SendSQL("SELECT user_id FROM user_group_map @@ -348,12 +376,16 @@ if ($action eq 'del') { # if ($action eq 'delete') { - my $gid = trim($cgi->param('group') || ''); - ThrowUserError("group_not_specified") unless ($gid); - detaint_natural($gid); - - SendSQL("SELECT name FROM groups WHERE id = $gid"); - my ($name) = FetchSQLData(); + # Check that an existing group ID is given + my $gid = CheckGroupID($cgi->param('group')); + my ($name, $isbuggroup) = + $dbh->selectrow_array("SELECT name, isbuggroup FROM groups " . + "WHERE id = ?", undef, $gid); + + # System groups cannot be deleted! + if (!$isbuggroup) { + ThrowUserError("system_group_not_deletable", { name => $name }); + } my $cantdelete = 0; @@ -424,14 +456,14 @@ if ($action eq 'postchanges') { $action = 3; } - my ($gid, $chgs) = doGroupChanges(); + my ($gid, $chgs, $name, $regexp) = doGroupChanges(); $vars->{'action'} = $action; $vars->{'changes'} = $chgs; $vars->{'gid'} = $gid; - $vars->{'name'} = $cgi->param('name'); + $vars->{'name'} = $name; if ($action == 2) { - $vars->{'regexp'} = $cgi->param("rexp"); + $vars->{'regexp'} = $regexp; } print Bugzilla->cgi->header(); @@ -441,15 +473,12 @@ if ($action eq 'postchanges') { } if (($action eq 'remove_all_regexp') || ($action eq 'remove_all')) { - # remove all explicit users from the group with gid $cgi->param('group') - # that match the regexp stored in the DB for that group - # or all of them period + # remove all explicit users from the group with + # gid = $cgi->param('group') that match the regular expression + # stored in the DB for that group or all of them period - my $gid = $cgi->param('group'); - ThrowUserError("group_not_specified") unless ($gid); - detaint_natural($gid); + my $gid = CheckGroupID($cgi->param('group')); - my $dbh = Bugzilla->dbh; my $sth = $dbh->prepare("SELECT name, userregexp FROM groups WHERE id = ?"); $sth->execute($gid); @@ -513,39 +542,59 @@ ThrowCodeError("action_unrecognized", $vars); # Helper sub to handle the making of changes to a group sub doGroupChanges { my $cgi = Bugzilla->cgi; - - my $gid = trim($cgi->param('group') || ''); - ThrowUserError("group_not_specified") unless ($gid); - detaint_natural($gid); - + my $dbh = Bugzilla->dbh; + my $sth; + + $dbh->do("LOCK TABLES groups WRITE, group_group_map WRITE, + user_group_map WRITE, profiles READ, + namedqueries READ, whine_queries READ"); + + # Check that the given group ID and regular expression are valid. + # If tests are successful, trimmed values are returned by CheckGroup*. + my $gid = CheckGroupID($cgi->param('group')); + my $regexp = CheckGroupRegexp($cgi->param('regexp')); + + # The name and the description of system groups cannot be edited. + # We then need to know if the group being edited is a system group. SendSQL("SELECT isbuggroup FROM groups WHERE id = $gid"); my ($isbuggroup) = FetchSQLData(); + my $name; + my $desc; + my $isactive; my $chgs = 0; - if (($isbuggroup == 1) && ($cgi->param('oldname') ne $cgi->param("name"))) { - $chgs = 1; - SendSQL("UPDATE groups SET name = " . - SqlQuote($cgi->param("name")) . " WHERE id = $gid"); - } - if (($isbuggroup == 1) && ($cgi->param('olddesc') ne $cgi->param("desc"))) { - $chgs = 1; - SendSQL("UPDATE groups SET description = " . - SqlQuote($cgi->param("desc")) . " WHERE id = $gid"); - } - if ($cgi->param("oldrexp") ne $cgi->param("rexp")) { - $chgs = 1; - - my $rexp = $cgi->param('rexp'); - ThrowUserError("invalid_regexp") unless (eval {qr/$rexp/}); - - SendSQL("UPDATE groups SET userregexp = " . - SqlQuote($rexp) . " WHERE id = $gid"); - RederiveRegexp($::FORM{"rexp"}, $gid); + # We trust old values given by the template. If they are hacked + # in a way that some of the tests below become negative, the + # corresponding attributes are not updated in the DB, which does + # not hurt. + if ($isbuggroup) { + # Check that the group name and its description are valid + # and return trimmed values if tests are successful. + $name = CheckGroupName($cgi->param('name'), $gid); + $desc = CheckGroupDesc($cgi->param('desc')); + $isactive = $cgi->param('isactive') ? 1 : 0; + + if ($name ne $cgi->param('oldname')) { + $chgs = 1; + $sth = $dbh->do("UPDATE groups SET name = ? WHERE id = ?", + undef, $name, $gid); + } + if ($desc ne $cgi->param('olddesc')) { + $chgs = 1; + $sth = $dbh->do("UPDATE groups SET description = ? WHERE id = ?", + undef, $desc, $gid); + } + if ($isactive ne $cgi->param('oldisactive')) { + $chgs = 1; + $sth = $dbh->do("UPDATE groups SET isactive = ? WHERE id = ?", + undef, $isactive, $gid); + } } - if (($isbuggroup == 1) && ($cgi->param("oldisactive") ne $cgi->param("isactive"))) { + if ($regexp ne $cgi->param('oldregexp')) { $chgs = 1; - SendSQL("UPDATE groups SET isactive = " . - SqlQuote($cgi->param("isactive")) . " WHERE id = $gid"); + $sth = $dbh->do("UPDATE groups SET userregexp = ? WHERE id = ?", + undef, $regexp, $gid); + RederiveRegexp($regexp, $gid); } foreach my $b (grep {/^oldgrp-\d*$/} $cgi->param()) { @@ -602,5 +651,6 @@ sub doGroupChanges { # mark the changes SendSQL("UPDATE groups SET last_changed = NOW() WHERE id = $gid"); } - return $gid, $chgs; + $dbh->do("UNLOCK TABLES"); + return $gid, $chgs, $name, $regexp; } diff --git a/template/en/default/admin/groups/edit.html.tmpl b/template/en/default/admin/groups/edit.html.tmpl index d6044ad0f..92b8e9c2e 100644 --- a/template/en/default/admin/groups/edit.html.tmpl +++ b/template/en/default/admin/groups/edit.html.tmpl @@ -26,7 +26,7 @@ # group_id: number. The group ID. # name: string. The name of the group. [grantor] # description: string. The description of the group. - # rexp: string. The regular expression for the users of the group. + # regexp: string. The regular expression for the users of the group. # isactive: boolean int. Shows if the group is still active. # isbuggroup: boolean int. Is 1 if this is a bug group. # groups: array with group objects having the properties: @@ -83,8 +83,8 @@ <tr> <th>User Regexp:</th> <td> - <input type="hidden" name="oldrexp" value="[% rexp FILTER html %]"> - <input type="text" name="rexp" size="40" value="[% rexp FILTER html %]"> + <input type="hidden" name="oldregexp" value="[% regexp FILTER html %]"> + <input type="text" name="regexp" size="40" value="[% regexp FILTER html %]"> </td> </tr> diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 41661886c..5c293bd2b 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -309,11 +309,11 @@ [% ELSIF error == "empty_group_description" %] [% title = "The group description can not be empty" %] - You must enter a description for the new group. + You must enter a description for the group. [% ELSIF error == "empty_group_name" %] [% title = "The group name can not be empty" %] - You must enter a name for the new group. + You must enter a name for the group. [% ELSIF error == "entry_access_denied" %] [% title = "Permission Denied" %] @@ -402,6 +402,11 @@ [% title = "Group not specified" %] No group was specified. + [% ELSIF error == "system_group_not_deletable" %] + [% title = "System Groups not deletable" %] + <em>[% name FILTER html %]</em> is a system group. + This group cannot be deleted. + [% ELSIF error == "group_unknown" %] [% title = "Unknown Group" %] The group [% name FILTER html %] does not exist. Please specify |