Skip to:
Content

BuddyPress.org

Opened 2 weeks ago

Last modified 9 days ago

#8355 new defect (bug)

Site Admins cannot edit BP emails

Reported by: shanebp Owned by: DJPaul
Milestone: 7.0.0 Priority: normal
Severity: normal Version: 6.2.0
Component: Emails Keywords: has-patch 2nd-opinion
Cc: shanebp

Description

As the only site admin, I can edit BP Emails at ...wp-admin/edit.php?post_type=bp-email.
If I create another site admin, that user can only view BP Emails.

Forum Topic:
https://buddypress.org/support/topic/admins-can-not-edit-buddypress-emails/

Attachments (5)

8355.patch (3.2 KB) - added by imath 2 weeks ago.
8355.2.patch (1.9 KB) - added by imath 13 days ago.
8355.tests.patch (4.5 KB) - added by imath 10 days ago.
8355.3.patch (2.0 KB) - added by imath 10 days ago.
8355.4.patch (7.8 KB) - added by imath 10 days ago.

Download all attachments as: .zip

Change History (21)

#1 @shanebp
2 weeks ago

  • Cc shanebp added

#2 @imath
2 weeks ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 7.0.0

@shanebp I confirm, thanks for reporting the issue. Let's try to fix it during the 7.0.0 dev cycle.

Last edited 2 weeks ago by imath (previous) (diff)

#3 @shanebp
2 weeks ago

I took a look, but the only permissions check I found for the BP Emails admin page was for user > can manage options which should be true for all site admins.
So I am curious as to where the issue lies.

#4 @imath
2 weeks ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

So here's the situation, WordPress uses 3 meta capabilities (edit_post, read_post, and delete_post) that are mapped to corresponding primitive capabilities depending on the context (for our case it's edit_others_posts & edit_published_posts).
See https://developer.wordpress.org/reference/functions/register_post_type/#capabilities.

Although we map primitive caps to bp_moderate, this cap is not part of the Administrator's all caps but is mapped to manage_options into _bp_enforce_bp_moderate_cap_for_admins().

When WordPress checks for the edit_post cap, it adds the bp_moderate cap to the caps to check, but the $cap that is passed to _bp_enforce_bp_moderate_cap_for_admins() is still edit_post and not bp_moderate.

So _bp_enforce_bp_moderate_cap_for_admins() receives:

  • $cap = 'edit_post',
  • $caps = array( 'bp_moderate', 'edit_published_posts' ),
  • The user ID,
  • and $args = array( $post_id ).

and doesn't return array( 'manage_options' ), but the unchanged $caps. As the first admin is the author of the bp_email post_type, he can edit it, but other administrators cannot because it's not edit_other_posts that is checked against the Administrator's role database capabilities but bp_moderate, which is not into the Administrator's role database capabilities 🤪

A possible way to fix this, is to use the capability_type argument of the register_post_type() function instead of the capabilities one and to edit _bp_enforce_bp_moderate_cap_for_admins() to check some BP Administrator's caps which includes bp_moderate and the BP Email post type capabilities.

That's what I did in 8355.patch to fix the issue. I'd be happy to have a second thought about it 🤔 What do you think about it @boonebgorges ?

@imath
2 weeks ago

#5 @shanebp
2 weeks ago

Yikes, nasty... thanks for unearthing this issue.

#6 @boonebgorges
2 weeks ago

8355.patch frightens me :) I'll take some time next week to try understanding this issue a bit better.

For clarification, "site admin" in this context means "Administrator on the BP root blog", not "super admin", right?

#7 @imath
2 weeks ago

Ahah 😆 Thanks Boone. Absolutely, site admin is the Administrator of the root blog.

#8 @boonebgorges
13 days ago

This bug shows the weakness in our _bp_enforce_bp_moderate_cap_for_admins() approach. Our approach assumes that WP will always check the bp_moderate check directly, but the CPT capability mapping is a case where this assumption is incorrect.

The strategy in 8355.patch seems to work. But I worry that it creates more complexity that will provide difficult to maintain down the line. Will we have to add more caps to the 'administrator' role in this way in the future? Especially as we leverage more CPTs and taxonomies in the future.

