Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MANAGE_ROLES/MANAGE_PERMISSIONS does not implicitly grant MANAGE_MESSAGE #487

Closed
schnapster opened this issue Jan 19, 2018 · 15 comments
Closed
Labels
api API issue

Comments

@schnapster
Copy link
Contributor

schnapster commented Jan 19, 2018

Hi,
the docs say

Permissions in Discord are sometimes implicitly denied or allowed based on logical use.

According my understanding a user that may manage roles/permissions, can end up giving themselves the permissions to manage messages, so that permission should be granted implicitly. However, in reality, it is not.
Informally, this was verified with a chat with Jake/binzy (sorry I don't remember who exactly it was anymore, and can't find it in DAPI anymore :^) ) that this is indeed the desired behavior. It has now been almost 6 months and the behavior has not been changed, so I'm opening the issue here, to make this a bit more formal. JDA already supports this behavior, which leads to 50013s unless a ton of explicit checks are done. They basically said they do not intend to change the behavior as long as there is no official stance that this implicit permission grant is wrong.
Please let us know what the plans are with regards to this issue, so this discrepancy can be resolved soon.

related issue: discord-jda/JDA#414

@z64
Copy link
Contributor

z64 commented Jan 19, 2018

a user that may manage roles/permissions, can end up giving themselves the permissions to manage messages, so that permission should be granted implicitly.

Please correct me if I'm misunderstanding, but you're saying that a person with Manage Roles can and needs to explicitly grant themselves Manage Messages in order to do so, so everyone with Manage Roles should just always have Manage Messages because they might grant it to themselves? I'm not sure I see the problem, as it's so much easier to check for explicit permissions granted in the permissions bits than it is to figure out implicit permissions.

Furthermore, as a bot, if your library were to assume that just because you have Manage Roles that it has Manage Messages, that would lead to further errors, unless it were to do something like catch the exception and grant itself permission and retry (which, would be awful)

And as it says:

Permissions in Discord are sometimes implicitly denied or allowed based on logical use

There is no logical connection between managing roles (a server-level task) and managing messages. They have nothing to do with each other. Contrast this with say, View Channel - you obviously cannot manage messages in a channel you cannot view.

@schnapster
Copy link
Contributor Author

Please correct me if I'm misunderstanding, but you're saying that a person with Manage Roles can and needs to explicitly grant themselves Manage Messages in order to do so, so everyone with Manage Roles should just always have Manage Messages because they might grant it to themselves?

Yes. A user with Manage Roles permissions can create a permission override for themselves or anyone else or any role, giving them Manage Messages (interestingly, they cannot create or edit a role granting Manage Messages).

I'm not sure I see the problem, as it's so much easier to check for explicit permissions granted in the permissions bits than it is to figure out implicit permissions.

That's not really the point. The point is that there appears to not be a clear documentation of whether Manage Messages should be implicit to Manage Roles, or not. There are other implicit permissions already.

Furthermore, as a bot, if your library were to assume that just because you have Manage Roles that it has Manage Messages, that would lead to further errors

That's exactly what's been happening for at least the past 6 months in the JDA lib.

There is no logical connection between managing roles (a server-level task) and managing messages. They have nothing to do with each other. Contrast this with say, View Channel - you obviously cannot manage messages in a channel you cannot view.

I don't really care whether there is a logical connection or not (I think there is, but I'm fine with any decision by the developers, as long as they make an official one and follow through with it), I just want the lib and Discord to agree to treat this similarly.

@z64
Copy link
Contributor

z64 commented Jan 19, 2018

The point is that there appears to not be a clear documentation of whether Manage Messages should be implicit to Manage Roles, or not. There are other implicit permissions already.

It would not be practical to provide an exhaustive list of all permission combinations to fully describe what permissions are implicit, and why that note about "logical use" is there.

In any case, this is how the API currently behaves with Manage Messages not being implicitly granted with Manage Roles. It is easy to test this, you already know that this is how it works. That is clearly a bug with the library, and there's no point in waiting for Discord to change anything, if they will even do that. (it would take much less time to fix and simply revert later)

@vladfrangu
Copy link
Contributor

I don't see why giving a user Manage Roles should give them Manage Messages automatically. While a user can make themselves an overwrite and give themselves Manage Messages (loophole much?), Manage Roles should NOT give Manage Messages automatically.

The fact that JDA assumes that Manage Roles yields Manage Messages automatically sounds like a bug on their side. Either way, I hope the loophole will hopefully be fixed one day. Or maybe it isn't a loophole. Who knows?

This is just my opinion, please don't scream at me

@MinnDevelopment
Copy link
Contributor

When I first reported this issue to @jhgg I was told it was going to be worked on. This is clearly an inconsistency on discord's end and I'm not willing to change our logic when this will change in the future.
The permission allows a user to grant themself other permissions, this implies they implicitly also hold those permissions. As long as that is the case I consider this an inconsistency on discord's end.

