Implement user capabilities for Activity component

[DJPaul]

I've been working on a patch to switch the Activity component over to using real user capabilities. Our bp_moderate workaround is pretty old, and isn't very flexible (it was never meant to be). Real caps will let people manage permissions more granularly.

[DJPaul]

So, 7176.01.patch is a first go. It contains a bunch of stuff that would be committed individually (i.e. swapping current_user_can for bp_current_user_can), but it helps see what the change would involve. I gave it a perfunctory test comparing different capabilities for different roles, and it seems to work ok.

My concern with my patch (or lack of knowledge with implementing this) is that the capabilities work on a "take away" basis, rather than on a "grant". For example, the patch doesn't let you grant edit_bp_activities to some custom role and have it work (because the meta cap functions checks manage_options, which is a direct port of what bp_moderate does). I think this is because BP can't? use WP capabilities' primitives, so things get confusing during the meta mapping.

I'd love some feedback from anyone who knows how the roles/caps API works really well, and can help guide the patch.

I did briefly consider whether I should implement bbPress' dynamic roles, but I think that can come later. It doesn't seem a technical prerequisite at this stage, nor is it a small or easy thing to implement.

[boonebgorges]

I spent some time looking at this and hacking around with 7176.01.patch​ and with a patch of my own.

WP's Roles system is quite flawed, and I am not eager to use it. Just thinking about the activation/deactivation and other database nonsense associated with roles makes me cringe.

However, Roles plays a critical part in the way that WP's capabilities system works:

1. Most permissions checks are "derived", which means that they're mapped to "primitive" caps.
2. "Primitive" caps come in packages called "roles" - things like "Subscriber", "Editor", etc.
3. Roles are stored in the database as serialized arrays of their associated caps.
4. Users are associated with roles based on "capabilities" (bad name!) keys stored in usermeta.

Without Roles, our capability mapping system would have to map to a *WordPress* primitive role - specifically, a role that we can guarantee that all users will have. We already do this in BP, with 'exist' ​https://buddypress.trac.wordpress.org/browser/tags/2.6.0/src/bp-xprofile/bp-xprofile-caps.php#L13 (we could use read in most cases, but here we needed to cover non-logged-in users). The problem with this strategy is that the only way for plugins to modify the behavior is to filter map_meta_cap, and then reproduce all of the logic for a given capability in order to grant it in a different way. You can't simply grant or revoke a cap in order to prevent a user from doing something, since everything would map to exist or read, which you can't revoke for obvious reasons.

(bbPress, which does interesting things with this limited system, works around the fact that you can only have two layers of caps - primitive and derived - by doing user_can() checks inside of the map_meta_cap callback. This is clever, but in my experience it can cause performance issues and even "nesting limit reached" PHP fatal errors.)

For this reason, I think we should keep the concept of Roles, even if we decide not to use the full-fledged, stored-in-the-database version that WP has. Our roles might be defined in (pseudo)code like this:

'member' => array( 'bp_edit_activities', 'bp_create_activities' ... )
'admin' => 'member' + array( 'bp_edit_others_activities', 'bp_delete_others_activities' ... )

We hardcode our default roles, and allow them to be filtered, so that new roles can be added or existing ones can be modified by plugins. Our map_meta_cap() function will follow WP by mapping derivative caps to primary ones:

$caps = array();
...
case 'bp_edit_activity' :
    ...
    if ( $activity->user_id === $user_id ) {
        $caps[] = 'bp_edit_activities';
    } else {
        $caps[] = 'bp_edit_others_activities';
    }
break;
...

Users can be granted a "role" dynamically, using the user_has_cap filter:

function bp_grant_user_caps( $allcaps, $caps, $args, WP_User $user ) {
    $user_role = bp_get_user_role( $user->ID );
    $user_caps = bp_get_role_caps( $role );

    $allcaps = array_merge( $allcaps, $user_caps );
    return $allcaps;
}
add_filter( 'user_has_cap', 'bp_grant_user_caps', 10, 4 );

I can see a couple different kinds of plugins that might be built with this sort of system:

1. Create a "moderator" role that can edit others' activity, but not delete:

add_filter( 'bp_get_roles', function( $roles ) {
    $roles['mod'] = $roles['member'] + array( 'bp_edit_others_activities' );
    return $roles;
} );

2. Prevent users from being able to delete their activities:

   add_filter( 'bp_get_roles', function( $roles ) {
       unset( $roles['member']['bp_delete_activities'] );
       return $roles;
   } );
   

It's only when you're doing something very advanced - say, revoking a given cap only when an activity item meets a given criteria - that you'd need to filter map_meta_cap.

