Skip to:
Content

bbPress.org

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#2393 closed defect (bug) (fixed)

bbp_user_can_view_forum() and custom user ID

Reported by: alex-ye Owned by: johnjamesjacoby
Milestone: 2.5 Priority: normal
Severity: normal Version: 2.3.2
Component: Component - Users Keywords: has-patch
Cc:

Description

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.

Attachments (2)

template.php.patch (854 bytes) - added by alex-ye 8 years ago.
2393.patch (707 bytes) - added by alex-ye 8 years ago.

Download all attachments as: .zip

Change History (18)

#1 @johnjamesjacoby
8 years ago

  • Milestone changed from Awaiting Review to 2.4

Thanks for the patch. Committing.

#2 @johnjamesjacoby
8 years ago

  • Owner set to johnjamesjacoby
  • Resolution set to fixed
  • Status changed from new to closed

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
8 years ago

#3 follow-up: @alex-ye
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#4 @netweb
8 years ago

  • Milestone 2.4 deleted

#5 @netweb
8 years ago

  • Milestone set to Awaiting Review

#6 in reply to: ↑ 3 ; follow-up: @johnjamesjacoby
8 years ago

  • Milestone changed from Awaiting Review to 2.4.1

Replying to alex-ye:

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

What other bug does this fix?

#7 in reply to: ↑ 6 @alex-ye
8 years ago

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.

#8 follow-up: @johnjamesjacoby
8 years ago

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

#9 in reply to: ↑ 8 ; follow-up: @alex-ye
8 years ago

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

#10 in reply to: ↑ 9 ; follow-up: @johnjamesjacoby
8 years ago

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?

#11 @johnjamesjacoby
8 years ago

  • Milestone changed from 2.4.1 to 2.5

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

#12 in reply to: ↑ 10 @alex-ye
8 years ago

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

Last edited 8 years ago by alex-ye (previous) (diff)

#13 @johnjamesjacoby
8 years ago

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.

#14 @johnjamesjacoby
8 years ago

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.

#15 @johnjamesjacoby
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

#16 @alex-ye
8 years ago

Nice solution John, Always learn from you :)

Note: See TracTickets for help on using tickets.