register_meta and capabilities aren't working as expected

[tharsheblows]

The first part of this is #38284, there aren't capabilities for object types other than posts.

The second part is best described by a use case:

I want logged in users to be able to flag inappropriate comments. After 10 flags, the comment gets unpublished and a notice goes to a moderator to check it. I'm going to store these flags and the user in the comment meta table using something like

<?php
if( current_user_can( 'edit_comment_meta' ) ){
add_comment_meta(  $comment_id, 'flagged', $user_id, false );
}

with register_meta and the auth callback looking something like

<?php
$args =  array( 
'type' => 'string', 
'show_in_rest' => true, 
'auth_callback' => 'check_logged_in' );

register_meta( 'comment', 'flagged', $args );

function check_logged_in(){
return is_user_logged_in();
}

However, I don't want them to be able to edit the comment itself so current_user_can( 'edit_comment' ) should return false.

So that's the use case.

What happens at the moment is, well, no one can update the comment because there's no edit_comment_meta capability. But it's not a problem making the capabilities work like that.

However, edit_post_meta currently doesn't work like that. For current_user_can( 'edit_post_meta' ) to return true, a user also has to have the edit_post capability. It's straightforward to change, but does have one backwards incompatibility: if someone is using current_user_can( 'edit_post_meta' ) with a registered meta key which has an auth_callback 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's a slightly convoluted edge case, admittedly.

Attached is a patch that shows how it would work with unit tests.

[tharsheblows]

add capabilities used in register_meta() with tests

[jeremyfelt]

Some quick history spelunking...

We somewhat noted this during the development of #35658. From comment 10:

The filter applied for sanitization is straight forward as it is applied for all object types (meta types). The auth callback is only applied for the post object type, so does not currently fire for user, comment, or term. It may make sense to expand this, but it may also be an opportunity to revisit how authorization works around meta. /shrug

And as "explicit" of a decision as there probably was during that period came from a ​patch review in Slack and boils down to: Add the auth_callback filters for all meta types in 4.6, leave it up to early adopters to apply_filters() on their own. Fix later.

That's not a great long term answer and we should definitely use this ticket to find the right answer for applying the auth callback filters across all core meta types.

38303.diff​ looks like a great start.

I'm going to move this to 4.7 to get some attention from the REST API effort. It's also probably an enhancement rather than a bug.

/cc @joehoyle @rmccue @rachelbaker @danielbachhuber

[tharsheblows]

I don't quite agree with the exact implementation the patch uses anymore, I think we should add the capabilities to roles then update the capabilities cases based on those.

The reason is that if someone creates a custom role which includes eg 'edit_term_meta' it will have unintended consequences and allow them to always update term meta even if there is an auth_callback which disallows it. This is a side-effect of the implementation details and there are ways around it but I think putting the capabilities in roles is the best way forward and is how most people would expect to use them.

I can make a patch for that tomorrow but realise there might be issues I'm not aware of around adding new capabilities to roles, so thought I'd mention now. (Are there?)

[tharsheblows]

So this is with adding the edit/add/delete meta capabilities to roles on upgrade or install and updates map_meta_cap to reflect that they're now proper capabilities in roles. This includes a few more tests.

The second patch will allow people to assign edit/add/delete meta capabilities to custom roles and have them work as expected.

[tharsheblows]

adds capabilities to roles and updates map_meta_cap

[jorbin]

@jeremyfelt Can you review the latest patch?

[tharsheblows]

Related: the patch for #38412 depends on this.

[jeremyfelt]

I think this should be fixed for 4.7, but I'm not confident I have the time to own it. Tagging a few other possible commit owners.. :) /cc @johnbillion @jorbin @joehoyle @rachelbaker @rmccue

[jorbin]

I'm not sure these should be distinct primitive capabilities. I would rather see them map to the object they are attached to.

I'm sadly going to punt this. If another committer has the time to see this through to completion before beta1, they should move it back into the milestone.

[tharsheblows]

@jorbin Do you mean like in the first diff here? https://core.trac.wordpress.org/attachment/ticket/38303/38303.diff

If this isn't fixed, should the register_meta docs be updated to indicate that the auth_callback param is not functional for any post type other than posts? I can see how that could be an issue for someone who only tested that with posts where it would work as expected and not the other object types (the unit tests do this for a few things).

It would be good to also have a preferred way for them to add in those apply_filters hooks that the auth_callback uses too as once this is fixed, they will be hooked in twice if someone adds them in themselves now.

[tharsheblows]

fix typos in two of the post meta tests

[rmccue]

This is being worked on in #38412. Reclassifying as a bug, since currently register_meta doesn't really work with non-post meta: the auth_callback you pass in has no effect at all.

[rmccue]

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.

[rmccue]

Reopening here, since it wasn't actually fixed in the above commit. My apologies for misunderstanding this. Some discussion has continued on #38412, but we should continue it here, as it's a different issue.

38412.5.diff:ticket:38412​ has me concerned that we're changing an existing cap very late in a cycle. In addition, if there's any plugin code relying on auth_callback being set, this could be problematic.

---

The tl;dr of this issue: if a meta key has auth_callback set, the actual check is current_user_can( 'edit_post', $id ) && $auth_callback(). This means you can't write an auth callback that allows people without edit_post to edit the meta.

The change would make auth_callback authoritative (if you return true, the user can edit it, no further checks), which requires changing the default auth_callback currently in place, and is hence kinda a BC break. If you pass in __return_true as the auth_callback currently, the permissions required to edit that meta would change from edit_post to nothing.

Since this is very late in the cycle, and a potential BC break, I think we need to punt to 4.8. There's lots that could break here, and I think we're locked in with BC.

I'm going to punt this to 4.8, and we reconsider the approach. We could instead add an option to register_meta that allows you to specify whether you want the edit_post check (something like 'require_edit' => true).

[tharsheblows]

I think adding extra complexity to register_meta which is already complex by necessity is a mistake. It would be best to let the functions be more intuitive and the auth_callback filter to work like a regular filter, ie as you said: have authoritative control over the content passed to it.

One of the main reasons for putting this is in 4.7 is that currently only edit_post_meta exists in any form (edit_comment_meta, edit_user_meta and edit_term_meta don't exist at all) so there is only one break in backwards compatibility.

The BC break is the undocumented edge case where someone is using the auth_callback filter to allow edit_post_meta but relying on a user needing edit_post to keep them from actually editing post meta. I can't imagine this is common. In all other cases it will work as expected. (And again, it is *only* edit_post_meta as the others don't exist.)

I also think it's important to have register_meta and its auth_callback working as expected due to the introduction of the REST API -- that function's objective is to make it easy and safe to add meta to the object endpoints. There is a decent chance that a fair amount will be written about how to use the REST API after 4.7 is released; now is time to decide this although I fully appreciate it is late in the cycle.

The discussion about primitive vs meta capabilities can wait, these would be straightforward to change later.

[obenland]

@rmccue It looks like this was closed after a commit and then reopened and moved out of the milestone without reverting the commit?

It missed 4.8, but I'd suggest closing this ticket as fixed in 4.7 and opening a new one to address whatever issue it cuased in a future release.

[DrewAPicture]

@rmccue In[39179], the auth_{$object_type}_meta_{$meta_key} and auth_{$object_type}_{$sub_type}_meta_{$meta_key} filters were never actually documented anywhere.

The duplicate hook comments points to wp-includes/meta.php but these are the only place those filters actually evaluated.

[DrewAPicture]

See #36319 for where docs are apparently be added spurring from a ticket created before the rename.