Opened 2 years ago
Last modified 2 years ago
#56166 new defect (bug)
get_item_permissions_check
Reported by: | 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.
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:
↓ 2
@
2 years ago
#2
in reply to:
↑ 1
;
follow-up:
↓ 3
@
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:
- 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.
- 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>
- 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 functionget_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 theuser_has_cap
, i cannot deny access.
Perhaps i'm approaching this the wrong way, maybe there is another way to achieve what i want?
#3
in reply to:
↑ 2
;
follow-up:
↓ 4
@
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:
- 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.
- 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>
- 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 functionget_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 theuser_has_cap
, i cannot deny access.
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:
↓ 5
@
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:
- 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.
- 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>
- 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 functionget_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 theuser_has_cap
, i cannot deny access.
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:
↓ 6
@
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 theuser_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:
- 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.
- 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>
- 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 functionget_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 theuser_has_cap
, i cannot deny access.
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:
↓ 7
@
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 theuser_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:
- 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.
- 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>
- 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 functionget_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 theuser_has_cap
, i cannot deny access.
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:
↓ 9
@
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.
#8
@
2 years ago
- Component changed from REST API to Users
Moving to the Users component, while keeping the rest-api
focus.
#9
in reply to:
↑ 7
@
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.
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: