bbp_user_can_view_forum() and custom user ID

[alex-ye]

A small bug in bbp_user_can_view_forum() , it doesn't work properly with a custom user ID. because it doesn't pass the $user_id variable to bbp_is_user_keymaster() check.

[johnjamesjacoby]

Thanks for the patch. Committing.

[johnjamesjacoby]

In 5087:

In bbp_user_can_view_forum(), pass the $user_id into bbp_is_user_keymaster().

Fixes issue where bbp_user_can_view_forum() may return incorrect results for users with the Keymaster role.

Props alex-ye. Fixes #2393.

[alex-ye]

The above patch fixing another bug when you pass user_id as zero.

[johnjamesjacoby]

Replying to alex-ye:

The above patch fixing another bug when you pass user_id as zero.

What other bug does this fix?

[alex-ye]

Replying to johnjamesjacoby:

Replying to alex-ye:

The above patch fixing another bug when you pass user_id as zero.

What other bug does this fix?

The bbPress core doesn't call the the bbp_user_can_view_forum() with user_id as zero, but I face it when I was working on some client plug-in.

When you pass the user_id as zero, the developers expect to refer to the current logged-in user ID like the other bbPress functions.

so you can describe it as an enhancement.

[johnjamesjacoby]

Can you detail exactly what bug this causes and how to reproduce it?

[alex-ye]

Replying to johnjamesjacoby:

Can you detail exactly what bug this causes and how to reproduce it?

It's simple, test this code with a private forum and an user with moderator role:

echo bbp_user_can_view_forum( array( 'user_id' => 0, 'forum_id' => 94 ) );

[johnjamesjacoby]

Replying to alex-ye:

Replying to johnjamesjacoby:

Can you detail exactly what bug this causes and how to reproduce it?

It's simple, test this code with a private forum and an user with moderator role:

echo bbp_user_can_view_forum( array( 'user_id' => 0, 'forum_id' => 94 ) );

It's not simple, as I still don't understand what the actual bug is. My guess is that bbp_user_can_view_forum needs a fix, more than bbp_get_user_id needing a change in behavior that effects all of bbPress.

If you're not passing a user_id into bbp_user_can_view_forum() then it should always return false, since returning true would be double-plus bad. How can a nonexistent user possibly access a forum?

[johnjamesjacoby]

Unclear how to reproduce an unexpected circumstance, so moving to 2.5 for more reporter-feedback.

[alex-ye]

Replying to johnjamesjacoby:

If you're not passing a user_id into bbp_user_can_view_forum() then it should always return false, since returning true would be double-plus bad.

Currently the function use the logged-in user ID if you don't pass user_id arg.. the bugs happens when you pass user_id arg as 0 only.

What are those bugs?!
When the developer pass user_id as 0, he may expect one of those results:

1- The function will return false.
2- The function will replace the zero user_id with the logged-in user ID.

but what actually happens is a mess!

• $user_id local variable will have 0 value.
• bbp_is_user_keymaster( $user_id ) will check against the logged-in user ID.
• user_can( $user_id, 'read_private_forums' ) check will returnfalse.
• And apply_filters( 'bbp_user_can_view_forum', $retval, $forum_id, $user_id )

Replying to johnjamesjacoby:

How can a nonexistent user possibly access a forum?

Again, there is not security bug in the bbPress core! because bbPress core doesn't pass the user_id as zero with bbp_user_can_view_forum()

I hope you understand me :)

[johnjamesjacoby]

In 5188:

In bbp_user_can_view_forum() check the $user_id before passing it into bbp_is_user_keymaster(). Prevents accidental role escalation if $user_id is empty. See #2393.

[johnjamesjacoby]

In 5189:

Check the $author_id before passing it into bbp_is_user_keymaster() in blacklist and moderation functions. Prevents accidental role escalation if $author_id is empty. See #2393.

[johnjamesjacoby]

Marking this as closed.

The two above changesets better reflect the intent of bbp_is_user_keymaster() checks, particularly inside of bbp_user_can_view_forum().

If an empty $user_id is explicitly passed as part of the $args array, we do not want to assume the developer intended it to be the current user ID (especially since the default value is already the current user ID.) This is to prevent accidental role escalation, since it is possible you'd want to check if an anonymous user can access a specific forum ID.

If I've misunderstood you, please reopen (again) and place in the 2.6 milestone.

Thanks for your diligence with this. :)

[alex-ye]

Nice solution John, Always learn from you :)