WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#14122 closed task (blessed) (fixed)

Custom "capabilities" appears broken on custom post types

Reported by: jakemgold Owned by: nacin
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.0
Component: Posts, Post Types Keywords:
Focuses: Cc:

Description

It's impossible to create a functional equivalent to a "contributor" role unique to a new custom content type, no what approach one takes. Consider the code below (the most stripped down code that exemplifies the point):

register_post_type('movies',array(
	'label' => 'Movies',
	'singular_label' => 'Movie',
	'public' => true,
	'capabilities' => array( 'edit_posts' => 'edit_movies' ),
	'supports' => array('title','editor','author')
));

if ( !get_role('producer') )
{
	global $wp_roles;
	if ( !isset( $wp_roles ) ) $wp_roles = new WP_Roles();
	
	$caps = $wp_roles->get_role('subscriber')->capabilities;
	$caps = array_merge( $caps, array( 'edit_movies' => true ) );
	$wp_roles->add_role( 'producer', 'Movie Producer', $caps );
}

Note that the "Movies" admin menu does appear for the "producer" role, along with the "add new" option. However, entering content into the editor produces a "not allowed" type error, and hitting "submit for review" produces a similar error.

Even if I'm missing something (maybe with the meta capabilities?) if the user is not allowed to edit a content type, they should not be presented with the menu options, including "add new".

Attachments (4)

14122.diff (13.9 KB) - added by nacin 7 years ago.
Entirely untested. Probably missing some components.
fix-capabilities-on-default.patch (469 bytes) - added by WraithKenny 7 years ago.
Adds a check to see if register_post_type is using default capabilities and turns on map_meta_cap
14122.remove.post_type_supports.author.diff (1.9 KB) - added by nacin 7 years ago.
Removes the checks on post_type_supports('author'). Leaving this in would be a change from 3.0, but if most people are using 'supports' for meta boxes but still storing values in post_author, then I'm unsure of the side effects here.
14122.capability_type.post.diff (1.2 KB) - added by nacin 7 years ago.
map_meta_cap becomes true if capability_type = post and no capabilities were specified. This allows capability_type = post (and nothing else) to work exactly as it did in 3.0, so I'm leaning towards commit on this one.

Download all attachments as: .zip

Change History (61)

#1 @nacin
7 years ago

  • Keywords 2nd-opinion added; capabilities roles custom post types removed

We didn't implement the mapping of capabilities of custom post types. Right now you have to roll our own handling of custom capabilities entirely, using the map_meta_cap filter. I imagine the errors are due to edit_post being the capability, which then goes through map_meta_cap and maybe it determines you need edit_posts (instead of edit_movies)? I'm not sure.

I'm not sure if it was our intent entirely to offer such fine-grained control and at least not offer the ability for map_meta_cap to do its magic.

#2 @nacin
7 years ago

We also have this bit of code:

		$post_type = get_post_type_object( $post->post_type );
		if ( $post_type && 'post' != $post_type->capability_type ) {
			$args = array_merge( array( $post_type->cap->edit_post, $user_id ), $args );
			return call_user_func_array( 'map_meta_cap', $args );
		}

But that doesn't take into account that $post_type->capability_type might be 'post', yet $post_type->cap->edit_psot might not be 'edit_post'. Hmm.

Definitely a lot of things to think about here.

#3 follow-up: @jakemgold
7 years ago

"I imagine the errors are due to edit_post being the capability, which then goes through map_meta_cap and maybe it determines you need edit_posts (instead of edit_movies)?"

No. Even if you change the code above to use a completely custom "capability_type" (rather than defining granular capabilities) like "movie" (which gets "edit_movie", "edit_movies" etc) this same behavior persists.

Same ability to access Add New screen, same errors in editor and upon saving. Even more strange, assigning "edit_movie" (singular) to "producer" (in example) as a primitive capability will allow the user to successfully save the post, but the editor error still persists. It will also, of course, allow the producer to edit other users posts (despite lacking that capability), but this is to be expected, I suppose, since it's supposed to be a meta capability.

We can debate the definition of a "bug" - it may not be broken / crashing code, but I think it's broken from the perspective of "expected" API behavior for developers. But it seems to me that mapping meta capabilities should be "auto-magic" for custom post types, especially public ones.

#4 in reply to: ↑ 3 @duck_
7 years ago

Replying to jakemgold:

