Closed
Bug 1201026
Opened 9 years ago
Closed 9 years ago
editusers.cgi doesn't list users properly
Categories
(Bugzilla :: Administration, task)
Tracking
()
RESOLVED
FIXED
Bugzilla 5.0
People
(Reporter: gerv, Assigned: gerv)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
1023 bytes,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
For some reason, if $is_enabled is undef, ($is_enabled == 0 || $is_enabled == 1) is returning true. Specifically, it's comparing as true with 0. :-| So then we are adding "profiles.is_enabled = ?" to the query and pushing undef to the bind values, which is ignored, so we get a literal ? in the query, which of course doesn't match anything. Patch coming. Gerv
Assignee | ||
Comment 1•9 years ago
|
||
Comment 2•9 years ago
|
||
Before bug 331529, $is_enabled (which was named $enabled_only) was always defined, so this wasn't a problem. This is a regression in 5.0.
Comment 3•9 years ago
|
||
Comment on attachment 8655915 [details] [diff] [review] Patch v.1 >+ if (defined($is_enabled)) { >+ detaint_natural($is_enabled); >+ if ($is_enabled == 0 || $is_enabled == 1) { Things are done in the wrong order. I already reported this problem when the original patch has been proposed, see bug 331529 comment 4, but my comment has been ignored, it looks like: if you pass a string, then $is_enabled will pass the defined() check, but then detaint_natural() will undef it because it's not an integer, and it will again match $is_enabled == 0, and so the bug is still there. Please do it differently: if $is_enabled is undefined (around line 73), fall back to 2, which means "all users" (enabled and disabled). This way, we are sure detaint_natural() has something to validate, and then you check if it's a valid integer or not: detaint_natural($is_enabled); if ($is_enabled && ( ... == 0 || ... == 1))
Attachment #8655915 -
Flags: review?(LpSolit) → review-
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8655915 -
Attachment is obsolete: true
Attachment #8655923 -
Flags: review?(LpSolit)
Comment 5•9 years ago
|
||
Comment on attachment 8655923 [details] [diff] [review] Patch v.2 If $is_enabled is undefined, you get: Use of uninitialized value $_[0] in pattern match (m//) at Bugzilla/Util.pm line 52 So please fall back to 2 as I suggested above. r=LpSolit with this fix.
Attachment #8655923 -
Flags: review?(LpSolit) → review+
Updated•9 years ago
|
Flags: approval?
Flags: approval5.0?
Updated•9 years ago
|
Flags: approval?
Flags: approval5.0?
Flags: approval5.0+
Flags: approval+
Assignee | ||
Comment 6•9 years ago
|
||
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git fedc614..a666b2a master -> master To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git ea25c5c..e76030a 5.0 -> 5.0 Gerv
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•