WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#2775 closed enhancement (fixed)

Ability for all users to add users with less capabilities

Reported by: doit-cu Owned by: doit-cu
Milestone: 2.3 Priority: normal
Severity: normal Version: 2.1
Component: Administration Keywords: has-patch needs-testing developer-feedback
Focuses: Cc:

Description

It would ease some administration of large deployments if users could add other users with roles that had a subset of their capabilities. For example, in cases where subscription is not to be open to the general public, if authors could add subscribers. This should probably be optional, as some deployments might want to more tightly control access. If this is of interest, I can create a patch.

Thank you for your attention,
--Joel

Attachments (7)

2775.diff (5.6 KB) - added by doit-cu 8 years ago.
user_caps.diff (4.5 KB) - added by ryan 8 years ago.
More fine grained user caps.
2775-pluggable.diff (13.0 KB) - added by doit-cu 8 years ago.
Make user management permissions pluggable.
user-caps.php (2.5 KB) - added by doit-cu 8 years ago.
Example pluggin (does what I had described as a feature request).
metacaps.diff (11.5 KB) - added by doit-cu 8 years ago.
Added a filter in the default case of meta caps, made user editing situation-aware
user-caps-meta.php (2.4 KB) - added by doit-cu 8 years ago.
Sample plugin
2775-pluggable-meta-caps2.diff (11.5 KB) - added by doit-cu 8 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 skeltoac8 years ago

I don't want editors adding authors, authors adding contributors, etc. I understand that you might want this, so I encourage you to write or commission a plugin to manupulate the roles and capabilities. Maybe the API needs some work before this can be done, in which case the hooks will almost certainly be cheerfully added.

comment:2 doit-cu8 years ago

After thinking about it, I agree. I think the best way to approach this would be maybe more of a bugfix. Users who have the edit_user capability shouldn't be able to edit users who's capabilities are not a proper subset of the editing user's capabilities. True, you don't have to worry about this in the default installation. Instead of enabling this as a feature, why not fix that piece and then people who manage their roles outside the default are protected from that kind of vulnerability. I don't think I'm alone here- there are plugins for role managment if I remember correctly.

This is looking like it won't be too terrible to code up. Basically it involves adding a capabilities comparison function to the WP_Roles class and a few extra checks in addition to the current if(!current_user_can('edit_users')) deal.

Thoughts?

comment:3 ryan8 years ago

Roles aren't proper subsets of each other though. A hierarchy cannot be assumed, especially in environments where people have tweaked the roles with the Role Manager plugin. Eliminating the confusion of hierarchical user editing was one of primary goals when designing the capability system. If you want this, it should be a plugin. As skeltoac mentioned, we'll happily add new plugin hooks if necessary.

comment:4 doit-cu8 years ago

I guess I was thinking more along the lines that a user should not be able to edit a user who has a capability that the editing user does not have. So, for instance, if role foo has capabilities biz and baz, and role bar has capabilities biz, baz, and bam, foo should not be able to edit bar, but bar should be able to edit foo. The reasoning in my mind is that bar is more trusted in the system, having the ability to "bam." Also, if gaz has capability qux, and zxc has the capability asd, neither should be able to edit the other.

The real point of what I'm trying for in my instance is not really editing though, but the ability to create users with a subset (defined as a role) of capabilities. Now this ability can be added in to the wordpress code base without changing the default functionality- as far as I know, only administrator has the edit_users capability out of the gate. I guess it seems to me that user management should be a bit more granular than an on/off switch.

I agree that there should not be a hierarchy set up between roles, and I can understand that this might not be desireable default behavior. What this would do though is make the application more amenable to Plugins and other "aftermarket" modifications. I believe it will make the application more customizable, rather than simply being a customization in and of itself.

I have the code all written up, I just need to drop in some cleaner UI errors as opposed to simply die(). I'll upload a diff later this weekend and see what you all think... this seems to be a difficult one for me to get my choice of words correct on.

Barring that, I think the cleanest way to provide a plugin with this ability would be to allow redefinition of the current_user_can function. users.php and user-edit.php would need to pass in a WP_User object or an array of said objects by reference (so that you can clean out the denied users while maintaining the allowed users) into current_user_can in order for that to be of any real use. I still see a problem here, because if someone has edit_user and activate_plugins, they could turn off the filtering plugin and be able to edit and users again. The protection should, in my opinion, really be a part of the application.

Finally, thank you both for your time on this.

comment:5 markjaquith8 years ago

I know what you're saying. Basically, "edit_users" is God mode. You can edit yourself and give yourself any other capabilities you want, or upgrade other users to have capabilities that you didn't originally have.

The only way I see around it is to have two capabilities... edit_users and edit_lesser_users. People with edit_lesser_users wouldn't be able to edit their own capabilities, and would only be allowed to give other people capabilities that they have themselves (minus edit_lesser_users, natch), and wouldn't be allowed to edit users who have capabilities that they don't have themselves.

The word "lesser" suggests a hierarchy, but it's a binary hierarchy... they can only edit people whose capabilities are common to their own. It's really more like edit_less_capable_users. It's also role-agnostic, which is very important.

