Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#53131 new feature request

Disjunctive normal form for WP_User::has_cap

Reported by: manfcarlo's profile manfcarlo Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Users Keywords:
Focuses: Cc:

Description

The following block of code is from the has_cap method on the WP_User class:

    // Must have ALL requested caps.
    foreach ( (array) $caps as $cap ) {
        if ( empty( $capabilities[ $cap ] ) ) {
            return false;
        }
    }
 
    return true;

$caps is an array of capabilities that are returned from the map_meta_cap filter hook.

This assumes there is only one possible set of capabilities that can pass the capability check.

In reality, you might want it to be possible to pass the capability check in multiple ways. One possible way to implement this is an array of arrays that represent disjunctive normal form:

    $has_cap_set = true;

    // Must have ALL caps in some set.
    foreach ( $caps as $cap_set ) {
        foreach ( $cap_set as $cap ) {
            if ( empty( $capabilities[ $cap ] ) ) {
                $has_cap_set = false;
                break;
            }
        }

        if ( $has_cap_set ) {
            return true;
        }
    }
 
    return false;

Currently, plugins can simulate this behaviour by calling user_can from within the map_meta_cap filter and returning exist if user_can returns true, but this is not ideal for two reasons:

  1. map_meta_cap is meant to simply map to capabilities, not check whether the user actually has them
  2. Simply returning exist might be misleading to other plugins that use the map_meta_cap or user_has_cap filter

It would make this a much simpler job if core offered support for map_meta_cap to return a disjunctive normal form of capabilities.

Change History (5)

#1 @joyously
3 years ago

Can you give an example of when multiple capabilities are checked, and the user has to have all of them to pass the check?
Your proposed code would fail on the second set because the boolean is still the value from the previous set (assuming data of the first set doesn't match but second does).

#2 follow-up: @peterwilsoncc
3 years ago

@manfcarlo Thanks for the suggestion.

I'm interested to know how this would be an improvement on something like:

<?php
if ( current_user_can( 'permission_one' ) || current_user_can( 'permission_two' ) {
   do_something();
}

I worry that including the OR relationship in the native WP_Core::has_cap() check could lead to developer confusion so both a strong use case and significant benefit would be needed for such a change. Are you able to expand on each?

#3 in reply to: ↑ 2 @manfcarlo
3 years ago

Replying to joyously:

Your proposed code would fail on the second set because the boolean is still the value from the previous set (assuming data of the first set doesn't match but second does).

You're right. $has_cap_set = true; should be moved inside the first foreach loop. Thanks for pointing out.

Also, the code is intended more as an illustrative example than an exact proposal. The tricky part is backward compatibility.

Replying to joyously:

Can you give an example of when multiple capabilities are checked, and the user has to have all of them to pass the check?

An example is when a user tries to edit a post that is not their own and is already published. They would need all three of edit_posts, edit_others_posts and edit_published_posts.

Replying to peterwilsoncc:

@manfcarlo Thanks for the suggestion.

I'm interested to know how this would be an improvement on something like:

<?php
if ( current_user_can( 'permission_one' ) || current_user_can( 'permission_two' ) {
   do_something();
}

I worry that including the OR relationship in the native WP_Core::has_cap() check could lead to developer confusion so both a strong use case and significant benefit would be needed for such a change. Are you able to expand on each?

Because it needs to be able to filter the result of core capability checks, using the map_meta_cap filter.

One of my plugins dynamically registers post types through a UI. To be able to edit a post of one of those dynamically registered post types, a user can either have the explicit capabilities for that post type (saved in the database) or they can alternatively pass the capability check if they can edit the record that represents the post type, which is a post itself.

Currently, the plugin does this by prematurely calling up user_can( $user_id, 'edit_post', $post_type_record ) and returning exist if user_can returns true. It works, but it's not really the correct behaviour of map_meta_cap. Ideally, it should be able to just call map_meta_cap( 'edit_post', $user_id, $post_type_record ) and return the result as a second array.

#4 follow-up: @peterwilsoncc
3 years ago

Thanks for expanding on your use case, I really appreciate it.

I've been doing some thought experiments on this over the last couple of weeks and am a little concerned about how this would work with plugins expecting the older one-dimensional array vs plugins making use of the multi-dimensional array.

The cap check array might become:

<?php
$caps = [
        'first_plugin_alternative_one' => [ /* set one */ ],
        'first_plugin_alternative_two' => [ /* set two */ ],
        'second_plugin_alt_one' = [ /* set one */ ],
        'second_plugin_alt_two' = [ /* set two */ ],
        'edit_published_posts',
        'edit_others_published_posts',
        'third_plugin_custom_capability_actions'
];

I've been trying to work out how WordPress could safely resolve such an array and have been unable to do so. It becomes even more complicated if a do_not_allow is in all of one plugin's sets but not another plugin's, or even at the top level from a plugin expecting a single dimensional array.

I do understand why you think including current_user_can() in a meta cap check isn't ideal but I think it might be the most secure solution.

Rather than immediately close the ticket, I am wondering if you have any suggestions on a way WordPress could handle such a situation?

#5 in reply to: ↑ 4 @manfcarlo
3 years ago

Replying to peterwilsoncc:

When I opened the ticket, I just wanted to "pitch" the idea in the simplest way possible, but I was kind of aware that it would be backward incompatible. Plugins that use the map_meta_cap filter expect an array of strings, so I don't think it could be implemented through the existing filter.

However, I wonder if it could be achieved by adding a second filter. For example:

<?php
$strings = apply_filters( 'map_meta_cap', $strings );

$dnf = apply_filters( 'map_meta_cap_arrays', array( $strings ) );

// Now user must have ALL from ANY set in $dnf

I believe that disjunctive normal form can be alternatively expressed as conjunctive normal form, so the following could be an alternative implementation:

<?php
$strings = apply_filters( 'map_meta_cap', $strings );

$cnf = array();

foreach ( $strings as $string ) {
    $cnf[] = apply_filters( 'map_meta_cap_strings', array( $string ) );
}

// Now user must have ANY from ALL sets in $cnf

These are just ideas off the top of my head, so it's entirely possible there are other issues with these suggested implementations.

For example, which of the two filters should run first and which should run second? Should the output of the first filter be passed into the second filter, or should they run separately and be combined at the end? These kinds of implications would have to be considered, so as not to unexpectedly change the behaviour of existing plugins.

Note: See TracTickets for help on using tickets.