Skip to:
Content

Opened 8 months ago

Closed 2 months ago

Last modified 2 months ago

#7610 closed enhancement (fixed)

Use bp_user_can for some group-related permissions.

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

Description

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 8 months ago.
Use bp_current_user_can() for permission checks in the groups component.
7610.02.diff (23.8 KB) - added by dcavins 7 months ago.
Now with more strictness!
7610.groups_request_membership.patch (1.3 KB) - added by r-a-y 3 months ago.
7610-after-ray.patch (5.9 KB) - added by dcavins 2 months ago.
Add some special logic for site admins.

Download all attachments as: .zip

Change History (20)

@dcavins
8 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.


8 months ago

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

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

@dcavins
7 months ago

Now with more strictness!

#5 @DJPaul
6 months ago

  • Keywords commit added

Much better. :)

#6 @dcavins
6 months 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
3 months 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 bp_current_user_can() capability check returns true.

By the time, we check for groups_request_membership when registering the nav item, the return value is already true for super admins:
https://buddypress.trac.wordpress.org/browser/tags/3.0.0-beta1/src/bp-groups/classes/class-bp-groups-component.php#L619
https://buddypress.trac.wordpress.org/browser/tags/3.0.0-beta1/src/bp-groups/bp-groups-filters.php#L229

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.

Last edited 3 months ago by r-a-y (previous) (diff)

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


2 months ago

#9 @dcavins
2 months ago

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

#10 @r-a-y
2 months ago

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

@dcavins
2 months ago

Add some special logic for site admins.

#11 @dcavins
2 months 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
2 months ago

Looks good, @dcavins!

#13 @dcavins
2 months 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
2 months 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
2 months ago

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

#16 @r-a-y
2 months 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.