wp_roles displays incorrect roles in multisite

[ryanduff]

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.

[ryanduff]

List of roles confirmed via wp-cli

[ryanduff]

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

[ryanduff]

Adding user from site 2 dashboard, displays correct roles available

[ryanduff]

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.

[flixos90]

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]

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]

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.

[Ipstenu]

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?)

[flixos90]

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

[flixos90]

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.

[flixos90]

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

[flixos90]

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

[flixos90]

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.

[flixos90]

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]

@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!

[flixos90]

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]

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]

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.

[jeremyfelt]

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.

[flixos90]

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.

[johnjamesjacoby]

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]

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).

[flixos90]

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.