[403 ratelimit] Implement permission check and throw exceptions internally (not on API response)

[RheaAyase]

Hi all,

I guess that you've seen the news. I'd like to open this ticket as means of discussion / tracking ticket for dealing with this.

I think that this should be resolved on the api library level, and not on a user level. Thoughts?

If I were to resolve this locally it would be with Discord.NET extensions that would check for permissions (and throw) on every api request. This would involve checking guild permissions, hierarchy, channel permissions, and in the case of modifying permissions, whether we have those to be able to modify them... Quite a lot.

[Anu6is]

Regardless of this change, aren't these things that you should already be checking for. If via a command you'd use preconditions to ensure that the bot has access to do anything the command requires before the command can be executed. If it's some other non-command process, you use the GuildPermissions or Permissions check to determine if your bot has the required guild or channel permissions. I don't see why this change should impact any of the things you should already be doing in regards to your bot's permissions and instead hoisting that responsibility onto the lib.

[foxbot]

For many common-sense cases, we already prevent you from sending invalid data to the API. The existing API preconditions will not be overhauled for the current release series, and the stateless-first nature of the v3 series means that we will not be checking every edge case in the future, either.

Proper anti-abuse measures will vary from bot to bot, and for most of our users, running into a remote 403 will not be an issue. Private bots are essentially unaffected by this change, whereas larger bots may need to implement channel- and guild- wide rate-limiting on their commands, as well as preemptively throwing on edge-cases where the library absolutely cannot prevent a prior-403 (reactions and DMs to blocked/opting-out users, for instance).

If you are developing a large bot and need any advice on how to engineer proper anti-abuse mechanisms with this library, I'm happy to help you individually, or aid in a community package to provide the necessary extensions.

But for the vast majority of use-cases, the anti-abuse thresholds are ridiculously out of reach, and there's little reason to change the library.

[RheaAyase]

@Anu6is Because:

This would involve checking guild permissions, hierarchy, channel permissions, and in the case of modifying permissions, whether we have those to be able to modify them... Quite a lot.

There is much more to it than just checking permissions. And all of that would be implemented on the library call to be generic and for the user of it (e.g. myself) to not have to quite literally copypaste permission check all over the place, or implement a whole new system for it, or as I already suggested, write extensions to the library.

Every call has different requirements, every call has different target, e.g. sanitizing permissions in the beginning of executing a command is not possible because at that point we do not know who will be the target of the action. We can't know at the time of writing the code, or in the beginning of the execution, because it's determined during the execution. Now you could say "just call the check when you do know" but that's my point. Manually adding checks into every single command or any other action the bot does isn't a good idea. And because of this non deterministic behavior we can't just write a system where we specify via some properties what permissions do we need and the command system will check that before it's executed.

Plus there are many other things that are not commands that could be an issue here... Which is my actual main concern. Commands are not executed often enough to be a problem even if every single one of them resulted in 403. However, imagine antispam, and channel wallapapered with wrong permissions, and voila, we can't delete the messages. Or wrong hierarchy and we can't ban those 100 spambots, or whatever else. (I already have a solution in place, always had, so non-issue for me, an issue for others...)

It would be best if each action in the library could check for permissions on it's own, and throw if it isn't good. Say I'm banning someone, it checks guild permission and hierarchy difference. I'm deleting a message, it checks if my permissions aren't wrecked by channel overrides. I'm modifying guild permissions, it checks whether I can modify that specific permission (i.e. i have it.)

[FiniteReality]

Every call has different requirements, every call has different target, e.g. sanitizing permissions in the beginning of executing a command is not possible because at that point we do not know who will be the target of the action. We can't know at the time of writing the code, or in the beginning of the execution, because it's determined during the execution.

If you use the Discord.Net.Commands library, you will be able to write code to search and identify users such as target users before your command is ran, and use our preconditions system to enforce that they have the correct permissions.

[RheaAyase]

It's not that simple, yes you can make the "pre-check" use a delegate that will identify targets (and then use the same output in the command) but there isn't a unique solution, what if one command will do several things that all have different requirements between different users and channels, etc... The only generic solution is checking the permissions before every api call and throwing if it's not good.

and use our preconditions system to enforce that they have the correct permissions.

Are you talking about permissions of the person who is calling the command? I'm talking about e.g. banning 3rd party user and requiring hierarchy above them. It's about bots permissions, not the person who's calling commands.

[FiniteReality]

Ah, I misunderstood. In that case, you can still use our preconditions system to enforce that the bot has specific permissions.

[Anu6is]

@RheaAyase none of the points you've raised change my original response. These are all things you should have been checking already (as bothersome as they may be). It's always good practice to prevent errors where you can. So long as you can check for a condition that can raise an exception, you should.
For example, before you delete a message (spam), check if the bot has permissions to do so. I personally already have extension methods to do this. Now, I'm not saying I'm against the lib adding any of this... any convenience method that makes development easier is appreciated. But I don't see this API change as a driving force for that.

[RheaAyase]

Let's not shout in bold at each other.

I've been checking them, and I have all kinds of things in place to deal with various potential issues. This is not about me, this is about this library. If it was about my project, I'd be creating an issue in my project.

I personally already have extension methods to do this

Hence proving my point. You are extending the library, someone else will do the same, and we end up reinventing the wheel on almost every occasion.

[Anu6is]

Lol, my bad. Wasn't shouting, just a highlight :grimacing:

[foxbot]

Anti-abuse measures will vary from case to case, and as such are not within the scope of this library.