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.

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

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.

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/

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.

18934.diff looks fine.

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' );

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

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

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.

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

+1 on the fix and punt.

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.