current_user_can/has_cap fails when user has multiple roles

[mikejolley]

To replicate the issue, install a role editor. Setup a user with primary role 'author' and secondary role 'customer' (this is a WooCommerce role which has ONLY 'read' access, nothing else).

​https://dl.dropboxusercontent.com/s/xgucqvvh6no3skm/2016-03-30%20at%2017.49.png?dl=0

You can add a role with only:

'read' => true

permissions if you don't have WooCommerce installed.

Dump:

current_user_can( 'edit_posts' )

It will be false.

During get_role_caps() in class-wp-user.php, each role is retrieved and merged. The merge itself doesn't look at values, so if multiple roles have the same 'cap' but different value, these overwrite each other.

In my case, edit_posts was true for the author role, but false for customer role. Customer role false overwrote author role true.

Since caps only allow access to things if 'true', I think we can safely discard all 'false' caps when getting roles. If false caps are discarded, only true caps are left which works around the issue and fixes user capabilities if they have multiple roles at once.

Fix to follow (added array_filter to discard all 'false' caps, allowing us to merge only 'true' caps).

Had this reported to us in ​https://github.com/woothemes/woocommerce/issues/10612#issuecomment-203518038 but wasn't a WooCommerce issue.

[mikejolley]

Fix for 36376

[johnbillion]

Thanks for the report, @mikejolley. This will need some unit tests.

[jubstuff]

Hello @mikejolley,

I wrote a simple unit test but it seems that I'm unable to reproduce the issue.
Can you please have a look at that?

I also tried the WPFront User Role Editor, but the result was the same.

[mikejolley]

@jubstuff did you use customer role? The order of the roles is also important (author primary).

[johnbillion]

We discussed this issue at WCTRN contributor day, and we couldn't reproduce the issue even with various permutations of WPFront User Role Editor and WooCommerce activated or not.

36376.tests.diff​ is a test that demonstrates the current behaviour works as expected and that a user with the roles of Author and Customer does indeed have the edit_posts cap.

Is there anything missing from that test Mike?

[mikejolley]

@johnbillion Test looks fine but I definitely experienced it via UI. Maybe I can try programmatically to reproduce this.

@krogsgard originally reported it to me I believe.

[mikejolley]

@johnbillion So I tried again via UI and it did occur. I looked into it further and the customer role had edit_posts false.

Whilst different from my OP, this should still be considered a issue I think if the roles don't layer on top correctly since caps can be modified over time.

Propose making unit test the following:

$expected_caps = array( 
    'read' => true, 
    'edit_posts' => false,
);

Since the primary role is author, edit_posts should still be true if working (patch should resolve).

[dd32]

This is a fun bug involving a very little known functionality of roles I believe. (I could be completely off track here though)

WordPress roles are rather over complicated for how simple they look on the surface.
In WordPress, a user can be assigned to any number of roles (A Role being a group of capabilities) AND any capabilities directly.
Ie. User bob can be in the roles Role1, Role2 and have the capability Cap99.

Roles are groups of capabilities, but they're not just which capabilities that should be added to a user, they can also explicitly deny a user that capability. (fix.36376.diff​ removes this functionality I believe).
ie. Role2 could specifically say You shalt not have Cap99, even if you already do..

The problem I think may be occurring here, is two fold - 1) Someones role is denying a cap, and the author didn't realise they were doing so and 2) the order of operations here is special.

The following example demonstrates it kind of:

<?php

add_role( 'has_test_cap', 'Has the "test" cap', array( 'test' => true ) );
add_role( 'denied_test_cap', 'Denied the "test" cap', array( 'test' => false ) );

$user = new WP_User( 1 );

// Verify no caps = should be false.
var_dump( $user->has_cap( 'test' ) ); // FALSE (Correct)

// Add cap via role
$user->add_role( 'has_test_cap' );
var_dump( $user->has_cap( 'test' ) ); // TRUE! (Correct)

// Deny the cap via role
$user->add_role( 'denied_test_cap' );
var_dump( $user->has_cap( 'test' ) ); // FALSE! (Correct)

//  -- User now has both roles, and is denied the capability.

// Add the cap directly:
$user->add_cap( 'test', true );
var_dump( $user->has_cap( 'test' ) ); // TRUE! (Correct)

//  -- User now has both roles AND the capability, and is allowed to perform the action

// Cleanup
$user->remove_role( 'has_test_cap' ); $user->remove_role( 'denied_test_cap' ); $user->remove_cap( 'test' );
echo '<hr>';

// Lets add them in the reverse order now.

// Verify no caps = should be false.
var_dump( $user->has_cap( 'test' ) ); // FALSE (Correct)

// Deny the cap via role
$user->add_role( 'denied_test_cap' );
var_dump( $user->has_cap( 'test' ) ); // FALSE! (correct)

// Add cap via role
$user->add_role( 'has_test_cap' );
var_dump( $user->has_cap( 'test' ) ); // TRUE! (Correct)

// -- User now has both roles, and is is allowed to perform the action.

// Cleanup
$user->remove_role( 'has_test_cap' ); $user->remove_role( 'denied_test_cap' ); $user->remove_cap( 'test' );
echo '<hr>';

// Verify no caps = should be false.
var_dump( $user->has_cap( 'test' ) ); // FALSE (Correct)

// Add cap directly:
$user->add_cap( 'test', true );
var_dump( $user->has_cap( 'test' ) ); // TRUE! (Correct)

// Deny the cap via role
$user->add_role( 'denied_test_cap' );
var_dump( $user->has_cap( 'test' ) ); // TRUE! Wait, what? (Correct)

// -- User has denied_test_cap role, but yet is allowed to perform action as capabilities take precedence

// Cleanup 
$user->remove_role( 'has_test_cap' ); $user->remove_role( 'denied_test_cap' ); $user->remove_cap( 'test' );
remove_role( 'has_test_cap' );
remove_role( 'denied_test_cap' );

So clearly, the order to which the roles are assigned to a user is important, and it's likely that roles being applied in different orders when a role defines a capability as denied.

I don't think there's a bug here, I think it's working as intended, even if a little funky at present.

The issue boils down to:

• If a user has a role which denies a capability, should it take precedence over all roles (current and future) which allow it?

or

• If a user has a role which denies a capability, it should only take the capability away from current roles, and future additional roles can allow it

Based on the above, I'm fairly sure this ticket is invalid or wontfix.

[knutsp]

A tested capability may be true, false or undefined (null). If false or null, false is returned.

Explicitly setting a capability to false (denied) should take precedence, even over later added roles or explicit capabilities, since this is a special and more rare case. The lack of a capability is normal way of not giving that capability.

But when a user is explicitly denied a capability, adding a later role than includes this capability should not invalidate the first denial. Granting something that once was denied should be done by removing the denial, explicitly, or else it may be unintentional. Or there should be a warning that the new role invalidates an earlier denial, but I see that highly impractical to implement.

Logic (pseudo code):

<?php
$has_cap = false;
foreach ( $roles as $role ) {
    if ( isset( $role['my_cap'] ) && $role['my_cap'] ) {
        $has_cap = true;
        break;
    } elseif ( isset( $role['my_cap'] ) ) { // $role['my_cap'] must be false
        $has_cap = false;
        break;
    }
}

If I understand it correctly, the Members plugin​https://wordpress.org/plugins/members/ uses a logic like this, if set to. This should be the default.

[dd32]

Replying to knutsp:

Explicitly setting a capability to false (denied) should take precedence, even over later added roles or explicit capabilities, since this is a special and more rare case. The lack of a capability is normal way of not giving that capability.

I agree with this, lets make this happen.

[dd32]

Replying to dd32:

Replying to knutsp:

Explicitly setting a capability to false (denied) should take precedence, even over later added roles or explicit capabilities, since this is a special and more rare case. The lack of a capability is normal way of not giving that capability.

I agree with this, lets make this happen.

Actually, mostly. I think an explicit capability should take precedence over a role - but then the more I think about it, the more this is so ambiguous.

• Bob has a role of editor but a role of denied_publish_capabilities. Should bob be able to post? IMHO: No
• Alice has a role of contributor but a role of allowed_to_publish. Should alice be able to post? IMHO: Yes
• John has a role of editor, a role of denied_publish_capabilities AND allowed_to_publish. Should John be able to post? IMHO: Maybe. Implementation detail. I'd agree and err on the side of No

The scenario is an organisation where all editors are by default given denied_publish_capabilities and later given the allowed_to_publish role.

Then capabilities directly:

• Bob is given the role of editor, but then denied the right to publish through publish_posts => false. Should bob be able to post? IMHO: No
• Alice is given the role of contributor, but then allowed to publish through publish_posts => true. Should alice be able to post? IMHO: Yes
• John is given the role of editor, a role of denied_publish_capabilities, but then allowed to publish through publish_posts => true. Should John be able to post? IMHO: Yes.

[knutsp]

Replying to dd32:

• John is given the role of editor, a role of denied_publish_capabilities, but then allowed to publish through publish_posts => true. Should John be able to post? IMHO: Yes.

In such cases, denied_publish_capabilities must be removed before publish_posts => true can have effect. That will be consistent and easier to document than having exceptions that is hard to grasp.

[dd32]

Replying to knutsp:

Replying to dd32:

• John is given the role of editor, a role of denied_publish_capabilities, but then allowed to publish through publish_posts => true. Should John be able to post? IMHO: Yes.

In such cases, denied_publish_capabilities must be removed before publish_posts => true can have effect. That will be consistent and easier to document than having exceptions that is hard to grasp.

If we were designing it from scratch, I would maybe agree. However, I see a capability as more specific than a role, if I have a capability granted, I expect it should override all roles (That includes taking away the ability from a roll). Todays implementation also seems to apply direct capabilities prior to role application too, so that has to be taken into account.

I think the only change we can make here, would be to ensure that two users have the same cap rules, regardless of the order the roles are applied.

[bamadesigner]

Patch for ticket 36376

[bamadesigner]

I just came across this bug on a site, where users have multiple user roles but because one of those roles said "edit_posts" => 0, it removed that capability for the user even though they were also assigned as an "editor" who had the capability assigned.

I agree that if a capability is set as false, it should be kept, but only if no other roles set it as true.

I put together a quick diff to show what that could look like. This is repeated code that could be put in a function if we decide to go with it. This method will keep capabilities set to false if no other roles set the capability to true.

for36376.diff​

[pento]

This requires significantly more testing and consideration.

Unit tests would also help a lot.