Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

Sign In with Google Sign In with OpenID

User: privilege escalation

edited August 2014 in Apps

It seems that a user who can edit users may assign to themselves the type "Admin" even if it is not.

E.g. "Modify user accounts" set to "Yes" and "Modify user roles and permissions" set to "No"

How can I avoid that?

Thanks.

Comments

  • One solution would be to put restrictions on the "User role" field so users who can modify users but not roles wouldn't be able to edit the role a user belongs to. The problem there is that you still need to choose a role to assign to new users, and that could vary. So if someone can add a user, they need to be able to select the role it belongs to, in which case instead of escalating they could also just create a new user with increased permissions.

    Another solution would involve restricting which roles they can assign users to, so that they could add members and maybe editors, but not full admins. That would require new logic around which roles who can modify users can assign which roles to users (there's a tongue-twister!).

    Hmm... I'm inclined to argue that if you can already modify a user account, there's a certain level of trust assumed. I usually restrict modifying roles more so because it's more technical and I don't want people breaking things, in the same way that I restrict modifying the site settings.

    Another solution would be an approval process, where a user with lesser permissions requests a role change and a full admin has to approve it. But then we get into questions of which admin should receive notifications for which lesser admins, etc.

    I'm not sure the ideal solution just yet, but those are some of my thoughts on it. What do you think?

  • edited August 2014

    (I might be misunderstanding the goal, but here's a possible solution...)

    You could probably add a small wrapper logic to prevent non admin (or maybe it should be the 'default' acl) users from changing their own role for their account. Ex...

    user/views/edit.html ~14

    {% if User::$user->id != $data->id || User::is('admin') %}
    < select name="type">
    {% foreach types %}
        < option value="{{ loop_value }}"{% if type === $data->loop_value %} selected {% end %}>{{ loop_value|ucfirst }}
    {% end %}
    
    {% else %}
    {{ type|ucfirst }}
    {% end %}
    
    

    And for a little extra security...

    user/handlers/edit.php ~18

    if (User::$user->id != $u->id || User::is('admin') {
        $u->type = $_POST['type'];
    }
    
  • edited August 2014

    Thank you all.

    Because there is no hierarchy between user types (and this is good for me as it allows cross prerogative) I think that it is not possible to say "allow a user to assign the maximum level equal to his own"; and this seems not even necessary.

    Considering a simple case with only these types: Admin, Editor, Member 1, Member 2, Member 3, Member (just registered), I should allow Editor was able to modify users, for example by changing the type to just registered Member to Member 1 (or 2 or 3 or Editor) without be able to change the type of a user including himself to "admin". This is to avoid the Editors access to the Designer or other tools with which they could do damage.

    So one solution would be to reduce the list of possible choices for the Editors (or rather non-Admin) excluding the "admin" type but not the other types.

    user/handlers/edit.php ~46 and user/handlers/add.php ~36 could become:

    if (User::is('admin')) {
            $u->types = array_keys (User::acl ()->rules);   
    } else {
            $u->types = array_diff (array_keys (User::acl ()->rules), array('admin'));  
    }
    

    What do you think?

    Good weekend.

  • edited August 2014

    It would also be necessary to filter the list of users in admin.php in order to avoid that, modifying an admin, its type is changed.

    Is there a less verbose way of:

    if (User::is('admin')) {
        $users = User::query ('id, name, email, type, company, title')
        ->order ('name asc')
        ->fetch_orig ($limit, $offset);
        $count = User::query ()->count ();  
    } else {
        $users = User::query ('id, name, email, type, company, title')
        ->where ("type <> 'admin'")
        ->order ('name asc')
        ->fetch_orig ($limit, $offset);
        $count = User::query ()
        ->where ("type <> 'admin'")
        ->count (); 
    }
    

    ...to put the condition only on the "where"?

    (reference: user/handlers/admin.php ~15)

  • edited August 2014

    Would this be something like what you are looking for?

    $restrict = (User::is('admin') ? 'admin' : ' ');
    $users = User::query ('id, name, email, type, company, title')
    ->where ("type <> ". $restrict)
    ->order ('name asc')
    ->fetch_orig ($limit, $offset);
    $count = User::query ()
    ->where ("type <> ". $restrict)
    ->count (); 
    
  • edited August 2014

    Yup, great!

    Thanks.

  • For a general purpose solution, the only trouble is that you can't assume a specific 'admin' role, since you can define other roles with just as much privilege too.

    Maybe we could start by adding a new user/edit_role access rule that restricts from modifying user roles even if you can modify the other user info. So some site admins could modify users, but not change their roles. But for adding users, it would need to choose a default role for them, presumably via a default_role setting.

    If by default this was set to 'member', then a site admin that can add users but not change roles could only create site members but not new site admins.

    That would be a bit more limited than figuring out which roles are allowed, but would it solve the problem adequately for enough use cases?

  • You're right about the fact that we can't assume a specific 'admin' role.

    Your proposal responds to most of the cases that have happened to me.

    In addition to doing what you suggested, what do you think of adding a parameter of type array (say denied_user_types) in /conf/acl.php and handle it where necessary?

    The intent would be to be able to prohibit certain types of user to assign certain user types.

    This would not give any discomfort if omitted but would greatly increase the chances when needed.

  • @BronyBorn I slightly modified your code because the "if shorthand" had the swapped values ​​and the "where" gave error

    $restrict = (User::is('admin') ? ' ' : 'admin');
    $users = User::query ('id, name, email, type, company, title')
        ->where ("type <> '$restrict'")
        ->order ('name asc')
        ->fetch_orig ($limit, $offset);
    $count = User::query ()
        ->where ("type <> '$restrict'")
        ->count (); 
    
  • I just implemented the default_role setting (available under Tools > Members > Settings), and added a new access rule to allow/deny changing member roles. Adding new ones defaults to default_role, which defaults to "member".

    @alpi, I really like your suggestion for a denied_user_types setting, but I think it made more sense to make it a positive in the role forms, so there's now an "Allowed roles" checklist under a new user/edit_roles setting, as seen here:

    Imgur

    Thought I was just going to play around with it, but it came together easier than I thought it would :)

  • Sorry I missed your answer. Great, thank you very much, your solution is the best.

Sign In or Register to comment.