WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 4 months ago

#38962 closed defect (bug) (fixed)

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

Reported by: ocean90 Owned by: jeremyfelt
Milestone: 4.7 Priority: high
Severity: major Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit dev-reviewed
Focuses: multisite Cc:

Description

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.

Attachments (4)

wp-admin-delete-users.png (69.2 KB) - added by ocean90 5 months ago.
38962.diff (922 bytes) - added by jeremyfelt 5 months ago.
38962.2.diff (3.7 KB) - added by jeremyfelt 5 months ago.
38962.3.diff (4.0 KB) - added by rachelbaker 5 months ago.
Uses assertErrorResponse() helper in the tests

Download all attachments as: .zip

Change History (31)

#1 in reply to: ↑ description ; follow-up: @jeremyfelt
5 months ago

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.

#2 @joehoyle
5 months ago

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.

#3 in reply to: ↑ 1 @rmccue
5 months ago

  • Keywords needs-patch needs-unit-tests added

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.

#4 follow-up: @ocean90
5 months ago

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.

#5 in reply to: ↑ 4 ; follow-up: @rmccue
5 months ago

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.

#6 in reply to: ↑ 5 ; follow-up: @dd32
5 months ago

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

#7 in reply to: ↑ 6 @rmccue
5 months ago

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
5 months ago

#8 follow-up: @jeremyfelt
5 months ago

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

This ticket was mentioned in Slack in #core by helen. View the logs.


5 months ago

@jeremyfelt
5 months ago

#10 in reply to: ↑ 8 @jeremyfelt
5 months ago

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?

This ticket was mentioned in Slack in #core-restapi by jeremyfelt. View the logs.


5 months ago

#12 follow-up: @jnylen0
5 months ago

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.

#13 follow-up: @jnylen0
5 months ago

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.

#14 in reply to: ↑ 13 @jeremyfelt
5 months ago

  • Keywords has-patch has-unit-tests commit dev-feedback added; needs-patch needs-unit-tests removed

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.

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


5 months ago

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


5 months ago

#17 in reply to: ↑ 12 @jnylen0
5 months ago

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.

#18 @johnbillion
5 months ago

  • Keywords i18n-change added

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


5 months ago

#20 @jeremyfelt
5 months ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned

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.

#21 @pento
5 months ago

  • Keywords commit dev-feedback i18n-change removed

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
5 months ago

Uses assertErrorResponse() helper in the tests

#22 @rachelbaker
5 months ago

  • Keywords commit added

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

This needs another permanent committer to review.

#23 @pento
5 months ago

  • Keywords dev-reviewed added

Ship it.

#24 @jeremyfelt
5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#25 @jeremyfelt
5 months ago

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.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


4 months ago

Note: See TracTickets for help on using tickets.