"I imagine the errors are due to edit_post being the capability, which then goes through map_meta_cap and maybe it determines you need edit_posts (instead of edit_movies)?"

No. Even if you change the code above to use a completely custom "capability_type" (rather than defining granular capabilities) like "movie" (which gets "edit_movie", "edit_movies" etc) this same behavior persists. Same ability to access Add New screen, same errors in editor and upon saving.

nacin's right. If you use 'capabilities' => array( 'edit_posts' => 'edit_movies' ) then map_meta_cap will see that $post_type->capability_type is equal to 'post' and then decide that the edit_post maps to edit_posts. On the other hand, if you use 'capability_type' => 'movie' then map_meta_cap will notice the difference and call itself again checking the edit_movie meta-cap, however there is no matching case for this and so the default is to return edit_movie as the capability required. (In the latter case you could, as suggested, hook onto the map_meta_cap filter and check if $cap == 'edit_movie' etc.)

Even more strange, assigning "edit_movie" (singular) to "producer" (in example) as a primitive capability will allow the user to successfully save the post, but the editor error still persists. It will also, of course, allow the producer to edit other users posts (despite lacking that capability), but this is to be expected, I suppose, since it's supposed to be a meta capability.

When capability_type is movie and my producer has the edit_movie capability I do not see the autosave error in the editor.

#5 @kevinB
7 years ago

  • Cc kevinB added

#6 @nacin
7 years ago

  • Milestone changed from Awaiting Review to 3.1
  • Owner set to nacin
  • Severity changed from major to normal
  • Status changed from new to reviewing

Slating this for 3.1 Initial thoughts:

  • Opt-in automagic meta capability handling, the way core does it.
  • Splitting capability_type based on what appears to be two different functionalities -- re-routing in map_meta_cap, but also controlling the defaults for the capabilities array. That way we're splitting the default naming of the capabilities array from this remapping. Only if we can do this in a back compat way. Ideas welcome.
  • Allowing capability_type to take an array() such as array( 'box', 'boxes' ), to handle odd plurals.

#7 @mikeschinkel
7 years ago

  • Cc mikeschinkel@… added

#8 @hakre
7 years ago

Any follow-up to the initial thoughts we have so far?

#9 @thenbrent
7 years ago

For Jake and others looking to achieve this, here is a plugin that maps the meta caps for a custom post:
http://wordpress.org/extend/plugins/map-cap

Took a while to figure out how to do it for my own needs so thought I'd release to help others.

It's a bit different to the solution required for the core. I also imagine Andrew has a better idea for how to map the capabilties in the core, but if this feature goes ahead, the plugin should provide a good start for someone else thinking about providing the patch.

#10 @nacin
7 years ago

  • Keywords needs-patch added; 2nd-opinion removed
  • Type changed from defect (bug) to task (blessed)

This needs some love. Anyone want to pick it up?

#11 follow-up: @kevinB
7 years ago

  • Owner changed from nacin to kevinB
  • Status changed from reviewing to accepted

I've been pondering this topic extensively over the last few months and have some code to throw into the fray. Will plan on submitting a patch by Oct 13th.

#12 in reply to: ↑ 11 @nacin
7 years ago

Replying to kevinB:

I've been pondering this topic extensively over the last few months and have some code to throw into the fray. Will plan on submitting a patch by Oct 13th.

Our feature freeze is next week, and I don't want to cut it close, so I must warn you that I or someone else might end up getting to this first.

#13 @kevinB
7 years ago

Okay, I didn't realize it was that close. I'll try to put something together Saturday night.

@nacin
7 years ago

Entirely untested. Probably missing some components.

#14 @nacin
7 years ago

(In [15890]) Rough first pass on map_meta_cap for custom post types. see #14122.

#15 @nacin
7 years ago

(In [15891]) Only check the post author if the post type supports authors. see #14122.

#17 @kevinB
7 years ago

Are you interested in making map_meta_cap deal with custom stati?

I have a revision that does, but have been waiting to see if the status cap approach I proposed for #12706 will be accepted.

Along with the custom stati patch there, I attached a plugin which calls register_post_status and provides meta cap mapping which supports custom type and custom status.

If you and ptah take a look at that patch and plugin, I think you will find my code more advanced and WP-standards-compliant than my trac/svn protocol.

#18 @Utkarsh
7 years ago

  • Cc admin@… added

Notice: Undefined index: delete_posts in ...\wp-includes\post.php on line 1022

