WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#38412 closed defect (bug) (fixed)

cannot update or delete user, term or comment meta via REST API

Reported by: tharsheblows Owned by: rmccue
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch
Focuses: Cc:

Description

The capability checks used when updating meta for all object types are currently "edit_post_meta" which doesn't work for users, terms or comments. Adding in eg "edit_user_meta" is straightforward, however this and the other object type capabilities don't exist. (see #38303)

Attached is a patch which could be used if those capabilities did exist, otherwise we'll need to work around them. That's possible, just need to remember to apply_filters(auth_{$object_type}_meta_{$meta_key}); as these can be added in register_meta()

I think it'd make sense to fix it properly now though.

Attachments (5)

38412.diff (3.5 KB) - added by tharsheblows 4 years ago.
check meta update and delete capabilities using correct object type
38412.2.diff (8.4 KB) - added by rmccue 4 years ago.
Add meta caps for other object types
38412.3.diff (68.7 KB) - added by tharsheblows 4 years ago.
unit tests and patch to comments controller
38412.4.diff (81.2 KB) - added by boonebgorges 4 years ago.
38412.5.diff (5.2 KB) - added by tharsheblows 4 years ago.
respect filter on object meta, unit test

Download all attachments as: .zip

Change History (23)

@tharsheblows
4 years ago

check meta update and delete capabilities using correct object type

#1 @tharsheblows
4 years ago

  • Keywords needs-unit-tests added

#2 @joehoyle
4 years ago

  • Milestone changed from Awaiting Review to 4.7
  • Owner set to rmccue
  • Status changed from new to assigned

@rmccue
4 years ago

Add meta caps for other object types

#3 @rmccue
4 years ago

  • Keywords has-patch added

Added a patch that adds in the other relevant meta caps for other object meta, which completes the other half of this.

This could probably be split out into a separate, entirely new function.

#4 @tharsheblows
4 years ago

This includes a patch to class-wp-rest-comments-controller.php where you couldn't update comment meta if you didn't update the comment.

There are also working unit tests for each object type. I haven't gone through and pruned these, there is definitely a massive amount of redundancy. I'm not sure if you'd rather be redundant or combine these into one file with only test_set_value() for each object type. The meta passes through the object type update_item function on its way to being updated so that should stay in as those update_item functions are all a little different (eg the patch needed in class WP_REST_Comments_Controller)

@boonebgorges I was unsure how to make the test in tests/rest-api/rest-term-metafields.php lovely like these https://core.trac.wordpress.org/changeset/38975 and I apologise for that and the decided lack of frugal gaze in general.

@tharsheblows
4 years ago

unit tests and patch to comments controller

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


4 years ago

#6 @boonebgorges
4 years ago

  • Owner changed from rmccue to boonebgorges
  • Status changed from assigned to reviewing

@boonebgorges
4 years ago

#7 @boonebgorges
4 years ago

Thanks for the ping, and sorry for the delay.

The general approach of the map_meta_cap() part of the patch looks good to me.

38412.4.diff does the following:

  • Adds core (non-REST-API) tests for each of the new caps
  • Casts mine frugal gaze on the tests written by @tharsheblows
  • Fixes the user-meta-fields tests for multisite, where we have to dance around the fact that you need manage_network_users, but granting super-admin means that the metadata cap check will automatically pass
  • Some minor documentation and formatting cleanup

I haven't spent a huge amount of time looking at the endpoint-specific tests. They look good at a glance, but they should have a readthrough by someone more familiar with the pattern of the API tests.

One last note - I'm slightly uneasy about making so deep a change to the capability API at this point in the development cycle. Technically, this is a compat break - people are likely relying on edit_post_meta etc for non-post-related meta checks. Not a good situation to be sure, and something that very much should be fixed, but it should be carefully documented for public consumption. I'm OK with the change, but a signoff from @helen as the release lead would put my mind at ease somewhat.

#8 @boonebgorges
4 years ago

  • Keywords needs-unit-tests removed
  • Owner changed from boonebgorges to rmccue

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


4 years ago

#10 @jeremyfelt
4 years ago

It seems like this also fixes #38303. @tharsheblows, is that correct?

#11 @tharsheblows
4 years ago

@jeremyfelt yes, pretty much! I've just looked back at it - I'm sorry I didn't realise this earlier and whilst I think it's an issue, it might not be.

It's my understanding that the auth_{$object_type}_meta_{$meta_key} filter is, if set, what determines whether a user can update a meta key. If it isn't set, then there's a check to see if it's a protected meta key and then, if it's not a protected meta key, then you can update the meta key if you can update the object type.

(The auth_callback in register_meta is one thing that adds this filter.)

So the check should be:

  1. is there a filter on auth_{$object_type}_meta_{$meta_key})?
    • Yes -> do whatever it says
    • No -> proceed
  2. is the meta key protected?
    • Yes -> set the capability to edit_{$object_type}_meta
    • No -> set the capability to edit_{$object_type}

It currently doesn't work like this, the filter is applied after the edit_{$object_type} capability is set so you can't allow a user who doesn't have that capability to update meta even if you'd like to do so.

The change would affect this edge case: if someone is using current_user_can( 'edit_post_meta' ) with a filter on auth_{$object_type}_meta_{$meta_key} that returns true but they really don't want the person to update the post meta so are relying on the fact that they don't have the edit_post capability, then that will change and that person will be able to update the post meta. It will only affect post_meta because that's the only filter in there currently.

It's a straightforward fix and I have tests for the auth_callback filter that I can add to the capabilities tests but thought I'd ask about it now before cluttering up with another patch. One thing it does require is setting the auth_callback default to null rather than __return_true in register_meta. I don't think that would be an issue but want to mention.

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


4 years ago

#13 @rmccue
4 years ago

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

In 39179:

Roles/Capabilities: Add meta-caps for comment, term, and user meta.

Additionally, use these meta-caps in the REST API endpoints.

Previously, register_meta()'s auth_callback had no effect for non-post meta. This introduces {add,edit,delete}_{comment,term,user}_meta meta-caps to match the existing post meta capabilities. These are currently only used in the REST API.

Props tharsheblows, boonebgorges.
Fixes #38303, fixes #38412.

#14 @rmccue
4 years ago

@tharsheblows Re: 11, is this a change that needs to be made in 4.7?

#15 @tharsheblows
4 years ago

@rmccue Yes, I think so. Otherwise register_meta( 'post', $meta_key, array( 'auth_callback' => 'new_callback' ) ) doesn't work as intended. It'd be best if it did from now when the REST API has just been included in core and people are starting to use it, especially since meta has been separate from the WP-API plugin up until not too long ago.

Attached is patch with an additional unit test.

@tharsheblows
4 years ago

respect filter on object meta, unit test

#16 @helen
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#17 @helen
4 years ago

I reopened this for 15 - who is owning review and further commit here?

#18 @rmccue
4 years ago

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

That'd be on me for review. :)

The remaining issue is actually #38303, so I think I was wrong to close that. Going to move over there, since it makes more sense than in this ticket.

Note: See TracTickets for help on using tickets.