Skip to:
Content

BuddyPress.org

#7677 closed defect (bug) (fixed)

Group with no admin or mod causes heavy user query

Reported by: cyclic Owned by: boonebgorges
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9.2
Component: Groups Keywords: needs-patch
Cc:

Description

As mentioned in https://wordpress.slack.com/archives/C02RQBYUG/p1517333350000551

When a BP group has no admins or mods, BP_Groups_Group->set_up_admins_and_mods() runs a user query with an empty array for 'include' which ends up fetching all users instead of none. This negatively affects performance up to and including memory exhaustion fatal errors on sites with lots of users (I see extra memory usage of roughly 300MB on a site with 12000 users).

This is the line that runs the query: https://github.com/buddypress/BuddyPress/blob/master/src/bp-groups/classes/class-bp-groups-group.php#L563

Pasted here in case that link goes stale:

$admin_mod_users = get_users( array(
        'include' => array_merge( $admin_ids_plucked, $mod_ids_plucked ),
        'blog_id' => null,
) );

I'm interested in implementing a patch for this. Something like this?

$admin_mod_ids = array_merge( $admin_ids_plucked, $mod_ids_plucked );
$admin_mod_users = [];

if ( ! empty( $admin_mod_ids ) ) {
        $admin_mod_users = get_users( array(
                'include' => $admin_mod_ids,
                'blog_id' => null,
        ) );
}

Attachments (3)

7677.01.patch (1.7 KB) - added by cyclic 22 months ago.
patch including unit test
7677.02.patch (1.8 KB) - added by cyclic 22 months ago.
updated patch with test to check group->mods as well as group->admins
7677.03.patch (1.8 KB) - added by cyclic 22 months ago.
removed extraneous debug docblock group

Download all attachments as: .zip

Change History (16)

#1 @DJPaul
22 months ago

Any idea how exactly this group ended up with no admins or moderators? Have you any custom code, plugins, run any scripts that might have affected this?

Are you able to check the row in the bp_groups and groups member tables, and see what this group looks like there?

#2 @cyclic
22 months ago

It's likely that the only admin was deleted in a nonstandard way. We run Commons In A Box with a ton of custom code and integrate with a Shibboleth-based identity management stack to deal with authentication and user management, and there are scenarios where a WordPress user could be removed and might not run all relevant hooks, so the broken state of the data is probably related.

Here's what the database says for one affected group:

> select count(*) from wp_bp_groups_members where group_id = 288\G
*************************** 1. row ***************************
count(*): 20
1 row in set (0.00 sec)

> select * from wp_bp_groups_members where group_id = 288 and ( is_admin = 1 or is_mod = 1 )\G
Empty set (0.00 sec)

#3 @cyclic
22 months ago

  • Component changed from Core to Groups
  • Version set to 2.9.2

#4 @boonebgorges
22 months ago

  • Keywords needs-patch added

Thanks, @cyclic. If you discover that there's a way to delete the sole admin of a group in the standard BP interface, please let us know, as that'd count as a separate bug.

Since it's easy to introduce a failsafe here, let's go ahead and do so. Something like what you've suggested above should work. If you can generate a patch (and ideally, a unit test), that'd be great. Thank you!

#5 @DJPaul
22 months ago

  • Milestone changed from Awaiting Review to 3.0

#6 @cyclic
22 months ago

I may have found how the admins were removed: as part of an integration with an external membership database, we call BP_Groups_Member->demote() on users when certain conditions are met. It doesn't appear to me that this tries to manage the fact that the group may be left admin-less afterwards. Did I miss where that's handled or is this the aforementioned separate bug?

groups_demote_member() doesn't modify the user if they're an admin.

Last edited 22 months ago by cyclic (previous) (diff)

#7 @boonebgorges
22 months ago

It looks like our protection happens at a higher level. We don't allow users themselves to trigger a group-leave in the normal way (by clicking Leave Group) if they're the only admin. See https://buddypress.trac.wordpress.org/browser/tags/2.9.2/src/bp-groups/bp-groups-actions.php?marks=481-488#L466 There's nothing deeper in our API that prevents a group from being made admin-less or member-less. And, generally speaking, I don't think there needs to be - it doesn't cause any logical problems or anything if a group doesn't have members.

So I think what we need here is a fix for the runaway query, and possibly for any other untenable weirdness that might arise in cases where all members have been removed from a group. (where "untenable" means things like fatal errors or fundamentally broken parts of the interface)

#8 @hnla
22 months ago

Why do we not simply add a site admin as group admin if we query no admins or mods, a group generally has the group creator set as primary group admin if all members demoted then a site admin would still be able to administer the group so replace them as prime admin or even group creator?

#9 @DJPaul
22 months ago

One reason is that doing database writes from the front-end when loading a page (that’s not the result of some action a user specifically invoked, such as submitting a post) does not scale and is a bad idea.

The main point as Boone described is that while this is an unusual circumstance and not a feature we intend to support from the interface, our plugin shouldn’t fall apart given an unexpected set of data.

#10 @hnla
22 months ago

got ya.

@cyclic
22 months ago

patch including unit test

@cyclic
22 months ago

updated patch with test to check group->mods as well as group->admins

@cyclic
22 months ago

removed extraneous debug docblock group

#11 @cyclic
22 months ago

I think https://buddypress.trac.wordpress.org/attachment/ticket/7677/7677.03.patch solves the issue and tests that both $group->admins & $group->mods are empty when the last admin is demoted. Is there anything I should change?

#12 @boonebgorges
22 months ago

This looks perfect - thanks for the patch!

#13 @boonebgorges
22 months ago

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

In 11838:

Groups: Admin/mod queries should respect when a group has no admins/mods.

Without this patch, groups without admins or mods can trigger a lookup of
every user on the installation.

Props cyclic.
Fixes #7677.

Note: See TracTickets for help on using tickets.