#19 @nacin
7 years ago

(In [15897]) Only dumb down delete_others_posts if it deserves to exist. fixes a notice. see #14122.

#20 @Utkarsh
7 years ago

One more, possibly related.

Notice: Undefined property: stdClass::$read in ...\wp-includes\capabilities.php on line 916

#21 follow-up: @scribu
7 years ago

Yeah, seeing that in the post revisions box.

#22 in reply to: ↑ 21 @nacin
7 years ago

Replying to scribu:

Yeah, seeing that in the post revisions box.

That's what I get for temporarily turning off WP_DEBUG and forgetting about it.

Looking through everything now.

#23 @nacin
7 years ago

(In [15909]) Revisions and attachments should also generate meta capabilities as part of their cap object. see #14122.

#24 @andymacb
7 years ago

  • Cc andymacb added

#25 @automattor
7 years ago

(In [15934]) Allow capability_type to be an array, for odd plural situations such as story/storys/stories. After registration it reverts to a singular string. Lots of documentation for meta capabilities and post types, also some cleanups for register_post_type documentation. see #14122.

#26 @nacin
7 years ago

(In [15935]) Remove some capability_type code in WP_Query. Only build faux caps if we're querying for multiple post types or if the post type object can't be found. see #14122.

#27 @jane
7 years ago

There are a couple more days before we call freeze if you guys think you can wrap this up to get it into 3.1.

#28 @nacin
7 years ago

There's one specific bug reported over wp-testers I want to investigate, otherwise I'm prepared to close this and suggest new tickets should be opened.

#29 @kevinB
7 years ago

  • Owner changed from kevinB to nacin
  • Status changed from accepted to assigned

#30 @scribu
7 years ago

  1. Register a public post type:

register_post_type( 'foo', array('public' => true) );

  1. Go to /wp-admin/post-new.php?post_type=foo

The following notice shows in the Publish box:

Notice: Undefined property: stdClass::$delete_posts in /home/cristi/svn/wp/wp-includes/capabilities.php on line 852

#31 @WraithKenny
7 years ago

In addition to scribu's test, on /wp-admin/edit.php?post_type=foo the same notice is present.

Interestingly, if you don't publish, and just save draft, the entry is still editable.

If you publish, then the /wp-admin/edit.php?post_type=foo screen shows,
Notice: Undefined property: stdClass::$edit_published_posts in /mnt/stor1-wc1-dfw1/384190/384265/ken.unfocus.com/web/content/wp-includes/capabilities.php on line 890
twice above the table, and once below the view link, and also a
Notice: Undefined property: stdClass::$delete_published_posts in /mnt/stor1-wc1-dfw1/384190/384265/ken.unfocus.com/web/content/wp-includes/capabilities.php on line 846 above the link.

The entry is also editable.

#32 @WraithKenny
7 years ago

UN-editable is what I meant above.

#33 @WraithKenny
7 years ago

$edit_published_posts, $delete_posts, and $delete_published_posts (etc.) are checked for, but aren't set by register_post_type unless 'map_meta_cap' is true, but the default is set to false. (Not setting 'map_meta_cap' didn't break permissions in 3.0 so this is a regression?)

http://core.trac.wordpress.org/browser/trunk/wp-includes/post.php#L1066

#34 @scribu
7 years ago

So I guess 'map_meta_cap' should default to true, no?

#35 @WraithKenny
7 years ago

 if ( empty($args->capability_type) )
	                $args->capability_type = 'post';

This check doesn't seem to do anything because the default value is 'post' so it should never actually be empty right? (Though I suppose you could pass it )

If you add below that:

 if ( 'post' == $args->capability_type && empty($args->capabilities) ) )
	                $args->map_meta_cap = true;

and then the default can stay the same if its important that it not change... I'm not sure of the decisions that where made there.

@WraithKenny
7 years ago

Adds a check to see if register_post_type is using default capabilities and turns on map_meta_cap

#36 @WraithKenny
7 years ago

