Make WordPress Core

Opened 6 years ago

Closed 4 months ago

Last modified 4 months ago

#45197 closed task (blessed) (fixed)

Introduce `user_can_for_blog()`

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

Description

The available user capability checking functions include:

  • current_user_can()
  • user_can()
  • current_user_can_for_blog()

What's missing is user_can_for_blog() so that both a user ID and a site ID can be passed in order to check a given user's capabilities on a given site.

Attachments (1)

45197.diff (1.1 KB) - added by tmanoilov 6 years ago.

Download all attachments as: .zip

Change History (30)

#1 @johnjamesjacoby
6 years ago

+1 to this.

I have coded some version of this logic at least a few times (into private methods in plugins.)

I can adapt something into a patch, but it’s pretty straight forward if anyone wants to try.

Last edited 6 years ago by johnjamesjacoby (previous) (diff)

@tmanoilov
6 years ago

#2 @tmanoilov
6 years ago

  • Keywords has-patch added; needs-patch removed

Hey guys, I'm at WordCamp Sofia 2018 Contributors day and this is the first patch I'm submitting. I'm not able to fully test it so I'm looking forward to a review. Thanks in advance.

#3 @johnbillion
11 months ago

  • Milestone changed from Future Release to 6.6
  • Owner set to johnbillion
  • Status changed from new to reviewing

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


10 months ago
#4

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

Adding a new function named user_can_for_blog(), and unit tests for it.

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

#5 @rajinsharwar
10 months ago

Added the new function, along with its unit tests.

@johnbillion commented on PR #6418:


9 months ago
#6

@Rajinsharwar @JJJ Any thoughts on what's up here? The test_current_user_can_for_blog() test is failing when it checks the edit_posts capability of a user on a site that the user doesn't belong to. The result is incorrectly true instead of false.

@rajinsharwar commented on PR #6418:


9 months ago
#7

Fixed @johnbillion, used the user_can() instead in the function, the previous method didn't check for the current_blog I bielive.

#8 @rajinsharwar
9 months ago

Hi @johnbillion, wondering if we need separate manual testing for this, or if we can mark this for commit. Maybe we can get this committed as early as possible so that it can be tested out in beta.

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


9 months ago

#10 @oglekler
8 months ago

  • Keywords needs-testing needs-testing-info added
  • Milestone changed from 6.6 to 6.7

We have 2 days before Beta 1 and no time to test, so I am moving it into the next milestone. 

