Make WordPress Core

Opened 7 years ago

Last modified 6 days 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.8 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 (12)

#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.


5 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
5 months ago

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

#11 @eclev91
5 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
6 days ago

  • Milestone changed from Awaiting Review to 6.8
  • Owner set to johnbillion
  • Status changed from reopened to reviewing
Note: See TracTickets for help on using tickets.