Single-term API endpoints should use term-specific caps

[boonebgorges]

From ​https://github.com/WP-API/WP-API/issues/2777:

Since https://core.trac.wordpress.org/ticket/35614, core has supported cap checks that are specific to the action as well as the term - edit_term, delete_term, assign_term. The single-item checks in WP_REST_Terms_Controller should use these caps.

[boonebgorges]

I'm working on a patch.

[boonebgorges]

38505.diff​ implements 'edit_term' and 'delete_term' in the respective term endpoints. @johnbillion As the shepherd of #35614, I'd appreciate a sanity check on the approach (especially the hackish tests).

'assign_term' is trickier. Terms are assigned to posts in the /{post_type}/ create and update endpoints. So I suppose the proper approach is to add a check to create_item_permissions_check() and update_item_permissions_check() that looks like this:

$taxonomies = wp_list_filter( get_object_taxonomies( $this->post_type, 'objects' ), array( 'show_in_rest' => true ) );
foreach ( $taxonomies as $taxonomy ) {
	$base = ! empty( $taxonomy->rest_base ) ? $taxonomy->rest_base : $taxonomy->name;

	if ( ! isset( $request[ $base ] ) ) {
		continue;
	}

        foreach ( $request[ $base ] as $term_id ) {
            if ( ! current_user_can( 'assign_term', (int) $term_id ) ) {
                return new WP_Error( 'rest_cannot_assign_term', __( 'You are not allowed to assign this term as this user.' ), array( 'status' => rest_authorization_required_code() ) );
            }
        }
}

API team - does this seem like the right pattern?

I hesitate because #35614 doesn't do a similar sort of implementation for native WP UIs. The only place where 'assign_term' is implemented is in an XML-RPC endpoint. The post edit interface doesn't do any sort of check like this. @johnbillion Was this an intentional oversight? It seems like a bug to me, but maybe there's a reason behind it.

[johnbillion]

The post edit interface uses the higher level assign_terms capability check for tags (post_tags_meta_box()) because checking the assign_term cap for individual terms here would require some UI and UX work.

For categories, there's just a higher level edit_terms check instead of assign_terms (in post_categories_meta_box()) that looks like it was an oversight, but this may also need some UI work to switch it to individual assign_term cap checks. I'll take a look at this during beta.

@boonebgorges Those tests looks good, but I would also test the inverse, where an Editor/Admin level user (who can normally edit/delete terms) is required to have the do_not_allow cap to edit/delete a term and then assert that the correct error response is returned.

[boonebgorges]

In 38960:

REST API: Use term-specific caps for permission checks in term update and delete endpoints.

See #38505.

[boonebgorges]

Thanks for the eyeballs, @johnbillion. I've added the suggested tests as part of [38960].

The 'assign_term' question still stands. Three thoughts:

1. The fact that 'assign_term' isn't actually implemented in the UI severely limits how useful it'll be to developers. I can imagine this behavior leading to unexpected security issues.

2. If 'assign_term' is not going to be implemented in the UI for 4.7, maybe we don't bother implementing in the API either (ie, we continue to use taxonomy caps)? We're generally going for feature parity between the two, right?

3. The cap-check pattern I proposed above requires a bit more code duplication than simply putting the check in the hangle_terms() method. But (a) I assume that if we can't perform part of the request (assigning a term) we don't want to perform *any* of the request? (Though this is not how it currently works - the post will be created, but you'll get an error object from the API.) And (b) keeping permissions checks together seems more maintainable and readable.

1 and 2 are questions for @johnbillion, 3 is an architecture question for the APi team.

[rachelbaker]

Replying to boonebgorges:

Yes, this does fit our pattern for permission checks.

'assign_term' is trickier. Terms are assigned to posts in the /{post_type}/ create and update endpoints. So I suppose the proper approach is to add a check to create_item_permissions_check() and update_item_permissions_check() that looks like this:

$taxonomies = wp_list_filter( get_object_taxonomies( $this->post_type, 'objects' ), array( 'show_in_rest' => true ) );
foreach ( $taxonomies as $taxonomy ) {
	$base = ! empty( $taxonomy->rest_base ) ? $taxonomy->rest_base : $taxonomy->name;

	if ( ! isset( $request[ $base ] ) ) {
		continue;
	}

        foreach ( $request[ $base ] as $term_id ) {
            if ( ! current_user_can( 'assign_term', (int) $term_id ) ) {
                return new WP_Error( 'rest_cannot_assign_term', __( 'You are not allowed to assign this term as this user.' ), array( 'status' => rest_authorization_required_code() ) );
            }
        }
}

[boonebgorges]

Thank you, @rachelbaker.

38505.2.diff​ adds the 'assign_term' check for post create and update. I've centralized some of the logic.

current_user_can( 'assign_term', $term_id ) will fail if $term_id points to a term that doesn't exist. This poses a problem because the permission check happens very early in the request, which means that passing a bad term ID will result in a 403. Current behavior is that the post is created (200) but the term is not assigned (see test_create_post_with_invalid_categories()). I can see an argument for returning some variety of 40x in this case, but definitely not 403. I've added a check to make sure the term exists before doing a cap check on it.

The remaining question is whether we want to introduce this behavior here even though the 'assign_term' check isn't applied in the Dashboard UI. I lean yes on this, but I can also appreciate the desire to maintain parity.

[rachelbaker]

In 39108:

REST API: Return a WP_Error when a user does not have permission to create or update a post with the provided terms.

Add the 'assign_term' check for post create and update.

Props boonebgorges, johnbillion.
Fixes #38505.