WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 17 months ago

Last modified 16 months ago

#22361 closed defect (bug) (fixed)

Users with multiple roles show incorrect primary role in list-table and when editing

Reported by: johnjamesjacoby Owned by:
Milestone: 3.5 Priority: normal
Severity: major Version:
Component: Role/Capability Keywords: has-patch
Focuses: Cc:

Description (last modified by johnjamesjacoby)

Problem

If a user has multiple roles for a site (coming in bbPress 2.2) there are two places where their site role is not listed/calculated correctly:

  • user.php (via class-wp-users-list-table.php)
  • user-edit.php

Details

A few places in WordPress core assume a user can only have 1 role at a time. Because there currently is no wp_get_user_role() function, the logic to calculate a user's primary role varies in the above locations. There may be more than just this, but these are the two immediate problems.


Duplicate

To duplicate this bug:

  • Checkout the latest version of bbPress trunk.
  • On a single-site install, log in as admin.
  • Visit: Users
  • Edit a user other than yourself
  • Set: "Role" to "-- No role for this site --"
  • Set: "Forums Role" (at bottom of page) to "Participant"
  • Save the user
  • Notice that user "Role" now incorrectly shows "Administrator" (yikes)
  • Revisit: Users
  • Notice that user now shows: "Participant" in both "Site Role" and "Forums Role"

Solution

The gateway to separating out WordPress core roles from any additional roles right now is the get_editable_roles() function. Plugins that attempt to implement their own secondary roles must filter their roles out of this array to prevent overwriting the primary site role with a secondary role. Thus, intersecting a user's roles against the keys of get_editable_roles() ensures an accurate match.


Patch

The attached patch fixes the two files mentioned above, using the above solution. I consider this a critical flaw in the way roles are currently implemented, as it completely prevents plugins from extending roles in a way that doesn't potentially break other things.

Roles and capabilities deserve their own dedicated attention in a future release, but until then this is a major blocker for bbPress 2.2 and future versions of BuddyPress as well.

Attachments (2)

22361.patch (1.4 KB) - added by johnjamesjacoby 18 months ago.
22361.2.patch (2.2 KB) - added by johnjamesjacoby 18 months ago.
Revised patch includes all of the first version, plus prevents a user's capabilities from being completely deleted in multisite setups when setting 'role' to none

Download all attachments as: .zip

Change History (13)

johnjamesjacoby18 months ago

comment:1 johnjamesjacoby18 months ago

  • Description modified (diff)

comment:2 johnjamesjacoby18 months ago

  • Description modified (diff)

comment:3 johnjamesjacoby18 months ago

  • Description modified (diff)

johnjamesjacoby18 months ago

Revised patch includes all of the first version, plus prevents a user's capabilities from being completely deleted in multisite setups when setting 'role' to none

comment:4 scribu18 months ago

  • Keywords needs-unit-tests added

Do we have any unit tests for these things?

Related: #17924

comment:5 deltafactory18 months ago

  • Cc jeff@… added

comment:6 follow-up: nacin18 months ago

Some of this looks like enhancements, others bug fixes. Not sure what needs to be done, versus what wants to be done. It is also modifying some rather confusing code, so I'm really not sure what is going on.

comment:7 in reply to: ↑ 6 johnjamesjacoby18 months ago

Replying to nacin:

Some of this looks like enhancements, others bug fixes. Not sure what needs to be done, versus what wants to be done. It is also modifying some rather confusing code, so I'm really not sure what is going on.

I'm unsure what to add or remove to make it more clear. This isn't a small bug, so it's not something easily glanced and grokked.

comment:8 nacin17 months ago

In 22686:

Less insane multiple role handling in the users list table.

If the user has more than one role, opt to show the first role that is
'editable', if present. Otherwise, fall back to the remaining roles.

In the future, we should show a comma-separated list of all roles,
editable or otherwise, and this list should be filterable, either by user,
or by the roles which can appear. Probably both.

In multisite, only hide 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.

see #22361. fixes #17860.

comment:9 nacin17 months ago

In 22687:

As wp_dropdown_roles() only prints editable roles, ensure that the
"selected" role passed into it on the user-edit screen is editable.

props johnjamesjacoby. see #22361.

comment:10 nacin17 months ago

  • Keywords needs-unit-tests removed
  • Resolution set to fixed
  • Severity changed from critical to major
  • Status changed from new to closed

I'm not sure about the final bit — multisite role deletion. It makes sense in theory. But the raw query a few lines up prevents any deletion if any other roles or capabilities exist for that user.

Of course, a plugin like bbPress could be hooking into edit_user() and adding additional roles at the same time. That could then cause them all to disappear accidentally. But isn't that the point? The user doesn't have a role and we're trying to be careful to not accidentally grant anything. bbPress may be doing it deliberately, but that might not be the case for another plugin. I don't know how best to proceed, which leads me to strongly believe the remaining issue must be punted. I'm happy to revisit multiple roles in 3.6. See also my "In the future" comment in [22686].

It seems like the real fix here is to patch edit_user(), wp_update_user(), etc., rather than having this pretty terrible multisite hack. A plugin like bbPress that deliberately wants to assign (or keep) roles should be able to work around this bug for now, as they (may) have been.

So — closing this, and a new ticket please for the last bit.

comment:11 scribu16 months ago

Follow-up: #22959

Note: See TracTickets for help on using tickets.