Add user management operations

[FelixGail]

Adds user and permission management (#21)

I am personally unhappy with operationId: deleteDifferentUser, but I don't want to break existing implementations by renaming deleteUser.

[FelixGail]

Do you think it might be problematic to effectively allow users to impersonate another user by deleting and recreating them? Maybe that should be a separate permission.

This is indeed problematic. I think a separate permission for user deletions is the most straightforward change.

How do we handle attempts to change permissions of a user with more permission than the managing user? E.g.:

userA.permissions = [manage_users, skip, pause]
userB.permissions = [skip, pause, dislike, move, enqueue, alter_suggestions]
userA.setPermissions(userB, [skip, pause])
// What are userB's permissions now? Just skip and pause?

Do you think this could be solved by allowing a user to only change those permissions he owns? In the above example userA would only be able to bestow the skip and pause onto userB, but not remove any of the permissions he doesn't own.

I intentionally skipped the manage_users permission in the above example, because if we handle permissions this way this permission needs to be separately. Otherwise we would create a new loophole, where userA gives userB the manage_users permission who in turn can bestow all of his permissions.

I can think of three possible solutions:

• The Teamspeak way: We could add a new admin permission
• We change the manage_users permission to an admin permission, giving the user all other permissions implicitly
• In order to bestow the manage_users permission a user needs all other pemissions

[BjoernPetersen]

Otherwise we would create a new loophole, where userA gives userB the manage_users permission who in turn can bestow all of his permissions.

I don't think that's a loophole, really. Users who are given the manage_users permission may give that permissions to other users, but they could only escalate their own permissions by that if the other user with more permissions colludes with them. I'd personally be fine with that risk.

Do you think this could be solved by allowing a user to only change those permissions he owns? In the above example userA would only be able to bestow the skip and pause onto userB, but not remove any of the permissions he doesn't own.

That's a good approach, but I don't think it's enough. That limitation would still allow a user with manage_users permissions to remove certain permissions of any user, even the server owner who gave himself every possible permission.

It's a puzzler, really. This is going to need some time to think about it.

[BjoernPetersen]

After some discussion with a flatmate of mine, I think the best solution is making the manage_users permission an admin permission. The permission allows a user to grant or revoke any permissions for himself or other users.

When a user is granted the admin permission he is automatically granted all other permissions. If more possible permissions are added to the system later on, they'll have to be granted manually.

While the admin permission allows the user to escalate their own permissions, it doesn't in itself include any other permission, i.e. a user with the admin permission but without the enqueue permission will not be able to enqueue songs.

To prevent "coups", I suggest the following limitation: An admin can not edit the permissions of another admin, except for his own. Removing admins will thus not be possible via the API, although admins may step down of course.

[FelixGail]

FYI: The editorconfig check runs through, but the check is running on the fork and is not detected: https://github.com/FelixGail/MusicBot-API/actions/runs/363214297

[BjoernPetersen]

Sorry about that. Updating your branch should fix the problem.

[BjoernPetersen]

Okay, I really want to merge this branch. I do have one more idea though. It's kind of a rehash of some of our ideas to the current solution:

Introduce a manage_admins permission

Introducing this permission would allow almost all user management to be done via the API.

Granted rights

• Grant and revoke the admin permission
• Grant, but not revoke the manage_admins permission
  • Analogously to the previous approach for admins
  • The revocation aspect limits the API a bit, but prevents "coups". I'm not sure about it yet, so input is welcome.

Changes to admin

• Users with the admin permission can not add new admins anymore
• Making a user admin doesn't grant him the manage_admins permission

Benefits

• Appointing an admin temporarily is much more viable
  • Removing the admin permission of a user doesn't leave him a time window in which he could re-add the permission with the existing JWT
• "Rogue" admins can't do as much damage
• The manage_admins permission can be reserved for especially trusted users (maybe the server owner even completely reserves it for himself)
  • This is what makes me doubt the manage_admin revocation limitation

[BjoernPetersen]

Additionally, we should add/document a limitation to the deleteOtherUser operation: Only non-admins may be deleted.

[FelixGail]

Introduce a manage_admins permission

That's a nice solution

* The `manage_admins` permission can be reserved for especially trusted users (maybe the server owner even completely reserves it for himself)

  * This is what makes me doubt the `manage_admin` revocation limitation

I don't think this restriction is needed. Not having it could even proof beneficial as unused accounts could be removed over the API. To match the intention of this permission it could be named OWNER, setting as guideline that all users with this permission should be able to access the bot directly.

[BjoernPetersen]

Yeah, I also toyed with superuser, but owner is probably most obvious.

[BjoernPetersen]

The docs should also mention that admins (not owners) can't give others the ADMIN permission.

[BjoernPetersen]

Thanks for your contribution!