Non-admin user cannot change role of another user

[PaulRaschke]

I have a project running on Kirby 3.4.4 and I'm still facing this issue. The bug seems to emerge from UserPermissions->canChangeRole(), where the number of available user roles is checked with User->roles(), which always returns only the role of the specified user, unless the logged in user is an admin.

Originally posted by @PaulRaschke in https://github.com/getkirby/kirby/issues/1919#issuecomment-721215752

[afbora]

Yes, I wonder if changeRole permission covers all roles when we give permission (although it doesn't work)?

permissions:
    users:
      changeRole: true

---

I think it would be more correct to be like this.

permissions:
    users:
      changeRole: 
        - editor
        - customer

[PaulRaschke]

I would appreciate it if both options were available. true to allow changing to all roles except admin if the user isn't an admin and define an array to allow changing to certain roles only.

[crisgraphics]

I'd also love to see a fix on this. 🙌🏼 v3.5.2

[michnhokn]

Me too ✋ v3.5.3

[simon-hayden]

Me three 🙏

[lukasbestle]

I think it's difficult to get this right without making it possible to circumvent permissions (e.g. first change the role to role B and then from there change it to an even more powerful role C).

What are your use-cases for this feature?

[michnhokn]

Sure, I understand that it can be a security risk. I had the case that an editor, who must not be an administrator, must be able to create users while also assigning his own role, but not a more privileged one. Quite an edge case and theoretically you could actually treat such editors as administrators. Just wondered why it is not possible.

[simon-hayden]

Use case for me is to have a role that has the privileges to only edit users, but not necessarily edit other things in the panel: site, pages and settings. A "User Manager" effectively.

Why does the users.changeRole permission even exist if not for this purpose?

Devs can prevent unwanted privilege escalation with a hook if not baked into core.

[simon-hayden]

@lukasbestle In the mean time is there a way I can get to the $user object of the currently logged in admin user from a page.update:before and site.update:before hook, so I can prevent particular 'admin' users from doing updated to pages, sites, and settings?

e.g.

'page.update:before' => function ($page, $values, $strings) {
      if ($user === 'aj2md12') { 
        throw new Exception('You are not allowed to edit pages.');
      }
    },

page.update:before only passes arguments for $page, $values, $strings?

[lukasbestle]

@simon-hayden Yes, with $this->user().

[lukasbestle]

Considering your replies (thanks!) I think the behavior should be the following:

• changeRole: true should be interpreted as "every role that is not admin"
• changeRole: editor or a list should be allowed as defined.
• The default for non-admins should be false, meaning the user cannot change any roles unless the dev has explicitly allowed that.

This behavior should apply both to the users and the user namespaces. I think it should also be possible to define a list of roles for the users.create permission as well. The default for that should probably be the user's current role.

What do you think?

[simon-hayden]

@lukasbestle Any ideas when this might be going into development and production for future release?

[lukasbestle]

Sorry, I missed your emoji reactions (unfortunately GitHub doesn't send notifications for those).

I think these enhancements would be really useful. @afbora Would this be something you are interested in working on?

[afbora]

@lukasbestle Please let me try. I'll let you know if I can't get out of it 😄

[afbora]

@lukasbestle All permissions are boolean and this development is changing the system in permissions, right?

[lukasbestle]

@afbora Do you mean that so far the configuration structure only supports setting each permission to a boolean value? That's a good point, we need to check if supporting such a custom syntax for these permissions breaks the general behavior of the Permissions class.

The advantage of the proposed syntax is that it's very natural and fits well into the configuration structure. But a downside is that these permissions (user.changeRole, users.changeRole and users.create) would suddenly support additional config while all the others are just booleans. Could be confusing.

What do you think @distantnative @bastianallgeier? Summary of what's planned is here: https://github.com/getkirby/kirby/issues/2919#issuecomment-834475018

[afbora]

Do you mean that so far the configuration structure only supports setting each permission to a boolean value?

Exactly! The whole permission system is based on boolean. It was a little difficult for me to change.

[distantnative]

Wouldn't this be applicable to a few other permissions, e.g. pages.create, pages.changeTemplate etc.?

I am not too sure this new syntax alongside the boolean syntax would be very well to read and understand (what those role names refer to - could be who is allowed, could be what values are available).

[lukasbestle]

It should always be what values are available. The permissions can already be set differently per role via user blueprints.

[distantnative]

Exactly, but will that be clear to developers from reading this syntax? That's where I'm not so sure.

[lukasbestle]

Yes, good point. Do you see an alternative that's more obvious and easier to understand?

[distantnative]

Not one easy one. Given the various requests (https://kirby.nolt.io/129, https://kirby.nolt.io/12 etc.), I am wondering whether this one change could back us into a dead end for the others. And instead we need to think over the bigger picture of a syntax that could cover all of these... but yes, a hard one.

[lukasbestle]

Yeah, it all gets incredibly complex.

A long time ago (Kirby 2 times) I had the idea to have programmatic blueprints. Basically a PHP file instead of YAML that would then be able to call different methods. At the end the blueprint would return an array with the "basic" blueprint definition. All the complex permissions and rules would already be set up outside of the blueprint array. This would make such complex configuration simpler as we wouldn't need to expose all features to YAML syntax. Downside: It would make the blueprint dynamic, so we would need to ensure that the Panel can deal with a changing blueprint definition.

[teichsta]

would also be interested in a solution, too :-)

Although i appreciate your efforts in enhancing the whole the whole user/role system i would be great if we could firstly "fix" the original issue which prevents changing the role for roles other than admins once the (boolean) permission has been granted.

From this starting point we could than think about smarter solutions what do you think?

[bastianallgeier]

There's finally a draft for a fix: https://github.com/getkirby/kirby/pull/3850 I'm really sorry that it took so long.

[teichsta]

@bastianallgeier but the referenced PR is closed now without being merged so it seems. Would be great if you could probably give us a short update on this issue. Thanks, Thomas

[teichsta]

any news on this issue @bastianallgeier @distantnative @lukasbestle ? As the corresponding PR has been closed i am wondering about the status of this issue. Thanks in advance, Thomas E.-E.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. This is for us to prioritize issues that are still relevant to our community. It will be closed if no further activity occurs within the next 14 days. If this issue is still relevant to you, please leave a comment.