Opened 8 years ago
Closed 8 years ago
#39701 closed defect (bug) (fixed)
Do not allow editing users from a different site in REST API
Reported by: | flixos90 | Owned by: | flixos90 |
---|---|---|---|
Milestone: | 4.7.3 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | has-patch has-unit-tests fixed-major |
Focuses: | multisite | Cc: |
Description
Currently it is possible to edit any user via the REST API when sending a request to wp-json/wp/v2/users/<id>
, even when the user with that ID is not part of the current site. As discussed in multisite office-hours, this is not desired and considered a bug. Only users of the site where the route is accessed should be editable.
Managing users beyond a single site will only become available in a future release, and it will work differently than this.
Attachments (2)
Change History (31)
#3
@
8 years ago
- Keywords has-patch has-unit-tests added; needs-patch removed
39701.diff is the patch where I've implemented my two suggestions from the above comment.
- Only super admins can
GET
users from another site. - Nobody can add an existing user to the current site (via
UPDATE
). This is handled by returning an error if roles of a user who is not part of the site should be updated.
The patch also includes 5 tests to verify everything works as expected. One existing test has been basically reversed because it was assuming that a user would automatically be added to the current site in an UPDATE
request. So this change is obviously backward-incompatible, but I think that's okay since it was a bug before.
Ping @jeremyfelt and @jnylen0 for feedback.
#4
@
8 years ago
Shouldn't super admins be allowed to add an existing user to any site they want, current or otherwise?
#5
@
8 years ago
Shouldn't super admins be allowed to add an existing user to any site they want, current or otherwise?
@johnjamesjacoby Totally, but currently the REST API doesn't allow removing at all, and we need to put some thought into how adding and removing should be handled. Since the current implementation is probably far from optimal, I would rather remove it now in 4.7.3 so we have all room for our better ideas.
#6
@
8 years ago
Thought did go into it, and this is what the original authors thought was best, even if we don't agree ourselves. :)
If it's a bug, we should fix the bug, but that doesn't seem to be the case.
If this is just the way the v1
API works, we can't change it because it's a public API. If anything, parity with core functions to restrict it to super admins seems like the bug fix that's least likely to cause breakage.
We've been lucky to be able to take some liberty with private multisite APIs, but public ones are pretty much for life until deprecated. If we're deprecating this already, I imagine we'll want to run that past the REST API team to discuss what that looks like in core.
#7
@
8 years ago
We talked a bit about a global
parameter during multisite office hours one week that could help with context switching.
I think that matching existing wp-admin/
behavior here makes sense, which somewhat makes this a bug. Only users with edit_users
can edit other users that are members of the current site or network depending on which admin screen the action is being performed on.
I'd be okay with allowing the edit of a user that is not a member of the current site if a global
parameter is passed so that intention is clear. Ideally there would also be another parameter that said "and add this user to this site" so that global users could be managed from any site's endpoint.
We may be okay in breaking back-compat here (with guidance from the REST API team), but if we do then we need to really make sure it's the decision that we want.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
8 years ago
#9
@
8 years ago
REST API support for multisite is almost non-existent so far. It seems to me that the few features that exist today are more likely to cause a support burden in the future than to be useful.
I agree with this:
Since the current implementation is probably far from optimal, I would rather remove it now in 4.7.3 so we have all room for our better ideas.
This ticket was mentioned in Slack in #core by jnylen. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
8 years ago
#13
@
8 years ago
- Owner changed from flixos90 to jnylen0
- Status changed from assigned to reviewing
After the discussion in today's office hours we decided to do the following for 4.7.3:
- Fail when
GET
to/users/<id>
and that user is not part of the current site. - Fail when
PUT
to/users/<id>
and that user is not part of the current site.
In addition, I think the DELETE
request to /users/<id>
should fail in a similar way. It already fails now as it is not supported on multisite, but it should return the same type of error response if the user is not part of the current site.
I implemented this behavior in 39701.2.diff, including updated unit tests. I adjusted another existing unit test and removed another one entirely as it didn't make sense anymore. I decided to return a 404 error, since multisite is not really supported and in single site scope that user simply does not exist.
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
8 years ago
#16
@
8 years ago
39701.2.diff is looking good, thanks @flixos90.
User configuration:
- User 1 is a member of sites 1 and 2 and is a published author on both sites.
- User 2 is a member of site 1.
- User 3 is a member of site 2.
- User 4 does not exist.
When unauthenticated:
GET
tosite.com/site-two/wp-json/wp/v2/users/1
returns user information.PUT
tosite.com/site-two/wp-json/wp/v2/users/1
returns arest_cannot_edit
error.GET
tosite.com/site-two/wp-json/wp/v2/users/2
returns arest_user_invalid_id
error.PUT
tosite.com/site-two/wp-json/wp/v2/users/2
returns arest_user_invalid_id
error.GET
tosite.com/site-two/wp-json/wp/v2/users/3
returns arest_user_cannot_view
error.PUT
tosite.com/site-two/wp-json/wp/v2/users/3
returns arest_cannot_edit
error.GET
tosite.com/site-two/wp-json/wp/v2/users/4
returns arest_user_invalid_id
error.PUT
tosite.com/site-two/wp-json/wp/v2/users/4
returns arest_user_invalid_id
error.
When authenticated:
GET
tosite.com/site-two/wp-json/wp/v2/users/1
returns user information.PUT
tosite.com/site-two/wp-json/wp/v2/users/1
with new nickname updates nickname.GET
tosite.com/site-two/wp-json/wp/v2/users/2
returns arest_user_invalid_id
error.PUT
tosite.com/site-two/wp-json/wp/v2/users/2
returns arest_user_invalid_id
error.GET
tosite.com/site-two/wp-json/wp/v2/users/3
returns user information.PUT
tosite.com/site-two/wp-json/wp/v2/users/3
with new nickname updates nickname.GET
tosite.com/site-two/wp-json/wp/v2/users/4
returns arest_user_invalid_id
error.PUT
tosite.com/site-two/wp-json/wp/v2/users/4
returns arest_user_invalid_id
error.
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
8 years ago
#18
@
8 years ago
- Keywords commit added
- Status changed from reviewing to accepted
This is good to go for 4.7.3. We should get it committed to trunk within the next day or so, @flixos90 do you want to do that or shall I?
#19
@
8 years ago
- Owner changed from jnylen0 to flixos90
Thanks for reviewing @jnylen0! I'll commit it today.
#20
@
8 years ago
I see that test_update_existing_network_user_on_sub_site_adds_user_to_site()
is removed, does that mean I can't add an existing user to a site anymore?
#21
@
8 years ago
@ocean90 Correct, that has been removed since we need to work on a real implementation for adding/removing users to/from sites. As discussed before, the way the API currently handles this was not put much effort into for 4.7 as other issues were more important. Multisite functionality should not be considered supported at this point, and we would definitely bring this functionality back in a better way in 4.8.
#22
@
8 years ago
- Keywords commit removed
This type of change reminds me of a question I had asked before the merge.
I've one concern/question about the REST API. Is there already a document how we deal with future API changes? Does a change mean we've to bump it to v2.1 or v3? Do we need a special deprecation policy?
I don't think this question was ever answered which is kind of a bummer. Please point me to a public document if I'm wrong.
This and [40078] are breaking our back-compat rule. While we did type changes in the past we always documented the changes with a changelog entry in a @since
annotation. The patch and [40078] don't include anything like that.
I'm worried about the fact that changes like these are harming the reliability of the API. It's no longer a plugin, it's part of core. It's not beta, it's a stable version (at least that's my assumption, otherwise the API wouldn't be in core).
At the very least, can we create a page which mentions all the API changes at https://developer.wordpress.org/rest-api/changelog (or something similar)? Keep in mind that the API allows other services to interact with WordPress whose maintainer don't necessarily read our make blogs.
The REST API already supports rest_handle_deprecated_function()
and rest_handle_deprecated_argument()
can we use that here or introduce a new header which contains a deprecation notice?
#23
@
8 years ago
The current multisite handling in the users endpoint makes little to no sense. It's not something we can remotely consider enabling on WP.com, for example. I'd like to get to a place where we can.
This, and a few other specific parts of the REST API never should have shipped in 4.7. They're things we wish we would have addressed, but didn't have time. IMO, not making these fixes soon after 4.7 is more harmful to the future reliability and maintainability of the API.
The necessary first step for this particular change is to have the REST API default to single-site mode for all operations, then add multisite support in a careful and reasoned manner. Our public document for this specific change is here: https://make.wordpress.org/core/2017/02/08/improving-the-rest-api-users-endpoint-for-multisite-in-4-7-3-and-4-8/
I'm not sure that we ever created a public document for larger API changes after 4.7, but at the very least it's been discussed in dev chats, and @pento and I agreed that we would make limited backwards-incompatible fixes into the first few 4.7.x releases to provide a solid foundation for future development.
The REST API still has a few ugly quirks remaining that we need to fix ASAP before people start depending on these behaviors. I wish we had caught and fixed them all before 4.7, but we didn't. If I had anticipated that we would lose the capability to make these fixes, I would have pushed back much harder about including the API in 4.7.
At the very least, can we create a page which mentions all the API changes at https://developer.wordpress.org/rest-api/changelog (or something similar)?
I'm OK with this, as well as adding any missing @since
annotations. I'll get the page started within the next few days.
I don't expect this specific change to be a cause for concern because we are doing it early enough in the history of the REST API, which is really important. If I'm wrong about that, then let's address it by creating a plugin that preserves the ability to add users in multisite.
To summarize, this change is something that should've been done before 4.7, and if we don't do this first step now, it essentially means that the REST API will never support multisite installations in a sane and consistent manner.
#24
follow-up:
↓ 26
@
8 years ago
I created a "Changelog" document for the REST API handbook. Here is a preview while it is awaiting review:
@ocean90 in the absence of further feedback I would like to move forward with this change. It has been planned and documented for several weeks now.
#25
@
8 years ago
- Keywords commit added
I agree with @jnylen0 here. While it's certainly not optimal that this behavior has been introduced at all, it would be much worse if we had to live with it long-term, so I think it's a necessary fix. I think providing a changelog like the above will be sufficient documentation for anyone who uses the REST API currently to edit or view users from another site (which should be a low number considering that this feature and generally any multisite functionality have never been documented).
#26
in reply to:
↑ 24
@
8 years ago
Replying to jnylen0:
@ocean90 in the absence of further feedback I would like to move forward with this change.
Yep, go ahead. Thanks for the page!
While working on a patch, I took a closer look at the current state of users in the REST API in a multisite environment. Some of our initial observations were slightly incorrect. Referring to https://make.wordpress.org/core/2017/01/11/controlling-access-to-rest-api-user-functionality-for-multisite/:
The thing certain to be a bug currently is that it is possible to read any user from any site with a
GET
request. If possible at all, this should only be available if the current user is a super admin.The updating bit of the above quote is wrong: Updating a user in multisite is only available to super admins, no site administrator can send a
POST/PUT/PATCH
successfully, as in the permission checkcurrent_user_can( 'edit_user', $user_id )
returns false unless a super admin. That is because inmap_meta_cap()
'edit_user' maps to 'do_not_allow' in a multisite unless the current user canmanage_network_users
. Also when updating a user through the REST API as a super admin, that user is automatically added to the current site if they haven't been a member of it before. I kind of see where this is coming from, but I think that should be removed as well as we'll probably wanna have more clear control about that.So I would suggest to do the following for 4.7.3:
GET
request) should only be available to super admins (can be checked withcurrent_user_can( 'edit_user', $user_id )
because of the mapping explained above).UPDATE
request. I would suggest though to remove the automatic call toadd_user_to_blog()
and instead return an error if a request tries to set roles on a user that is not part of the current site. This change would ensure that in the current state both adding and removing a user from a site is not possible, which would allow us to come up with a sophisticated approach for 4.8.