Our current approach was always meant to be a temporary workaround. See #4296, [6185]. Can we revisit to see if there is a proper fix? Specifically, is it now possible in WP to dynamically filter a role's capabilities? If we were able to add 'bp_moderate' to the Administrator role, then this problem - and future problems like it - would go away. cc @johnjamesjacoby

#9 @imath
13 days ago

Well since WordPress 4.7 there's a new action wp_roles_init inside the WP_Roles->init_roles() method which was added in WordPress 4.9.

I've tested to use this action and it seems possible to add the bp_moderate cap to the Administrator role without having it saved into the database. It's an easier fix for this issue.

Whether it's available since 4.7 (action) or 4.9 (method), it's anyway available for the BP 7.0.0 required WP version.

See 8355.2.patch

Last edited 13 days ago by imath (previous) (diff)

@imath
13 days ago

#10 @boonebgorges
11 days ago

IMHO 8355.2.patch is a much more elegant solution to the current ticket, as well as to the overall issue of how to implement 'bp_moderate'. (Even better would be if there were proper filters in place so that we didn't have to manipulate globals, but it looks like this is not possible at the moment.) @imath are you happy with the solution?

@johnjamesjacoby I know you have put a great deal of thought into the WP role system, and it would be really valuable to have your thoughts on the approach in 8355.2.patch before we go forward with it. I feel like it's fairly safe, but 'bp_moderate' touches everything in BP, so I'd like as many eyes on it as possible.

#11 @imath
11 days ago

@boonebgorges I'm happy with it: "less code for the same result" 👍. I haven't tested on multisite: I believe we should probably improve .2.patch to only add this cap to Administrator(s) of the BP Root blog.

#12 @johnjamesjacoby
11 days ago

Remember that there is no guarantee that the administrator role exists, as well as its capabilities array key. I would add some isset() checks to avoid unintentionally invoking a role that wasn’t there previously.

_bp_enforce_bp_moderate_cap_for_admins() makes a similar assumption, which is not ideal but apparently hasn’t been an issue for anyone yet. It doesn’t really seem right, but I don’t have a better idea right now.

The idea with bp_moderate was that it replaced all of the is_super_admin() calls, so it is intentional for it to be a higher than normal type of privilege. If it’s not mapping correctly, is it possible that WordPress core is checking the incorrect capability in this location?

@imath
10 days ago

@imath
10 days ago

#13 @imath
10 days ago

8355.tests.patch contains unit tests for regular an multisite WordPress config.

8355.3.patch is improving 8355.2.patch adding isset checks for the administrator role and making sure only Super Admins can bp_moderate when BuddyPress is network activated. With 8355.3.patch applied tests are all successful, without 1 test is failing (Administrator should be able to edit others emails)

is it possible that WordPress core is checking the incorrect capability in this location?

I don't think so, WordPress checks for edit_post and the bp_moderate capability is added to caps to check the user against. The specific problem here is _bp_enforce_bp_moderate_cap_for_admins() checks for the requested cap and not for the allowed caps. 8355.patch is a way to fix this into _bp_enforce_bp_moderate_cap_for_admins() including specific emails caps, I guess another easier way could be to do:

$caps = array_unique( array_merge( $caps, array( $cap ) ) );

if ( ! in_array( 'bp_moderate', $caps, true ) ) {
    return $caps.
}

#14 @boonebgorges
10 days ago

Thanks, all. John's point about the 'administrator' role is a good one, both that it should be checked for, and that it must not be a common issue if it's never been reported in all these years :) 8355.3.patch still feels like the most natural solution to me, since it's a way of adding the 'bp_moderate' cap to certain roles, rather than using (abusing?) the map_meta_cap logic.

@imath
10 days ago

#15 @imath
10 days ago

I'm fine with 8355.3.patch, I believe there can be custom roles containing the bp_moderate cap but I doubt they would contain the manage_options cap. But if we're worried about custom roles having the manage_options cap, we could also check for the manage_options cap instead of the role. That's what I've tested in 8355.4.patch.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


9 days ago

Note: See TracTickets for help on using tickets.