Ability for all users to add users with less capabilities

[doit-cu]

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

[skeltoac]

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.

[doit-cu]

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?

[ryan]

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.

[doit-cu]

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.

[markjaquith]

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.

[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?

[ryan]

More fine grained user caps.

[ryan]

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.

[markjaquith]

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.

[ringmaster]

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?

[ryan]

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

[doit-cu]

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.

[doit-cu]

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.

[markjaquith]

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.

[doit-cu]

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

[ringmaster]

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-cu]

Make user management permissions pluggable.

[doit-cu]

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

[doit-cu]

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.

[ringmaster]

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.

[doit-cu]

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.

[ringmaster]

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

[doit-cu]

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-cu]

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

[doit-cu]

Sample plugin

[doit-cu]

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.

[ryanscheuermann]

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?

[doit-cu]

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.

[ryanscheuermann]

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

[doit-cu]

Fixed the bug reported by ryanscheuermann. Thank you!

Is there anything else needed fromme to get this committed?

[convan23]

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.

[doit-cu]

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

[ryan]

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

[Nazgul]

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?

[lloydbudd]

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