Skip to:

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#7610 closed enhancement (fixed)

Use bp_user_can for some group-related permissions.

Reported by: dcavins's profile dcavins Owned by: dcavins's profile dcavins
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9.0
Component: Groups Keywords: has-patch commit
Cc: dcavins


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 (4)

7610.01.diff (23.1 KB) - added by dcavins 7 years ago.
Use bp_current_user_can() for permission checks in the groups component.
7610.02.diff (23.8 KB) - added by dcavins 7 years ago.
Now with more strictness!
7610.groups_request_membership.patch (1.3 KB) - added by r-a-y 6 years ago.
7610-after-ray.patch (5.9 KB) - added by dcavins 6 years ago.
Add some special logic for site admins.

Download all attachments as: .zip

Change History (20)

7 years 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.

7 years ago

#2 @DJPaul
7 years 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 @DJPaul
7 years 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 @dcavins
7 years ago

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

7 years ago

Now with more strictness!

#5 @DJPaul
7 years ago

  • Keywords commit added

Much better. :)

#6 @dcavins
7 years ago

  • Owner set to dcavins
  • 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.

#7 @r-a-y
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Encountered a bug where the "Request Membership" tab was showing up on public group pages for a multisite install.

To duplicate:

  1. Ensure BuddyPress is installed on a multisite network.
  2. Login as a super admin. This is key.
  3. Navigate to any public group page.
  4. You'll see the "Request Membership" nav item in the group nav.

The reason why this is happening is super admins, by default, are allowed to do anything and thus the capability check returns true.

By the time, we check for groups_request_membership, the return value is already true.

To fix this, we should set the return value to false before doing our request membership check.

I've also written a unit test that duplicates the problem:
phpunit -c tests/phpunit/multisite.xml --group BP7610

We might need to audit the rest of the group capability checks in this ticket to ensure the default values are working as we expect them to.

Version 0, edited 6 years ago by r-a-y (next)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.

6 years ago

#9 @dcavins
6 years ago

@r-a-y Can you share the unit test you wrote? I'm not seeing it in trunk. Thanks!

#10 @r-a-y
6 years ago

@dcavins - The unit test is part of groups_request_membership.patch. Sorry for not being clear!

6 years ago

Add some special logic for site admins.

#11 @dcavins
6 years ago

Thanks, @r-a-y. I took another spin through the original patch (finding a logic inversion from the original) and fixed up two more cases where site admins should clear additional hurdles before being allowed to do something. These three capabilities have been updated (including your earlier work):
groups_receive_invitation, groups_request_membership and groups_join_group

Thanks for finding this issue and also for writing new tests. (It's so great to have tests covering these capabilities.)

If no one has any further comments, let's get these changes committed. :)

#12 @r-a-y
6 years ago

Looks good, @dcavins!

#13 @dcavins
6 years ago

In 11970:

Fix admin user cases in bp_groups_user_can_filter().

If a user is an admin, he or she is pretty much allowed to do anything, so the capability has been approved before bp_groups_user_can_filter() filters the value. In a few cases, even admins need to satisfy a few other requirements before being allowed to do something, like request membership in a group, where even site admins need to not be members of the group.

Props r-a-y, dcavins.

See #7610.

#14 @dcavins
6 years ago

In 11971:

Correct bp_user_can() logic error.

In r11776, I introduced a logic error in groups_action_join_group(). The logic should be that if a user can't simply join a group, then we check for outstadning invitations.

See #7610.

#15 @dcavins
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#16 @r-a-y
6 years ago

In 11972:

Unit Tests: After r11970, fix fatal error when running WordPress 4.5.

grant_super_admin() doesn't exist for single-site on WP 4.5.

Thanks Travis!

See #7610.

Note: See TracTickets for help on using tickets.