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

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

Open
Bloonatics opened this issue Jul 21, 2023 · 3 comments

Comments

@Bloonatics
Copy link

Bloonatics commented Jul 21, 2023

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])

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

memberPermissions(member, checkAdmin) {
if (checkAdmin && member.id === this.guild.ownerId) {
return new PermissionsBitField(PermissionsBitField.All).freeze();
}
const roles = member.roles.cache;
const permissions = new PermissionsBitField(roles.map(role => role.permissions));
if (checkAdmin && permissions.has(PermissionFlagsBits.Administrator)) {
return new PermissionsBitField(PermissionsBitField.All).freeze();
}
const overwrites = this.overwritesFor(member, true, roles);
return permissions
.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)
.freeze();
}

GuildMember.permissions

get permissions() {
if (this.user.id === this.guild.ownerId) return new PermissionsBitField(PermissionsBitField.All).freeze();
return new PermissionsBitField(this.roles.cache.map(role => role.permissions)).freeze();
}

GuildMember.permissionsIn(channel)

permissionsIn(channel) {
channel = this.guild.channels.resolve(channel);
if (!channel) throw new DiscordjsError(ErrorCodes.GuildChannelResolve);
return channel.permissionsFor(this);
}

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
Copy link
Contributor

jaw0r3k commented Jul 30, 2023

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

@Jiralite
Copy link
Member

Jiralite commented Nov 30, 2023

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
Copy link
Author

Suggestion

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

GuildChannel#permissionsFor()

Before:

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

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:

memberPermissions(member, checkAdmin) {
if (checkAdmin && member.id === this.guild.ownerId) {
return new PermissionsBitField(PermissionsBitField.All).freeze();
}
const roles = member.roles.cache;
const permissions = new PermissionsBitField(roles.map(role => role.permissions));
if (checkAdmin && permissions.has(PermissionFlagsBits.Administrator)) {
return new PermissionsBitField(PermissionsBitField.All).freeze();
}
const overwrites = this.overwritesFor(member, true, roles);
return permissions
.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)
.freeze();
}

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants