Skip to content

Slash Commands Permissions #2737

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

Merged
merged 9 commits into from
Apr 7, 2021
Merged

Conversation

msciotti
Copy link
Contributor

This code is actually already live in production, so feel free to play around with it. Documentation is WIP not because we're not done building it, just because it might not be 100% correct yet 😄

@msciotti msciotti requested a review from night March 25, 2021 19:23
@msciotti msciotti changed the title [WIP] Slash Commands Permissions Slash Commands Permissions Mar 25, 2021
@msciotti msciotti marked this pull request as draft March 25, 2021 19:23
@cmsteffey
Copy link
Contributor

this is so handy, thanks for pushing this out!

Copy link
Contributor

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick but consistency

msciotti and others added 2 commits March 25, 2021 12:31
@MinnDevelopment
Copy link
Contributor

There probably should be a way to PUT/DELETE single entries in these whitelists similar to how permission overwrites and role add/remove work. Seems like right now you can only replace the entire list.

@Herbstblatt
Copy link

Herbstblatt commented Mar 25, 2021

For now, disabled commands still show up in the command picker, but are unable to be used.

Do you plan to change this soon or is it postponed? I imagine the picker flooded with commands that I can't even use and this makes me disappointed.

@msciotti
Copy link
Contributor Author

Do you plan to change this soon or is it postponed? I imagine the picker flooded with commands that I can't even use and this makes me disappointed.

It's a difficult problem. I can't promise it'll be soon, but it is top of mind. In a pathologically bad case of a bot having 100 slash commands and they are all disabled, our clients would have to do multiple requests before we had a full list of commands to show a user (and the user would think Discord/your bot is being super slow).

Next steps are to scope possible solutions.

@cmsteffey
Copy link
Contributor

does admin/owner override slash command per-command permissions? or does the app really have full control over who gets to use what

Co-authored-by: Jan <[email protected]>
Co-authored-by: Tiemen <[email protected]>
Co-authored-by: Zomatree <[email protected]>
Co-authored-by: SpaceEEC <[email protected]>
Copy link

@dawnofmidnight dawnofmidnight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I just had a few possible changes in mind.

@GamingGeek
Copy link
Contributor

Does the @everyone role id not work for permissions?

I set permissions for the everyone role in a guild for a command with default_permission set as false and from looking at the client's logs I can see the permissions are being returned to the client in the GUILD_APPLICATION_COMMANDS_UPDATE event (payload below) but I am still unable to use the command

{
  "application_commands":[
    {
      "application_id":"764995504526327848",
      "default_permission":false,
      "description":"Lock all channels in the server, useful for stopping raids",
      "id":"822814700604882945",
      "name":"lockdown",
      "options":[...],
      "permissions":[
        {
          "id":"692925064135311390",
          "permission":true,
          "type":1
        }
      ],
      "version":"824757314275901495"
    }
  ],
  "guild_id":"692925064135311390",
  "nonce":"29",
  "updated_at":"1616708947925"
}

@ckohen
Copy link
Contributor

ckohen commented Mar 25, 2021

Setting default_permission: false disables the command in DM's as well. I personally like this, but some commands may want to be overridden for DM's. If this can't be done, it should be noted.

Also, it seems like many bots will end up with a /config or similar to set up moderator roles and / or users (maybe buttons when those are a thing). Is there a plan for a system to allow those with MANAGE_SERVER to set these overwrites in the client? E.g. if something along the lines of allow_custom_permissions: true to allow bot management commands to still be always disabled.

Finally, I was under the impression that we might get the ability to disable specific slash commands in specific channels, is that still planned or just not happening?

@ckohen
Copy link
Contributor

ckohen commented Mar 25, 2021

Oh, all the create / edit endpoints also didn't document the additional default-permission param

Copy link
Contributor

@ThaTiemsz ThaTiemsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSON bodies for the POST and PATCH command endpoints (both guild and global) are missing the default_permission param from them:

| default_permission? | boolean (default `true`) | whether the command is enabled by default when the app is added to a guild |

Additionally, the command permissions endpoints return an object or array of objects, so a new object should be documented for them:

id: snowflake
application_id: snowflake
guild_id: snowflake
permissions: array of ApplicationCommandPermissions

@rxdn
Copy link

rxdn commented Mar 26, 2021