@rajinsharwar I wonder if it is worth to provide code example for testing, how actually this new functions can be used because it already covered in unit tests, but still it will be easier to use (but can limit tester's creativity and the same time).

#11 @rajinsharwar
7 months ago

  • Keywords has-testing-info added; needs-testing-info removed

Yeah, maybe some code testing info might be helpful!

So this is a new function, especially useful for multisites. Used to check if a current user can a particular capability in a particular site in the network.

Testing instructions:

  1. Make sure you are in a multisite environment and have more than one subsite. Let's say, you have a editor user with an ID of 2 in your subsite 1. And another subscriber user with an ID of 3, in your subsite 2.
  2. In the functions.php of your active theme on your main site, add this code:
    
    error_log('User Capability for Editor User ID 2, in subsite ID 1: ' . user_can_for_blog( 2, 1, 'delete_pages' ) );
    error_log('User Capability for Subscriber User ID 3, in subsite ID 1: ' . user_can_for_blog( 3, 1, 'delete_pages' ) );
    
    error_log('User Capability for False cap User ID 2, in subsite ID 1: ' . user_can_for_blog( 2, 1, 'foo_bar' ) );
    error_log('Read User Capability for Subscriber User ID 3, in subsite ID 1: ' . user_can_for_blog( 3, 1, 'read' ) );
    
    

Here the:
"User Capability for Editor User ID 2, in subsite ID 1" should be true.
"User Capability for Subscriber User ID 3, in subsite ID 1" should be false.
"User Capability for False cap User ID 2, in subsite ID 1" should be false.
"Read User Capability for Subscriber User ID 3" should be true.

Feel free to add/edit this @oglekler or anyone working on this ticket if you feel. 🙂

#12 @n8finch
5 months ago

Just tested this patch manually, and things look good, as expected 👍.

Testing Instructions and Steps to Test

  1. I applied the patch to a local WordPress multisite installation.
  2. I created 2 subsites and a user for each of of those subsites, one with Editor permissions and one with Author permissions.
  3. Then I added the following code to the active theme's functions.php file and refreshed the page:
<?php

echo 'Super Admin User:</br>';
echo 'manage_sites Capability for Super Admin User ID 1, in subsite ID 1: ' . var_export( user_can_for_blog( 1, 1, 'manage_sites' ), true ) . '<br>';
echo 'delete_posts Capability for Super Admin User ID 1, in subsite ID 2: ' . var_export( user_can_for_blog( 1, 2, 'delete_posts' ), true ) . '<br>';
echo 'edit_posts Capability for Super Admin User ID 1, in subsite ID 3: ' . var_export( user_can_for_blog( 1, 3, 'edit_posts' ), true ) . '<br>';
echo '</br>';
echo 'Editor on subsite 2:</br>';
echo 'manage_sites Capability for Editor User ID 4, in subsite ID 1: ' . var_export( user_can_for_blog( 4, 1, 'manage_sites' ), true ) . '<br>';
echo 'delete_others_pages Capability for Editor User ID 4, in subsite ID 2: ' . var_export( user_can_for_blog( 5, 2, 'delete_others_pages' ), true ) . '<br>';
echo 'edit_posts Capability for Editor User ID 5, in subsite ID 2: ' . var_export( user_can_for_blog( 5, 2, 'edit_posts' ), true ) . '<br>';
echo 'edit_posts Capability for Editor User ID 5, in subsite ID 3: ' . var_export( user_can_for_blog( 5, 3, 'edit_posts' ), true ) . '<br>';
echo '</br>';
echo 'Author on subsite 3:</br>';
echo 'delete_posts Capability for False cap User ID 4, in subsite ID 2: ' . var_export( user_can_for_blog( 4, 2, 'delete_posts' ), true ) . '<br>';
echo 'edit_posts Capability for Subscriber User ID 4, in subsite ID 3: ' . var_export( user_can_for_blog( 4, 3, 'edit_posts' ), true ) . '<br>';
wp_die();

✅ Results are as expected!:

Super Admin User:
manage_sites Capability for Super Admin User ID 1, in subsite ID 1: true
delete_posts Capability for Super Admin User ID 1, in subsite ID 2: true
edit_posts Capability for Super Admin User ID 1, in subsite ID 3: true

Editor on subsite 2:
manage_sites Capability for Editor User ID 4, in subsite ID 1: false
delete_others_pages Capability for Editor User ID 4, in subsite ID 2: true
edit_posts Capability for Editor User ID 5, in subsite ID 2: true
edit_posts Capability for Editor User ID 5, in subsite ID 3: false

Author on subsite 3:
delete_posts Capability for False cap User ID 4, in subsite ID 2: false
edit_posts Capability for Subscriber User ID 4, in subsite ID 3: true

Note: I have not yet run the associated Unit Tests, but plan to and will report back.

Last edited 5 months ago by n8finch (previous) (diff)

This ticket was mentioned in Slack in #core-test by n8finch. View the logs.


5 months ago

#14 @johnbillion
4 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 59123:

Role/Capability: Introduce the user_can_for_blog() function.

This complements the existing user capability checking functions and enables checking a capability of any user on any site on a Multisite network.

Props tmanoilov, rajinsharwar, n8finch, johnbillion

Fixes #45197

#16 @swissspidy
4 months ago

@johnbillion I thought for new functions we try to use site instead of blog, i.e. user_can_for_site. Perhaps @flixos90 or @spacedmonkey can confirm.

#17 @spacedmonkey
4 months ago

You are correct @swissspidy. This function should be called user_can_for_blog and the parameter should site_id and note blog_id.

This needs to be changed or reverted before beta. CC @johnbillion

#18 @johnbillion
4 months ago

  • Keywords has-patch has-unit-tests needs-testing has-testing-info removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Do we have that documented anywhere? While I agree that using site instead of blog is preferable going forward (which I did for the docblock), in this case I believe consistency with the existing current_user_can_for_blog() function is more valuable. Otherwise we end up with current_user_can_for_blog() and user_can_for_site().

#19 @spacedmonkey
4 months ago

Do we have that documented anywhere?

Not that I have to hand. @flixos90 @jeremyfelt do you remember where this was documented?

#20 @flixos90
4 months ago

I don't think it's documented anywhere, but that doesn't mean it's not true. That said, I see the concern outlined by @johnbillion in terms of consistency. I agree that we should avoid having current_user_can_for_blog() and user_can_for_site() coexist as the two recommended functions.

I don't like introducing a new function using the old naming convention, but the consistency between two functions as closely related as these two would be is also important.

I see two alternative paths here:

  • Either we settle for introducing a new function under the old name and go with user_can_for_blog().
  • Or we clean this up by deprecating current_user_can_for_blog() in favor of current_user_can_for_site(), and then introduce user_can_for_site().

The latter means there will be deprecation messages, which is always a bit annoying especially when it's just for renaming. But in the long term it's the better choice.

I think I'm personally slightly leaning towards the latter, but curious what others think.

#21 @johnjamesjacoby
4 months ago

I can clearly Imagine future versions of ourselves wanting to deprecate current_user_can_for_blog() in favor of _site(), and wondering why we didn’t just do that back then (which is now).

I’m +1 to renaming to today’s pattern now because it prevents an extra deprecation later.

#22 @spacedmonkey
4 months ago

Or we clean this up by deprecating current_user_can_for_blog() in favor of current_user_can_for_site(), and then introduce user_can_for_site().

This would be my recommendation as well. Maybe instead of adding a warning message, we could “soft deprecate” it, by not adding the warning message.

#23 @davidbaumwald
4 months ago

  • Milestone changed from 6.7 to 6.8

With 6.7 Beta 1 releasing shortly, this is being moved to 6.8 given recent momentum towards a resolution.

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


4 months ago

#25 @davidbaumwald
4 months ago

  • Milestone changed from 6.8 to 6.7
  • Type changed from enhancement to task (blessed)

@johnjamesjacoby pointed out to me that there's a commit(I missed it). So, pulling this back in to the 6.7 milestone for iteration as a Task (Blessed). Sorry for the noise folks.

#26 @johnbillion
4 months ago

  • Keywords needs-patch added

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


4 months ago
#27

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

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

  • Renames the newly introduced user_can_for_blog() to user_can_for_site()
  • Deprecates current_user_can_for_blog() and introduces current_user_can_for_site()
    • This is a soft deprecation which doesn't trigger a deprecation warning at runtime, my preference would be to wait until 6.8 to add the call to _deprecated_function()
  • Updates docs and tests

#28 @johnbillion
4 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 59198:

Role/Capability: Introduce the current_user_can_for_site() and user_can_for_site() functions.

The current_user_can_for_site() function is a replacement for current_user_can_for_blog() which is now deprecated. user_can_for_site() is a renaming of the user_can_for_blog() function which was introduced in [59123]. The intention of this change is to prevent the introduction of a new function which uses the old "blog" naming structure.

Props swissspidy, spacedmonkey, flixos90, johnjamesjacoby

Fixes #45197

Note: See TracTickets for help on using tickets.