Skip to:

Opened 5 months ago

Closed 3 months ago

#7610 closed enhancement (fixed)

Use bp_user_can for some group-related permissions.

Reported by: David Cavins Owned by: David Cavins
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9.0
Component: Groups Keywords: has-patch commit
Cc: David Cavins


In many places in the groups component, we're applying checks to determine if a user can do something. These checks can be fairly complicated and are duplicated wherever they are needed. I'd like to move to a more centralized approach using bp_user_can() when possible. In addition to standardizing the permission checks, this will add a filter point that devs can use.

This also helps lay the groundwork for future changes making group statuses more flexible.

Thanks for your feedback!

Attachments (2)

7610.01.diff (23.1 KB) - added by David Cavins 5 months ago.
Use bp_current_user_can() for permission checks in the groups component.
7610.02.diff (23.8 KB) - added by David Cavins 4 months ago.
Now with more strictness!

Download all attachments as: .zip

Change History (8)

@David Cavins
5 months ago

Use bp_current_user_can() for permission checks in the groups component.

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

5 months ago

#2 @Paul Gibbs
5 months ago

Hey @dcavins
I haven't checked you've implemented the logic as per the original, but assuming that's good, the patch is pretty good. Some minor feedback:

  • in_array() third parameter should be true, for type strictness.
  • Use type strict logical operators, e.g. === instead of ==.
  • The break indentation in the switch statements is a little off in places, I think. Worth double-checking.
  • The test names should be human readable. Take test "user_can_groups_see_group_private_logged_in_user" -- I can see it's built off the capability name, but it should be readable if you see it in the terminal output. e.g. "user_can_join_private_groups" (or whatever).
  • The test names also currently read positive, if that makes sense. If a test uses assertFalse, then the test name should make it clear that it is testing something that should not happen. e.g. "anonymous_user_cannot_join_private_groups".

#3 @Paul Gibbs
4 months ago

  • Milestone changed from Awaiting Review to 3.0

Putting this in 3.0 assuming that mr @dcavins will get around to improving this patch in the next couple of months. :)

#4 @David Cavins
4 months ago

Here's an updated version with strictness and renaming/organizing/cleaning up of the tests. Thanks again for the feedback.

@David Cavins
4 months ago

Now with more strictness!

#5 @Paul Gibbs
3 months ago

  • Keywords commit added

Much better. :)

#6 @David Cavins
3 months ago

  • Owner set to David Cavins
  • Resolution set to fixed
  • Status changed from new to closed

In 11776:

Use bp_user_can for group-related permissions.

Centralize and de-duplicate permissions checks for group-related functions, like checking whether a user can join a group or send invitations from a group.

Props dcavins, djpaul.

Fixes #7610.

Note: See TracTickets for help on using tickets.