I think this is a decent compromise. It keeps the developer-facing ease-of-use of the Roles system, without mucking around with the database. The one big downside of not doing database integration is that we aren't automatically compatible with existing Role Editor plugins.

Obviously there's lots of non-working and naive pseudocode above. We may want components to register their own primitive caps, or other such niceties. But I think that I've given a rough sketch of how the system might work.

@DJPaul Does this seem like a reasonable approach? @r-a-y @johnjamesjacoby it would be helpful to have your general thoughts too, given that you've both done lots of work with custom role/cap stuff. If we like the direction, I can take the next round of iteration on 7176.01.patch​ to flesh out some of the details as I envision them.

[DJPaul]

Thanks for spending time looking into this @boonebgorges. This looks ok. I'm also interested to hear other developers' opinions.

[johnjamesjacoby]

WP's Roles system is quite flawed

This isn't really fair. WordPress's WP_Role class is one of best examples of proper class usage in WordPress core. And the $wp_roles global is a relic similar to the others.

I'd call bbPress's approach a hybrid of dynamic & persistent roles & caps. Dynamic in that the roles are registered at run-time (vs. stored in _options); persistent in that each user continues to have their bbPress role saved in usermeta along with any role they have already in the rest of the site.

Most permissions checks are "derived", which means that they're mapped to "primitive" caps.

This isn't really true. Most permissions checks are primitive, meaning they're looking for actual capabilities that a user is known to have in the database. Some caps are mapped – singles, specifically, like delete_post with a $post_id get mapped to "meta capabilities" as defined by the post object itself, like delete_published_posts, et all...

Users are associated with roles based on "capabilities" (bad name!) keys stored in usermeta.

It's not a bad name. ​(Role) and ​(capability) access control are simple, well known, and versatile ways to define a set of allowances and constraints on any system. WordPress's hybrid approach to this, coupled with map_meta_cap allowing JIT overrides, is an incredibly flexible & powerful API.

we could use read in most cases, but here we needed to cover non-logged-in users

We can't use read ever, unless we:

• Make every user a "Subscriber" to every site
• Re-init caps for the root-site on every switch_to_blog() to check our own capability mapping to read on root

For this reason, I think we should keep the concept of Roles, even if we decide not to use the full-fledged, stored-in-the-database version that WP has

We can extend the WP_Role base class for user. To extend WP_Roles would require either:

• Modifications to allow the role_key to be filtered
• A filter on get_option( $this->db->get_blog_prefix() . 'user_roles' ) which would likely not be very efficient, and kinda difficult for humans to unwind

---

Because BuddyPress has the ​(6 horsemen of the installation-type apocalypse), I'm confident (and do agree) that we are forced to off-script at least a little bit to build something that's flexible enough to work in all environments.

And we'll need to make sure bp_moderate force-allows to maintain backwards compatibility.

I have 2 separate pieces of feedback:

• About the patch
• About the general approach

---

Patch

