WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 13 months ago

Last modified 12 months ago

#39000 closed defect (bug) (fixed)

REST API: Require the reassign parameter when deleting users

Reported by: jeremyfelt Owned by: pento
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Users Keywords: has-patch has-unit-tests commit
Focuses: rest-api Cc:

Description

In both single site and multisite, when deleting a user through the WordPress admin, a specific decision is presented - whether to assign all of the user's posts to another user, or to delete all of the posts.

Via discussion in Slack and in #38962, which disables the DELETE method for the user endpoint in multisite, we need to require reassign as a parameter so that content isn't accidentally lost.

Because any int, especially 0 and above, is a valid value for reassigning post_author, we also need change reassign so that it allows for null as an indication that a deleted user's posts should also be deleted.

Attachments (4)

39000.diff (2.1 KB) - added by jeremyfelt 13 months ago.
39000.2.diff (4.4 KB) - added by jeremyfelt 13 months ago.
39000.3.diff (7.5 KB) - added by jeremyfelt 13 months ago.
39000.4.diff (7.8 KB) - added by jeremyfelt 13 months ago.

Download all attachments as: .zip

Change History (17)

@jeremyfelt
13 months ago

#1 @jeremyfelt
13 months ago

39000.diff is an initial attempt at this, using a sanitize_callback to accept either null or an integer for the reassign parameter.

I may be missing something, but it seems like this check in has_valid_params() won't allow a required parameter to also be null.

#2 follow-up: @joehoyle
13 months ago

@jeremyfelt why the 444 status code for the response here?

I may be missing something, but it seems like this check in has_valid_params() won't allow a required parameter to also be null.

This sounds correct, I think we probably shouldn't use required here - the whole null means "delete", but null needs to be passed is somewhat funky. null is typically not a value in the rest api. Did we discuss using false to mean "don't reassign the posts"? That seems like the most logical / semantic value to indicate this to me.

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


13 months ago

#4 in reply to: ↑ 2 @jeremyfelt
13 months ago

Replying to joehoyle:

@jeremyfelt why the 444 status code for the response here?

Accidental leftover. :) I was using 444 to look for the test error vs another.

I may be missing something, but it seems like this check in has_valid_params() won't allow a required parameter to also be null.

This sounds correct, I think we probably shouldn't use required here - the whole null means "delete", but null needs to be passed is somewhat funky. null is typically not a value in the rest api. Did we discuss using false to mean "don't reassign the posts"? That seems like the most logical / semantic value to indicate this to me.

Summing up the discussion in Slack a bit - We're probably okay to look for a falsey value and not use null. This could be an empty string, 'false', or false. 0 must be counted as a valid non-falsey value in this case.

I don't have time to work on an updated patch at the moment, but I don't think it will be too far off from where it is now.

@jeremyfelt
13 months ago

#5 @jeremyfelt
13 months ago

39000.2.diff looks for reassign as a falsy value.

I'm going to look at a couple more tests for invalid conditions, but an initial review would be good. :)

@jeremyfelt
13 months ago

@jeremyfelt
13 months ago

#6 @jeremyfelt
13 months ago

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

39000.4.diff enforces expected values for the now required reassign parameter.

  • 1 and above will be treated as a possibly valid post_author value.
  • 0 is treated as a valid post_author value.
  • false', 'false', and '' are treated as falsy and specify that posts for the user should be trashed when that user is deleted.

Additional tests are added to test these new possibilities.

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


13 months ago

#8 @jeremyfelt
13 months ago

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

#9 @pento
13 months ago

  • Owner changed from jeremyfelt to pento
  • Status changed from assigned to reviewing

#10 @pento
13 months ago

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

In 39426:

REST API: Require the reassign parameter when deleting users.

When deleting a user through the WordPress admin, a specific decision is presented - whether to assign all of the user's posts to another user, or to delete all of the posts.

This change requires reassign as a parameter in the corresponding REST API endpoint, so that content isn't accidentally lost.

Props jeremyfelt.
Fixes #39000.

#11 @pento
13 months ago

In 39427:

REST API: Require the reassign parameter when deleting users.

When deleting a user through the WordPress admin, a specific decision is presented - whether to assign all of the user's posts to another user, or to delete all of the posts.

This change requires reassign as a parameter in the corresponding REST API endpoint, so that content isn't accidentally lost.

Merges [39426] to the 4.7 branch.

Props jeremyfelt.
Fixes #39000.

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


12 months ago

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


12 months ago

Note: See TracTickets for help on using tickets.