Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#56166 new defect (bug)

get_item_permissions_check

Reported by: marijnboekel's profile marijnboekel Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 6.0
Component: Users Keywords:
Focuses: rest-api Cc:

Description

I'm using the REST Api to fetch users. The logged in user should only have access to some specific user ID's.

I'm trying to deny access to certain users by using the user_has_cap filter, but cannot get it to work.

After reading through the code from WP_REST_Users_Controller i found that the function get_item_permissions_check uses the AND && operator, while i think it should be OR ||? The ! count_user_posts( $user->ID, $types ) is always false (assuming the user has posts), so regardless of what i do in the user_has_cap, i cannot deny access.

https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445

Perhaps i'm approaching this the wrong way, maybe there is another way to achieve what i want?

Change History (9)

#1 in reply to: ↑ description ; follow-up: @anonymized_20055759
2 years ago

Hey, I looked up the users endpoint in the docs, and you can include or exclude users by ID:
https://developer.wordpress.org/rest-api/reference/users/#list-users

I haven't tried it with users specifically, but I know you can also do the same thing with posts and pages. I think that may work for you?

Replying to marijnboekel:

I'm using the REST Api to fetch users. The logged in user should only have access to some specific user ID's.

I'm trying to deny access to certain users by using the user_has_cap filter, but cannot get it to work.

After reading through the code from WP_REST_Users_Controller i found that the function get_item_permissions_check uses the AND && operator, while i think it should be OR ||? The ! count_user_posts( $user->ID, $types ) is always false (assuming the user has posts), so regardless of what i do in the user_has_cap, i cannot deny access.

https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445

Perhaps i'm approaching this the wrong way, maybe there is another way to achieve what i want?

#2 in reply to: ↑ 1 ; follow-up: @marijnboekel
2 years ago

Replying to christinavoudouris:
I understand, and i have already filtered the list returned by the list-users to only return the users the client has access too.
In this particular case i'm using the https://developer.wordpress.org/rest-api/reference/users/#retrieve-a-user endpoint

Look at it like this:

  1. Frontend requests list of users from API /wp/v2/user/ . This returns a list of users that the logged-in user has access too. (i managed to filter that request through rest_user_query filter.
  1. User clicks on of the users (at a url like 'myfrontend.tld/user/<userid>') and makes a request to the API wp/v2/user/<id>
  1. Then change the ID in the browser-url to an ID the logged-in user does not have access to. The REST Api still returns the userdata

Hey, I looked up the users endpoint in the docs, and you can include or exclude users by ID:
https://developer.wordpress.org/rest-api/reference/users/#list-users

I haven't tried it with users specifically, but I know you can also do the same thing with posts and pages. I think that may work for you?

Replying to marijnboekel:

I'm using the REST Api to fetch users. The logged in user should only have access to some specific user ID's.

I'm trying to deny access to certain users by using the user_has_cap filter, but cannot get it to work.

After reading through the code from WP_REST_Users_Controller i found that the function get_item_permissions_check uses the AND && operator, while i think it should be OR ||? The ! count_user_posts( $user->ID, $types ) is always false (assuming the user has posts), so regardless of what i do in the user_has_cap, i cannot deny access.

https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445

Perhaps i'm approaching this the wrong way, maybe there is another way to achieve what i want?

Version 0, edited 2 years ago by marijnboekel (next)

#3 in reply to: ↑ 2 ; follow-up: @anonymized_20055759
2 years ago

Replying to marijnboekel:
Well, under #1, you say you're using /wp/v2/user; you need /wp/v2/users. Unless that's just a typo. If that's the case I'm not sure if it's a bug, or it's not working because you're calling a new endpoint of /wp/v2/user/<id> later on.

I understand, and i have already filtered the list returned by the list-users to only return the users the client has access too.
In this particular case i'm using the https://developer.wordpress.org/rest-api/reference/users/#retrieve-a-user endpoint

Look at it like this:

  1. Frontend requests list of users from API /wp/v2/user/ . This returns a list of users that the logged-in user has access too. (i managed to filter that request through rest_user_query filter.
  1. User clicks on of the users (at a url like 'myfrontend.tld/user/<userid>') and makes a request to the API wp/v2/user/<id>
  1. Then change the ID in the browser-url to an ID the logged-in user does not have access to. The REST Api still returns the userdata

Replying to christinavoudouris:

Hey, I looked up the users endpoint in the docs, and you can include or exclude users by ID:
https://developer.wordpress.org/rest-api/reference/users/#list-users

I haven't tried it with users specifically, but I know you can also do the same thing with posts and pages. I think that may work for you?

Replying to marijnboekel:

I'm using the REST Api to fetch users. The logged in user should only have access to some specific user ID's.

I'm trying to deny access to certain users by using the user_has_cap filter, but cannot get it to work.

After reading through the code from WP_REST_Users_Controller i found that the function get_item_permissions_check uses the AND && operator, while i think it should be OR ||? The ! count_user_posts( $user->ID, $types ) is always false (assuming the user has posts), so regardless of what i do in the user_has_cap, i cannot deny access.

https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445

Perhaps i'm approaching this the wrong way, maybe there is another way to achieve what i want?

#4 in reply to: ↑ 3 ; follow-up: @marijnboekel
2 years ago

My bad, typo. The requests are made against wp/v2/users and wp/v2/users/:id
I am getting results so the API is working.
I'm just unable to deny access to specific /users/:id endpoints depending on the logged-in user, using the user_has_cap filter.
I think that is because of an incorrect if-statement on this line:https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445
I think it should be something like

if ( ! count_user_posts( $user->ID, $types ) || ( ! current_user_can( 'edit_user', $user->ID ) && ! current_user_can( 'list_users' ) ) )

instead of

if ( ! count_user_posts( $user->ID, $types ) && ! current_user_can( 'edit_user', $user->ID ) && ! current_user_can( 'list_users' ) )

Replying to christinavoudouris:

Replying to marijnboekel:
Well, under #1, you say you're using /wp/v2/user; you need /wp/v2/users. Unless that's just a typo. If that's the case I'm not sure if it's a bug, or it's not working because you're calling a new endpoint of /wp/v2/user/<id> later on.

I understand, and i have already filtered the list returned by the list-users to only return the users the client has access too.
In this particular case i'm using the https://developer.wordpress.org/rest-api/reference/users/#retrieve-a-user endpoint

Look at it like this:

  1. Frontend requests list of users from API /wp/v2/user/ . This returns a list of users that the logged-in user has access too. (i managed to filter that request through rest_user_query filter.
  1. User clicks on of the users (at a url like 'myfrontend.tld/user/<userid>') and makes a request to the API wp/v2/user/<id>
  1. Then change the ID in the browser-url to an ID the logged-in user does not have access to. The REST Api still returns the userdata

Replying to christinavoudouris:

Hey, I looked up the users endpoint in the docs, and you can include or exclude users by ID:
https://developer.wordpress.org/rest-api/reference/users/#list-users

I haven't tried it with users specifically, but I know you can also do the same thing with posts and pages. I think that may work for you?

Replying to marijnboekel:

I'm using the REST Api to fetch users. The logged in user should only have access to some specific user ID's.

I'm trying to deny access to certain users by using the user_has_cap filter, but cannot get it to work.

After reading through the code from WP_REST_Users_Controller i found that the function get_item_permissions_check uses the AND && operator, while i think it should be OR ||? The ! count_user_posts( $user->ID, $types ) is always false (assuming the user has posts), so regardless of what i do in the user_has_cap, i cannot deny access.

https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445

Perhaps i'm approaching this the wrong way, maybe there is another way to achieve what i want?

#5 in reply to: ↑ 4 ; follow-up: @anonymized_20055759
2 years ago

Replying to marijnboekel:
You may be right. I am not familiar enough with PHP so that code would have to be tested. I suggest since you already have your app set up, to clone the repository, make a branch with your edit and try it locally. If there is a reason to keep the original code, your solution could still be added as an option.

My bad, typo. The requests are made against wp/v2/users and wp/v2/users/:id
I am getting results so the API is working.
I'm just unable to deny access to specific /users/:id endpoints depending on the logged-in user, using the user_has_cap filter.
I think that is because of an incorrect if-statement on this line:https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445
I think it should be something like

if ( ! count_user_posts( $user->ID, $types ) || ( ! current_user_can( 'edit_user', $user->ID ) && ! current_user_can( 'list_users' ) ) )

instead of

if ( ! count_user_posts( $user->ID, $types ) && ! current_user_can( 'edit_user', $user->ID ) && ! current_user_can( 'list_users' ) )

Replying to christinavoudouris:

Replying to marijnboekel:
Well, under #1, you say you're using /wp/v2/user; you need /wp/v2/users. Unless that's just a typo. If that's the case I'm not sure if it's a bug, or it's not working because you're calling a new endpoint of /wp/v2/user/<id> later on.

I understand, and i have already filtered the list returned by the list-users to only return the users the client has access too.
In this particular case i'm using the https://developer.wordpress.org/rest-api/reference/users/#retrieve-a-user endpoint

Look at it like this:

  1. Frontend requests list of users from API /wp/v2/user/ . This returns a list of users that the logged-in user has access too. (i managed to filter that request through rest_user_query filter.
  1. User clicks on of the users (at a url like 'myfrontend.tld/user/<userid>') and makes a request to the API wp/v2/user/<id>
  1. Then change the ID in the browser-url to an ID the logged-in user does not have access to. The REST Api still returns the userdata

Replying to christinavoudouris:

Hey, I looked up the users endpoint in the docs, and you can include or exclude users by ID:
https://developer.wordpress.org/rest-api/reference/users/#list-users

I haven't tried it with users specifically, but I know you can also do the same thing with posts and pages. I think that may work for you?

Replying to marijnboekel:

I'm using the REST Api to fetch users. The logged in user should only have access to some specific user ID's.

I'm trying to deny access to certain users by using the user_has_cap filter, but cannot get it to work.

After reading through the code from WP_REST_Users_Controller i found that the function get_item_permissions_check uses the AND && operator, while i think it should be OR ||? The ! count_user_posts( $user->ID, $types ) is always false (assuming the user has posts), so regardless of what i do in the user_has_cap, i cannot deny access.

https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445

Perhaps i'm approaching this the wrong way, maybe there is another way to achieve what i want?

#6 in reply to: ↑ 5 ; follow-up: @marijnboekel
2 years ago

I have tested it locally in different ways (i.e. filtering count_user_posts etc)

I'm trying to understand why this count_user_posts is there in the first place.
In my opinion we should be able to fetch a user, regardless if the user has zero posts.
if so, we could eliminate the count_user_posts entirely

Replying to christinavoudouris:

Replying to marijnboekel:
You may be right. I am not familiar enough with PHP so that code would have to be tested. I suggest since you already have your app set up, to clone the repository, make a branch with your edit and try it locally. If there is a reason to keep the original code, your solution could still be added as an option.

My bad, typo. The requests are made against wp/v2/users and wp/v2/users/:id
I am getting results so the API is working.
I'm just unable to deny access to specific /users/:id endpoints depending on the logged-in user, using the user_has_cap filter.
I think that is because of an incorrect if-statement on this line:https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445
I think it should be something like

if ( ! count_user_posts( $user->ID, $types ) || ( ! current_user_can( 'edit_user', $user->ID ) && ! current_user_can( 'list_users' ) ) )

instead of

if ( ! count_user_posts( $user->ID, $types ) && ! current_user_can( 'edit_user', $user->ID ) && ! current_user_can( 'list_users' ) )

Replying to christinavoudouris:

Replying to marijnboekel:
Well, under #1, you say you're using /wp/v2/user; you need /wp/v2/users. Unless that's just a typo. If that's the case I'm not sure if it's a bug, or it's not working because you're calling a new endpoint of /wp/v2/user/<id> later on.

I understand, and i have already filtered the list returned by the list-users to only return the users the client has access too.
In this particular case i'm using the https://developer.wordpress.org/rest-api/reference/users/#retrieve-a-user endpoint

Look at it like this:

  1. Frontend requests list of users from API /wp/v2/user/ . This returns a list of users that the logged-in user has access too. (i managed to filter that request through rest_user_query filter.
  1. User clicks on of the users (at a url like 'myfrontend.tld/user/<userid>') and makes a request to the API wp/v2/user/<id>
  1. Then change the ID in the browser-url to an ID the logged-in user does not have access to. The REST Api still returns the userdata

Replying to christinavoudouris:

Hey, I looked up the users endpoint in the docs, and you can include or exclude users by ID:
https://developer.wordpress.org/rest-api/reference/users/#list-users

I haven't tried it with users specifically, but I know you can also do the same thing with posts and pages. I think that may work for you?

Replying to marijnboekel:

I'm using the REST Api to fetch users. The logged in user should only have access to some specific user ID's.

I'm trying to deny access to certain users by using the user_has_cap filter, but cannot get it to work.

After reading through the code from WP_REST_Users_Controller i found that the function get_item_permissions_check uses the AND && operator, while i think it should be OR ||? The ! count_user_posts( $user->ID, $types ) is always false (assuming the user has posts), so regardless of what i do in the user_has_cap, i cannot deny access.

https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/rest-api/endpoints/class-wp-rest-users-controller.php#L445

Perhaps i'm approaching this the wrong way, maybe there is another way to achieve what i want?

#7 in reply to: ↑ 6 ; follow-up: @SergeyBiryukov
2 years ago

Hi there, welcome to WordPress Trac! Thanks for the ticket.

Replying to marijnboekel:

I'm trying to understand why this count_user_posts is there in the first place.
In my opinion we should be able to fetch a user, regardless if the user has zero posts.
if so, we could eliminate the count_user_posts entirely

I have tracked down the count_user_posts() check to this commit, when REST API was still a feature plugin.

It was later merged to core in [38832] / #38373. Based on this part of the commit message:

Users: Read and write access to all user data. This includes public access to some data for post authors.

I think the check is there because user profiles of authors with published posts are considered public and can be viewed regardless of current user's permission. For that purpose, the check looks correct to me as is.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#8 @SergeyBiryukov
2 years ago

  • Component changed from REST API to Users

Moving to the Users component, while keeping the rest-api focus.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#9 in reply to: ↑ 7 @marijnboekel
2 years ago

Hi Sergey, thanks for your reply!

Replying to SergeyBiryukov:

It was later merged to core in [38832] / #38373. Based on this part of the commit message:

Users: Read and write access to all user data. This includes public access to some data for post authors.

I think the check is there because user profiles of authors with published posts are considered public and can be viewed regardless of current user's permission. For that purpose, the check looks correct to me as is.

From that perspective it makes sense, thanks for clearing that up :) Although you could consider private and public post-types (in my case i only work with private custom post types)

Do you have another approach i can make in order to block access to specific user-requests? I can't find any other usefull hooks or filters. I'm thinking to overwrite rest_prepare_user and add some conditions there, but it would be nicer to check this in the permissions check.

Note: See TracTickets for help on using tickets.