WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 22 months ago

#5540 closed enhancement (wontfix)

User roles overhaul

Reported by: tellyworth Owned by: jacobsantos
Milestone: Priority: normal
Severity: normal Version:
Component: Role/Capability Keywords: needs-patch
Focuses: Cc:

Description

This patch changes the way user roles are stored, moving them out of the usermeta table into a new user_role table. The API is unchanged and should work exactly the same as before, but operations such as fetching a list of users by role are much faster and easier to implement with joins.

It's mostly important to mu and bbpress, but is also relevant to blogs with large user tables. I'm posting it here first for review, an mu patch is forthcoming.

The main changes are:

  • User roles are no longer stored in usermeta wp_capabilities entries. They're moved to the new user_role table (blog_id, user_id, role).
  • Per-user capabilities are still supported, and still stored in usermeta as before.
  • The upgrade process removes any usermeta wp_capabilities that match role names, and insert user_role entries in their place.
  • Old style wp_capabilities roles are still honored if they exist, to permit logging in before the upgrade has completed, but will be removed during the upgrade.

The API and upgrade process are unit tested. The capabilities API works exactly the same as before for all cases I've tested. The main thing that needs review here is the upgrade process: it has the potential to lock administrators out of the blog if it doesn't work, and it's possible it could take a long time on blogs with many users.

Attachments (3)

user_role-r6506.patch (8.1 KB) - added by tellyworth 6 years ago.
user_role-r6506-a.patch (8.2 KB) - added by tellyworth 6 years ago.
Updated patch fixes an issue with get_users_of_blog()
get_users_with_cap.patch (1.8 KB) - added by tellyworth 6 years ago.
sample implementation of get_users_with_cap()

Download all attachments as: .zip

Change History (27)

tellyworth6 years ago

comment:1 tellyworth6 years ago

Unit tests are here: http://svn.automattic.com/wordpress-tests/wp-testcase/test_user_capabilities.php

Most of it covers the API level and should pass with or without the patch applied. It covers uncommon cases such as users with multiple roles and per-user caps. The additional tests specific to the user_role patch are at the bottom, test_upgrade() in particular.

comment:2 matt6 years ago

I think this would be useful as a plugin that mirrored the data rather than replacing the internal storage, which is okay for single-blog use.

comment:3 ryan6 years ago

See also #5255. This fits in with that. Having this as a plugin means we can't clean up our crappy queries. :-)

comment:4 tellyworth6 years ago

This could be implemented as a plugin, though some core changes are probably necessary to support that. An alternative to mirroring data with actions and filters would be to make the entire WP_Role and WP_User classes pluggable or overrideable.

comment:5 ryan6 years ago

Role and capability storage and queries are a known headache that should just be fixed.

This fixes role queries, but querying all users with a given capability is still a pain. The gist of #5255 was not allow individual caps so that we could always associate a given cap with a role or set of roles.

comment:6 follow-up: tellyworth6 years ago

I can see one relatively simple way to handle the query-users-by-cap thing with this patch: Implement a find_roles_with_capability($cap_name) function that would return a list of roles with a given capability. Then query the user_role table where role in(a, b, c).

That method could work independent of this patch if we assume there is a get_users_with_roles($role_names) function. Obviously that function would be trivial to implement with this patch (just a simple query of the user_role table), and slow and ugly without it (iterating over usermeta wp_capabilities entries).

A more complete solution would be to have a user_capabilities and/or role_capabilities table and query a join on that. But I think that's well and truly overkill, with little extra benefit, so I'll mention it only to rule it out.

comment:7 in reply to: ↑ 6 ; follow-up: ryan6 years ago

Replying to tellyworth:

I can see one relatively simple way to handle the query-users-by-cap thing with this patch: Implement a find_roles_with_capability($cap_name) function that would return a list of roles with a given capability. Then query the user_role table where role in(a, b, c).

Yes, but retaining the ability to put caps in user_meta means not all of a user's caps will be in a role, correct? But maybe that's not worth worrying about. Such caps are not supported out of the box.

Moving the roles to a separate table, implementing the functions you mention, and not going out of our way for caps directly associated with a user (without an intervening role), seems like a good-enough solution that removes the biggest problems with the current arrangement.

comment:8 in reply to: ↑ 7 darkdragon6 years ago

Replying to ryan:

Moving the roles to a separate table, implementing the functions you mention, and not going out of our way for caps directly associated with a user (without an intervening role), seems like a good-enough solution that removes the biggest problems with the current arrangement.

