Make WordPress Core

Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#36961 closed defect (bug) (fixed)

wp_roles displays incorrect roles in multisite

Reported by: ryanduff's profile ryanduff Owned by: flixos90's profile flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.2
Component: Role/Capability Keywords: has-patch, has-unit-tests
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 (13)

Screen Shot 2016-05-27 at 10.56.57 AM.png (46.9 KB) - added by ryanduff 8 years 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 8 years 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 8 years ago.
Adding user from site 2 dashboard, displays correct roles available
36961.diff (1.0 KB) - added by flixos90 7 years ago.
36961.2.diff (4.1 KB) - added by flixos90 7 years ago.
36961.3.diff (4.6 KB) - added by flixos90 7 years ago.
36961.4.diff (4.9 KB) - added by flixos90 7 years ago.
36961.5.diff (5.0 KB) - added by flixos90 7 years ago.
36961.6.diff (8.5 KB) - added by flixos90 7 years ago.
36961.7.diff (1.2 KB) - added by jeremyfelt 7 years ago.
36961.8.diff (9.1 KB) - added by flixos90 7 years ago.
36961.9.diff (9.6 KB) - added by flixos90 6 years ago.
36961.10.diff (10.4 KB) - added by flixos90 6 years ago.

Download all attachments as: .zip

Change History (42)

@ryanduff
8 years ago

List of roles confirmed via wp-cli

@ryanduff
8 years ago

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

@ryanduff
8 years ago

Adding user from site 2 dashboard, displays correct roles available

#1 @ryanduff
8 years ago

  • Keywords dev-feedback added

#2 @ryanduff
8 years 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.


8 years ago

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


7 years ago

#5 @flixos90
7 years 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
7 years ago

#6 @flixos90
7 years 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
7 years ago

#7 @flixos90
7 years 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 years 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.


7 years ago

#10 @flixos90
7 years 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
7 years ago

#11 @flixos90
7 years 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.


7 years ago

#13 @flixos90
7 years 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
7 years 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.


7 years ago

@flixos90
7 years ago

@flixos90
7 years ago

#16 @flixos90
7 years 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
7 years 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
7 years ago

#18 @flixos90
7 years 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.


7 years ago

#20 @flixos90
7 years ago

A bit late, but Mika pointed out via Slack that usage of WP_User::for_blog() was very low. And since we're only talking about deprecation here, not removal, we should be safe to proceed.

@jeremyfelt
7 years ago

#21 @jeremyfelt
7 years ago

To sum up the ticket so far, it looks like 2 issues:

  • The role dropdowns on wp-admin/network/site-users.php show only the roles of the main site. See 36961.7.diff for a quick/possible approach on that.
  • When called from site 1, new WP_User( $user_id, '', 2 ) provides a user object that contains only roles assigned to the user that exist on both site 1 and 2. This is primarily because wp_roles() is used to populate available roles without switching site context.

I added 36961.7.diff to handle the first case. I don't think that was addressed in the other patches on this ticket.

36961.6.diff looks good so far. I think my only concern is the removal of the protected _init_caps(). I think we should leave __call() in for at least the near term. I was able to find a handful of unit tests via GitHub that use _init_caps() to work around some stuff. We can show a deprecated notice instead of causing a fatal for 4.9.

At a glance it looks like wp_get_users_with_no_role() and count_users() could also return incorrect data when called for another site, even though site ID is supported as a passed parameter. It's unlikely for these, but we may want new tickets to cover that as well.

@flixos90
7 years ago

#22 follow-up: @flixos90
7 years ago

36961.8.diff brings back the __call() method to be more backward-compatible.

@jeremyfelt

When called from site 1, new WP_User( $user_id, '', 2 ) provides a user object that contains only roles assigned to the user that exist on both site 1 and 2. This is primarily because wp_roles() is used to populate available roles without switching site context.

Not sure I understand. I think the site context is switched, see get_role_caps(). Or do you mean something else?

Regarding 36961.7.diff, wp_get_users_with_no_role() and count_users(), I think all of these should be dealt with through separate tickets that can be merged shortly after this one and #38645. Some of the fixes might also be based on the improved foundation that #38645 will introduce.

#23 in reply to: ↑ 22 @jeremyfelt
7 years ago

Replying to flixos90:

36961.8.diff brings back the __call() method to be more backward-compatible.

@jeremyfelt

When called from site 1, new WP_User( $user_id, '', 2 ) provides a user object that contains only roles assigned to the user that exist on both site 1 and 2. This is primarily because wp_roles() is used to populate available roles without switching site context.

Not sure I understand. I think the site context is switched, see get_role_caps(). Or do you mean something else?

Sorry, I meant in its current state, pre-patch. The patch fixes the issue. :)

Regarding 36961.7.diff, wp_get_users_with_no_role() and count_users(), I think all of these should be dealt with through separate tickets that can be merged shortly after this one and #38645. Some of the fixes might also be based on the improved foundation that #38645 will introduce.

I'm okay with that.

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


7 years ago

@flixos90
6 years ago

#25 @flixos90
6 years ago

36961.9.diff adds an extra test to make sure the new internal WP_User::get_caps_data() method works correctly. Since it's private, it is not called directly, but its return value is stored in WP_User::$caps, so this can be checked.

This is good to go IMO, one final question: Should the new $site_id property be public or private? It's currently private, but I've been thinking it may be useful to be able to detect for which site a user's capabilities are currently initialized. If we wanna be able to do that, we could alternatively add a public get_site_id() method and still keep the property private.

#26 @johnjamesjacoby
6 years ago

Should the new $site_id property be public or private?

I think keep it private and add a public get_site_id() method.

My reasoning is if the property were public, I'd want changing it to call for_site( $this->site_id ) via a magic __set() method, and that magic is outside the scope of this ticket and not present in core anywhere else.

@flixos90
6 years ago

#27 @flixos90
6 years ago

36961.10.diff introduces the WP_User::get_site_id() method to allow retrieving the ID of the site for which the user's capabilities are currently initialized for, as suggested above. It also adds two integration tests for the method. One of the tests added in 36961.9.diff was furthermore made available for non-multisite setups too (it was previously restricted to multisite-only for no reason).

#28 @flixos90
6 years ago

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

In 41624:

Multisite: Initialize a user's roles correctly when setting them up for a different site.

While it has always been possible to initialize a user's roles and capabilities for another site than the current one in a multisite, the actual roles available were not switched prior to this change, possibly causing invalid roles to show up or actually valid capabilities not being available.

In order to fix this bug in a clean way, relevant parts of the WP_User class have been refactored. The ID of the site for which capabilities are currently initialized are now stored in a private property WP_User::$site_id. The WP_User::for_blog( $blog_id ) and WP_User::_init_caps( $cap_key ) methods have been deprecated in favor of WP_User::for_site( $site_id ). In addition, a new method WP_User::get_site_id() has been introduced to retrieve the site ID for which the user's capabilities are currently initialized.

Props ryanduff, jeremyfelt, flixos90.
Fixes #36961.

#29 @jeremyfelt
6 years ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.