Some of the other errors I thought were from this (but weren't) are because I used Upper-case/underscore in the $post_type name.

See #14910 (unrelated but the errors would look similar)

#37 @WraithKenny
7 years ago

  • Keywords has-patch added; needs-patch removed

#38 @nacin
7 years ago

  • Keywords needs-patch added; has-patch removed

So here's what I've been able to track down. My code was based on a false assumption.

Let's say you have a thing post type: capability_type = post (default), capabilities = array( edit_things, edit_thing, etc. )

The problem is that capability_type acts not only as a base, or as a switch for map_meta_cap, but also as a faulty switch in map_meta_cap. The end result is that if you want to edit_thing 4, you end up with also needing edit_published_posts etc. This only occurs when capability_type = post, of course. Even 'page' will not trigger this kind of handling.

To me, it's faulty meta capability handling, so I'm calling this a bug, and I intend to change functionality so edit_published_posts will not be required for editing thing 4, regardless of capability_type.

The solution (which I'll work on) is to re-work the code in map_meta_cap to not map when map_meta_cap is false, end of discussion.

Does anyone see an issue with this? Alternative would be WraithKenny's patch, but keep in mind it would then notice out when capability_type = post and a custom capabilities array is set, so it's not enough.

#39 follow-up: @WraithKenny
7 years ago

The only issue I see is that http://core.trac.wordpress.org/browser/trunk/wp-includes/capabilities.php#L890 checks the edit_published_post capability which means any post_type registered with map_meta_cap => false can not be deleted after publishing, even by admins, right?

My patch was merely a work-around for default settings, but unless I'm missing something, it's a bigger issue with map_meta_cap => false. I would figure, admins should be granted permissions for all post_types.

#40 @WraithKenny
7 years ago

Perhaps the use case is a special user with exclusive rights (that even admins don't have I guess) to a CPT, in which case, a custom role with custom caps would be created, so they may as well set map_meta_cap explicitly to false. This would be a rare and limited case and would suggest that the default be true on map_meta_cap.

#41 in reply to: ↑ 39 @nacin
7 years ago

Replying to WraithKenny:

The only issue I see is that http://core.trac.wordpress.org/browser/trunk/wp-includes/capabilities.php#L890 checks the edit_published_post capability which means any post_type registered with map_meta_cap => false can not be deleted after publishing, even by admins, right?

No, I'm saying, that code would be completely skipped. No mapping if map_meta_cap = false, regardless of capability_type.

My patch was merely a work-around for default settings, but unless I'm missing something, it's a bigger issue with map_meta_cap => false. I would figure, admins should be granted permissions for all post_types.

This would be a rare and limited case and would suggest that the default be true on map_meta_cap.

It's by default false already. The only exception is when capability_type = post, but I consider that to be a bug in most cases. On the other hand a straight capability_type = post with no capabilities array should probably be map_meta_cap = true. That I can agree with. I think I will need to diagram it out.

#42 @WraithKenny
7 years ago

#15334 closed as duplicate of this one.

#43 @nacin
7 years ago

  • Keywords needs-unit-tests added

Here's a big issue with 3.0 functionality.

{{{
register_post_type( 'book', array(
		'capability_type' => 'post',
		'capabilities' => array( 'edit_post' => 'edit_book' ),
	)
);
$user_id = 1;
$post_id = wp_insert_post( array( 'post_title' => 'book', 'post_type' => 'book', 'post_status' => 'private', 'post_author' => 1 ) );
var_dump( map_meta_cap( 'edit_post', $user_id, $post_id ) );
$post_type_obj = get_post_type_object( 'book' );
var_dump( map_meta_cap( $post_type_obj->cap->edit_post, 1, $post_id ) );
}}}
You should get two arrays that match. They won't, though. Checking through the post type cap will give you edit_book (no mapping), but checking through edit_post will give you edit_private_posts based on the faulty meta cap.

The bug everyone has been seeing is caused by a direct edit_post call in the list table. While core shouldn't have any of these, we need to remain compat for plugins. They should work the same.

#44 @nacin
7 years ago

Re-post:


Here's a big issue with 3.0 functionality.

register_post_type( 'book', array(
		'capability_type' => 'post',
		'capabilities' => array( 'edit_post' => 'edit_book' ),
	)
);
$user_id = 1;
$post_id = wp_insert_post( array( 'post_title' => 'book', 'post_type' => 'book', 'post_status' => 'private', 'post_author' => 1 ) );
var_dump( map_meta_cap( 'edit_post', $user_id, $post_id ) );
$post_type_obj = get_post_type_object( 'book' );
var_dump( map_meta_cap( $post_type_obj->cap->edit_post, 1, $post_id ) );

You should get two arrays that match. They won't, though. Checking through the post type cap will give you edit_book (no mapping), but checking through edit_post will give you edit_private_posts based on the faulty meta cap.

The bug everyone has been seeing is caused by a direct edit_post call in the list table. While core shouldn't have any of these, we need to remain compat for plugins. They should work the same.

#45 @nacin
7 years ago

(In [16273]) Cripple capability_type. Produced inconsistent, janky meta cap mapping; now only acts as a capability base. see #14122.

#46 @nacin
7 years ago

(In [16337]) Use cap->edit_post in WP_Posts_List_Table. see #14122.

@nacin
7 years ago

Removes the checks on post_type_supports('author'). Leaving this in would be a change from 3.0, but if most people are using 'supports' for meta boxes but still storing values in post_author, then I'm unsure of the side effects here.

@nacin
7 years ago

map_meta_cap becomes true if capability_type = post and no capabilities were specified. This allows capability_type = post (and nothing else) to work exactly as it did in 3.0, so I'm leaning towards commit on this one.

#47 @nacin
7 years ago

Two more patches attached based on further testing for inconsistencies and regressions.

14122.capability_type.post.diff allows capability_type = 'post' to kick on map_meta_cap, provided that no custom caps were defined. This makes sense to commit.

14122.remove.post_type_supports.author.diff would remove the post_type_supports('author') checks on edit_others_posts and delete_others_posts. The side effects could be interesting if people are storing and retrieving post_author (and even expecting caps to be mapped on it), and simply using 'supports' for the meta boxes and quick edit field. This seems to be what people have been doing with the 'supports' args, unfortunately, so it is probably best to pull this.

#48 @nacin
7 years ago

Justifying 14122.capability_type.post.diff further:

In 3.0, per duck_ earlier in this ticket:

If you use 'capabilities' => array( 'edit_posts' => 'edit_movies' ) then map_meta_cap will see that $post_type->capability_type is equal to 'post' and then decide that the edit_post maps to edit_posts.

Because we used strings in map_meta_cap until now, then if capabilities is not empty, you should assume mapping did not work (without a filter).

Continuing:

On the other hand, if you use 'capability_type' => 'movie' then map_meta_cap will notice the difference and call itself again checking the edit_movie meta-cap, however there is no matching case for this and so the default is to return edit_movie as the capability required. (In the latter case you could, as suggested, hook onto the map_meta_cap filter and check if $cap == 'edit_movie' etc.)

Thus, capability_type = post && empty( capabilities ) means that mapping should be triggered.

#49 @nacin
7 years ago

(In [16338]) Set map_meta_cap to true when capability_type = post and no custom caps are specified. see #14122.

#50 @nacin
7 years ago

This should get a 31compat post on wpdevel, but who knows where to start.

#51 @nacin
7 years ago

Another issue I see is that a plugin may have been using meta capability handling via capability_type = post but only partially. i.e. they mapped edit_posts to edit_movies and may have handed that on their own, but then let delete_post take its course. (Very similar to the OP.)

At that point, we'd need fine-grained control for map_meta_cap, not just on/off, but rather on/off for read, edit, and delete independently. That seems silly.

Other than a decision on 14122.remove.post_type_supports.author.diff, I am inclined to let this sit, and wait for bug reports once beta is out.

#52 @nacin
7 years ago

(In [16387]) capability_type = page and no custom caps should also kick on map_meta_cap. see #14122.

#53 @nacin
7 years ago

  • Keywords needs-patch needs-unit-tests removed

Okay, for 14122.remove.post_type_supports.author.diff, I realized that the changes to each file can be optionally considered separately.

We can make the default capabilities be edit_posts/delete_posts if the post type doesn't support an author -- that doesn't prevent further customization from relying on the author, however, so no real restriction. So I would not revert the change from wp-includes/post.php.

On the other hand, we can continue to check post_author in map_meta_cap() even if the post type doesn't support author. If it ends up using cap->edit_others_posts, then it'll be either the default 'edit_posts', or whatever is specified.

Going to remove the bits from map_meta_cap().


Unit tests can be handled in #UT11.

#54 @nacin
7 years ago

(In [16422]) Don't check post_type_supports in map_meta_cap. see #14122.

#55 @nacin
7 years ago

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

(In [16825]) Remove post_type_supports check from register_post_type caps all together. fixes #14122.

#56 @nacin
7 years ago

Aftermath: #15779

Note: See TracTickets for help on using tickets.