Opened 8 years ago
Last modified 5 years ago
#40922 new enhancement
Use finer-grained capabilities with `customize_changeset` post type
Reported by: | dlh | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | Customize | Keywords: | needs-unit-tests needs-patch |
Focuses: | Cc: |
Description
The customize_changeset
post type is currently registered with all of its post type capabilities set to customize
. As part of adding changeset endpoints in the REST API (#38900):
fine-grained capabilities should be introduced for the
customize_changeset
postcaps
, instead of mapping all tocustomize
.
@westonruter has compiled links to previous discussions and efforts around changeset capabilities here: https://github.com/WP-API/wp-api-customize-endpoints/pull/5#discussion_r118804994.
An example of unexpected behavior caused by the current mapping is where a post ID is passed to current_user_can()
, such as
current_user_can( get_post_type_object( 'customize_changeset' )->cap->edit_post, $changeset_post_id )
This is equivalent to current_user_can( 'customize' )
, which means the post ID is ignored because map_meta_cap()
doesn't check the $args
when mapping the 'customize'
meta cap.
Attachments (7)
Change History (39)
#3
in reply to:
↑ 1
@
8 years ago
If the changeset post hasn't been saved yet (such as if you open a new Customizer session and save a change quickly),
$changeset_post_id
won't have a valid ID, so the check fails.
One approach could be to fall back to the publish_posts
capability when no valid changeset post ID exists:
if ( $is_publish ) {
if ( $changeset_post_id ) {
$current_user_can_publish = current_user_can( 'publish_post', $changeset_post_id );
} else {
$current_user_can_publish = current_user_can( get_post_type_object( 'customize_changeset' )->cap->publish_posts );
}
if ( ! $current_user_can_publish ) {
wp_send_json_error( 'changeset_publish_unauthorized', 403 );
}
}
This approach is simple, but perhaps not as useful for anyone filtering map_meta_cap
or user_has_cap
.
Alternatively, we could create a valid ID. To do that, of course, we would need to create a changeset post. But what should the changeset post's content be? Empty?
I experimented with this approach:
if ( $is_publish ) {
if ( ! $changeset_post_id ) {
$create_changeset = $this->save_changeset_post();
if ( is_wp_error( $create_changeset ) ) {
wp_send_json_error( 'changeset_publish_unauthorized', 403 );
}
$changeset_post_id = $this->changeset_post_id();
}
if ( ! current_user_can( 'publish_post', $changeset_post_id ) ) {
wp_send_json_error( 'changeset_publish_unauthorized', 403 );
}
}
Which creates the changeset post, but the resulting post content is something of a hybrid because it includes data from unsanitized_post_values()
, which is called within save_changeset_post()
.
#4
follow-up:
↓ 5
@
8 years ago
@dlh I think your first approach is ideal. It would give the maximum flexibility for plugins to filter map_meta_cap
to manage whether a user can publish a specific post that has been created, or if the post hasn't been created yet to publish posts of that type generally. Right?
Another question: I see capabilities
is now mapping meta caps to edit_theme_options
. Just to confirm that is desired as opposed to introducing new primitives like edit_customize_changeset
which are then added to the administrator? I suppose in that case it lies in the realm of plugins to supply a different primitive capability if they're wanting, as in the case of Customize Snapshots, to optionally restrict publishing changesets for specific roles?
#5
in reply to:
↑ 4
@
7 years ago
Replying to westonruter:
Another question: I see
capabilities
is now mapping meta caps toedit_theme_options
. Just to confirm that is desired as opposed to introducing new primitives likeedit_customize_changeset
which are then added to the administrator?
It was intentional, but I'm glad you brought it up. I've kept edit_theme_options
so far primarily because of the concerns raised in ticket:36368#comment:5 about new primitive caps. Do you think we should try to use primitive caps along with, say, a user_has_cap
filter to provide backwards-compatibility?
#6
follow-up:
↓ 7
@
7 years ago
@dlh ok, good point. Yeah, re-using the existing primitive cap edit_theme_options
is probably the right. But, will not switching to not use the customize
meta cap cause problems for plugins (like Customize Posts) that grant the customize
cap to allow users to access the Customizer to edit posts there? Since those users don't have edit_theme_options
would they not be prevented from being able to make any changes? Wouldn't that mean that granting customize
capability then no longer be sufficient to grant effective access?
#7
in reply to:
↑ 6
@
7 years ago
Replying to westonruter:
But, will not switching to not use the
customize
meta cap cause problems for plugins (like Customize Posts) that grant thecustomize
cap to allow users to access the Customizer to edit posts there?
Yes, you're right. Any code checking whether 'customize'
was the capability passed to current_user_can()
would break under the approach in the patch (more specifically, whether 'customize'
was passed to WP_User::has_cap()
.
If I'm following correctly, then to maintain compatibility with that code, we have to continue using current_user_can( 'customize' )
. I'm not sure where that leaves us?
#8
@
7 years ago
@dlh As we were just chatting about over Slack, perhaps what we can do is introduce a “meta meta cap” via the map_meta_cap
filter. In core, we essentially need to map edit_post
to customize
and then customize
to edit_theme_options
. We could do this if the post type is registered with edit_post
cap mapped to customize
and then we have a map_meta_cap
filter at a low priority that does:
<?php add_filter( 'map_meta_cap', function( $caps, $cap ) { $post_type_object = get_post_type_object( 'customize_changeset' ); if ( isset( $post_type_object->cap->$cap ) && false !== ( $i = array_search( 'customize', $caps ) ) ) { $customize_caps = map_meta_cap( 'customize' ); // Get caps a plugin may require for Customizer. array_splice( $caps, $i, 1, $customize_caps ); } return $caps; }, 1, 2 );
Then plugins can further use map_meta_cap
to map edit_post
for a customize_changeset
post to a separate discrete primitive cap like edit_customize_changesets
.
#9
follow-up:
↓ 10
@
7 years ago
While I'm not opposed to trying that approach, it seems like we would run into the same issue you raised in comment:6, if I'm following. The user_has_cap
filter outlined in ticket:28605#comment:8 checks the capability string passed to current_user_can()
, and in the snippet, wouldn't $cap
be edit_post
or similar, not customize
?
#10
in reply to:
↑ 9
@
7 years ago
Replying to dlh:
While I'm not opposed to trying that approach, it seems like we would run into the same issue you raised in comment:6, if I'm following. The
user_has_cap
filter outlined in ticket:28605#comment:8 checks the capability string passed tocurrent_user_can()
, and in the snippet, wouldn't$cap
beedit_post
or similar, notcustomize
?
Yeah, I think you're right that the allow_users_who_can_edit_posts_to_customize
for the user_has_cap
filter then would not work anymore. Maybe it would be possible to add another user_has_cap
filter that could do a call_user_func_array( 'current_user_can', array_merge( array( 'customize' ), array_slice( $args, 1 ) ) )
somewhere when the mapped cap was customize
.
But in the end, if it is the right change to make, then we can do an education push and identify plugins in the repo that could be affected by this. The number of plugins that do filter this I suspect could be counted on a pair of hands, but I could be very wrong.
#13
@
7 years ago
40922.2.diff tries a new approach after thinking about 40922#comment:8:
- Set the
map_meta_cap
argument tofalse
when registering thecustomize_changeset
post type.
- Add cases to
map_meta_cap()
for those capabilities set by default inget_post_type_capabilities()
. In each case, map the capability to whatevercustomize
maps to.
- In the
capabilities
argument to the post type, manually specify the caps that won't be set inget_post_type_capabilities()
now thatmap_meta_cap
is false.
The combination of 'map_meta_cap' => false
and adding to the switch
statement would allow us to bypass the default core handling in map_meta_cap()
for (edit|read|delete)_post
, which, it seems to me, has been our goal. For backwards-compatibility, if we want a call like this:
current_user_can( $customize_changeset->cap->edit_post, $changeset_post_id )
to be equivalent to:
current_user_can( 'customize' )
then by bypassing the default post
mappings, we could skip an intermediate filter and provide that compatibility directly through the new case
s.
Additionally, passing the *_customize_changeset
capabilities to map_meta_cap()
means that those caps would also be passed to the 'map_meta_cap'
filter, which seems comparatively easier for developers to act on than 'edit_post'
or other generic values.
However, two concerns with this approach are:
- What should happen with the other primitive capabilities noted in point (3) above, like
'delete_others_customize_changesets'
? Unless we provide amap_meta_cap()
filter (or similar) for those, then they won't be granted to any role by default -- which might be OK, because core doesn't check for those capabilities directly, and they're set now to'customize'
, which also isn't granted to any role by default.
- As I understand it, once we set
'map_meta_cap' => false
, we would be committing to handling changeset capabilities separately in perpetuity.
#14
@
7 years ago
40922.3.diff fixes the $args
passed to map_meta_cap( 'customize' )
.
#15
@
7 years ago
@dlh I think that current_user_can( 'edit_post', $changeset_post_id )
should still work, should it not? In the Customize Snapshots plugin, if it didn't extend the post type's capabilities, then none of the changeset posts' edit post screens would be accessible. For one thing, the links in the post list table would not appear due to calls like current_user_can( 'edit_post', $post->ID )
in class-wp-posts-list-table.php. Secondly, when opening a single edit post screen due to the same call in wp-admin/post.php.
Currently listing out changesets in the WP Admin is plugin territory, but in the future the UI may be part of core. Are all of the admin usages of the edit_post
meta cap wrong? Or should the changeset post type correctly account for using these meta caps?
#16
@
7 years ago
40922.4.diff would move the mapping to a 'map_meta_cap'
filter, which would ensure it applies when either '(delete|edit|read)_post'
or '(delete|edit|read)_customize_changeset'
is used with current_user_can()
.
We discussed this question in our earlier Slack chat, but after some further digging, I'm back to having the impression that current_user_can( 'edit_post', $changeset_post_id )
is at least technically incorrect because it bypasses the post type's map_meta_cap
setting.
I'll outline the explanation as I understand it, but I would be eager for additional guidance or corrections:
If a book
post type sets 'capability_type' => 'book'
and 'map_meta_cap' => false
, then the expectation is that current_user_can( $book->cap->edit_post, $post_id )
(or 'edit_book'
) would fall all the way to the bottom of the switch
statement in map_meta_cap()
: https://github.com/WordPress/WordPress/blob/master/wp-includes/capabilities.php#L499.
The logic in the default
case that can call map_meta_cap()
again wouldn't fire because 'edit_book'
wouldn't be present in the $post_type_meta_caps
global. Developers would have to apply custom mapping through the 'map_meta_cap'
filter.
Core defends against someone calling current_user_can( 'edit_post', $post_id )
on a book
by checking the post type's map_meta_cap
property before applying the default mapping.
If it's false
, then the $cap
and $caps
are changed to what they would have been with current_user_can( $book->cap->edit_post )
, and we break
straight to the 'map_meta_cap'
filter: https://github.com/WordPress/WordPress/blob/master/wp-includes/capabilities.php#L149.
That defensive logic ensures the expected $cap
and $caps
are passed to the 'map_meta_cap'
filter, so plugins and themes don't notice the difference if someone calls current_user_can( 'edit_post' )
.
But it becomes a problem for core in 40922.3.diff because the break
would prevent us from reaching the new case
statements, whereas current_user_can( $changeset->cap->edit_post )
would reach them.
For core, unlike the book
example, the default post types (except custom_css
) either use the 'post'
capability type or have 'map_meta_cap' => true
, so I don't think there are case
s in map_meta_cap()
being missed. But that would no longer be true with 40922.3.diff.
Even if this description is accurate, though, I agree that current_user_can( 'edit_post', $changeset_post_id )
shouldn't break.
#17
@
7 years ago
@dlh I just had a look at the latest patch. In #41332 I have one up for a similar enhancement for the 'attachment' post type which uses another approach. We should probably use the same for both post types.
I'd appreciate if we could discuss this together for both tickets at some point, which method should be eventually used.
#18
@
7 years ago
@flixos90 I'm happy to discuss these tickets with you. Would you prefer to talk here or in Slack? (If in Slack, feel free to ping me at your convenience.)
In the meantime, I'll see what a patch for changesets might look like using the "fake primitive" approach from #41332.
#19
@
7 years ago
@flixos90 40922.5.diff outlines what I think might be involved in granting changeset capabilities as "fake primitives," if I'm understanding the idea correctly.
As you can see, however, the filter no longer has the expected effect if the 'customize' meta cap is mapped to any of the changeset primitives. So, the patch wouldn't work as written, but I hope it provides a starting point for discussion.
#20
@
7 years ago
@dlh 40922.6.diff is how I'd envision it:
- The capabilities checked are
edit_customize_changesets
and so on (as in your patch). - The singular capabilities like
edit_customize_changeset
,delete_customize_changeset
etc. are mapped inmap_meta_cap()
anyway due to themap_meta_cap
argument when registering the post type, so we only need to take care of the plural capabilities. - In
wp_maybe_grant_customize_changesets_capabilities()
, give the capability to every user that has theedit_theme_options
capability. This should preferably use thecustomize
capability, but that is not a primitive capability, thus not available in$allcaps
.customize
however maps toedit_theme_options
, so it would have the same effect for core.
The reason I like this approach is that it's simpler than yet another clause in map_meta_cap()
, and it's much easier to adjust. If I want to give users this capability directly (as a primitive cap) in my custom setup, all I need to do is remove the filter hook. If it happens in map_meta_cap()
, I need to use the filter there to override the default way with custom logic, which is much more involved.
Another reason is that I consider map_meta_cap()
mainly useful for mapping actual meta capabilities (like singular capabilities for one object to more general plural capabilities): Things like edit_customize_changesets
(and even customize
although it's currently part of map_meta_cap()
) are in my opinion primitive capabilities, and the only reason we don't treat them as such is that we can't just add new primitive capabilities in WordPress because of all the database migration that would require. In other words, if we had known from the beginning of core development about these capabilities, they would live in the database alongside the others. Therefore adding these capabilities through the user_has_cap
filter makes more sense to me because that actually filters the data as if the user had these capabilities as primitive ones. If that makes sense. That's obviously just my POV.
To clarify my above thoughts: Every capability that is a general/plural capability and only maps to one other capability without any further conditions should not be in map_meta_cap()
I think. Examples for this are add_users
, manage_post_tags
, edit_categories
, edit_post_tags
, assign_categories
, customize
, ...
Of course we won't be able to change all these, just wanna explain my thoughts.
#21
@
7 years ago
Thinking further about the above, 40922.7.diff would actually solve it in a clean way:
- Grant users with
edit_theme_options
thecustomize
capability. - Then grant users with the
customize
capability all thecustomize_changeset
capabilities.
This would be the most flexible way IMO.
#22
@
7 years ago
@flixos90 The reasoning for trying to use user_has_cap
to grant would-have-been-primitives instead of map_meta_cap
makes sense to me.
Currently, developers can currently control access to the Customizer by filtering map_meta_cap
to remap customize
. However, if I'm reading correctly, those existing filters would no longer work as expected under 40922.6.diff or 40922.7.diff.
For example, the two current_user_can()
calls in this block are currently equivalent to current_user_can( 'customize' )
: https://github.com/WordPress/wordpress-develop/blob/c1694b35d424b8fbc4abaff94e607c8d52111b11/src/wp-includes/class-wp-customize-manager.php#L2081-L2087.
As such, any map_meta_cap
filters that remap customize
have an effect today on those current_user_can()
calls and other similar calls, but I don't think they would have an effect on those calls under 6.diff or 7.diff.
Most of the effort in 40922.4.diff and 40922.5.diff has gone into preserving compatibility with any existing map_meta_cap
filters that remap customize
. But do you think that preserving that compatibility might not be required?
Also, about this point:
The reason I like this approach is that it's simpler than yet another clause in map_meta_cap(), and it's much easier to adjust. If I want to give users this capability directly (as a primitive cap) in my custom setup, all I need to do is remove the filter hook. If it happens in map_meta_cap(), I need to use the filter there to override the default way with custom logic, which is much more involved.
Just noting that 40922.4.diff uses a filter too because adding clauses for changesets to map_meta_cap()
potentially comes with other backwards-compatibility problems (see comment:15).
This ticket was mentioned in Slack in #core-customize by dlh. View the logs.
7 years ago
#24
@
7 years ago
I just found another case where the edit_post
meta cap needs to map correctly for changeset posts in the area of working with autosaves, as calling wp_create_post_autosave()
will in turn call _wp_translate_postdata()
which then does a bare edit_post
meta cap check and it needs to map to the primitive to work: https://github.com/xwp/wordpress-develop/pull/256/commits/dcf300b
#25
@
7 years ago
- Priority changed from normal to high
Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.
This ticket was mentioned in Slack in #core-customize by jeffpaul. View the logs.
7 years ago
#27
@
7 years ago
- Milestone changed from 4.9 to Future Release
- Priority changed from high to normal
We'll keep working on this especially with Customizer v2 feature plugin during 5.0 and with work on Customizer endpoints (#38900).
This ticket was mentioned in Slack in #core by westonruter. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-customize by joyously. View the logs.
6 years ago
#30
@
5 years ago
Stumbled upon this while working on #50128 which didn't work out for the customizer. There's now also WP_Customize_Manager::grant_edit_post_capability_for_changeset()
which shouldn't have been necessary in the first place.
@dlh What's the latest patch that you think should be in core?
#31
@
5 years ago
@ocean90 I'll have to re-familiarize myself with the patches and earlier comments. I'll try to have an answer for you this week.
#32
@
5 years ago
- Keywords needs-patch added; has-patch removed
@ocean90 Tentatively, I'm not sure any of the patches here would be appropriate for #50128. As I see it, the sticking point is that backwards compatibility is lost unless developers are given the opportunity to remap customize
in the map_meta_cap
filter. Using edit_post
, etc. directly means that edit_post
is the filtered $cap
, and the previous patches don't handle that case very well as currently written.
40922.4.diff kinda provided this compatibility by calling map_meta_cap( 'customize' )
again in a filter, but it required setting the post type to map_meta_cap => false
, which might not be ideal.
The only other idea that comes to mind right now that's not expressed in a patch would be to change the post type capabilities
to edit_theme_options
, then hardcode exceptions within map_meta_cap()
for read_post
, etc. to change the $cap
back to customize
(or variations on this, which all come back to hardcoding something in map_meta_cap()
).
I'll be interested to hear your thoughts.
40922.diff is a first pass:
capability_type
and custom(delete|edit|read)_post
meta capabilities.customize_changeset
post type fromcustomize
toedit_theme_options
. The goal is forcurrent_user_can( get_post_type_object( 'customize_changeset' )->cap->edit_post, $post_id )
to map toedit_theme_options
as effectively happens now.current_user_can( 'publish_post' )
where possible.A few notes:
create_posts
capability set tocustomize
. As far as I can tell, thecreate_posts
property is intended to be passed directly tocurrent_user_can()
, not mapped inmap_meta_cap()
.capability_type
should be or needs to be removed. It looks to me likemap_meta_cap()
will call itself again with a*_post
capability when the custom capability in use, so I'm unsure what the difference is.If the changeset post hasn't been saved yet (such as if you open a new Customizer session and save a change quickly),
$changeset_post_id
won't have a valid ID, so the check fails.