Opened 9 years ago
Last modified 6 years ago
#36376 accepted defect (bug)
current_user_can/has_cap fails when user has multiple roles
Reported by: | mikejolley | Owned by: | dd32 |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Role/Capability | Keywords: | dev-feedback needs-unit-tests |
Focuses: | Cc: |
Description
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.
Attachments (3)
Change History (21)
#2
@
9 years ago
- Component changed from General to Role/Capability
- Focuses administration removed
- Keywords needs-unit-tests added
- Version trunk deleted
#4
@
9 years ago
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.
#5
@
9 years ago
@jubstuff did you use customer role? The order of the roles is also important (author primary).
#6
@
9 years ago
- Keywords reporter-feedback has-unit-tests added; dev-feedback needs-unit-tests removed
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?
#7
@
9 years ago
@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.
#8
@
9 years ago
@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).
#10
@
7 years ago
- Keywords close added; has-patch removed
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
.
#11
follow-up:
↓ 12
@
7 years ago
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 pluginhttps://wordpress.org/plugins/members/ uses a logic like this, if set to. This should be the default.
#12
in reply to:
↑ 11
;
follow-up:
↓ 13
@
7 years ago
- Keywords close removed
- Milestone changed from Awaiting Review to 5.0
- Owner set to dd32
- Status changed from new to accepted
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.
#13
in reply to:
↑ 12
;
follow-up:
↓ 14
@
7 years ago
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 ofeditor
but a role ofdenied_publish_capabilities
. Should bob be able to post? IMHO: NoAlice
has a role ofcontributor
but a role ofallowed_to_publish
. Should alice be able to post? IMHO: YesJohn
has a role ofeditor
, a role ofdenied_publish_capabilities
ANDallowed_to_publish
. Should John be able to post? IMHO: Maybe. Implementation detail.
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 ofeditor
, but then denied the right to publish throughpublish_posts => false
. Should bob be able to post? IMHO: NoAlice
is given the role ofcontributor
, but then allowed to publish throughpublish_posts => true
. Should alice be able to post? IMHO: YesJohn
is given the role ofeditor
, a role ofdenied_publish_capabilities
, but then allowed to publish throughpublish_posts => true
. Should John be able to post? IMHO: Yes.
#14
in reply to:
↑ 13
;
follow-up:
↓ 15
@
7 years ago
Replying to dd32:
John
is given the role ofeditor
, a role ofdenied_publish_capabilities
, but then allowed to publish throughpublish_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.
#15
in reply to:
↑ 14
@
7 years ago
Replying to knutsp:
Replying to dd32:
John
is given the role ofeditor
, a role ofdenied_publish_capabilities
, but then allowed to publish throughpublish_posts => true
. Should John be able to post? IMHO: Yes.In such cases,
denied_publish_capabilities
must be removed beforepublish_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.
#16
@
7 years ago
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.
Fix for 36376