Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update capability checks in conditional field display for editors/administrators #778

Closed
jomurgel opened this issue Oct 6, 2020 · 1 comment

Comments

@jomurgel
Copy link
Contributor

jomurgel commented Oct 6, 2020

Potentially related: #41

When creating fields that are restricted to editors AND/OR administrators, FieldManager will utilize the following syntax:

if ( ! current_user_can('editor') && ! current_user_can('administrator') ) {

Flagged by VIP

The current_user_can() method was never really meant to check for roles, only capabilities. Otherwise, custom roles may not work as expected, including on the VIP Support side.

From @joemcgill

For arcane purposes, user roles are available to be checked using current_user_can() but it definitely is not best practice, even if it works 90% of the time. Usually, managing capabilities is a much better way of going about it.

From @dlh01

the common example of why it’s not a great practice to use current_user_can() to check for roles is with super admins in multi-sites: super admins will always pass current_user_can( 'editor' ) but might not actually have that role on the site

Replacing the code above with something like the following would circumvent that issue:

if ( ! empty( wp_get_current_user()->roles ) && ! in_array( 'administrator', wp_get_current_user()->roles, true) && ! in_array( 'editor', wp_get_current_user()->roles, true) ) {

NOTE: This may extend to other roles, but these two seem to be the most common.

VIP Pull Request for reference: https://github.com/wpcomvip/brookings/pull/2049

@mboynes
Copy link
Contributor

mboynes commented Oct 9, 2020

Followed up with @jomurgel privately and it turned out this was not Fieldmanager itself, but rather, some custom code on a site. We audited Fieldmanager's use of current_user_can() and all looks good, so we can resolve this!

@mboynes mboynes closed this as completed Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants