Move permission checks in `bp_activity_screen_single_activity_permalink` into new function

[DJPaul]

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

[DJPaul]

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.

[espellcaste]

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

[espellcaste]

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

[DJPaul]

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. :)

[espellcaste]

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

[DJPaul]

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]

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

[DJPaul]

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.

[espellcaste]

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]

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

[boonebgorges]

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

[DJPaul]

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?

[espellcaste]

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. :)

[DJPaul]

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

[DJPaul]

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.

[espellcaste]

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

[DJPaul]

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]

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 :)

[DJPaul]

Related: #7655

[DJPaul]

		// 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]

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. :)

[DJPaul]

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

[DJPaul]

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.

[djpaul]

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

[r-a-y]

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]

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.