-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Account for user installations in Channel.permissions_for #9837
Conversation
Ok no idea how I fix those two tests failing |
Run black on channel.py, then those should pass. |
Well that didn't work |
Discord provides the permissions for your app/bot in the current channel with |
That's fair, but for migrating purposes and because it seems not great to have internal error I still think this has a place |
Sorry, I forgot that discord.py runs a specific version (22.6) in CI, which formats stuff a bit different. Before your |
We debated this long before but I wasn't sure if it was worth going further, and I still don't. Code that is installed and used in these new contexts should just use the correct code. |
Just crossposting from bikeshedding here
|
As discussed I have removed this for dms and gdms. I have kept in the |
I have also added a recommendation to use Interaction.app_permissions instead in the docs. I didn't feel like more explanation for user apps for dms was necessary since, as the docstring already mentions, dm permissions are not really a thing and the default is close enough. |
Co-authored-by: owocado <24418520+owocado@users.noreply.github.com>
discord/abc.py
Outdated
default = self.guild.default_role | ||
if default is None: | ||
return Permissions._user_installed_permissions(in_guild=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need a check to ensure it's the bot user? This could return incorrect permissions if one uses permissions_for
on a user other than the bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point.
What do you think would be the most sensible way of returning user permissions? Just 0? Also, how do I access the bot/bot ID in this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get the bot id from channel._state.self_id
. I don't have any better ideas other than 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers, that should be implemented now
Wouldn't this be returning wrong information? The following wouldn't work with the current permissions returned, even though it's a sane assumption to make:- if ctx.channel.permissions_for(ctx.me).read_message_history:
async for message in ctx.channel.history(): ... Same goes for |
I think that would be a little inconvenient for a number of things though such as
Though I suppose you can work around that with an extra if statement. |
Summary
Currently
Channel.permissions_for
do not account for user installed apps. So in dms they will have_dm_permissions()
permissions even though they cannot see the chat history and in guilds the bot is not on, the library will error because it attempts to access guild data it does not have access to. To address this, I am introducingPermissions._user_installed_permissions()
which includes a default for what permissions a user installed app has. This method is used whenever permissions are checked for the Bot/App when it is not part of the server or dm the permissions are checked for.I know Discord provides this permission information in Interaction.app_permissions and Context.bot_permissions, but I think this is still important for compatibility and "migrating" to user installed commands (the reason I encountered this in the first place because I changed one of my commands to support user installs.). Furthermore it is never great to get an internal dpy error without explanation.
Please let me know if my code style needs change or the permissions in my new method need changing.
Checklist
Minimal reproducible example
Here is some code to reproduce the issues d.py currently has: