CUNY Academic Commons

Curiouser and curiouser - she has made me an admin and now it is happening to me!

I've looked into this a bit more and the changes bbPress did in v2.6.18 to strengthen access controls to private and hidden forums caused this problem.

Boone, I've added two commits to address this. See https://github.com/cuny-academic-commons/cac/compare/837c4ef...33fc6b5. First one is the hotfix we usually roll into each bbPress 2.6 release, the second one has to do with the new map_group_forum_read_meta_caps() method: https://github.com/bbpress/bbPress/blob/c38e72873728f877ac7d1c1c48538ca29765d2f2/src/includes/extend/buddypress/groups.php#L214-L234. This method is hooked to 'bbp_map_meta_caps' filter at priority 20. Since priority 20 overrides the existing map_group_forum_meta_caps() method at priority 10, I dropped the new method to priority 9. I haven't done too much debugging since it's getting late here, but this fixes the problem.

Marilyn, can you ask Nantina to check her group forums to see if everything is working again?

Boone, I made one additional change that I haven't committed yet and that is to comment out another new addition meant to exclude all private and hidden forum IDs that a user cannot view: https://github.com/bbpress/bbPress/blob/c38e72873728f877ac7d1c1c48538ca29765d2f2/src/includes/extend/buddypress/groups.php#L146. When I checked Marilyn's user account against the Labor & Globalization group forum, it was taking up to 15 seconds to load the group forum index page. When I commented out this line, the group forum index page loads under a second again. The new exclude forum IDs method does a lot of traversing through all private and hidden group forums, which is the probable cause of the slowdown. I'm thinking that if a logged-in user is a member of the group, we can bypass this exclude forum IDs check entirely. I can work on this tomorrow when I have a fresh set of eyes.

Thanks for jumping on this, Ray.

Boone, I made one additional change that I haven't committed yet and that is to comment out another new addition meant to exclude all private and hidden forum IDs that a user cannot view: https://github.com/bbpress/bbPress/blob/c38e72873728f877ac7d1c1c48538ca29765d2f2/src/includes/extend/buddypress/groups.php#L146. When I checked Marilyn's user account against the Labor & Globalization group forum, it was taking up to 15 seconds to load the group forum index page. When I commented out this line, the group forum index page loads under a second again. The new exclude forum IDs method does a lot of traversing through all private and hidden group forums, which is the probable cause of the slowdown. I'm thinking that if a logged-in user is a member of the group, we can bypass this exclude forum IDs check entirely. I can work on this tomorrow when I have a fresh set of eyes.

I'm seeing the same behavior, which makes it extremely hard to test anything. It's not clear to me from the commit message https://bbpress.trac.wordpress.org/changeset/7495 the specific vulnerability that's being protected against here. Presumably someone discovered that in some context, perhaps a REST API request or somewhere else outside of regular page routing (like /groups/my-group/forum/), it was possible to spoof/override the default forum ID. This seems like a legit vector for attack, but in that case why not specifically check that the user has access to the requested forum_id, rather than collecting a list of all items that are off-limits? I guess it's because this is being used upstream to build a set of WP_Query arguments, and you have to work with the tools you're given (post__not_in). If we're simply going to remove this, I'd want to be sure we're not introducing a real bug. Perhaps we can remove the hook only when bp_is_group_forum() (or whatever the modern version of this is), because in this case we can safely assume that BP's routing logic and bbPress's regular meta-cap mapping will be sufficient?

the second one has to do with the new map_group_forum_read_meta_caps() method: https://github.com/bbpress/bbPress/blob/c38e72873728f877ac7d1c1c48538ca29765d2f2/src/includes/extend/buddypress/groups.php#L214-L234. This method is hooked to 'bbp_map_meta_caps' filter at priority 20. Since priority 20 overrides the existing map_group_forum_meta_caps() method at priority 10, I dropped the new method to priority 9. I haven't done too much debugging since it's getting late here, but this fixes the problem.

Interestingly, I can't reproduce the issue in a local installation. Is it failing because user_can_view_group_forum() in the new map_group_forum_read_meta_caps() is returning false? Simply switching the order, as you've done, feels dangerous to me. Priority 9 means that the "read" cap check is happening before bbPress's regular cap mapping takes place. I guess it ought to be OK because these 'map_meta_cap' fallbacks conservatively bail if they come upon a 'do_not_allow', but can override an 'allow' in a later callback? I'm sorry I don't have more insight here - if you are able to share with me some of the conditions that you need to reproduce to make it fail, I can try to understand it better.

Interestingly, I can't reproduce the issue in a local installation.

I was able to duplicate this issue locally just now.

Marilyn's user account does not have a bbPress role for her account. When combined with the latest hardening changes in bbPress 2.6.18, this causes the "This group does not currently have a forum" problem.

