1201026 - editusers.cgi doesn't list users properly

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Gervase Markham [:gerv]]

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

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Comment 2

[Frédéric Buclin]

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

[Frédéric Buclin]

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))

Comment 4

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Comment 5

[Frédéric Buclin]

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.

Comment 6

[Gervase Markham [:gerv]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   fedc614..a666b2a  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   ea25c5c..e76030a  5.0 -> 5.0

Gerv