@msciotti
Copy link
Contributor

OK, I think the case has been fairly laid out here. Thank you for the note @napstr. @jhgg will check it out and see if and when we plan to change things. I'll mark this issue and keep it open. Thanks.

@msciotti msciotti added the api API issue label Jan 19, 2018
@night
Copy link
Member

night commented Jan 21, 2018

Manage Roles permission is essentially a "super power" on Discord currently. While it follows role hierarchy on the server level, the same does not apply for channel level permission overwrites. We have discussed changing how Manage Roles acts for channel overwrites, including the requirement of Read Messages and/or Manage Channels permission in addition to Manage Roles for modifying channel overwrites and possibly restricting permissions to those you have been explicitly granted.

With that being said, due to the complexity of the permissions system I don't think I'd feel comfortable implicitly granting extra permissions with Manage Roles, especially given we may change how it functions in the future.

@kantenkugel
Copy link
Contributor

kantenkugel commented Jan 25, 2018

I think that the behavior of channel-specific Manage Roles (aka Manage Permissions in channels), as in granting the user all permissions for that channel (effectively giving him admin mode for given channel) is perfect as it is, as that allows servers to give some members their own "realm" in which they can do whatever they want.

But that means that it is in theory granting all permissions implicitly which is neither documented to not be the case nor is it taken into account in all the API endpoints that require some specific permission in that channel.

So in my opinion, it should either be documented that it is not in fact giving implicit perms (like someone would assume) or changed all together to not give implicit permissions, which would be kinda bad as i really love the possibilities it currently opens.

Would be nice to hear what exactly the plan from Discords side is (possibly final statement) so that it can be written into stone and coded against.

As it is right now it is definitely not 100% clear what Discords intention with that permission is and that is the actual problem of this thread.

@MinnDevelopment
Copy link
Contributor

MinnDevelopment commented Jan 25, 2018

The current work-around I see is granting yourself the permissions explicitly.
I don't believe this is how it should be and it only makes things even more complicated for users and library authors.

def can_manage(channel, user):
    if user.has(channel, Permission.MESSAGE_MANAGE):
        return True
    if user.has(channel, Permission.MANAGE_PERMISSIONS): # equivalent to ADMINISTRATOR in channel context
        user.grant(channel, Permission.MESSAGE_MANAGE)
        return True
    return False

I suggest one of either:

  • Add administrator as override permission (grants all permissions for that channel)
    and don't allow users with manage permissions to grant themself permissions they do not have already (similar to how manage roles works on guild level)
  • Grant permissions whenever a user is able to grant it to others or themself

@almostSouji
Copy link

almostSouji commented Aug 4, 2018

While the use case kantenkugel describes is perfectly reasonable and in fact an amazing feature i feel like i should point a slight irregularity in logical use out that i have not seen mentioned before.

If you explicitly deny VIEW_CHANNEL for a bot you generally don't want it to interfere with this channel at all. The affected target is however able to set overwrites to the channel although "view" is explicitly denied (as long as it has MANAGE_ROLES in its final permissions - eg granted as base permission on the bot rolel), meaning it can effectively force access to a channel it's not supposed to be in.

I have not seen that specific point covered here and feel like it fosters malicious intend.
As editing channels is explicitly denied with "view" I really feel like that should also be the case for setting overwrites, blocking any and all interaction with that channel apart from requesting the information for it.

@justcool393
Copy link

So in my opinion, it should either be documented that it is not in fact giving implicit perms (like someone would assume) or changed all together to not give implicit permissions, which would be kinda bad as i really love the possibilities it currently opens.

Honestly, implicit permissions just make things more of a hassle, because we don't know exactly what we're getting. MANAGE_ROLES implicitly granting MANAGE_CHANNELS is the big confusing one. I guess maybe VIEW_CHANNEL for SEND_MESSAGES, etc, but nothing really other than that.

@night
Copy link
Member

night commented Nov 7, 2019

Long time, no updates. I'm working towards a proper fix to this condition which will introduce the following breaking change to updating permission overwrites:

  • You will only be able to (un)set permissions you have within the guild in channel permission overwrites (both allows and denies). If you have MANAGE_ROLES within the channel as a permission overwrite you will bypass this restriction.

@night night closed this as completed Nov 8, 2019
@night
Copy link
Member

night commented Nov 8, 2019

This is deployed.

@webhp
Copy link
Contributor

webhp commented Nov 9, 2019

@night Correct me if I'm wrong, but the only role permission that currently allows you to override MANAGE_ROLES in the channel is ADMINISTRATOR?

@night
Copy link
Member

night commented Nov 9, 2019

Owners/administrators at the guild level can assign a MANAGE_ROLES overwrite, and users who have a MANAGE_ROLES overwrite can also assign a MANAGE_ROLES overwrite within that channel.

ThaTiemsz referenced this issue in Discord-Datamining/Discord-Datamining Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API issue
Projects
None yet
Development

No branches or pull requests

10 participants