• Capabilities are the wrong place to do name-spacing, like edit_bp_activity. The name-spaces should be: the role the capability belongs to, and the more-broad array of roles that belong to BuddyPress.
• Changes to bp_add_caps() and bp_remove_caps() look sound, even as a separate patch.
• Adding true/false to bp_get_caps_for_role() seems OK, but the Every other role part is scary. bbPress has a "Blocked" role, for example, and now BuddyPress is allowing a blocked user in (maybe that's OK?). I think it's arguably easier to keep to mapping to WordPress's known roles, since they are almost guaranteed to exist in a predictable way.
• The deprecation (and subsequent abandonment) of bp_get_community_caps() is sad to see. I think I'd imagined it as the funnel where all derived caps could be hooked in by each BuddyPress component, but I suppose whatever route we go will probably require a different approach than this one.
• For code like $activity && $user_id === $activity->user_id can we please do ! empty( $activity ) && ( $user_id === $activity->user_id ) – wrapping inline conditionals should be standard practice for all of us by now.
• I think I agree with @boonebgorges, that calling bp_add_caps() on every site is not a good idea.

---

Approach

I think I also agree with @boonebgorges, in that piggy-backing directly on-top-of WordPress's per-site role-based capability mapping system is not a great way to implement access control in an environment as complex as BuddyPress's.

Which is to say, I think we need to treat each BuddyPress component like it is it's own namespace, with it's own roles that a user may-or-may-not have, defaulting to a "Subscriber" equivalent where all users are allowed, to maintain backwards compatibility.

I think the way WordPress has a $wp_roles global, we should have bp()->roles as the place where BuddyPress components store their multi-dimensional role & capability arrays, completely outside of WordPress's, but using a similar & familiar approach.

We could also then store our own role assignments in usermeta in a way that doesn't conflict with WordPress's per-site implementation, or pollute that space later if BuddyPress is deactivated, but I'm not sure this is good or bad etiquette.

If each component has their own roles with their own caps, imagine having:

• Activity Administrator
• Group Administrator
• Friends Administrator
• Notifications Administrator
• Messages Administrator
• XProfile Administrator
• Settings Administrator
• Blogs/Sites Administrator

Admins being the role that can always see and perform all actions on all data for all users. Whether this role is stored in the user's literal wp_ROOT_BLOG_capabilities meta, or some other meta, doesn't really matter very much, so long as it's persistent to the user, and force-granted by the presence of bp_moderate.

Similar to bbPress, we could then have "Moderator", "Participant", "Spectator", and "Blocked" roles with hard-coded primitive capabilities to define what their intentions are, and allow each component to derive from them based on whatever the conditions are at the time. (I don't really care about the literal names, so much as the spirit of their ranking & abilities.)

I also think, that role & cap checks should not be flat, or part of the global namespace. bp_current_user_can( 'edit_bp_activity' ) for example, is going to break-down very quickly as soon as we want to add more crud actions to more places for more user-to-user type things.

Instead, I've always imagined something more like:

$single_activity_check = bp()->activity->current_user_can( 'edit_activity', $activity_id );

Basically, a way make each component responsible for the registration, mapping, checking, and confirming of their own relative caps. Whether bp_current_user_can() continues to be a funnel or not, will be a funny implementation detail we'll have to discover once we get started.

---

@DJPaul thanks or taking a first stab at this. User permissions are arguably one of the more difficult concepts to work with, barring maybe time & date formats. We will all get this wrong a hundred times before we get it right, so I think this is an important first step, and Activity is a great component to start with.

[boonebgorges]

Hi @johnjamesjacoby - Thanks for your thoughts. Your point-by-point defense of WP's roles API is noble :) I think we're in agreement that mirroring WP's system directly - especially the storage of role definitions in site-specific options table - is something we want to avoid.

Your ideas about fine-grained, highly configurable, component-specific roles are very cool, and I think are good as a long-term vision. The one part I don't understand conceptually is the component-specific capability methods - bp()->activity->current_user_can(). This only seems valuable if the following two statement both make sense and could return different values:

bp()->activity->current_user_can( 'edit_activity', $activity_id );
bp()->notifications->current_user_can( 'edit_activity', $activity_id );

It seems to me that the second one doesn't really make sense. The "namespacing" - the separation of concerns by component - is inherent in the fact that different capabilities (edit_activity vs edit_notification, etc) are registered by, and specific to, their components. Developers are not interested in whether a given component says that a user can perform a certain action, they are interested in whether a user can perform a certain action, period, and behind the scenes the component is responsible for answering the question.

In some cases, multiple components will be involved. I'm envisioning something like this:

// in bp-activity
function bp_activity_map_meta_caps( ... ) {
    // ...
    switch ( $cap ) {
        case 'edit_activity' :
            // checks that are specific to the activity component
        break;
    }

    return apply_filters( 'bp_activity_map_meta_caps', ... );
}

// in bp-groups - this is separate from the main meta-cap-mapper
function bp_groups_map_activity_meta_caps( ... ) {
    switch ( $cap ) {
        case 'edit_activity' :
            if ( this is a group activity ) {
                override or append the default caps based on group permissions
            }
        break;
    }

    return ....
}
add_filter( 'bp_activity_map_meta_caps', 'bp_groups_map_activity_meta_caps', 10, 4 );

In this way, bp_current_user_can( 'edit_activity', $activity_id ) will be pretty simple for developers. But behind the scenes, individual components will be responsible for managing caps.

I like the future of fine-grained, configurable roles. For the time being, I want to be sure we pick an infrastructure that lends itself to this kind of thing being built in a plugin, a process that'll become progressively easier as we make our system more sophisticated. I think that a minimum first version will involve two roles that are more or less hardcoded: one role containing the primitive caps that correspond to a regular logged-in user, and one containing all primitive caps (corresponds to bp_moderate). Roles will then be filterable. As for user-role assignment, I'd argue that for development, we can hardcode this: the users with manage_options/manage_network_options get admin role, while everyone else gets the "normal user" role. Functionally, this is identical to how we currently handle the bp_moderate cap, and I think it's a good starting point. This can also be filterable.

Does this sound like a good approach for the time being?