Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 22 months ago

Last modified 20 months ago

#7048 closed enhancement (fixed)

Move permission checks in `bp_activity_screen_single_activity_permalink` into new function

Reported by: DJPaul Owned by: djpaul
Milestone: 3.0 Priority: high
Severity: normal Version:
Component: Activity Keywords: has-patch has-unit-tests
Cc: espellcaste@…

Description

We need to re-use the identical logic for access checks in the BP REST API for the Activity component. See https://github.com/buddypress/BP-REST/issues/28#issuecomment-216719183

Attachments (8)

7048.diff (7.3 KB) - added by espellcaste 22 months ago.
7048-2.diff (7.8 KB) - added by espellcaste 22 months ago.
7048-3.diff (14.9 KB) - added by espellcaste 22 months ago.
7048-4.diff (7.8 KB) - added by espellcaste 22 months ago.
7048-5.diff (9.4 KB) - added by espellcaste 22 months ago.
7048-6.diff (9.1 KB) - added by espellcaste 22 months ago.
7048-7.diff (9.3 KB) - added by espellcaste 22 months ago.
7048-8.diff (1.6 KB) - added by r-a-y 20 months ago.

Download all attachments as: .zip

Change History (40)

#1 @DJPaul
3 years ago

  • Keywords needs-patch added
  • Owner set to DJPaul
  • Status changed from new to assigned

Anyone is welcome to do this but otherwise I will get this done for 2.6 one way or another as it's required for our REST API implementation.

#2 @DJPaul
3 years ago

  • Milestone changed from 2.6 to Future Release

#3 @espellcaste
3 years ago

  • Cc espellcaste@… added

#4 @espellcaste
3 years ago

I'll see what I can do with... PR soon!

Version 0, edited 3 years ago by espellcaste (next)

#5 @espellcaste
23 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Future Release to 3.0

@DJPaul Could you take a look at how I'm using it here: https://github.com/buddypress/BP-REST/commit/b24743f1ff005678a3c57cd79835df6109541246

Also, I'd like some suggestions for the location where this function will lie in BuddyPress activity folder and names for the function.

Last edited 23 months ago by espellcaste (previous) (diff)

#6 @DJPaul
23 months ago

Old ticket and I don't have the details in my mind, but something like that, yes.

Into bp-activity/bp-activity-functions.php, don't know about function name. :)

#7 @DJPaul
22 months ago

  • Owner DJPaul deleted

#8 @espellcaste
22 months ago

I'll be sending the patch this week! Thanks for reminding me via ticket update notification. :)

@espellcaste
22 months ago

#9 @espellcaste
22 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

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


22 months ago

#11 @DJPaul
22 months ago

The unit tests test_bp_activity_permalink_access tests multiple things, so should be split up into smaller separate tests.
The test names should also not be the same as a function or a class. Make them readable by humans -- something like "test_user_can_access_their_own_activity", "test_user_cant_access_someone_elses_activity". That way, when they fail or pass, you can see in the terminal output what the purpose of the test was that has passed or failed (no need to open the code just to find that out).

Not sure why get_activity_object() was made in the tests, can we not use the activity factory?

@espellcaste
22 months ago

#12 @espellcaste
22 months ago

I didn't bother to look for the get_object_by_id support at first. ;)

#13 @DJPaul
22 months ago

Looking at the new function, I'm not sure the name "permalink access" is correct -- it's not specifically dealing with the permalink. Perhaps something like bp_activity_user_can_read()? Maybe @boonebgorges has an idea.

What do you think about moving the "If activity is from a group, do an extra cap check" underneath "If activity author match user, allow access as well", and add a check - e.g. if ( ! $retval && bp_is_active( 'groups' ) && $activity->component === $bp->groups->id ) { -- I'm not sure if $retval = $group->user_has_access; could ever be false and so overwrite a true we explicitly set earlier, but this would avoid that.

And finally for the PHPDoc throughout, $activity is probably of type BP_Activity_Activity so we can use that to describe it instead of the more general object.

Last edited 22 months ago by DJPaul (previous) (diff)

#14 @espellcaste
22 months ago

Updated as requested! :)

On the group access note, there are instances it can be false: https://github.com/renatonascalves/BuddyPress/blob/master/src/bp-groups/classes/class-bp-groups-group.php#L614

@espellcaste
22 months ago

@espellcaste
22 months ago

#15 @espellcaste
22 months ago

The last one is the correct one. I missed updating the filter with the new function name. Feedback welcome! :)

Last edited 22 months ago by espellcaste (previous) (diff)

#16 @boonebgorges
22 months ago

bp_activity_user_can_read() seems good to me - it follows our conventions elsewhere in BP.

#17 @DJPaul
22 months ago

I withdraw my !$retval suggestion because it could be the creator of a (group) activity could subsequently be removed from that group and then should not be able to see it — right?

Probably just move the cap check below that entire block?

#18 @espellcaste
22 months ago

No! He would still be able to see it.

( $user_id === $activity->user_id ) would return true and would bypass the group cap check, ultimately showing the activity for its creator.

Another way of looking at this:

  • If the user is an admin/moderator, allow access.
  • Allow access to its creator.

If the group component is active and it is a group activity:

  • $group->user_has_access Allow access to members of this particular group and admins/moderators.
  • Allow access to group moderators and admins.

The last one is a double check. Someone could argue it is a duplicate, but I'd rather keep it in case the user does not have the bp_current_user_can( 'bp_moderate' ) cap. :)

#19 @DJPaul
22 months ago

I have spent over an hour this morning looking at the patch very carefully. :)

The bp_do_404() bit needs to be added back: that protects against accessing a Group Activity when the Groups component is disabled. I suspect that's why the otherwise incorrect-looking isset($bp->groups->id) is in there. Otherwise the patch leaks private/hidden Group Activity items.

As part of the above, I think the order of the clauses should be flipped back, and that !$retval suggestion I made should be taken out -- see https://imgur.com/a/UUoPB (left is current trunk, right is patch).

#20 @DJPaul
22 months ago

The bp_do_404() bit needs to be added back

Specifically, both BP and its Activity REST API needs to prevent access to Group Activity items when the Groups component is disabled. On the website, because the content basically doesn't exist, we need to throw a 404. It may be that the new bp_activity_user_can_read() function is not the best place to do this.

Last edited 22 months ago by DJPaul (previous) (diff)

#21 @espellcaste
22 months ago

@DJPaul The current implementation took that into account.

if ( ! $retval && bp_is_active( 'groups' ) && $activity->component === $bp->groups->id ) {

This line check if the group component is active and if the current activity is group related. The isset($bp->groups->id part was a mistake in my opinion, used after the bp_is_active( 'groups' ) was not necessary.

Otherwise the patch leaks private/hidden Group Activity items

It only shows to those that we allow access to, regardless if it is private or not.
It's worth mentioning this is not blocking user to see the group, it is allowing/blocking to see the activity url.

Does that mean users that created activities and later the group was disabled cannot see, via url, their activities and its comments?
Is it removed from his notification feed, history, after a group is disabled? So that he can go see and see a 404 page?

needs to prevent access to Group Activity items when the Groups component is disabled

So no access to its creators, admins, mods?? :/

$retval = $group->user_has_access;

Then it goes to check if the user has access to the group activity.
So no leakage here.

Last edited 22 months ago by espellcaste (previous) (diff)

#22 @DJPaul
22 months ago

Does that mean users that created activities and later the group was disabled cannot see, via url, their activities and its comments?
Is it removed from his notification feed, history, after a group is disabled? So that he can go see and see a 404 page?

The easiest way to find out is test this, but I'm pretty sure this should be happening currently.

@espellcaste
22 months ago

#23 @espellcaste
22 months ago

New patch adds some improvements.

It adds the tests requested showing a user can see an activity after the group status is updated to hidden or private.

Also, I added a check to not allow non-members from a group to see its activities when the group is public. This is something https://github.com/renatonascalves/BuddyPress/blob/master/src/bp-groups/classes/class-bp-groups-group.php#L614 does by default and it has its reasons but for this particular use case, we won't allow that.

Let me know what you think @DJPaul :)

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


22 months ago

#25 @DJPaul
22 months ago

Related: #7655

#26 @DJPaul
22 months ago

		// Non-members have no access.
		if ( ! groups_is_user_member( $user_id, $group_id ) ) {
			$retval = false;
		}

		// Group admins and mods have access as well.
		if ( groups_is_user_admin( $user_id, $group_id ) || groups_is_user_mod( $user_id, $group_id ) ) {
			$retval = true;
		}

These are redundant. The $group->user_has_access takes care of groups_is_user_member, groups_is_user_admin, groups_is_user_mod. I've verified this.

@espellcaste
22 months ago

#27 @espellcaste
22 months ago

Strange.

If you remove if ( ! groups_is_user_member( $user_id, $group_id ) ) {.
And run PHPUnit test_non_member_cannot_access_to_someone_elses_activity_in_a_group will give you an error. Suggesting a user outside of a public group can see other people's activities.

I presume that's the correct approach. Hence, I updated above with your suggestion. The comment above is just so that you are aware. :)

Last edited 22 months ago by espellcaste (previous) (diff)

#28 @DJPaul
22 months ago

In bp_activity_screen_single_activity_permalink(), the following needs to be added to prevent a regression:

	// If activity author does not match displayed user, block access.
	if ( true === $has_access && bp_displayed_user_id() !== $activity->user_id ) {
		$has_access = false;
	}

This prevents accessing someone else's (public) activity item at the wrong URL.

e.g. http://bpcore.local/members/admin/activity/25/
activity 25 belongs to "admin"

http://bpcore.local/members/another_user/activity/25/

  • with the patch, this works (incorrectly - duplicating content at multiple URLs)
  • without the patch, it redirects you to your profile and says you do not have access (correct).

Locally I tested putting this back in just after the $has_access = apply_filters line, and it fixed it.

(Activity 25 is a activity_update - no group.)

Last edited 22 months ago by DJPaul (previous) (diff)

#29 @DJPaul
22 months ago

stuff about test_non_member_cannot_access_to_someone_elses_activity_in_a_group

I believe so, yes. It's confusing. We'll end up tweaking this function when we do #7655 anyway, I think.

@espellcaste
22 months ago

#30 @djpaul
22 months ago

  • Owner set to djpaul
  • Resolution set to fixed
  • Status changed from assigned to closed

In 11806:

Activity: add function to check if a user has access to a single activity.

This change extracts the existing logic from bp_activity_screen_single_activity_permalink() into a new function, allowing it to be used in multiple places, such as the REST API, or a WP-CLI extension.

Fixes #7048

Props espellcaste

#31 @r-a-y
20 months ago

Re-opening because at the moment, anyone can access private group activity permalinks.

The problem is we're passing the displayed user ID instead of checking against the current user ID in bp_activity_user_can_read().

7048-8.diff also fixes an issue with the login URL redirect for logged-out users.

@r-a-y
20 months ago

#32 @r-a-y
20 months ago

In 11881:

Activity: After r11806, fix read access checks to private activity items.

Previously, after r11806, anyone could access private activity items.

The problem is passing the displayed user ID instead of the current user
ID in bp_activity_user_can_read().

Commit also fixes an issue with the login URL redirect for logged-out users.

Fixes #7048.

Note: See TracTickets for help on using tickets.