Fix or remove the "delete revision" endpoint

[azaozz]

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.

[azaozz]

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.

[rmccue]

We could handle this in map_meta_cap: if the cap is delete_post and the post is a revision, return do_not_allow.

[danielbachhuber]

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.

[azaozz]

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 revisions. This should always be "do_not_allow" by default.

[danielbachhuber]

In 43709.1.diff:

• do_not_allow is always applied for a delete_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.

[azaozz]

Replying to danielbachhuber:

always applying do_not_allow in map_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 :)

[desrosj]

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?

[johnbillion]

On the surface this looks like a sensible change, but I don't have any capacity to look into it further at the moment.

[kadamwhite]

Punting to 5.3-early after discussion in ​March 14th REST API component meeting.

[TimothyBlynJacobs]

@danielbachhuber are you still interested in working on this?

[danielbachhuber]

@TimothyBlynJacobs Not particularly, to be honest. I remember being reasonably confident in 43709.1.diff​ at the time though.

[kadamwhite]

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

[dlh]

Unless @TimothyBlynJacobs is on it, I can take a shot at this!

[TimothyBlynJacobs]

That'd be brilliant!

[dlh]

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

[kadamwhite]

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'

[TimothyBlynJacobs]

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.

[kadamwhite]

Correct a PHPCS error around strict array comparison.

[kadamwhite]

In 45812:

REST API: Prevent deletion of post revisions.

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.
While not recommended, a plugin may opt-in to the previous behavior by setting a custom 'delete_post' capability for the revisions post type.

Props dlh, danielbachhuber, TimothyBlynJacobs, azaozz, kadamwhite.
Fixes #43709.

[kadamwhite]

In 45820:

Add @ticket annotations for [45812].

Props birgire.
See #43709.

[kadamwhite]

Dev note: ​https://make.wordpress.org/core/2019/10/16/the-rest-api-in-wordpress-5-3/