Otherwise, you have to resign yourself to the fact that anyone who can edit users can do anything they want.

comment:6 doit-cu8 years ago

  • Owner changed from anonymous to doit-cu

Yes, exactly. The only thing I hadn't thought of is the new edit_less_capable_users role. Is there a benifit to making a seperate role as opposed to using the existing edit_users role? Administrators usually have all capabilities, but this might not always be the case. Then again, if you give someone edit_users as it stands (not filtered like edit_less_capable_users), they can pretty much gain all capabilities anyways. It would be trivial to add the edit_less_capable_users capability, but I think it might be redundant. Thoughts?

doit-cu8 years ago

comment:7 doit-cu8 years ago

  • Keywords bg|has-patch bg|needs-testing added

ryan8 years ago

More fine grained user caps.

comment:8 ryan8 years ago

Attached patch adds some new caps and meta caps. This should allow plugins to hook onto the delete_user and edit_user caps and add their own logic.

comment:9 markjaquith8 years ago

doit-cu,

Would want it to be two caps, so that someone could grant the edit_less_capable_users cap to other users! Quis Custodiet Ipsos Custodes? :-)

Ryan,
Patch is a good start, but we'll also need to know what is being done to the user on a finer level.. i.e. "changing this user's role to X" or "Adding/Removing Capability Y"

so maybe:

current_user_can('edit_user', $id, 'add_capability', 'manage_options')

OR

current_user_can('edit_user', $id, 'set_role', 'Author')

Then a plugin could hook in and grab all those extra args and make an educated decision on whether or not the user is allowed to do that.

comment:10 ringmaster8 years ago

Perhaps set this as a goal:

Modify meta caps so that a plugin can alter edit_user to re-institute user levels for editing other users' caps.

To that end, I agree with markjaquith's sentiment in the previous comment, as long as no user-level functions are added to the core, and no heirarchical functions for user permissions be added to the core. Anything not like what we already have should be implemented through a plugin.

Here's an raw idea: Can we add a hook to the default case in the meta cap function's switch to handle other cases, rather than adding all of these individually?

comment:11 ryan8 years ago

[3846] is the first round for more fine-grained user management caps.

comment:12 doit-cu8 years ago

This actually seems to work fairly well. Some issues though:

  • UI troubles... the checkbox for delete/promote still shows up even when the user does not have permission. This is compounded by the seperation of delete_user/edit_user user. You would need to reduce back to one capability or change the UI so that there was a different line of check boxes for each task, which in my opinion would be a bit confusing.
  • Users with activate_plugin can still deactivate the plugin and become god. Maybe what's needed here is a seperate permission, external_edit_user or the like. This would always fail unless handled by a plugin.
  • On the UI, should users that cannot be edited be displayed at all? See my diff at @@ -151,9 +181,11 @@ ; @@ -209,8 +241,9 @@ ; and @@ -238,6 +271,7 @@ ; for possible fixes.
Overall, I would propose eliminating delete_user and create_user, changing comparisions from (current_user_can('edit_user', $userid)) to (current_user_can('external_edit_user', $userid)
current_user_can('edit_user', $userid)), and having external_edit_user fail unless intercepted by a plugin. Additionally, there should be comparisions on display as well as on change.

Thank you again for your effort on this.

comment:13 doit-cu8 years ago

Quick update- I have a working model of a /pluggable/ way to do accomplish this with failure on plugin deactivation. I don't think it's very pretty- there's just got to be a more extesible way to do this... You all know more than me on the subject, so I'll post it for review/repair.

comment:14 markjaquith8 years ago

Hm, yeah, you could just prevent certain users from deactivating the plugin. Just have to make sure they can't edit it... i.e. chmod it appropriately. Please do post this.

comment:15 doit-cu8 years ago

Sorry, went to make a final diff after posting this and got a whole lot of conflicts. I'll post when merged.

comment:16 ringmaster8 years ago

Instead of chmod, you might be able to hook user_has_cap based on the values passed to the form and the script being executed. It's a long way around, but it could prevent the need for mode changes. Just a thought.

doit-cu8 years ago

Make user management permissions pluggable.

doit-cu8 years ago

Example pluggin (does what I had described as a feature request).

comment:17 doit-cu8 years ago

There we go (fingers /all/ crossed).

Ringmaster- I think what markjaquith is pointing out is that users with the ability to edit the pluggin would be able to make it do nasty things, so it would be a good idea to make the pluggin read only.

Markjaquith- It doesn't prevent anyone from deactivating it, the core changes make it so that users with external_edit_users can't edit users unless a pluggin makes it so. In essence, a user could go deactivate the pluggin and take away everyone but admin's rights to edit users, but they could not exploit a privilege escalation vulnerability.

comment:18 ringmaster8 years ago

Right. What I'm suggesting is that you could use the user_has_cap capability to restrict a user's ability to edit that single plugin via WordPress. If they attempted to load the plugin editor with that file, use user_has_cap to temporarily deny that cap so that the page won't load.

