Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 21 months ago

#5121 closed enhancement (maybelater)

Review and improve capabilities checks

Reported by: ericlewis Owned by: johnjamesjacoby
Milestone: Priority: normal
Severity: normal Version:
Component: Core Keywords: dev-feedback, trac-tidy-2018
Cc: eric.andrew.lewis@…, mercijavier@…, espellcaste@…, stephen@…

Description

As mentioned in comment:1:ticket:5115, there are places where is_super_admin has been used in lieu of a capabilities check. This is a follow-up to that, and can serve as a catch-all ticket for reviewing and replacing prior pseudo-capabilities checks with the capabilities API.

Attachments (5)

5121.patch (13.3 KB) - added by imath 5 years ago.
5121.01.patch (3.5 KB) - added by thebrandonallen 4 years ago.
Proof-of-concept
5121.01.groups.patch (29.3 KB) - added by thebrandonallen 4 years ago.
5121.02.groups.patch (29.0 KB) - added by thebrandonallen 4 years ago.
5121.03.groups.patch (28.9 KB) - added by thebrandonallen 4 years ago.

Download all attachments as: .zip

Change History (35)

#1 @ericlewis
6 years ago

Notes from speaking with John at WordCamp contributor day:

As a first step, create a list of all the new granular caps throughout BuddyPress, and from there we can imagine the roles that may align with these capabilities.

Last edited 6 years ago by ericlewis (previous) (diff)

#2 @ericlewis
6 years ago

Groups

view_groups
view_group
view_group_members
create_groups
create_group
edit_groups
edit_group
edit_group_members
delete_groups
delete_group

Activity

publish_activity
publish_activity_comments
publish_activity_comment
favorite_activities
favorite_activity
view_activities
view_activity
delete_own_activities
delete_others_activities
delete_activity

Extended Profiles

view_members
view_member
edit_members
edit_member

Friend Connections

send_friendship_requests
send_friendship_request

Account Settings

edit_members_settings
edit_member_settings
delete_members
delete_member

Private Messaging

send_public_messages
send_private_messages
reply_to_messages
reply_to_message
view_messages
view_message
delete_messages
delete_message

#3 @ericlewis
6 years ago

  • Cc eric.andrew.lewis@… added

#4 follow-up: @DJPaul
6 years ago

  • Component changed from Core to Roles/Capability
  • Milestone changed from Awaiting Review to 2.0

So... yes. :)

I dislike punting tickets to Future Release because it becomes a graveyard, so I'm going to put it in 2.0 to see if there's anyone interested in working on this for that release. If you're interested in working on this feature to help get this built, happy to spend some time with you and we can figure out a rough schedule breakdown.

#5 in reply to: ↑ 4 @ericlewis
6 years ago

Replying to DJPaul:

If you're interested in working on this feature to help get this built, happy to spend some time with you and we can figure out a rough schedule breakdown.

Thanks for the offer! I don't have much bandwidth right now, but I'll check in at the beginning of dev cycle for 2.0 to see where we're at and if I have time to commit then.

#6 @mercime
6 years ago

  • Cc mercijavier@… added

#7 @imath
6 years ago

it would be great!

#8 @r-a-y
6 years ago

  • Milestone changed from 2.0 to 2.1

Moving this to 2.1 to see if we have any other volunteers ;)

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.


5 years ago

#10 @imath
5 years ago

  • Keywords dev-feedback added

I began writing a patch for this ticket, then i thought i should ask for your opinion about the road i took before going on, because i'm not sure it's the best one.

In 5121.patch, i'm not really using dynamic roles, it's more like dynamic caps. I'm adding a method to BP_Component class so that each component (if active) can add to the current user allcaps their specific caps, then i'm using a mapping function to adjust depending on the context.

I see an advantage doing so, as custom components can easily add their own caps directly from their extended class.

In my suggestion, i've added some code to illustrate in case of publishing/deleting an activity, creating a group or accessing the delete my account settings subnav.

@imath
5 years ago

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.


5 years ago

#12 @DJPaul
5 years ago

  • Milestone changed from 2.1 to Future Release

#13 @johnjamesjacoby
5 years ago

  • Milestone changed from Future Release to 2.3
  • Owner set to johnjamesjacoby
  • Status changed from new to accepted

#14 @r-a-y
4 years ago

  • Milestone changed from 2.3 to 2.4

Moving to 2.4.

#15 @DJPaul
4 years ago

  • Milestone changed from 2.4 to Future Release

This has been perpetually punted for the last 2 years. I am moving it back to FR until someone commits to working on this for 2.4.

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


4 years ago

#17 @thebrandonallen
4 years ago

Proof-of-concept patch built around the create_group, edit_group, and delete_group caps. I'll be around in Slack if you have any questions or concerns about the approach I've taken.

@thebrandonallen
4 years ago

Proof-of-concept

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


4 years ago

#19 @imath
4 years ago

Hi @thebrandonallen

Just gave a first look at 5121.01.patch and it looks very promising.
+1 to what boonebgorges & johnjamesjacoby said in slack about it.

Thanks a lot for your work on this very important piece (imho).

#20 @espellcaste
4 years ago

  • Cc espellcaste@… added

#21 @thebrandonallen
4 years ago

  • Type changed from defect (bug) to enhancement

Up is a first pass at adding group capabilities.

All tests still pass. We probably need more. @boone, I'm looking to you for direction here.

There are a few spots where current_user_can() is used instead of bp_current_user_can(), but since it was code added fairly recently (2.1), I left them alone for now as I was unsure if there was a specific reason for this.

The goal of this initial pass was only to remove the usage of the bp_moderate cap within groups.

The approach I've taken is completely dynamic. The caveat of this approach is that it likely won't work with role managers without additional work. On our part, or that of the plugin authors. This may not be an issue, but I wanted to make sure it was considered.

I've written a basic plugin to demonstrate how this approach might be used, which can be found here https://github.com/thebrandonallen/bp-groups-moderator-demo.

It should also be noted that, in order for attachment:5121.01.groups.patch to work, you will also need to apply the patch from #6501, until it's committed to trunk.

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


4 years ago

#23 @r-a-y
4 years ago

Nice work so far, thebrandonallen.

One thing I dislike in 5121.01.groups.patch is needing to hardcode bp_get_root_blog_id() everywhere when using bp_current_user_can().

attachment:6501.02.patch:ticket:6501 should fix this.

Then, you can omit the bp_get_root_blog_id() call:

$can = bp_current_user_can(
	'edit_group',
	array( 'group_id' => $args['item_id'], )
);

And bp_current_user_can() will automatically fallback to using the BP root blog ID.

To set a custom blog ID, you can do something like this:

$can = bp_current_user_can(
	'edit_group',
	array( 'blog_id' => 999, 'group_id' => $args['item_id'], )
);

#24 @thebrandonallen
4 years ago

@r-a-y Thanks for that! It bothered me too, but I hadn't considered what you've done.

Patch updated for attachment:6501.03.patch:ticket:6501

Last edited 4 years ago by thebrandonallen (previous) (diff)

#25 @boonebgorges
4 years ago

In 9957:

Convert second parameter of bp_current_user_can() to an $args array.

This makes the syntax simpler for the vast majority of use cases, where an
explicit blog_id doesn't need to be specified. It also makes it possible to
pass additional arguments, such as an item_id, to bp_current_user_can().

An integer passed as the second parameter will trigger the backward-
compatibility layer.

Props thebrandonallen, r-a-y.
See #5121. Fixes #6501.

#26 @johnjamesjacoby
4 years ago

Great progress so far. A few thoughts:

  • Does this belong in BP_Component, or should each component have a new file and Core a new base class?
  • What about more complex components like Groups, which may have capabilities for each sub-navigation item?
  • What about less complex components like Friends, which need many-to-many capabilities?
  • What about roles and capability groups, so each component has it's own ranking system for roles? Group(s) Administrator/Moderator/Member/Participant/Blocked
  • Like Eric Lewis started 23 months ago, what exactly can we learn & copy from WordPress posts?
  • Note that BuddyPress has it's own bp_map_meta_caps sub-filter to hook into – any reason not to use it; priority, or otherwise?

We need to start somewhere, and this momentum is good, and the approach so far seems sound. I just know between core & bbPress, the map_meta_cap space becomes very crowded, and we have a lot of future architecting to do here, and I want to make sure we nail this the first time.

Has anyone (else) looked deeply into WordPress's WP_Roles API to see if there are other optimizations or approaches worth exploring? Any other CMS's doing anything interesting in this space, where maybe an entirely new bp-capabilities component could own this entire experience?

#27 @netweb
4 years ago

  • Cc stephen@… added

#28 @DJPaul
3 years ago

  • Component changed from API - Roles/Capability to Core

#29 @DJPaul
21 months ago

  • Keywords trac-tidy-2018 added

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: https://make.wordpress.org/chat/

#30 @DJPaul
21 months ago

  • Milestone Awaiting Contributions deleted
  • Resolution set to maybelater
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.