WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#18934 accepted defect (bug)

Multisite issue cleaning up empty capabilities

Reported by: jammitch Owned by: PeteMall
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Role/Capability Keywords: has-patch needs-testing
Focuses: multisite Cc:

Description

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.

Attachments (2)

18934.diff (1.7 KB) - added by PeteMall 2 years ago.
18934.2.diff (1.6 KB) - added by PeteMall 2 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 nacin2 years ago

  • Milestone changed from Awaiting Review to 3.3

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

comment:2 ocean902 years ago

  • Keywords needs-patch added

comment:3 follow-up: wpmuguru2 years ago

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.

comment:4 in reply to: ↑ 3 wpmuguru2 years ago

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/

comment:5 follow-up: nacin2 years ago

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.

PeteMall2 years ago

comment:6 in reply to: ↑ 5 wpmuguru2 years ago

18934.diff looks fine.

comment:7 brianlayman2 years ago

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

Last edited 2 years ago by brianlayman (previous) (diff)

comment:8 brianlayman2 years ago

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

comment:9 brianlayman2 years ago

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

PeteMall2 years ago

comment:10 PeteMall2 years ago

  • Keywords dev-feedback added
  • Owner set to PeteMall
  • Status changed from new to accepted

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.

comment:11 ryan2 years ago

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

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

comment:12 brianlayman2 years ago

+1 on the fix and punt.

comment:13 PeteMall2 years ago

  • Keywords has-patch added; needs-patch dev-feedback removed

comment:14 ryan2 years ago

  • Milestone changed from Future Release to 3.4

comment:15 ryan2 years ago

  • Milestone changed from 3.4 to Future Release

comment:16 wonderboymusic19 months ago

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

comment:17 wpmuguru18 months ago

  • Version changed from 3.2.1 to 3.0

comment:18 nacin18 months ago

  • Milestone changed from 3.5 to Future Release

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.

comment:19 jeremyfelt8 months ago

  • Milestone changed from Future Release to 3.7

comment:20 nacin8 months ago

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

comment:22 nacin7 months ago

  • Milestone changed from 3.7 to 3.8

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

comment:23 jeremyfelt5 months ago

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.

comment:24 jeremyfelt5 months ago

  • Milestone changed from 3.8 to Future Release

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

comment:25 jeremyfelt3 months ago

  • Component changed from Multisite to Role/Capability
  • Focuses multisite added
  • Keywords needs-testing added
Note: See TracTickets for help on using tickets.