discord.js icon indicating copy to clipboard operation
discord.js copied to clipboard

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

Open Bloonatics opened this issue 1 year ago • 3 comments

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.

image

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

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

Bloonatics avatar Jul 21 '23 12:07 Bloonatics

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

jaw0r3k avatar Jul 30 '23 11:07 jaw0r3k

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.

Jiralite avatar Nov 30 '23 08:11 Jiralite

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();
 }

Bloonatics avatar Mar 24 '24 22:03 Bloonatics