The bulk permission update endpoint (PUT https://discord.com/api/v8/applications/{application.id}/guilds/{guild.id}/commands/permissions) isn't documented in this PR

@rxdn
Copy link

rxdn commented Mar 26, 2021

A few bugs / issues / discrepancies with the new slash command update + permissions that me and others have encountered today:

  • Limit of 10 permissions (users + roles combined) per command. This simply isn't enough. For example, in my use case, I have advanced users that might have 3/4 support categories, with a separate support team for each category, with multiple roles and users in each team.
    • Obviously users could assign a single staff role to all members. However, this is adding a TON of complexity for end users, the vast majority of which will be completely confused.
    • One solution would be to raise the limit to 25/50. However, this could lead to very large payloads...
    • ... another solution which would deal with this would be to assign a permission node to each command to group them. Then, for each permission node, 25-50 users and roles could be assigned to it to reduce payload size.
  • @merlinfuchs: default_permission should take a permission bitmask integer, not true or false. This would allow for commands to be restricted to users with certain permissions, without having to add all users and roles individually for each guild.
  • If you create a message after sending a deferred ack response (type 5) fast (e.g. ack and then send a message 2ms later), the bot is left in the thinking state forever, since Discord seems to process the message before it's fully acknowledged your ack. Note, I am using slash commands over HTTP, not the gateway. This has been resolved!
    • Night has already said this is a wontfix, however, this leaves the response type 5 in a completely broken state. You have 2 choices:
      1. Night's suggestion, use the PATCH /messages/@original endpoint and retry until your request is accepted. This is a bad solution, since big bots could hit the CloudFlare 10k 4XX ban, and you generally just don't want to have to retry sending every message multiple times. This happens consistently with most of my commands that have data cached and don't need to do any extra API requests to pad the time.
      1. Add an artificial time.sleep to your code after ACKing, before processing. This isn't a good solution: why should users have to wait?
        Thinking
  • Various clientside issues: I'm sure you're aware of many, and they should go to dtesters but anyway
    • Many users reporting crashes when using / seeing slash commands on iOS, claiming to have latest app version. Can't verify since I don't own an iPhone. Apparently this has been fixed, however the update isn't automatically downloading
    • If you type out the full name of a greyed out command, you are prompted for the arguments, and the client lets you send it, just to receive an interaction failed message
  • @ everyone in webhooks. Now that the scope migration has gone ahead, I'm sure many bots will add /say commands without using allowed_mentions. Enough said. Fixed
  • PUT https://discord.com/api/v8/applications/{application.id}/guilds/{guild.id}/commands/permissions is undocumented
  • For the permission types, role=1, user=2 where in most other places in the API, such as permission overwrites, role=0, user=1. Gonna tick this off since it's already shipped, so it can't be changed at this point. Would just be nice to have some consistency in the future.

@tbnritzdoge
Copy link
Contributor

Also in addition a description of the permission should be added so whenever a user is blocked from using a command they will actually have an idea why, because otherwise they will likely be confused and not understand what is going on.

@GitSparTV
Copy link

Is it just for me but when the command doesn't have any permissions set it errors 404?
I'm using GET with specificed application command

@HuyaneMatsu
Copy link
Contributor

HuyaneMatsu commented Apr 2, 2021

Is it just for me but when the command doesn't have any permissions set it errors 404?
I'm using GET with specificed application command

It is not only for you. Try checking whether you get error code 10066 or other one.

I need to admit, an empty payload for this case would be more intuitive.

@GitSparTV
Copy link

GitSparTV commented Apr 5, 2021

It is not only for you. Try checking whether you get error code 10066 or other one.

yeah it's
nil 'HTTP Error 10066 : Unknown application command permissions'

msciotti added 2 commits April 5, 2021 15:20
- Clarify disabled state for commands without proper permissions
- Clarify guild owner behavior
- Fix python example
- Remove redundant disabled explanation
- Add missing default_permission on routes
@msciotti
Copy link
Contributor Author

msciotti commented Apr 5, 2021

If you type out the full name of a greyed out command, you are prompted for the arguments, and the client lets you send it, just to receive an interaction failed message

@rxdn this is tracked internally now

@msciotti msciotti marked this pull request as ready for review April 5, 2021 23:57
@msciotti msciotti requested a review from typpo April 5, 2021 23:58
@night night merged commit 47eab31 into master Apr 7, 2021
@night night deleted the interactions/slash-commands-permissions branch April 7, 2021 00:39
DRSchlaubi added a commit to DRSchlaubi/kord that referenced this pull request Apr 9, 2021
DRSchlaubi added a commit to DRSchlaubi/kord that referenced this pull request Apr 9, 2021
Nihlus added a commit to Remora/Remora.Discord that referenced this pull request Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.