WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#10763 closed enhancement (worksforme)

Abstract function for authorization check in admin pages

Reported by: johnjosephbachir Owned by: ryan
Milestone: Priority: normal
Severity: normal Version: 2.8.4
Component: Security Keywords:
Focuses: Cc:

Description

This patch provides an abstract function to reduce code repetition in cases of authorization check, with accompanying user feedback throughout the codebase, such as this:

if ( ! current_user_can('manage_options') )
    wp_die(__('You do not have sufficient permissions to manage options for this blog.'));

I tried testing it, but actually couldn't think of a case that these checks accomodate for the menu.php/admin.php whitelist system takes care of all the unauthorized access cases that I tried.

I made the $full_message parameter in order to accomodate for messages with non-standard structure, but it turned out that they were all standard. The only slight exceptions were the couple of instances that were wrapped in <p> tags, which I deemed unnecessary in the rare case of simply telling a user that they aren't allowed to do something -- I figured these must have been formatted differently only for historical reasons and not because the behavior is actually different in that cast -- but correct me if I'm wrong.

So-- feel free to remove the $full_message logic (since it's not currently being used in any of the cases) -- or, maybe it will come in handy for plugin developers?

Let me know what you think!

Attachments (1)

check_authorization.patch (17.9 KB) - added by johnjosephbachir 5 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 johnjosephbachir5 years ago

Gah-- didn't preview-- please excuse syntax and spelling errors in the ticket...

comment:2 dd325 years ago

  • Keywords needs-patch dev-feedback added
  • Milestone changed from 2.8.5 to Future Release

Translation functions must be passed a string.

__($message); will insert "$message" as a string into the translation file, Its not aware that t might be set to something else elsewhere.

sprintf() also needs to be used to concencate, ie. sprintf(__('Sorry but you cannot %1$s this blog'), $message); However combining multiple strings this way does not work in all languages, "Cannot manage options" may not always have the same structure within foriegn languages (In that, It cant just be plonked within another string without words being shuffled between them both)

Future release pending Developer feedback and/or workable patch

comment:3 johnjosephbachir5 years ago

How about this invocation then?

check_authorization('manage_options', __('You do not have sufficient permissions to manage options for this blog.'));

comment:4 azaozz5 years ago

DD32 is right, looks like we can either have a customized message or introduce a small function to check permissions and output a standard ('You do not have sufficient permissions to access this page') error message.

Don't think we need the complexity of a check_authorization() function that tries to construct custom messages, won't be useful for plugins too. And it becomes redundant if we have to pass the full message to it, better to leave it as is.

comment:5 johnjosephbachir5 years ago

I'm a bit lost-- what are you saying is more complex, one flavor of the abstracted function vs. another? Or the abstracted function vs. repeating the same code dozens of times?

comment:6 johnjosephbachir5 years ago

The reason I ask is, if it's the former, I would like to adopt my patch to your vision, and if it's the latter, then this will help me provide more helpful patches in the future.

comment:7 dd325 years ago

How about this invocation then?

Would work.

Whilst i dont like repeating code.. The auth checks are simple if statements, It makes it obvious as to what happens. "If not current user can perform manage_options, then die with message BlahBlahBlah". I'd favour leaving them be for the simple fact of readable code.

comment:8 scribu5 years ago

  • Keywords needs-patch dev-feedback removed
  • Resolution set to worksforme
  • Status changed from new to closed

I agree with dd32: you loose readability and you don't get much benefit: 1 line instead of 2.

comment:9 westi4 years ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.