WordPress.org

Make WordPress Core

Opened 15 months ago

Last modified 2 weeks ago

#36961 reviewing defect (bug)

wp_roles displays incorrect roles in multisite

Reported by: ryanduff Owned by: flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.2
Component: Role/Capability Keywords: has-patch has-unit-tests needs-dev-note
Focuses: multisite Cc:

Description

The WP_User class accepts a 3rd parameter for a site ID, but this never translates over to roles.

In class-wp-user.php WP_User->_init_caps() the correct capabilities are retrieved from the database, but when WP_User->get_role_caps() gets called, the first thing it does is fire wp_roles() in capabilities.php This will global $wp_roles or if not set, it will initialize a new WP_Roles class.

Where this becomes a problem is if you're not on the site you're checking the roles on, wp_roles() returns the roles of the current site instead. Back in WP_User->get_role_caps(), the arrays get filtered and since there's a mismatch of roles defined on current site vs roles actually assigned to user, you will get an empty array as a result when looking at WP_User->roles

To see this in action, I set up a test multisite install to confirm. I deleted all roles but admin on the main site, then created 2 new arbitrary roles. I then created a second site in the install, deleted all roles but admin, and created another arbitrary role. I added a new user to the install. If I go to network admin and attempt to assign the user to site 2, the roles drop down displays the roles defined on site 1 rather than site 2. If I go to the dashboard of site 2 and go to add the user, I see the correct roles available.

I'm not sure if this is intended behavior, though I can't imagine it is as it leads to a bug in network admin allowing a user to be set to a role that potentially doesn't even exist on the site you assign them to.

I've tested this from WP 4.2-4.5 and get the same results in wp-admin

Logging as roles/capabilities component, because even though it affects multisite, I think the core issue exists in roles and capabilities.

Attachments (9)

Screen Shot 2016-05-27 at 10.56.57 AM.png (46.9 KB) - added by ryanduff 15 months ago.
List of roles confirmed via wp-cli
Screen Shot 2016-05-27 at 11.36.04 AM.png (82.3 KB) - added by ryanduff 15 months ago.
Adding user in network admin, displaying site 1 roles, not site 2
Screen Shot 2016-05-27 at 11.37.59 AM.png (80.2 KB) - added by ryanduff 15 months ago.
Adding user from site 2 dashboard, displays correct roles available
36961.diff (1.0 KB) - added by flixos90 11 months ago.
36961.2.diff (4.1 KB) - added by flixos90 8 months ago.
36961.3.diff (4.6 KB) - added by flixos90 5 months ago.
36961.4.diff (4.9 KB) - added by flixos90 3 weeks ago.
36961.5.diff (5.0 KB) - added by flixos90 3 weeks ago.
36961.6.diff (8.5 KB) - added by flixos90 3 weeks ago.

Download all attachments as: .zip

Change History (28)

@ryanduff
15 months ago

List of roles confirmed via wp-cli

@ryanduff
15 months ago

Adding user in network admin, displaying site 1 roles, not site 2

@ryanduff
15 months ago

Adding user from site 2 dashboard, displays correct roles available

#1 @ryanduff
15 months ago

  • Keywords dev-feedback added

#2 @ryanduff
15 months ago

I'll also add that while I later discovered this resulted a bug in network admin, I first noticed it when trying to get and display a user's role on another site in a multisite install via the following method:

$user = new WP_User( $user_id, '', $site_id );
$role = reset( $user->roles );

If that runs on the $site_id I'm checking the correct role is displayed. However, if I'm on site 2, and pass in 3 as $site_id, I get an empty array back.

This ticket was mentioned in Slack in #core-multisite by soderlind. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


11 months ago

#5 @flixos90
11 months ago

The problem here is that the WP_Roles class does not support passing a site ID, so it will always retrieve the roles registered on the current site. A possible solution for it could be to add a switch_to_blog() (and restore_current_blog()) before the wp_roles() call in WP_User.

This switch must only happen if we're on a multisite setup and if the $cap_key property is different from the one of the current site (and then we'd need to extract the site ID out of it, or alternatively find a suitable way to directly pass the site ID to the get_role_caps() method).

@flixos90
11 months ago

#6 @flixos90
11 months ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

36961.diff should fix the problem by switching the site if necessary and then retrieving the roles while in switched state.

The patch can probably be improved, especially my initial logic to extract the site ID out of the $cap_key property is a bit sloppy. Not sure if we should actually store the site ID as property in WP_User instead. If we did that, we could possibly run into sync issues between the site ID and the cap key (which should reference to the same site ID).

