Do not allow editing users from a different site in REST API

[flixos90]

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.

[flixos90]

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

It is possible to both read and edit any user from any site with a request to wp-json/wp/v2/users/<id>, regardless of whether the user is part of that site.

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 check current_user_can( 'edit_user', $user_id ) returns false unless a super admin. That is because in map_meta_cap() 'edit_user' maps to 'do_not_allow' in a multisite unless the current user can manage_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:

• Reading a user from another site (via GET request) should only be available to super admins (can be checked with current_user_can( 'edit_user', $user_id ) because of the mapping explained above).
• Since updating is currently only available to super admins anyway, we don't need to worry too much about permissions of an UPDATE request. I would suggest though to remove the automatic call to add_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.

[flixos90]

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.

[johnjamesjacoby]

Shouldn't super admins be allowed to add an existing user to any site they want, current or otherwise?

[flixos90]

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.

[johnjamesjacoby]

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.

[jeremyfelt]

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.

[jnylen0]

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.

[flixos90]

After the discussion in today's office hours we decided to do the following for 4.7.3:

1. Fail when GET to /users/<id> and that user is not part of the current site.
2. 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.

[jeremyfelt]

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 to site.com/site-two/wp-json/wp/v2/users/1 returns user information.
• PUT to site.com/site-two/wp-json/wp/v2/users/1 returns a rest_cannot_edit error.
• GET to site.com/site-two/wp-json/wp/v2/users/2 returns a rest_user_invalid_id error.
• PUT to site.com/site-two/wp-json/wp/v2/users/2 returns a rest_user_invalid_id error.
• GET to site.com/site-two/wp-json/wp/v2/users/3 returns a rest_user_cannot_view error.
• PUT to site.com/site-two/wp-json/wp/v2/users/3 returns a rest_cannot_edit error.
• GET to site.com/site-two/wp-json/wp/v2/users/4 returns a rest_user_invalid_id error.
• PUT to site.com/site-two/wp-json/wp/v2/users/4 returns a rest_user_invalid_id error.

When authenticated:

• GET to site.com/site-two/wp-json/wp/v2/users/1 returns user information.
• PUT to site.com/site-two/wp-json/wp/v2/users/1 with new nickname updates nickname.
• GET to site.com/site-two/wp-json/wp/v2/users/2 returns a rest_user_invalid_id error.
• PUT to site.com/site-two/wp-json/wp/v2/users/2 returns a rest_user_invalid_id error.
• GET to site.com/site-two/wp-json/wp/v2/users/3 returns user information.
• PUT to site.com/site-two/wp-json/wp/v2/users/3 with new nickname updates nickname.
• GET to site.com/site-two/wp-json/wp/v2/users/4 returns a rest_user_invalid_id error.
• PUT to site.com/site-two/wp-json/wp/v2/users/4 returns a rest_user_invalid_id error.

[jnylen0]

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?

[flixos90]

Thanks for reviewing @jnylen0! I'll commit it today.

[ocean90]

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?

[flixos90]

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

[ocean90]

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?

[jnylen0]

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.

[jnylen0]

I created a "Changelog" document for the REST API handbook. It's live at ​https://developer.wordpress.org/rest-api/changelog/.

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

[flixos90]

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

[ocean90]

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!

[flixos90]

In 40106:

REST API: Do not allow access to users from a different site in multisite.

It has been unintendedly possible to both view and edit users from a different site than the current site in multisite environments. Moreover, when passing roles to a user in an update request, that user would implicitly be added to the current site.

This changeset removes the incorrect behavior for now in order to be able to provide a proper REST API workflow for managing multisite users in the near future. Related unit tests have been adjusted as well.

Props jnylen0, jeremyfelt, johnjamesjacoby.
Fixes #39701.

[flixos90]

Reopening for back-porting to 4.7.3.

[SergeyBiryukov]

In 40111:

REST API: Do not allow access to users from a different site in multisite.

It has been unintendedly possible to both view and edit users from a different site than the current site in multisite environments. Moreover, when passing roles to a user in an update request, that user would implicitly be added to the current site.

This changeset removes the incorrect behavior for now in order to be able to provide a proper REST API workflow for managing multisite users in the near future. Related unit tests have been adjusted as well.

Props jnylen0, jeremyfelt, johnjamesjacoby.
Merges [40106] to the 4.7 branch.
Fixes #39701.