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

UNCONFIRMED

Status

()

UNCONFIRMED
2 years ago
2 years ago

People

(Reporter: christophe.jaillet, Assigned: christophe.jaillet)

Tracking

5.0.3

Details

Attachments

(1 attachment)

806 bytes, patch
christophe.jaillet
: review?
dylanAtHome
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
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'
(Assignee)

Comment 2

2 years ago
Created attachment 8846126 [details] [diff] [review]
1346294.patch
Attachment #8846126 - Flags: review?(dkl)
Assignee: general → christophe.jaillet
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... :-)
Flags: needinfo?(gerv)
(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 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);
Attachment #8846126 - Flags: review?(dkl) → review?(dylan)
(Assignee)

Comment 6

2 years ago
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
dylan: bug 1201026. Apologies for the bad commit message.

Gerv
Flags: needinfo?(gerv)
You need to log in before you can comment on or make changes to this bug.