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

fix(GuildMember): properly check permissions for hasPermissions #4677

Merged
merged 2 commits into from Oct 17, 2020

Conversation

NotSugden
Copy link
Contributor

@NotSugden NotSugden commented Aug 2, 2020

Please describe the changes this PR makes and why it should be merged:
This PR fixes a bug to do with GuildMember.hasPermission, to explain:
if a server had 2 roles, @everyone and Role A, if @everyone only had SEND_MESSAGES, and Role A only had VIEW_CHANNEL, GuildMember.hasPermission(['SEND_MESSAGES', 'VIEW_CHANNEL']) would inaccurately return false, this is because GuildMember.hasPermission checks if any of the members roles has the provided permissions, instead of combining all the permissions the member has, and then checking that
Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@vaporoxx
Copy link
Contributor

vaporoxx commented Aug 3, 2020

This wouldn't fully work as expected, GuildMember#permissions returns all permissions if the GuildMember is the owner, making the checkOwner parameter irrelevant.

@NotSugden
Copy link
Contributor Author

NotSugden commented Aug 3, 2020

This wouldn't fully work as expected, GuildMember#permissions returns all permissions if the GuildMember is the owner, making the checkOwner parameter irrelevant.

it would be a breaking change to remove the parameter, and even so works the same either way

@vaporoxx
Copy link
Contributor

vaporoxx commented Aug 3, 2020

I'm not talking about removing the parameter, but the method needs to be changed because this PR makes it return true for owners, no matter what you pass as checkOwner

@tipakA
Copy link
Contributor

tipakA commented Aug 3, 2020

Why would anyone want to see if guild owner has a role with specific permission?
I always wondered what is the use case for the checkOwner and checkAdmin parameters, and was never able to find even one.
If you want to know it someone has permissions, there is no reason to force a guild owner to have a role (checkOwner), and then still ignore that role and refuse to work, because role that guild owner made for themselves has only admin (checkAdmin), just because you happily check for SEND_MESSAGES ignoring both ownership and administrator permissions.

@iCrawl iCrawl merged commit 7db6978 into discordjs:master Oct 17, 2020
GiorgioBrux pushed a commit to GiorgioBrux/discord.js that referenced this pull request Oct 17, 2020
@NotSugden NotSugden deleted the patch-13 branch July 28, 2021 22:09
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.

None yet

8 participants