Make WordPress Core

Opened 7 years ago

Last modified 2 months ago

#43421 reviewing enhancement

The $capabilities argument in the `add_role()` function requires an explicit grant

Reported by: eclev91's profile eclev91 Owned by: johnbillion's profile johnbillion
Milestone: 6.9 Priority: normal
Severity: normal Version: 4.9.4
Component: Role/Capability Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Reproduce:

Add a role to WP using add_role(), and pass it custom capabilities using the third argument. Like:

<?php
$userCaps = [
        'read',
        'my_cap',
];
add_role('my_new_role', 'My New Role', $userCaps);

Create a user of said role. Then:

<?php
$user = new WP_User($id_of_created_user);
return user_can($user, 'my_cap'); // returns false, expected return true

If, alternatively, I create my role this way:

<?php
$userCaps = [
        'read',
        'my_cap',
];
$role = add_role('my_new_role', 'My New Role');
foreach($userCaps as $cap) {
        $role->add_cap($cap);
}

And repeat testing, it returns true as expected.

This has to do with how the caps are saved. The first way, when capabilities are retrieved in WP_User::has_cap(), they look like this:

<?php
array(4) {
  [0]=>
  "read",
  [1]=>
  "my_cap",
  ["my_new_role"]=>
  bool(true)
  ["exist"]=>
  bool(true)
}

And when done via add_cap, they look like this:

<?php
array(4) {
  ["read"]=>
  bool(true)
  ["my_cap"]=>
  bool(true)
  ["my_new_role"]=>
  bool(true)
  ["exist"]=>
  bool(true)
}

The subsequent empty array check is true for the first, and false for the second.

Change History (14)

#1 @eclev91
7 years ago

The way around this is to format your capabilities when using add_role like so:

<?php
$userCaps = [
  'read' => true,
  'my_cap' => true,
];

I really feel like it should support a generic array with true being assumed.

#2 @soulseekah
7 years ago

The add_cap method has the following signature: add_cap( $cap, $grant = true )

Notice the 2nd argument. To add a capability you need two things: a capability name, and a grant status (true, false).

When adding capabilities using add_role, the third argument needs a way to pass both the capability name and the grant status. How does it do it? A key-value array.

As stated in the docblock:

* @param array $capabilities List of capabilities, e.g. array( 'edit_posts' => true, 'delete_posts' => false );

and the Codex examples show this, too: https://codex.wordpress.org/Function_Reference/add_role#Example

You have to pass both these things. When you pass $userCaps = [ 'read', 'my_cap', ]; you are effectively passing in the following array: [ 0 => 'read', 1 => 'my_cap' ], which in turn tries to perform two calls: add_cap( 0, 'read' ) and add_cap( 1, 'my_cap' ), which is, indeed, incompatible with what you're trying to achieve.

This is not a bug, in my opinion.

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

#3 @johnbillion
7 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Thanks for the report, but yes this isn't a bug and the documentation for both functions is correct.

#4 @eclev91
7 years ago

I actually agree, but think it might be worth reviewing as an enhancement. If $capabilities is passed a numerically-keyed array (or "not associative", or whatever), that it be assumed each cap should be assigned as "true".

Thoughts?

Semi-related, what's the use case for explicitly saying someone doesn't have a capability ("my_cap" => false)?

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

#5 @eclev91
7 years ago

  • Type changed from defect (bug) to enhancement

#6 @soulseekah
7 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Interesting though. add_role doesn't come with any predefined capabilities that you would want to "disable". So theoretically this sugary syntax could be supported. Any thoughts?

#7 @eclev91
7 years ago

By "this sugary syntax", you mean passing an array of caps without associated booleans? Yeah, I think from a UX perspective it makes sense. Even though the nuts and bolts of how it's accomplished are different, when I add_cap, there's a mechanism that assumes I mean true (the default second arg). A consistent UX would allow for the same assumption to be made when assigning caps at role creation.

#8 @johnbillion
7 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone set to Awaiting Review

This ticket was mentioned in PR #7351 on WordPress/wordpress-develop by @eclev91.


8 months ago
#9

  • Keywords has-patch added; needs-patch removed

Updates WP_Role::add_role to assume capabilities should be granted for capabilities not passed associatively.

Trac ticket: https://core.trac.wordpress.org/ticket/43421

#10 @eclev91
7 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#11 @eclev91
7 months ago

  • Summary changed from The $capabilities argument in the `add_role()` function is incompatible with `user_can` to The $capabilities argument in the `add_role()` function requires an explicit grant

#12 @johnbillion
3 months ago

  • Milestone changed from Awaiting Review to 6.8
  • Owner set to johnbillion
  • Status changed from reopened to reviewing

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 months ago

#14 @audrasjb
2 months ago

  • Milestone changed from 6.8 to 6.9

As per today's bug scrub: It appears this ticket is still under discussion. As 6.9 is very close, I'm moving it to 6.9. Feel free to move it back to 6.8 if it can be committed by Monday.

Note: See TracTickets for help on using tickets.