1346294 - When you try to display only the disabled users, the complete list is returned instead

Status: ASSIGNED

(Bugzilla :: Administration, defect)

Description

[Christophe JAILLET]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20170301181722

Steps to reproduce:

Administration --> Users --> Restrict search to Disabled users


Actual results:

The list with ALL users is displayed


Expected results:

Only disabled users should be returned

Comment 1

[Christophe JAILLET]

This looks like a regression introduced by this commit:
   https://github.com/bugzilla/bugzilla/commit/a666b2a74f565a5ebb38f0ce0b400d04b1ea7ca4


The condition:
   if ($is_enabled && ($is_enabled == 0 || $is_enabled == 1))
is false if $is_enabled == 0.
So the filter in the request is not added and we receive the complete list, as if the parameter was missing or was 2 (i.e. All)


The "// 2" on line 74 should be, IMOH, enough. Removing the '$is_enabled' in the test should be fine.
At least, it should be tested with 'defined($is_enabled)' after the call to 'detaint_natural'

Comment 2

[Christophe JAILLET]

Attachment: 1346294.patch

Comment 3

[Dylan Hardison [:dylanAtHome]]

Probably the fix in https://github.com/bugzilla/bugzilla/commit/a666b2a74f565a5ebb38f0ce0b400d04b1ea7ca4 is fine for this...

Hey gerv, what bug is https://github.com/bugzilla/bugzilla/commit/a666b2a74f565a5ebb38f0ce0b400d04b1ea7ca4 from?
the commit message doesn't list the bug, but it says it was reviewed... :-)

Comment 4

[Dylan Hardison [:dylanAtHome]]

(In reply to Christophe JAILLET from comment #1)

> The "// 2" on line 74 should be, IMOH, enough. Removing the '$is_enabled' in
> the test should be fine.
> At least, it should be tested with 'defined($is_enabled)' after the call to
> 'detaint_natural'
You could actually not detaint at all and use the '!!' operator to make sure it is a truthy value for the bind param.

Comment 5

[Dylan Hardison [:dylanAtHome]]

Comment on attachment 8846126 [details] [diff] [review]
1346294.patch

Review of attachment 8846126 [details] [diff] [review]:
-----------------------------------------------------------------

r- -- do these proposed changes seem to work?

::: editusers.cgi
@@ +169,1 @@
>          }

somewhere up above:  my $is_enabled = $cgi->param('is_enabled');
this just because "2" seems like a magic number... undefined is a perfectly reasonable value.

@@ +169,3 @@
>          }
>  
>          detaint_natural($is_enabled);

remove the above line

@@ +169,4 @@
>          }
>  
>          detaint_natural($is_enabled);
> +        if (defined($is_enabled) && ($is_enabled == 0 || $is_enabled == 1)) {

do: if (defined $is_enabled) {

@@ +173,3 @@
>              $query .= " $nextCondition profiles.is_enabled = ?";
>              $nextCondition = 'AND';
>              push(@bindValues, $is_enabled);

push(@bindValues, !!$is_enabled);

Comment 6

[Christophe JAILLET]

1) Without the 'detaint_natural()', I get:

Software error:

Insecure dependency in parameter 4 of DBI::db=HASH(0x5558d7c2dda8)->selectall_arrayref method call while running with -T switch at /home/tititou36/Code_Source/git_bugzilla/editusers.cgi line 179.



2) The '($is_enabled == 0 || $is_enabled == 1)' is still needed, otherwise when looking for all users, $is_enabled is 2, and we find nothing

Comment 7

[Gervase Markham [:gerv]]

dylan: bug 1201026. Apologies for the bad commit message.

Gerv

Comment 8

[Frédéric Buclin]

Comment on attachment 8846126 [details] [diff] [review]
1346294.patch

This is the right fix. I missed it when I rewiewed bug 1201026. Thanks for catching that. The newline removal is not needed, though, and should not be committed. r=LpSolit