Group with no admin or mod causes heavy user query

[cyclic]

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

[DJPaul]

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?

[cyclic]

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)

[boonebgorges]

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!

[cyclic]

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.

[boonebgorges]

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)

[hnla]

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?

[DJPaul]

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.

[hnla]

got ya.

[cyclic]

patch including unit test

[cyclic]

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

[cyclic]

removed extraneous debug docblock group

[cyclic]

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?

[boonebgorges]

This looks perfect - thanks for the patch!

[boonebgorges]

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.