Make WordPress Core

Opened 7 years ago

Last modified 4 years ago

#40922 new enhancement

Use finer-grained capabilities with `customize_changeset` post type

Reported by: dlh's profile 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 post caps, instead of mapping all to customize.

@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)

40922.diff (6.3 KB) - added by dlh 7 years ago.
40922.2.diff (2.2 KB) - added by dlh 7 years ago.
40922.3.diff (2.3 KB) - added by dlh 7 years ago.
40922.4.diff (2.8 KB) - added by dlh 7 years ago.
40922.5.diff (3.3 KB) - added by dlh 7 years ago.
40922.6.diff (3.1 KB) - added by flixos90 7 years ago.
40922.7.diff (3.8 KB) - added by flixos90 7 years ago.

Download all attachments as: .zip

Change History (39)

@dlh
7 years ago

#1 follow-up: @dlh
7 years ago

40922.diff is a first pass:

  • Remove the post type's custom capability_type and custom (delete|edit|read)_post meta capabilities.
  • Switch most capabilities registered with the customize_changeset post type from customize to edit_theme_options. The goal is for current_user_can( get_post_type_object( 'customize_changeset' )->cap->edit_post, $post_id ) to map to edit_theme_options as effectively happens now.
  • Use current_user_can( 'publish_post' ) where possible.

A few notes:

  • I kept the create_posts capability set to customize. As far as I can tell, the create_posts property is intended to be passed directly to current_user_can(), not mapped in map_meta_cap().
  • I'm not positive about whether the custom capability_type should be or needs to be removed. It looks to me like map_meta_cap() will call itself again with a *_post capability when the custom capability in use, so I'm unsure what the difference is.
  • There is (at least) one issue still to address with this change:
if ( $is_publish && ! current_user_can( 'publish_post', $changeset_post_id ) ) {

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.

#2 @westonruter
7 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.9

#3 in reply to: ↑ 1 @dlh
7 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: @westonruter
7 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 @dlh
7 years ago

Replying to westonruter:

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?

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: @westonruter
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 @dlh
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 the customize 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 @westonruter
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: @dlh
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 @westonruter
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 to current_user_can(), and in the snippet, wouldn't $cap be edit_post or similar, not customize?

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.

Last edited 7 years ago by westonruter (previous) (diff)

#11 @westonruter
7 years ago

  • Keywords needs-unit-tests added

#12 @dlh
7 years ago

Potentially related: #41332.

@dlh
7 years ago

#13 @dlh
7 years ago

40922.2.diff tries a new approach after thinking about 40922#comment:8:

  1. Set the map_meta_cap argument to false when registering the customize_changeset post type.
  1. Add cases to map_meta_cap() for those capabilities set by default in get_post_type_capabilities(). In each case, map the capability to whatever customize maps to.
  1. In the capabilities argument to the post type, manually specify the caps that won't be set in get_post_type_capabilities() now that map_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 cases.

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:

  1. What should happen with the other primitive capabilities noted in point (3) above, like 'delete_others_customize_changesets'? Unless we provide a map_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.
  1. As I understand it, once we set 'map_meta_cap' => false, we would be committing to handling changeset capabilities separately in perpetuity.

@dlh
7 years ago

#14 @dlh
7 years ago

40922.3.diff fixes the $args passed to map_meta_cap( 'customize' ).

#15 @westonruter
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?

@dlh
7 years ago

#16 @dlh
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 cases 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 @flixos90
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 @dlh
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.

@dlh
7 years ago

#19 @dlh
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.

@flixos90
7 years ago

#20 @flixos90
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 in map_meta_cap() anyway due to the map_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 the edit_theme_options capability. This should preferably use the customize capability, but that is not a primitive capability, thus not available in $allcaps. customize however maps to edit_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.

Version 0, edited 7 years ago by flixos90 (next)

@flixos90
7 years ago

#21 @flixos90
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 the customize capability.
  • Then grant users with the customize capability all the customize_changeset capabilities.

This would be the most flexible way IMO.

#22 @dlh
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 @westonruter
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 @westonruter
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 @westonruter
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.


5 years ago

#30 @ocean90
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 @dlh
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 @dlh
4 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.

Note: See TracTickets for help on using tickets.