editusers.cgi doesn't list users properly

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Administration
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gerv, Assigned: gerv)

Tracking

({regression})

Bugzilla 5.0
regression
Bug Flags:
approval +
approval5.0 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Created attachment 8655915 [details] [diff] [review]
Patch v.1
Assignee: administration → gerv
Status: NEW → ASSIGNED
Attachment #8655915 - Flags: review?(LpSolit)

Comment 2

3 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.
Depends on: 331529
Keywords: regression
Target Milestone: --- → Bugzilla 5.0

Comment 3

3 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-
Created attachment 8655923 [details] [diff] [review]
Patch v.2
Attachment #8655915 - Attachment is obsolete: true
Attachment #8655923 - Flags: review?(LpSolit)

Comment 5

3 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

3 years ago
Flags: approval?
Flags: approval5.0?

Updated

3 years ago
Flags: approval?
Flags: approval5.0?
Flags: approval5.0+
Flags: approval+
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.