`GuildMember` and `GuildChannel` permission-based methods do not reflect permission changes on timeouts

Bloonatics commented 1 year ago

Which package is this bug report for?

discord.js

Issue description

Description

According to docs, timed out members should have lost all permissions except VIEW_CHANNEL and READ_MESSAGE_HISTORY, if they do not have ADMINISTRATOR permission. However, the methods mentioned below do not reflect the permission changes on timeouts. I understand that we can check if a guild member is timed out with GuildMember.isCommunicationDisabled() and many other ways, and then make a function to obtain more accurate permissions, but having a all-in-one method to check the current permissions accurately is more convenient for us.

Impacts

Making the methods less accurate, as they are not totally up-to-date with Discord permission rules.
Unexpected errors: Users may highly rely on those methods to check their bot permissions (e.g. channel.permissionsFor(client.user)?.has(['ViewChannel', 'SendMessages']) ?? false) before requesting the API (e.g. send a message to a text channel). Without reading the code deeply and additional testing, they may not know that the methods do not support timed out member permissions (when the client user is timed out in a guild, plus having no admin permission, suppose its GuildMember is cached, the above example will still return true if it has the permissions when timeout is not applied).

Code reference

GuildChannel.permissionsFor(memberOrRole, [checkAdmin])

https://github.com/discordjs/discord.js/blob/d26e022afcc84448bb91deb4dd621aea20df6617/packages/discord.js/src/structures/GuildChannel.js#L174-L179

https://github.com/discordjs/discord.js/blob/d26e022afcc84448bb91deb4dd621aea20df6617/packages/discord.js/src/structures/GuildChannel.js#L215-L237

GuildMember.permissions

https://github.com/discordjs/discord.js/blob/d26e022afcc84448bb91deb4dd621aea20df6617/packages/discord.js/src/structures/GuildMember.js#L254-L257

GuildMember.permissionsIn(channel)

https://github.com/discordjs/discord.js/blob/d26e022afcc84448bb91deb4dd621aea20df6617/packages/discord.js/src/structures/GuildMember.js#L320-L324

Code sample

No response

Versions

discord.js@14.11.0
discord.js@dev

Issue priority

Medium (should be fixed soon)

Which partials do you have configured?

Not applicable

Which gateway intents are you subscribing to?

Not applicable

I have tested this issue on a development release

No response

jaw0r3k commented 1 year ago

Thats just a bitfield, whose behaviour we shouldnt change Better to add methods like TextChannel#sendableFor to allow more complex checks

Jiralite commented 11 months ago

One thing I have noticed is an interaction's appPermissions will change its bit field if timed out. Currently, non-interaction-based permission checks do not change their bitfield.

Bloonatics commented 8 months ago

Suggestion

This is one of the solutions which is a breaking change.

`GuildChannel#permissionsFor()`

Before:

https://github.com/discordjs/discord.js/blob/a1a3a95c94194a8ab789d567a778b376e13ea973/packages/discord.js/src/structures/GuildChannel.js#L175-L180

After:

permissionsFor(memberOrRole, { checkAdmin = true, checkTimeout = true } = {}) {
   const member = this.guild.members.resolve(memberOrRole);
   if (member) return this.memberPermissions(member, { checkAdmin, checkTimeout });
   const role = this.guild.roles.resolve(memberOrRole);
   return role && this.rolePermissions(role, checkAdmin);
 }

`GuildChannel#memberPermissions()` `Private Method`

Before:

https://github.com/discordjs/discord.js/blob/a1a3a95c94194a8ab789d567a778b376e13ea973/packages/discord.js/src/structures/GuildChannel.js#L216-L238

After:

memberPermissions(member, { checkAdmin, checkTimeout }) {
   if (checkAdmin && member.id === this.guild.ownerId) {
     return new PermissionsBitField(PermissionsBitField.All).freeze();
   }

   const roles = member.roles.cache;
   const rolePermissions = new PermissionsBitField(roles.map(role => role.permissions));

   if (checkAdmin && rolePermissions.has(PermissionFlagsBits.Administrator)) {
     return new PermissionsBitField(PermissionsBitField.All).freeze();
   }

   const overwrites = this.overwritesFor(member, true, roles);

   const channelPermissions = rolePermissions
     .remove(overwrites.everyone?.deny ?? PermissionsBitField.DefaultBit)
     .add(overwrites.everyone?.allow ?? PermissionsBitField.DefaultBit)
     .remove(overwrites.roles.length > 0 ? overwrites.roles.map(role => role.deny) : PermissionsBitField.DefaultBit)
     .add(overwrites.roles.length > 0 ? overwrites.roles.map(role => role.allow) : PermissionsBitField.DefaultBit)
     .remove(overwrites.member?.deny ?? PermissionsBitField.DefaultBit)
     .add(overwrites.member?.allow ?? PermissionsBitField.DefaultBit);

   if (
     !checkTimeout
     || member.id === this.guild.ownerId
     || channelPermissions.has(PermissionFlagsBits.Administrator)
     || !member.isCommunicationDisabled()
   ) {
     return channelPermissions.freeze();
   }

   return new PermissionsBitField(
     channelPermissions.has(PermissionFlagsBits.ViewChannel) ? PermissionFlagsBits.ViewChannel : PermissionsBitField.DefaultBit |
     channelPermissions.has(PermissionFlagsBits.ReadMessageHistory) ? PermissionFlagsBits.ReadMessageHistory : PermissionsBitField.DefaultBit
   ).freeze();
 }

discordjs / discord.js