277768 - Some validations are missing when editing groups

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

When editing an already existing group from editgroups.cgi, no validation is
made to check if the name has been deleted or is already used by another group.
In the same way, we should check that a description is also given and that the
regexp is valid (if defined). Finally, we should always check that the given
group ID is not only defined but is also valid! This will prevent many SQL
errors and unexpected behavior. Patch coming...

This will fix some bugs listed in the dependeny list.

Note that this bug applies to both the 2.18 branch and the trunk and should be
fixed on both.

Comment 1

[Frédéric Buclin]

Attachment: patch for the trunk, v1

Checks are centralized in CheckGroup*() subroutines.

Comment 2

[Vlad Dascalu]

Comment on attachment 170828 [details] [diff] [review]
patch for the trunk, v1

-sub TestGroup ($)
-{
-    my $group = shift;
+# Check if the group ID exists
+sub CheckGroupID {
+    my $gid = trim($cgi->param('group') || 0);

We shouldn't have $cgi->param('group') inside a subroutine. We should make the
subroutine as generic as possible. It should receive a parameter with the group
that should be validated, (or, in a more generic way, with the value upon which
processing is done). This will allow better code modularization and easier unit
testing upon specific routines.

-    <td><input size="30" name="regexp"></td>
+    <td><input size="30" name="rexp"></td>

Unless there is a valid reason, I'd prefer to leave those because it really
creates incompatibilities between templates and creates headaches for
translators to forward-port their templates to new Bugzilla versions.

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
> -    <td><input size="30" name="regexp"></td>
> +    <td><input size="30" name="rexp"></td>
> 
> Unless there is a valid reason, I'd prefer to leave those because it really
> creates incompatibilities between templates and creates headaches for
> translators to forward-port their templates to new Bugzilla versions.

The reason for changing it is to have the same name for the regexp field in both
admin/groups/edit.html.tmpl and admin/groups/create.html.tmpl. I think it makes
things easier to have the same name for the same fields in different files.

Comment 4

[Dave Miller [:justdave]]

For the HTML form field name, I don't think it matters which we use.  Having
them both consistant would be nice.  Just make sure you don't rename the perl
variable that holds it to the shorter version.  That one needs to contain
'regexp' somewhere in the variable name or the tests will yell at you for
putting a variable in a regular expression without quoting it.

Comment 5

[Max Kanat-Alexander]

Don't change the names at this point, even if it makes things more consistent.
We can change the name on the tip after we unfreeze, but not during a freeze.

Comment 6

[Frédéric Buclin]

Attachment: patch for the trunk, v2

This patch takes into account vladd's comment about parameters, still renames a
field from rexp to regexp and now prevents you from deleting a system group.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 171089 [details] [diff] [review]
patch for the trunk, v2

>+# Check if the group ID exists
>+sub CheckGroupID {
>+    my ($gid) = @_;

>+    $gid = trim($gid || 0);
>+    ThrowUserError("group_not_specified") unless $gid;

  That hides a CodeError, if somebody doesn't pass in $gid.

>+    my $isok = 0;
>+    if (detaint_natural($gid)) {

  *nit* This code path is confusing. It took me a minute to figure out that if
$gid isn't correctly detainted, $isok will still be 0. 

>+        SendSQL("SELECT id FROM groups WHERE id = $gid");

  SendSQL must *not* be used in brand-new code. Instead, create a $dbh handle
at the beginning of the function that points to Bugzilla->dbh and use that.

>+        $isok = FetchOneColumn() || 0;

  That || 0 is redundant. It will be undef if it doesn't return anything.

>+    }
>+    ThrowUserError("invalid_group_ID") unless $isok;
>+    return $gid;

  Why does it return $gid? If it's supposed to validate a $gid, the subroutine
name should definitely be different.

>+# Check if the group name is set and not already used
>+sub CheckGroupName {
>+    my ($name, $gid) = @_;

  You'll have to document the purpose of the $gid parameter here, I don't
understand what it's doing.

>+    $name = trim($name || '');
>+    ThrowUserError("empty_group_name") unless $name;

  Once again, this hides a CodeError if somebody doesn't pass in $name.

>+    my $excludeself = (defined $gid) ? " AND id != $gid" : "";
>+    SendSQL("SELECT name FROM groups WHERE name = " .
>+             SqlQuote($name) . $excludeself);
>+    if (FetchOneColumn()) {
>+        ThrowUserError("group_exists", { name => $name });
>+    }

  Once again, that deprecated DB.pm code must go.

>+# Check if the description is set
>+sub CheckGroupDesc {
>+    my ($desc) = @_;
>+    $desc = trim($desc || '');
>+    ThrowUserError("empty_group_description") unless $desc;

  Hiding a CodeError again.

>+    my $group_id = CheckGroupID($cgi->param('group'));
>+    SendSQL("SELECT name, description, userregexp, isactive, isbuggroup
>+             FROM groups WHERE id = $group_id");
>+    my ($name, $description, $rexp, $isactive, $isbuggroup) 
>         = FetchSQLData();

  This code can stay, since code that you're *not* touching still uses the
deprecated code. But you can also change it over to using $dbh, and I wouldn't
object.

>+    my $name = CheckGroupName($cgi->param('name'));
>+    my $desc = CheckGroupDesc($cgi->param('desc'));
>+    my $regexp = CheckGroupRegexp($cgi->param('regexp'));

  Oh, I do like this better than the old code. It's much cleaner. :-)

>-    my $gid = trim($cgi->param('group') || '');
>-    ThrowUserError("group_not_specified") unless ($gid);
>-    detaint_natural($gid);
>+    my $gid = CheckGroupID($cgi->param('group'));
>+    SendSQL("SELECT name, description, isbuggroup " .
>+            "FROM groups WHERE id = $gid");
>+    my ($name, $desc, $isbuggroup) = FetchSQLData();

  However, this SendSQL code should go.

>+    if (!$isbuggroup) {
>+        ThrowUserError("group_not_deletable", { name => $name });
>+    }

  *nit* Can we call that variable something like "is_user_created_group"? It
would be clearer. Any how, there should be underscores instead of
mashedtogethernames, for international readability.

> if ($action eq 'delete') {
>-    my $gid = trim($cgi->param('group') || '');
>-    ThrowUserError("group_not_specified") unless ($gid);
>-    detaint_natural($gid);
>+    my $gid = CheckGroupID($cgi->param('group'));
>+    SendSQL("SELECT name, isbuggroup FROM groups WHERE id = $gid");
>+    my ($name, $isbuggroup) = FetchSQLData();

  SendSQL.

>+    SendSQL("LOCK TABLES groups WRITE, group_group_map WRITE, 
>+             user_group_map WRITE, profiles READ, 
>+             namedqueries READ, whine_queries READ");

  $dbh->do()

>+    if ($isbuggroup) {
>+        if ($name ne $cgi->param('oldname')) {
>+            $chgs = 1;
>+            SendSQL("UPDATE groups SET name = " . SqlQuote($name) .
>+                    " WHERE id = $gid");

  SendSQL.

>+        if ($desc ne $cgi->param('olddesc')) {
>+            $chgs = 1;
>+            SendSQL("UPDATE groups SET description = " . SqlQuote($desc) .
>+                    " WHERE id = $gid");

  SendSQL.

>+        if ($isactive ne $cgi->param('oldisactive')) {
>+            $chgs = 1;
>+            SendSQL("UPDATE groups SET isactive = $isactive WHERE id = $gid");
>+        }

  SendSQL.

>-        SendSQL("UPDATE groups SET isactive = " . 
>-            SqlQuote($cgi->param("isactive")) . " WHERE id = $gid");
>+        SendSQL("UPDATE groups SET userregexp = " . SqlQuote($regexp) .
>+                " WHERE id = $gid");
>+        RederiveRegexp($regexp, $gid);

  SendSQL.

>-    return $gid, $chgs;
>+    SendSQL("UNLOCK TABLES");

  SendSQL.

>     <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="[% rexp FILTER html %]">
>+        <input type="text" name="regexp" size="40" value="[% rexp FILTER html %]">

  This is a ridiculous reason to break localizations on a frozen branch. But if
justdave says that's OK on the branch, then that's OK...

>+  [% ELSIF error == "group_not_deletable" %]
>+    [% title = "System Groups not deletable" %]
>+    <em>[% name FILTER html %]</em> is a system group.
>+    This group cannot be deleted.
>+

  *nit* The name should probably be group_system_not_deletable or
system_group_not_deleteable

  I do think that this whole method of doing things is generally cleaner than
what we were doing before, though.

Comment 8

[Frédéric Buclin]

Attachment: patch for the 2.18 branch, v1

Comment 9

[Dave Miller [:justdave]]

This definitely needs doing, but it's something that won't bother most folks
(we've been living with it since 2.17.3 with no major problems) so I won't block
release on it.  I will take it on the branch when it's ready though.

Comment 10

[Frédéric Buclin]

Attachment: patch for the trunk, v3

mkanat, new patch following the long discussion we had yesterday.... ;)

Comment 11

[Max Kanat-Alexander]

Comment on attachment 171689 [details] [diff] [review]
patch for the trunk, v3

>+# 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.

  Yay!! This is much better. :-)

>+# CheckGroupName checks that a non empty group name is given and
>+# is not already used by another group (useful when a new group is
>+# created or when a group is renamed). The group ID is used when
>+# renaming a group as the group name needs to be different from
>+# *other* group names. It returns the trimmed group name.

  *nit* I'm still very confused!! This is a very confusing subroutine. I looked
at the code, and here's what I THINK this function is doing:

  A) If the group name exists, throw an error.
  B) If the group name doesn't exist, return it, trimmed.
  C) If you specify an $gid, then if $name is already assigned to $gid, we're
OK.

  I understand the purpose of C, but it just seems like there's some other,
easier way to do it, outside of this subroutine. At the least, explain that the
intended purpose of C is for when you are editing a group, and you don't change
the name. Also, change the name of the $gid parameter to something more
intuitive.

>+sub CheckGroupName {
>+    my ($name, $gid) = @_;

  Each subroutine needs a "my $dbh = Bugzilla->dbh"

>+    if ($isbuggroup) {
>+        if ($name ne $cgi->param('oldname')) {
>+            $chgs = 1;
>+            trick_taint($name);
>+            $sth = $dbh->prepare("UPDATE groups SET name = ? WHERE id = ?");
>+            $sth->execute($name, $gid);

  *nit* Generally, UPDATE statements should just be a $dbh->do. For example,
this one could be:

  $dbh->do("UPDATE groups SET name = ? WHERE id = ?", undef, $name, $gid);

  I'm also going to do some testing of the actual functionality of this patch,
soon. But if you post a new patch before that, then I'll test that patch.

Comment 12

[Marc Schumann [:Wurblzap]]

Comment on attachment 171096 [details] [diff] [review]
patch for the 2.18 branch, v1

Most of comment 7 applies to the branch patch, too.

Comment 13

[Frédéric Buclin]

Attachment: patch for the trunk, v4

mkanat, here is the patch is was talking about yesterday on IRC. I asked you to
review it but I forgot to upload it first!!! :-p

Comment 14

[Max Kanat-Alexander]

Comment on attachment 171863 [details] [diff] [review]
patch for the trunk, v4

OK. Looks basically good.

Only one nit:

Why did you break the template interface by changing just the name of the input
variable to "regexp" instead of "rexp", but you left the template variable
itself as "rexp"?

Comment 15

[Max Kanat-Alexander]

Oh, also:

[Sun Jan 23 18:51:32 2005] [error] [client 216.233.214.179] [Sun Jan 23 18:51:32
2005] editgroups.cgi: Argument "" isn't numeric in numeric ne (!=) at
/var/www/html/mkanat3/editgroups.cgi line 606., referer:
http://landfill.bugzilla.org/mkanat3/editgroups.cgi?action=changeform&group=16
[Sun Jan 23 18:51:32 2005] [error] [client 216.233.214.179] [Sun Jan 23 18:51:32
2005] editgroups.cgi: Use of uninitialized value in numeric ne (!=) at
/var/www/html/mkanat3/editgroups.cgi line 620., referer:
http://landfill.bugzilla.org/mkanat3/editgroups.cgi?action=changeform&group=16

Any idea about those?

Comment 16

[Max Kanat-Alexander]

Oh, nevermind that approval2.18 -- the attached patch doesn't apply to 2.18,
apparently.

Comment 17

[Frédéric Buclin]

(In reply to comment #14)
> Why did you break the template interface by changing just the name of the input
> variable to "regexp" instead of "rexp", but you left the template variable
> itself as "rexp"?
> 

Yes, I could change that too. I think I missed this nit. The most important
point was to have the same field name in both admin/groups/edit.html.tmpl and
admin/groups/create.html.tmpl.

I won't backport it to 2.18 until I have a *clear* and *definitive* answer from
justdave about if he wants it or not for the branch. justdave, note that it is
possible to write the same patch without any change to the templates (may be
useful for 2.18).

Comment 18

[Frédéric Buclin]

Attachment: patch for the trunk, v4.1

Fix the nit reported by Max (rexp => regexp). Now all templates in
admin/groups/*.html.tmpl use regexp instead of rexp (edit.html.tmpl was the
only one to use rexp).

I also fix the error messages appearing in error_log. This is due to the fact
that unchecked checkboxes are not returned by the templates to editgroups.cgi
(undef) and comparing new values with old ones then generates these error
messages.

Comment 19

[Frédéric Buclin]

Comment on attachment 172250 [details] [diff] [review]
patch for the trunk, v4.1

oops!

All these patches do not work correctly when editing system groups! This is
because we check that the group name and the description are valid even for
system groups, but their name and description are not editable and are not
given by the template. Then these values are undefined and my CheckGroup*
functions complain.

Moreover, my explanation in my previous comment about undef error messages is
(completely) wrong. This has nothing to do with checkboxes as old values are
given by hidden fields. The point is that a group cannot inherit or grant privs
from itself (there are two missing checkboxes when editing groups) and then the
corresponding fields are undefined. A workaround is to write ($v != $gid)
before comparing new values with older ones. But I first need to check I don't
break anything and I prefer to fix this last minor point in another bug.

The new patch is ready but I cannot upload it from this computer so I will do
it later this evening when I come back home.

Comment 20

[Frédéric Buclin]

Attachment: patch for the trunk, v5

OK, now *everything* works as expected. But do some tests yourself to be
sure...

Comment 21

[Frédéric Buclin]

Comment on attachment 172280 [details] [diff] [review]
patch for the trunk, v5

mkanat seems busy, so any reviewer is welcome! ;)

Comment 22

[Marc Schumann [:Wurblzap]]

Comment on attachment 172280 [details] [diff] [review]
patch for the trunk, v5

Tested; works.

>+    my $isok = 0;
>+    if (detaint_natural($group_id)) {
>+        my $sth = Bugzilla->dbh->prepare("SELECT id FROM groups " .
>+                                         "WHERE id = ?");
>+        $sth->execute($group_id);
>+        $isok = $sth->fetchrow_array();
>+    }
>+    ThrowUserError("invalid_group_ID") unless $isok;

This should imo be

  detaint_natural() || ThrowUserError();
  Bugzilla->dbh->selectrow_array() || ThrowUserError();

This doesn't warrant r-, but it keeps me from r+...

[On a related nit, some prepare-execute-fetchxxxxx combinations might read
easier if they were dbh->selectxxxxx commands instead. That doesn't keep a r+
away, though.]

Comment 23

[Frédéric Buclin]

Comment on attachment 172280 [details] [diff] [review]
patch for the trunk, v5

i will resubmit then...

Comment 24

[Dave Miller [:justdave]]

clearing approval because it seems to have been obsoleted and I was blind when I
granted it or something :)

Comment 25

[Frédéric Buclin]

Attachment: patch for the trunk, v6

updated patch using $dbh->selectrow_array() as suggested by Marc + changes in
the CheckGroupID() subroutine.

Comment 26

[Marc Schumann [:Wurblzap]]

Comment on attachment 173676 [details] [diff] [review]
patch for the trunk, v6

Very nice and tidy.

I should have noticed this last time: please put trick_taint into
CheckGroupRegexp and CheckGroupDesc, like you put one into CheckGroupName. That
way, the untainting happens with the checking where it belongs. You can get rid
of the other trick_taint calls interspersed in your code, too. Please put up a
corrected patch and move r+ forward.

Comment 27

[Frédéric Buclin]

Attachment: final patch for the trunk, v6.1

forwarding Marc's review+

Comment 28

[Frédéric Buclin]

Attachment: patch for the 2.18 branch, v2

I wrote this patch in such a way that templates are not modified. Moreover, as
the 2.18 version of editgroups.cgi is not templatized, I cannot use
ThrowUserError() as I would like so that I had to use the old ShowError()
subroutine. Another reason not to use ThrowUserError() is that no error message
in user-error.html.tmpl already exists about groups. And I'm not sure it's a
good idea to add a plenty of new error messages here.

Comment 29

[Marc Schumann [:Wurblzap]]

Comment on attachment 173801 [details] [diff] [review]
patch for the 2.18 branch, v2

r=wurblzap by inspection (runtests.pl and comparison with the trunk patch).

Comment 30

[Shane H. W. Travis]

2.18:
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.38.2.2; previous revision: 1.38.2.1
done


Tip:
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.45; previous revision: 1.44
done
Checking in template/en/default/admin/groups/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/edit.html.tm
pl,v  <--  edit.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-
error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.87; previous revision: 1.86
done