Open
Bug 1346294
Opened 7 years ago
Updated 4 years ago
When you try to display only the disabled users, the complete list is returned instead
Categories
(Bugzilla :: Administration, defect)
Tracking
()
ASSIGNED
Bugzilla 5.0
People
(Reporter: christophe.jaillet, Assigned: christophe.jaillet)
References
Details
Attachments
(1 file)
806 bytes,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
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•7 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•7 years ago
|
||
Attachment #8846126 -
Flags: review?(dkl)
Updated•7 years ago
|
Assignee: general → christophe.jaillet
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
(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•7 years ago
|
||
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•7 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
Comment 7•7 years ago
|
||
dylan: bug 1201026. Apologies for the bad commit message. Gerv
Flags: needinfo?(gerv)
Comment 8•4 years ago
|
||
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
Attachment #8846126 -
Flags: review?(dylan+test) → review+
Updated•4 years ago
|
Status: UNCONFIRMED → ASSIGNED
Component: Bugzilla-General → Administration
Depends on: 1201026
Ever confirmed: true
Flags: approval?
Flags: approval5.0?
Target Milestone: --- → Bugzilla 5.0
You need to log in
before you can comment on or make changes to this bug.
Description
•