Because the bbPress 'participant' role automatically provisions the 'read_private_forums' cap: https://github.com/bbpress/bbPress/blob/c38e72873728f877ac7d1c1c48538ca29765d2f2/src/includes/core/capabilities.php#L191-L192, this is why most users will not see this problem, only users without a forum role will run into this issue.

Perhaps we can remove the hook only when bp_is_group_forum() (or whatever the modern version of this is), because in this case we can safely assume that BP's routing logic and bbPress's regular meta-cap mapping will be sufficient?

I've done this in https://github.com/cuny-academic-commons/cac/commit/fd55ebb41995b95929399541a09cd9a199fc76d3 and pushed the fix to production. I also rolled back yesterday's filter priority change because this commit addresses the same problem and will also fix issues with users without a forum role.

Thanks, Ray. I think we're getting closer to understanding what's going on here. The lack of the 'participant' role really does seem to be at the heart of this.

The 'participant' role seems to be at the heart of this. #25727 describes a poster hitting a current_user_can( 'read_topic' ) check that fails. I looked into this and found that the default behavior, in bbp_map_topic_meta_caps(), is to bail out when the current user doesn't have 'spectate'. Users without a bbPress role on the primary site will fail this check. Before the most recent bbPress update, this must not have mattered - the BP-extend layer must have restored the missing permission. But these new "'do_not_allow'" safeguards seem to prevent this from happening.

I've got a change on the production site, in wp-content/plugins/cac-bp-custom-includes/forums.php, that simply grants the 'participate' and 'spectate' caps to all users who are logged into the Commons. This helps us to work around all of bbPress's (well-documented, longstanding, always problematic) dependence on custom roles. Do you see danger in leaving this in place, or a better way of doing it?

On second thought, I have just made a change that scopes the 'participate' and 'spectate' auto-cap-provisioning much more narrowly - it's only added when a user is a memebr of the current group:

1647 // temp - add 'participate' cap for all group members.
1648 add_filter( 'user_has_cap', function ( $allcaps, $caps, $args, $user ) { 
1649     if ( $user instanceof WP_User && $user->exists() && bp_is_group() && groups_is_user_member( $user-     >ID, bp_get_current_group_id() ) ) {                                                                  
1650         $allcaps['participate'] = true;
1651         $allcaps['spectate'] = true;
1652     } 
1653     return $allcaps;

This will not cover REST contexts or other such tomfoolery but it does catch the primary posting case, and seems undangerous until we can figure out how to unravel these new problems in bbPress more properly.

Groan to this new 'read_topic' cap check.

It looks like we have some prior attempts at using BuddyPress group membership to override the 'bbp_map_meta_caps' filter.

What about adding the 'read_topic' cap to the following line:
https://github.com/cuny-academic-commons/cac/blob/54fad48cb190f873921f3ee78afa0c215803a681/wp-content/plugins/cac-bp-custom-includes/forums.php#L747

And bumping the priority of the openlab_bbp_map_group_forum_meta_caps() function hooked to 'bbp_map_meta_caps' to 30:
https://github.com/cuny-academic-commons/cac/blob/54fad48cb190f873921f3ee78afa0c215803a681/wp-content/plugins/cac-bp-custom-includes/forums.php#L781

This solves the issue too in my local testing.

Good call, Ray. This is indeed pretty much the same issue that openlab_bbp_map_group_forum_meta_caps() was introduced to solve. See #3649. Adding the 'read_topic' cap there is more focused, and keeps everything in place. I've made the change in https://github.com/cuny-academic-commons/cac/commit/c72ff7dcd9852506974a8d7fe96bd00e833a558e. Just to be explicit for the record, the priority change is necessary because the update bbPress has added its own 'map_meta_cap' callback at priority 20, and we need to be sure that we fire that; I've added this in https://github.com/cuny-academic-commons/cac/commit/cde00ddae76755734023a01eec682cdf0c431bd4

Marilyn, this fixes the immediate issues in our tests. Please let us know if the reporters continue to see problems.

Ray, I have a couple follow-up thoughts.

- can you help me to understand the bbp_get_excluded_forum_ids() fix a little better? To the extent that this is meant as a performance enhancement, I understand it. To the extent that it fixes the originally reported bug, I don't fully get it. Were group forum IDs mistakenly being added to the "exclude" paramater? Why? I was going to try to understand this better on my own, but after the most recent changes, when I commented out your remove_filter( 'bbp_get_excluded_forum_ids', array( $this, 'exclude_group_forum_ids' ), 20 ); line, I was no longer able to reproduce the original routing bug in this ticket. Is it possible that the 'read_topic' mapping fixes the issue? Or perhaps the bumping of the openlab_bbp_map_group_forum_meta_caps() priority? I wonder if we can roll back or rethink the direct mod to bbPress.

- What can/should we report to JJJ? He's liable not to care about the 'read_topic' issue, as it arises out of our refusal to rely on the 'participant' role that is baked so deeply into bbPress. But the performance implications of assembling a whole list of off-limits forums is likely to be significant on many installations, and I wonder if he might be willing to accept some suggestions about how to accomplish his security hardening without crashing every site with large numbers of forums.

- The 'read_topic' thing is obviously closely tied to the 'participant' role, which bbPress relies heavily on but which we don't want to enforce. As such, I don't think that JJJ is likely to care that his updates broke our installation. But the

but after the most recent changes, when I commented out your remove_filter( 'bbp_get_excluded_forum_ids', array( $this, 'exclude_group_forum_ids' ), 20 ); line, I was no longer able to reproduce the original routing bug in this ticket. Is it possible that the 'read_topic' mapping fixes the issue? Or perhaps the bumping of the openlab_bbp_map_group_forum_meta_caps() priority? I wonder if we can roll back or rethink the direct mod to bbPress.

The bumping of our existing openlab_bbp_map_group_forum_meta_caps() function to run at priority 30 allows the current user to pass the 'read_forum' cap checks in the BBP_Forums_Group_Extension::exclude_group_forum_ids() method and elsewhere in the bbPress codebase. This allows group members without a forum role to view the group forum index again.

It's possible to remove my remove_filter( 'bbp_get_excluded_forum_ids', array( $this, 'exclude_group_forum_ids' ), 20 ); addition, but this will cause a performance hit.

I just did a tiny benchmark with Marilyn's user account against the Labor & Globalization group forum index. With the remove_filter() line in place, the group forum index loads in 0.4 seconds. When I comment out the remove_filter() line, page loads in 2.8 seconds. So there is still a performance hit here, presumably because the exclude_group_forum_ids() method has to loop through all private and hidden group forums, check each forum to see if it is connected to a group and do the 'read_forum' cap check. Our install has roughly 2600 private and hidden forums to go through, so that's where the performance hit is occurring.

If we do not want to do the direct mod to bbPress, we can move the change to our cac-bp-custom-includes/forums.php file.

What can/should we report to JJJ?

The 'read_topic' thing is obviously closely tied to the 'participant' role, which bbPress relies heavily on but which we don't want to enforce. As such, I don't think that JJJ is likely to care that his updates broke our installation. But the

I jumped the gun and already posted a ticket on bbPress Trac yesterday - https://bbpress.trac.wordpress.org/ticket/3693. I think it's mostly an edge case because a lot of our users do not have a bbPress role, whereas most bbPress installs will set up the user with a forum role unless they turn off bbPress's Auto-Role functionality.

I still think there is a performance issue with the BBP_Forums_Group_Extension::exclude_group_forum_ids() method for the reasons I point out in Marilyn's benchmark above. I think this method is running too frequently at the moment. Shouldn't this method only run for post queries where post_type = 'any' or during post searches? I'm not sure I understand the need to add this exclusion to every WP_Query, but maybe I'm not fully understanding the new hardening changes yet.

Also if the bbPress install uses custom forum queries to display things outside the regular group forum area (I'm thinking widgets or in sidebars here), it is possible that the BBP_Forums_Group_Extension::exclude_group_forum_ids() method will run and cause performance issues for anonymous users or for users that do not have the proper 'read_forum' capability.

I also found another bug in bbPress where bbPress is not checking the capability before doing post type checks, which causes about 15 unnecesssary SELECT *
FROM wp_1_posts WHERE ID = 1 LIMIT 1 database queries to occur on each group forum page load. Here's a patch I intend to post on bbPress Trac:

diff --git a/wp-content/plugins/bbpress/includes/extend/buddypress/groups.php b/wp-content/plugins/bbpress/includes/extend/buddypress/groups.php
index aaa38538a0..12eeded266 100644
--- a/wp-content/plugins/bbpress/includes/extend/buddypress/groups.php
+++ b/wp-content/plugins/bbpress/includes/extend/buddypress/groups.php
@@ -418,6 +418,15 @@ class BBP_Forums_Group_Extension extends BP_Group_Extension {
      * @return array
      */
     public function map_group_forum_meta_caps( $caps = array(), $cap = '', $user_id = 0, $args = array() ) {
+        $caps_to_check = [
+            'publish_replies', 'publish_topics', 'moderate', 'edit_topic', 'edit_reply', 'view_trash', 'edit_others_replies', 'edit_others_topics', 'delete_topic', 'delete_reply', 'manage_forum_attributes'
+        ];
+
+        // If not checking our caps, bail.
+        if ( ! in_array( $cap, $caps_to_check ) ) {
+            return $caps;
+        }
+
         // Group request state only describes the logged-in user
         if ( bbp_get_current_user_id() !== (int) $user_id ) {
             return (array) apply_filters( 'bbp_map_group_forum_topic_meta_caps', $caps, $cap, $user_id, $args );

Update: I've posted the ticket on bbPress Trac here - https://bbpress.trac.wordpress.org/ticket/3694.