Why move the roles to a separate table when you can use the taxonomy API? I have an idea on how this could be implemented and the problem is as such that I'm willing to put forth an effort to prove my assertion.

comment:9 darkdragon6 years ago

The only problem with using the taxonomy API instead of a separate table, but you lose it anyway with tellyworth's proposal, is that capabilities aren't associated with capabilities by default and each user would have to have the capabilities added differently.

However, I believe that this could be solved by only creating a capabilities table, or having the term relationships for roles and assigning those capability relationship in with the roles.

comment:10 darkdragon6 years ago

So the problem would be that you would have a set of roles for each blog and those will be assigned to each user, which in turn would have a relationship to a single capability.

So the roles will increase by the amount of blogs, while the capabilities should stay the same.

I'm unsure and I forget if it is possible to have terms with the same name and slug, however, you could simply append the blog_id to the slug of the role. The group_id would be used for the roles belong to what blog. Group ID = Blog ID.

Since the capabilities don't really matter based on the blog, or so it can also. You can assign the group ID also by Blog ID. The Worse case shouldn't happen by in the rate of cases.

Oh yeah, by all means, looking at the post, taxonomy, and category APIs, it appears that it would make the role code very complex, but I think it would be well worth the effort.

tellyworth6 years ago

Updated patch fixes an issue with get_users_of_blog()

comment:11 tellyworth6 years ago

get_users_of_blog() didn't work correctly in the original patch. The -a patch fixes it.

tellyworth6 years ago

sample implementation of get_users_with_cap()

comment:12 follow-up: ryan6 years ago

We'll also need a CUSTOM_USER_ROLE_TABLE define for use by MU and by those doing the WP multi-blog hack so that the table can be made global.

darkdragon, this need to be able to make the table global across multiple blogs makes using taxonomy impractical, I think.

comment:13 tellyworth6 years ago

A simple implementation of get_users_with_cap() is attached. It doesn't cope with per-user capabilities, but there's a trivial way to implement that: drop per-user caps in favour of multiple roles.

i.e. if I want a particular Contributor user to have an addition upload_files capability, I create a new Uploader role with the required cap, then assign both roles (Contributor, Uploader) to that user. The code here can already handle this.

comment:14 ryan6 years ago

Sounds good to me.

comment:15 in reply to: ↑ 12 ; follow-up: filosofo6 years ago

Replying to ryan:

We'll also need a CUSTOM_USER_ROLE_TABLE define for use by MU and by those doing the WP multi-blog hack so that the table can be made global.

darkdragon, this need to be able to make the table global across multiple blogs makes using taxonomy impractical, I think.

Couldn't this be done with the general meta-data table? #5183

comment:16 in reply to: ↑ 15 darkdragon6 years ago

Replying to filosofo:

Replying to ryan:

We'll also need a CUSTOM_USER_ROLE_TABLE define for use by MU and by those doing the WP multi-blog hack so that the table can be made global.

darkdragon, this need to be able to make the table global across multiple blogs makes using taxonomy impractical, I think.

Couldn't this be done with the general meta-data table? #5183

It appears that whatever solution is approached would have to be one that is quite simple and would need to have the ability to link capabilities to both roles and users, as well as roles to users.

I think the general meta-data table should be in WordPress, but these people seem to have the goal in sight and after some contemplate, I agree.

comment:17 tellyworth6 years ago

I took a quick stab at a different approach to handling role-capabilitites in #5541. Roles aren't stored in the database at all, just in memory, with an action that can be used to define new roles and a filter for changing the capabilities of existing ones.

It works with or without the user_role patch, and passes all the relevant unit tests.

It would work well with the multiple-roles approach to deprecating user-caps that I outlined above.

comment:18 ryan6 years ago

Since there can be millions of users in a WPMU setup, I prefer to have dedicated user tables that have indices that are as fast a possible. Lumping user info in with general site meta would be less optimal.

comment:19 santosj6 years ago

  • Owner changed from anonymous to jacobsantos

comment:20 ryan5 years ago

  • Component changed from General to Role/Capability

comment:22 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 2.9 to Future Release

comment:23 Denis-de-Bernardy5 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

see #10201

comment:24 nacin22 months ago

In case anyone ever digs up this ticket and says "hey, look at that," here's the original tests from tellyworth: http://unit-tests.trac.wordpress.org/changeset/795

Note: See TracTickets for help on using tickets.