REST API: Don't delete posts/links when deleting/removing a user from a site in a multisite install

[ocean90]

In wp-admin of a site in a multisite install it's not possible to delete a user, you can only remove a user. Removing a user doesn't delete all their assigned posts/links.

The current endpoint uses wp_delete_user() which also uses remove_user_from_blog() but only after removing all the posts and links.

This is critical because a) the user isn't actually deleted and b) the behaviour differs from wp-admin and can lead to unexpected data lost. While the API supports a reassign parameter it's not required to be set unlike the force parameter.

The severity of this issue can probably be a bit reduced if we require the reassign parameter for all requests. For the current default behaviour I'd have to set reassign=>null explicitly.

A related issue: The capability check differs from wp-admin too, remove_users vs. delete_users. This was ​already reported on the GitHub repo but without a response yet.

I've also searched through some of the issues and found a general one about "​Deleting an item should always delete an item". It's also the issue where the question "​What should DELETE wp/users/1 do on single site vs. multisite?" was asked. I couldn't find an answer though.

If we don't want to handle removing users via the DELETE route we may have to think about disabling the route for multisite.

[jeremyfelt]

Replying to ocean90:

If we don't want to handle removing users via the DELETE route we may have to think about disabling the route for multisite.

I kind of like this idea.

From the "​Deleting an item should always delete an item" discussion, DELETE = trash, DELETE ?force=true = delete.

With users in single site, there is no trash, so we only handle delete.

Users in multisite are associated with individual objects - sites via capabilities, posts via authorship - and cannot be trashed or deleted on an individual site level.

Possible roadmap:

• 4.7 (this ticket) - Restrict the current DELETE route for users to single site.
• 4.8 - Provide a way for update_item() via PUT to remove the site capabilities for a user and remove object (post) associations. Require reassign.
• 4.8 - Create a global route for deleting a user, accounting for the case of multiple networks.

[joehoyle]

I'd be happy to disable DELETE for multisite for 4.7.

The severity of this issue can probably be a bit reduced if we require the reassign parameter for all requests. For the current default behaviour I'd have to set reassign=>null explicitly.

Not totally sure on this approach, but doesn't seem crazy.

[rmccue]

Replying to jeremyfelt:

• 4.7 (this ticket) - Restrict the current DELETE route for users to single site.
• 4.8 - Provide a way for update_item() via PUT to remove the site capabilities for a user and remove object (post) associations. Require reassign.
• 4.8 - Create a global route for deleting a user, accounting for the case of multiple networks.

Sounds like a good plan to me.

[ocean90]

I'd be fine with @jeremyfelt's roadmap, except that I think we should require reassign in 4.7 too. See attached screenshot, wp-admin has no default value either, you have to read and confirm the action.

[rmccue]

Replying to ocean90:

I'd be fine with @jeremyfelt's roadmap, except that I think we should require reassign in 4.7 too. See attached screenshot, wp-admin has no default value either, you have to read and confirm the action.

I'm not sure how this would actually work in practice, but requiring reassign doesn't sound great to me. What happens if you just want to update a user? You'd need to send e.g. { "name": "My New Name", "reassign": false } even if you're not removing roles.

[dd32]

Replying to rmccue:

Replying to ocean90:

I'd be fine with @jeremyfelt's roadmap, except that I think we should require reassign in 4.7 too. See attached screenshot, wp-admin has no default value either, you have to read and confirm the action.

I'm not sure how this would actually work in practice, but requiring reassign doesn't sound great to me. What happens if you just want to update a user? You'd need to send e.g. { "name": "My New Name", "reassign": false } even if you're not removing roles.

The reassign param should only be required for DELETE methods, and as per above, should be restricted to single-site usage only for 4.7

[rmccue]

Replying to dd32:

The reassign param should only be required for DELETE methods, and as per above, should be restricted to single-site usage only for 4.7

Right, but the proposal is to use PUT in 4.8, as you're not deleting the user, just removing their access to the site.

