summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authortravis%sedsystems.ca <>2005-02-17 01:25:53 +0100
committertravis%sedsystems.ca <>2005-02-17 01:25:53 +0100
commit8a225c2784f6343cd3958b2baabab5d462b46310 (patch)
tree7f156d27281486de65f2847beffaeb7b41354605
parentea7030a701bfdb503df542c3d7e814b8ea702929 (diff)
downloadbugzilla-8a225c2784f6343cd3958b2baabab5d462b46310.tar.gz
bugzilla-8a225c2784f6343cd3958b2baabab5d462b46310.tar.xz
Bug 277768 : Some validations are missing when editing groups
Patch by Frederic Buclin <LpSolit@gmail.com> r=wurblzap a=justdave
-rwxr-xr-xeditgroups.cgi254
-rw-r--r--template/en/default/admin/groups/edit.html.tmpl6
-rw-r--r--template/en/default/global/user-error.html.tmpl9
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