#43709 closed defect (bug) (fixed)
Fix or remove the "delete revision" endpoint
Reported by: | azaozz | Owned by: | kadamwhite |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | REST API | Keywords: | has-patch has-unit-tests early has-dev-note |
Focuses: | rest-api | Cc: |
Description
Allowing the client to delete revisions breaks the "audit trail" functionality. This is not allowed in WordPress and shouldn't be allowed through the API.
Ideally the delete revision endpoint should be removed. Alternatively it can require a specific permission which should be false
by default for all user roles.
Attachments (3)
Change History (32)
#2
@
7 years ago
We could handle this in map_meta_cap
: if the cap is delete_post
and the post is a revision, return do_not_allow
.
#3
@
7 years ago
- Owner set to danielbachhuber
- Status changed from new to assigned
For the historical context, this was introduced in https://github.com/WP-API/WP-API/pull/1110
I'm +1 for the capability-based approach.
#4
@
7 years ago
Yeah, capability based approach will make it easier for plugins to enable deleting of revisions. As far as I see the checks should be if the current user can:
- Delete the parent post.
- Delete attachments. This should always be "do_not_allow" by default.
#5
follow-up:
↓ 8
@
7 years ago
- Keywords has-patch has-unit-tests added; needs-patch removed
In 43709.1.diff
:
do_not_allow
is always applied for adelete_post
check on a revision.WP_REST_Revisions_Controller->delete_item_permissions_check()
also verifies that the user can delete the parent post.- Updates tests correspondingly.
Worth noting: always applying do_not_allow
in map_meta_cap()
will be a breaking change for any code dependent on current_user_can( $revision_post_type_object->cap->delete_post, $revision->ID );
. However, if this was the original intent, the change should be graceful.
This ticket was mentioned in Slack in #core-restapi by danielbachhuber. View the logs.
6 years ago
#8
in reply to:
↑ 5
@
6 years ago
- Milestone changed from 5.0 to 5.1
Replying to danielbachhuber:
always applying
do_not_allow
inmap_meta_cap()
will be a breaking change...
Seems this needs another look as it will trip plugins that want to delete revisions (and are checking user caps).
How about introducing another capability: delete_revisions
? It would need to be overridden when a (parent) post is deleted, all revisions should be deleted with it. Then it could be true for admins by default (so we don't break existing plugins) but false for editors, authors, and anybody else that can delete a post. Then installs with many editors and authors may want to grant it to other roles or remove it completely.
In any case, this has a potential to introduce regressions in plugins, lets look at it again after 5.0 :)
This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.
6 years ago
#10
@
6 years ago
- Keywords early added
- Milestone changed from 5.1 to 5.2
The patch still applies cleanly, but let's punt this to 5.2 since it still needs a decision. Getting it in early for testing purposes would also be beneficial, and an early dev-note
would also help.
@flixos90 @johnbillion do you have any thoughts on this?
This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.
6 years ago
#12
@
6 years ago
On the surface this looks like a sensible change, but I don't have any capacity to look into it further at the moment.
This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.
6 years ago
#14
@
6 years ago
- Milestone changed from 5.2 to 5.3
Punting to 5.3-early after discussion in March 14th REST API component meeting.
This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.
5 years ago
#18
@
5 years ago
- Owner danielbachhuber deleted
@TimothyBlynJacobs Not particularly, to be honest. I remember being reasonably confident in 43709.1.diff at the time though.
#19
@
5 years ago
@TimothyBlynJacobs Do you have an appetite to take this on, or is anybody else so inclined? We're still reasonably enough in the "early" phase for 5.3 but if we want to do this for the current cycle, we should do it in the next week or two.
I can't estimate from where I'm standing how impactful the proposed breaking change would be, but I'm generally in favor of @danielbachhuber's approach. Adding a new capability feels like an unneeded overstep but I have less working experience with the capabilities system than others on this thread.
This ticket was mentioned in Slack in #core-restapi by dlh. View the logs.
5 years ago
#23
@
5 years ago
From what I understand of this issue, I'm inclined to agree with the approach in 43709.1.diff.
First, if it's the case that deleting revisions isn't supposed to be allowed without a plugin, then the fact that the change to map_meta_cap()
isn't already in core strikes me as a bug, separate even from the REST endpoint.
It's true that changing the mapping for revisions is a backwards-compatibility break. I don't have the ability to search the plugin repo for the potential impact of such a break.
However, as @danielbachhuber says, the change should be graceful, and it would be straightforward for a plugin that wants to override the change to do so.
It also repairs a flaw, in that should core ever check the $delete_post
capability for a revision, the check has the potential to return true
when it would be assumed to return false
.
Second, I agree with @kadamwhite to be disinclined to adding a new capability. The new capability would have to be a meta capability — see ticket:45423#comment:10 And as a meta capability, it would just be mapped to do_not_allow
anyway to meet the requirement that no one be allowed to delete revisions.
(If new capabilities for revisions are going to be pursued, perhaps that could begin by setting the revision capability_type
[and providing backwards-compatibility] to facilitate plugins distributing all the different post capabilities?)
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
5 years ago
#25
@
5 years ago
The patch looks good, but I'm seeing this error:
1) WP_Test_REST_Revisions_Controller::test_delete_item_cannot_delete_parent Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'rest_cannot_delete' +'rest_cannot_read'
#26
@
5 years ago
I uploaded a patch to fix the test failure. It was using the contributor id which does not have permission to edit the post and so is blocked from deleting the revision earlier on.
I also made it return the rest_cannot_delete
error code instead of the generic rest_forbidden
.
#27
@
5 years ago
- Owner set to kadamwhite
- Resolution set to fixed
- Status changed from assigned to closed
In 45812:
This is a regression. Currently revisions can only be deleted (outside the API) from a plugin. Thinking we should keep it that way. The "audit trail" functionality is not that necessary for smaller sites with one or two users but is rather critical for larger sites that have many authors and editors.
Also see https://core.trac.wordpress.org/ticket/43316#comment:71.