Why? Because then you don't have to set the mode on the file, because it is inconvenient (since you can't necessarily do it from the admin console) and because many users don't understand the how or why of it. It makes it easier for the user, that's all. Like I said, just a crazy suggestion.

Perhaps what is needed instead is a way to install plugin-like features that are not available for deactivation or editing via the admin, but require server access to modify? This could be used as a more secure way to modify the user privilege system in totality.

comment:19 doit-cu8 years ago

Update:

After discussing with ringmaster, I am going to place a hook in the default case of map_meta_caps. I am also going to change the current_user_can calls, eliminating the external_edit_users calls and sending a stdobj along with the edit_users calls instead. The proof of concept is working, but I'm at a conference for the next three days, so I probably won't get to this until next week.

Thank you all once more for your help and guidance on this.

comment:20 ringmaster8 years ago

The new ideas we discussed sounded good.

Essentially, a new associative array would be formed that contains target information (what user is being edited, what role they're being set to, what action is being performed, etc) and standardized array keys (like "action", "target_user", etc.) and passed with calls to current_user_can() like:

current_user_can('edit_user', array('target_user'=>$target_user_id, ...))

A new hook will be added to the end of get_meta_cap() that will forward all of this data on to registered plugins. The plugins can then implement completely new security models, even ones based on the old level hierarchy.

This will also have the advantage of allowing the filtering of other caps for meta caps, which really enhances the capabilities of... er... capabilities.

Without a plugin, capability functionality will remain unchanged, but it's a great boon for WP as a CMS, and it should only be a few lines of change.

Plugins should be sure to filter for only the meta caps they need, otherwise some circular logic could be introduced (by calling current_user_can() with a meta cap from inside the hook sink for get_meta_cap).

comment:21 doit-cu8 years ago

Huh... patch is done, but I can't seem to upload. I get "Oops... Trac detected and internal error: Failed to create unique name: /path/to/file".

Any ideas?

doit-cu8 years ago

Added a filter in the default case of meta caps, made user editing situation-aware

doit-cu8 years ago

Sample plugin

comment:22 doit-cu8 years ago

I'm not 100% on how the return value of the apply_filter should be handled, nor the name of the filter. A first go, anyways.

comment:23 ryanscheuermann8 years ago

What's the status of this ticket? Is the metacaps.diff going to be committed? +1 to the latest patch, btw, I like the array method much better, less messy.

If I understand correctly, this patch and plugin will prevent an "Editor" with the "edit_users" ability from editing anything about an "Administrator", correct? It will also allow an "Editor" to create a user of an equal or lesser role? If so, this is exactly what I'm looking for. Thank you.

The create_users and delete_users capabilities are going away now, I hope. As it stands in trunk, they are completely redundant/useless caps with/without edit_users.

I smell a new version of Roles Manager. Maybe the introduction of a new "edit_roles" cap?

comment:24 doit-cu8 years ago

You're close- all the patch does is allow map_meta_caps to be plugged and adds situational awareness to edit_user checks. The plugin manages the behavior you're describing.

comment:25 ryanscheuermann8 years ago

Found a small bug in your metacaps.diff:

Lines 26-27:

               if(is_array($plugin_caps)){
                       array_merge($caps, $plugin_caps);

should be:

               if(is_array($plugin_caps)){
                       $caps = array_merge($caps, $plugin_caps);

comment:26 doit-cu8 years ago

Fixed the bug reported by ryanscheuermann. Thank you!

Is there anything else needed fromme to get this committed?

comment:27 convan238 years ago

Ok so wait, how exactley do I make the changes so this works? Do I have to make the changes from each and every patch that is uploaded? And do I have to have wordpress version 2.1? I have 2.0.3. I would really like to get this working.

comment:28 doit-cu8 years ago

You need to apply patch 2775-pluggable-meta-caps2.diff, and then download user-caps-meta.php. The patch was created against the trunk code, so you'll need to have that code (targeted at 2.1) to make it work. This is all, of course, until someone commits the patch...

comment:29 ryan8 years ago

Why have a cap of edit_users that is then modified by an action called promote? Just put the action in the cap and call it promote_user.

current_user_can('promote_user', array('user_id' => 1, 'role' => 'administrator'));

comment:30 Nazgul7 years ago

  • Keywords has-patch needs-testing developer-feedback added; bg|has-patch bg|needs-testing removed
  • Milestone set to 2.3

Part of this ticket went in, but not all.

Could somebody with more knowledge about the role system take a look at it and determine if it can be closed or that additional action is required?

comment:31 ryan7 years ago

  • Milestone changed from 2.3 to 2.4 (next)

comment:32 davidszp6 years ago

  • Summary changed from Ability for all users to add users of lesser cabable roles to Ability for all users to add users with less capabilities

comment:33 lloydbudd6 years ago

  • Milestone changed from 2.4 to 2.3
  • Resolution set to fixed
  • Status changed from new to closed

Closing as fixed as no response to Nazgul's question.

Note: See TracTickets for help on using tickets.