Multisite issue cleaning up empty capabilities

[jammitch]

The following multisite-specific block of code in user-edit.php doesn't seem right:

		$cap = $wpdb->get_var( "SELECT meta_value FROM {$wpdb->usermeta} WHERE user_id = '{$user_id}' AND meta_key = '{$blog_prefix}capabilities' AND meta_value = 'a:0:{}'" );
		if ( !is_network_admin() && null == $cap && $_POST[ 'role' ] == '' ) {
			$_POST[ 'role' ] = 'contributor';
			$delete_role = true;
		}

The query appears to be checking whether the user in question specifically has no capabilities for the given blog. However, on the next line, it checks whether the result set from that query is null - that is, if the user does not have a no-capability entry. This includes users who don't have any capability entries at all (so there wouldn't be anything to delete) and users who actually have valid capabilities (whom I wouldn't think should be deleted).

It's possible I'm misunderstanding this code, but my expectations here are that the line should read null !== $cap.

I'm not sure if this occurs in standard workflows, but I ran into it when writing a plugin. To work around it, I back-populated $_POST[ 'role' ] with the user's current role.

[nacin]

$wpdb->get_var( "SELECT meta_value FROM {$wpdb->usermeta} WHERE user_id = '{$user_id}' AND meta_key = '{$blog_prefix}capabilities' AND meta_value = 'a:0:{}'" );

Oh, oh, man. What is that? We need to do something here, I think.

[wpmuguru]

There were versions of MU which updated the user capability to an empty array instead of removing the capability. When a user signed up as a user, they were automatically added to the main site/blog.

I think in the above versions the user was added with no role but the security required that they have a capability role so they could access the dashboard to change their password, etc.

Those empty array rows could safely be removed from a core code perspective because the capability row is no longer required. The main concern would be to ensure the update function is still successful in larger installs.

[wpmuguru]

Replying to wpmuguru:

I think in the above versions the user was added with no role but the security required that they have a capability role so they could access the dashboard to change their password, etc.

s/capability role/capability row/

[nacin]

I was more concerned with the direct DB query (it could be get_user_meta() easily) rather than the obscure MU bug, but I appreciate the peek into the historical stuff here.

[wpmuguru]

18934.diff looks fine.

[brianlayman]

Looking at the code this seems to be a very specific band-aid to WPMU. There's a good chance this code could be completely removed because the original cause doesn't exist anymore. I suspect it is there to prevent the adding of the person to the blog, not the correction of something that already happened.

This is how I read the code:
The comment (~Line 120 ./wp-admin/user-edit.php) says:

WPMU must delete the user from the current blog if WP added him after editing.

The other conditions are that the current user editing a different user:

if ( $user_id != $current_user->ID ) {

And the specific SQL check confirms that we have removed all roles from a user who had either had a role, or had never had a capability defined. If we meet that criteria, we delete all capabilities we had for them:

if ( $delete_role ) stops users being added to current blog when they are edited

delete_user_meta( $user_id, $blog_prefix . 'capabilities' );

I think this makes the !is_network_admin check look weird too because that should be for the $user_id we are dealing with and not the current user. <--EDIT: IGNORE THIS...

I bet the right fix is to remove this code all together and confirm that when $_POST[ 'role' ] == , no roles are written out.

Is it safe to do this:
if ($_POST[ 'role' ] == ) delete_user_meta( $user_id, $blog_prefix . 'capabilities' );

[brianlayman]

heh it didn't like my empty quotes after $_POST[ 'role' ] ==
but you get the idea.

[brianlayman]

Never mind what I said about is_network_admin().. That function catches me every time I look at it. I always thing it has to do with the user, not which editor/admin page you are looking at...

[PeteMall]

There is a check for !is_network_admin() which means you are editing a user in the site admin and the user does not belong to that site (empty caps). Only way to get there is by editing the user id in the url. You can test by editing a user, with no caps on a site, using the edit-user page in the site admin. The user will be edited but wouldn't be added to the site.

[ryan]

Punting to 3.4 early since this is not a regression. Eliminating the code seems good.

[brianlayman]

+1 on the fix and punt.

[nacin]

This has been dancing around milestones for more than a year. One more bump doesn't hurt. Let's aim to clean up some of this crufty multisite code in 3.6. I'd rather not make a change now, late in a cycle, without fully understanding potential side effects.

[nacin]

I am fine with 18934.2.diff​. But first, we should confirm where it came from originally, why it was necessary originally, and why it isn't necessary anymore. (A unit test would be nice if that is found to be appropriate.)

[SergeyBiryukov]

Introduced in ​mu:797, ​mu:1590, and ​mu:1594.

[nacin]

After chatting with jeremyfelt, this code is either harmless, or removing it is harmful. Let's avoid ripping it out so close to release.

[jeremyfelt]

Chiming in with conjecture.

This behavior was introduced in ​mu:797 with the commit message "WP Merge - needs testing."

This appears to be continued work on the merge from WordPress core to WPMU done in ​mu:770 as well as other changesets in that time period.

This added behavior appears to imply that $errors = edit_user($user_id); was adding something that WPMU did not want and had to work around. I think it originates with #1825, but I haven't wrapped my head around it yet.

Basically:

1. If this user has no WPMU specified role on this site (indicated by the empty caps array)...
2. Assume site capabilities were added for the user in edit_user() by WordPress core functionality...
3. And remove all capabilities for this user on this site after edit_user() is complete.

As we don't store empty capabilities for user/site relationships, I do think this is a harmless removal. It seems that the only way it could be harmful is if another place in WordPress core is expecting an empty capabilities array.

If it's going to land in 3.8, it should go soon so that we can confirm. I can't imagine a way to unit test this right now.

[jeremyfelt]

Bumping to future release again due to impending 3.8 RC release. Obligatory note that we should look at this earlier in a cycle.

[jeremyfelt]

Let's fire this up again. :)

I'm of the opinion that removing a large chunk of this code is the right move.

A few interesting reads:

• In #2809, the concept of "No role for this blog" was introduced as a way to prevent the accidental assignment of a role when editing a user from user-edit.php on a site for which the user had no existing role.
• The commit message for [22686] mentions hiding "users that have no capabilities (in case they possess a leftover, empty wp_xx_capabilities key from the MU days), not users that have no role, as they may have a cap but no role."
• In @jjj's patch for 22361, he addresses this area of code as "prevents a user's capabilities from being completely deleted in multisite setups when setting 'role' to none". That ended up not being committed as part of the ticket, but is the issue at hand here.

When wp_insert_user() is used with an empty role, an empty array is stored. However, core treats this condition and that in which wp_xx_capabilities has been deleted completely exactly the same. That's ok, but what makes the existing code a bit confusing—the explicit check for a:0:{}.

The real issue:

If a user has a built in role assigned, and a custom role is added, the two roles are stored in the database alongside the existing role. With the current state of user-edit.php, setting that user to "No role for this site" will delete the row entirely, including the custom role. This means the user will no longer be shown in the site's user list table, even though the intent could be for the user to continue with an even more limited custom role.

By removing this section of code

• No change in behavior if editing your own profile.
• No change if editing a user in wp-admin/network/user-edit.php
• No change if editing a user and setting any role..
• If setting the role to "No role on this site", custom roles now remain.

We can also take this opportunity to clean up and combine what's happening in multisite and single site installs. 18934.3.diff does this.

[johnjamesjacoby]

Replying to jeremyfelt:

I'm of the opinion that removing a large chunk of this code is the right move.

Based on your investigation, I agree with your conclusion.

Can we cut this out early, please? As old as this bit is, I'm concerned what it might uncover from the more decrepit areas of wpcom.

[jeremyfelt]

Replying to johnjamesjacoby:

Can we cut this out early, please? As old as this bit is, I'm concerned what it might uncover from the more decrepit areas of wpcom.

Indeed.

I'm going to commit this as fixed now. I've thought it through a bunch the last few days and I think we're good, but if anybody has concerns, please shout.

[jeremyfelt]

In 31516:

Avoid clearing stored capabilities for a user when removing their built in role in multisite.

Previously, if “No role on this site” was assigned to a user AND that user did not have an empty role array stored for the site, the $blog_prefix . ‘capabilities’ meta for that user would be deleted completely after changes to the user were saved. Any custom capabilities stored (i.e. $user->add_role()) would be removed as well.

This removes the code controlling the old WPMU handling of “no role” and allows custom stored capabilities to remain. Users with no role and custom capabilities will now appear in the users list table with “None” as the role.

In the process we’re able to better clarify the multisite specific pieces that do occur.

Props PeteMall, jeremyfelt.

Fixes #18934.