[jeremyfelt]

Replying to rmccue:

Replying to dd32:

The reassign param should only be required for DELETE methods, and as per above, should be restricted to single-site usage only for 4.7

Right, but the proposal is to use PUT in 4.8, as you're not deleting the user, just removing their access to the site.

We'll skip DELETE for multisite in 4.7 (see 38962.diff​), but the reassign param should be required for single site.

What should be passed as reassign to delete content instead? Right now we only rely on passing the fallback null to wp_delete_user().

[jeremyfelt]

38962.2.diff​ adds tests and updates the error code to 501 (props @ocean90).

The tests assert the new error code rather than skipping entirely so that they break when we do add the functionality for multisite.

The error message itself isn't descriptive, but I copied from elsewhere to avoid a new string.

What should be passed as reassign to delete content instead? Right now we only rely on passing the fallback null to wp_delete_user().

We discussed just passing null in Slack, but because reassign is set as type integer, that results in an invalid parameter. Unless I'm missing something, which is definitely possible.

Can we use 0 or -1? Should there be another parameter to delete content separate from reassign?

[jnylen0]

As discussed in Slack we should use 0 instead of null. This is both easier and more consistent with the rest of the API (page and comment parent use 0 to unset, for example).

These tests can use the assertErrorResponse helper to test the error code and the HTTP status at the same time.

I would also suggest using a more descriptive error code since we can't add a new string, like rest_cannot_delete_multisite_user.

[jnylen0]

As discussed in Slack (​some more) 0 has special meaning for a post author.

Is reassign=0 a valid operation? If so, we should support null as a special value rather than using -1. This will require removing the integer type and adding a custom sanitize_callback - see #38971 for a similar example.

[jeremyfelt]

Replying to jnylen0:

As discussed in Slack (​some more) 0 has special meaning for a post author.

Is reassign=0 a valid operation? If so, we should support null as a special value rather than using -1. This will require removing the integer type and adding a custom sanitize_callback - see #38971 for a similar example.

I think this is the right move. I've opened #39000 as a new ticket as I think this decision is separate from the one to disable the DELETE method in multisite.

I'm going to submit 38962.2.diff​ as read for commit, this needs review.

[jnylen0]

Quoting from comment:12

These tests can use the assertErrorResponse helper to test the error code and the HTTP status at the same time.

I would also suggest using a more descriptive error code since we can't add a new string, like rest_cannot_delete_multisite_user.

[jeremyfelt]

As @johnbillion mentioned, there is a string change here. Other objects already have a "cannot delete" string for the REST API, but users do not. I haven't looked to see if there's a better temporary string that we can use for 4.7.

[pento]

As @jnylen0 suggested, the tests should use assertErrorResponse() to test the error.

I don't think we need to change the error code, though. There are no other multisite-specific error codes.

I removed the i18n-change keyword, because this isn't a new string.

Apart from that, I'm cool with this change.

[rachelbaker]

Uses assertErrorResponse() helper in the tests

[rachelbaker]

In 38962.3.diff​ modified @jeremyfelt's previous patch to use the assertErrorResponse() helper in the tests.

This needs another permanent committer to review.

[pento]

Ship it.

[jeremyfelt]

In 39438:

REST API: Disable DELETE requests for users in multisite.

In wp-admin, users are removed from individual sites rather than deleted. A user can only be deleted from the network admin.

Until support for a PUT request that removes a user's site and content associations is available, DELETE requests are disabled to avoid possible issues with lost content.

Props jnylen0, rachelbaker.
Fixes #38962.

[jeremyfelt]

In 39439:

REST API: Disable DELETE requests for users in multisite.

In wp-admin, users are removed from individual sites rather than deleted. A user can only be deleted from the network admin.
Until support for a PUT request that removes a user's site and content associations is available, DELETE requests are disabled to avoid possible issues with lost content.

Merges [34938] onto 4.7 branch.

Props jnylen0, rachelbaker.
Fixes #38962 for 4.7.