Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#38653 closed enhancement (maybelater)

Trigger a doing it wrong when checking a role name as a capability

Reported by: johnbillion's profile johnbillion Owned by:
Milestone: Priority: low
Severity: normal Version:
Component: Role/Capability Keywords: has-unit-tests has-patch 2nd-opinion
Focuses: Cc:

Description

Code which checks current_user_can( 'administrator' ) is essentially bypassing all the power of fine grained capability checking in the roles and capabilities API. Let's trigger a call to _doing_it_wrong() when a cap check is performed against any of the built-in roles, in order to persuade developers to use the capabilities API as it's intended.

There may be valid use cases for checking a user's role in this way. If there are, let's look at how to address those.

Attachments (3)

38653.diff (2.5 KB) - added by fibonaccina 7 years ago.
Change in cap check and unit tests
38653_refresh.diff (2.5 KB) - added by fibonaccina 7 years ago.
Patch refresh: allow cap check to continue after doing it wrong trigger
38653.2.diff (7.6 KB) - added by johnbillion 7 years ago.

Download all attachments as: .zip

Change History (9)

#1 @johnbillion
7 years ago

  • Keywords good-first-bug added; 2nd-opinion removed

#2 @fibonaccina
7 years ago

@johnbillion I attached a proof of concept change in 38653.diff and unit tests. Let me know if you were thinking about a more involved check than this.

@fibonaccina
7 years ago

Change in cap check and unit tests

#3 follow-up: @johnbillion
7 years ago

  • Keywords needs-refresh added

Thanks for the patch, @fibonaccina! The check needs to continue to work as it has previously (without the return false) so code that is checking role names doesn't stop working but still triggers the doing it wrong notice.

@fibonaccina
7 years ago

Patch refresh: allow cap check to continue after doing it wrong trigger

#4 in reply to: ↑ 3 @fibonaccina
7 years ago

@johnbillion Thanks for the feedback! I attached the updated diff. Please let me know in case something doesn't look right.

@johnbillion
7 years ago

#5 @johnbillion
7 years ago

  • Keywords has-unit-tests has-patch 2nd-opinion added; needs-patch needs-unit-tests good-first-bug needs-refresh removed

38653.2.diff is a slightly more comprehensive approach. However, I'm having second thoughts about adding this notice in. There's currently no clean alternative method for checking the current user's role.

#6 @johnbillion
7 years ago

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

Closing as maybelater as there's no great alternative method for checking a user's role (apart from looking at the roles property directly).

Note: See TracTickets for help on using tickets.