Unit tests need to be added as well.

@flixos90
8 months ago

#7 @flixos90
8 months ago

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

36961.2.diff is a more sophisticated approach to the problem. A new private property $blog_id containing the site ID the user is initialized for is introduced in the WP_User class. The property is set in the for_blog() method, and then the cap key is created based on it. The method get_role_caps() then does not need to do anything else than conditionally switching the site.
A caveat of this approach is some ugly code in the _init_caps() method (line 463-475). This clause would not be necessary though if we made removed the magic call method to have it available as a public method (which is in place for backward-compatibility). But even if we don't I think this approach is a better solution going forward.

The patch also adds a unit test for the issue. Another existing unit test failed upon making the changes, however this was solely caused by doing an object-to-array conversion via typecast, which exposes private properties. Using a proper get_object_vars() call instead ensures only public properties are included.

A further idea: We could remove the $cap_key property and only have the $blog_id property to rely on when building the cap key. This would make having the two properties out of sync impossible. For BC, we could have the $cap_key property available through magic methods, that would create the value from the $blog_id property on read-access and translate the input to the $blog_id property on write-access.

#8 @Ipstenu
7 months ago

BuddyPress calls the function of it's own devising in BackPress (so nothing to worry about there).

https://wordpress.org/plugins/event-espresso-decaf uses it as a filter. For example they filter FHEE__EE_Capabilities__init_caps_map__caps

Almost everyone else has copies of class WP_Roles for some reason (and by 'everyone' I mean 4 plugins).

The ONE plugin that calls this?

https://wordpress.org/plugins/unfiltered-mu/

https://plugins.trac.wordpress.org/browser/unfiltered-mu/trunk/unfiltered-mu.php#L84

(@donncha speaking of, is that still a plugin that you feel should be out there?)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 months ago

#10 @flixos90
6 months ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 4.8
  • Owner set to flixos90
  • Status changed from new to assigned

This needs a refreshed patch, since we can safely make _init_caps() private.

@flixos90
5 months ago

#11 @flixos90
5 months ago

  • Keywords needs-refresh removed

36961.3.diff removes the WP_User::__call() implementation that was previously used to make WP_User::_init_caps() publicly accessible. Furthermore the latter has been deprecated since it does not make sense to exist, it's better structured to have its code in the WP_User::for_blog() method. This should not have any effect as one should not extend the WP_User class.

If there are concerns regarding the removal of the WP_User::__call() implementation, let's discuss during the bug-scrub tomorrow.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


4 months ago

#13 @flixos90
4 months ago

  • Keywords early added
  • Milestone changed from 4.8 to Future Release

Let's come back to this early in the 4.9 cycle.

#14 @flixos90
6 weeks ago

  • Keywords early removed
  • Milestone changed from Future Release to 4.9

Putting it into the milestone per the early keyword, also it's related to #36196.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 weeks ago

@flixos90
3 weeks ago

@flixos90
3 weeks ago

#16 @flixos90
3 weeks ago

  • Keywords dev-feedback removed

36961.4.diff only comes with a few minor changes, like updated version numbers and improved docs.

In addition, 36961.5.diff has one change that is based on the proposed improvements from #38645: With WP_Roles getting a for_blog() method, it would become unnecessary to call switch_to_blog() - the more granular WP_Roles::for_blog() would do the magic. Note that this patch will not work correctly at the moment unless you also apply the latest patch from #38645.

Because of this dependency, let's wait with the merge until #38645 is in, although this one here is pretty much ready.

#17 @flixos90
3 weeks ago

  • Keywords needs-dev-note added
  • Status changed from assigned to reviewing

Actually, while we're doing these changes, let's do it right.

36961.6.diff (based on 36961.4.diff) introduces WP_User::for_site() and deprecates WP_User::for_blog(). Since the method has been changed anyway, let's strive for slowly making our way towards the current naming conventions in multisite. This will furthermore allow #38645 to use the current naming conventions and still have the necessary parity.

This looks ready, so it should get in. #38645 can then make the adjustment that I previously uploaded in 36961.5.diff. Both tickets together will require a dev-note to inform about the improvements and deprecated methods.

@flixos90
3 weeks ago

#18 @flixos90
3 weeks ago

@ipstenu Can I get another plugin directory slurp request out to you? :)

I'm curious how many plugins call WP_User::for_blog() and thus would be affected by the deprecation. I guess searching for ->for_blog( will let us know it (although it may very well be that other people have created a method of this name).

Thank you!

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 weeks ago

Note: See TracTickets for help on using tickets.