buddypress/BP-REST

Non-moderator user can't see notifications

Closed this issue · 7 comments

When a user doesn't have the bp_moderate capability, they will not be able to see any notifications.

This is due to using can_see() in:

https://github.com/buddypress/BP-REST/blob/master/includes/bp-notifications/classes/class-bp-rest-notifications-endpoint.php#L169

Which becomes 0 by default, which then fails this test:

https://github.com/buddypress/BP-REST/blob/master/includes/bp-notifications/classes/class-bp-rest-notifications-endpoint.php#L675

I believe this can be fixed by changing the condition to this:

if ( empty( $notification_id ) || (bool) BP_Notifications_Notification::check_access( bp_loggedin_user_id(), $notification_id ) ) {

as anyone should have access to notification_id 0 (all notifications)

On the surface, that makes sense but I'm not sure that's how BuddyPress works internally. Let me do a bit of digging to confirm something. When I added this, I remember I mirrored the BuddyPress implementation, so there was a reason of why an empty notification was not valid.

The alternative is to explicitly provide notification_id -1 or a new function without that specific check.

I reviewed the BuddyPress code again and the reason I had not added the option so that anyone could have access to get all notifications was because BuddyPerss doesn't allow it too. All their getters, helper functions and query classes, relies on a user_id being passed.

Meaning you can only get your notifications, not everybody else.

At the time, this made total sense but maybe I missed it there since in REST one might need to fetch all notifications for a UI, for example.

I'm going to propose that in a new ticket for the BuddyPress team and then update the REST API here with that if approved.

Ops, by the way, I fixed an issue at #373 where even when a user was logged in, it would not be able to see their notifications. Maybe that's the issue you were facing before.

@imath What are your thoughts about this issue?

imath commented

Let's do what is done in Core: users can only their notifications 👌

Cool! That makes sense to me! Closing then!