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

Can't deny a granted permission, PermissionOverride resource left in bad state #767

Closed
4 tasks done
schnapster opened this issue Sep 9, 2018 · 2 comments
Closed
4 tasks done
Labels
bug level: novice is doable as a java/jda novice priority: low status: completed has been completed but is not yet released

Comments

@schnapster
Copy link
Contributor

General Troubleshooting

Hey there! Before you report a bug or suggest a new feature,
please make sure to follow these steps first!

  • I have checked for similar issues.
  • I have updated to the [latest JDA version][download].
  • I have checked the branches or the maintainers' PRs for upcoming features/bug fixes.

Issue

Issue Type

  • Bug Report

Description

There seems to be a bug in how permissions are denied with the PermOverrideManager.

Creating a PO, then granting a permission (example: Attach Files), and then denying it again leaves the PermissionOverride in a bad state where the raw values of denied and allowed permissions are both containing the raw value of the permission in question. The Discord API will happily accept a raw value in both denied and allowed bitmasks, but will assume that the permission is allowed, making it impossible to deny the permission via JDA in this way.

Here's a JS eval to reproduce this:

var result = "";
var shardManager = context.getJda().asBot().getShardManager();
var tc = shardManager.getTextChannelById("488315931383169066");
var role = shardManager.getRoleById("488314982552043520");
var po = tc.createPermissionOverride(role).complete();
result += po.getAllowedRaw() + " " + po.getDeniedRaw() + "\n";
po.getManager().grant(Java.type("net.dv8tion.jda.core.Permission").MESSAGE_ATTACH_FILES.getRawValue()).complete();
Java.type("java.lang.Thread").sleep(1000);
po = tc.getPermissionOverride(role);
result += po.getAllowedRaw() + " " + po.getDeniedRaw() + "\n";
po.getManager().deny(Java.type("net.dv8tion.jda.core.Permission").MESSAGE_ATTACH_FILES.getRawValue()).complete();
Java.type("java.lang.Thread").sleep(1000);
po = tc.getPermissionOverride(role);
result += po.getAllowedRaw() + " " + po.getDeniedRaw() + "\n";
return result;

Giving the following output:

0 0
32768 0
32768 32768

The correct output should have been

0 0
32768 0
0 32768

I traced this issue down to the PermOverrideManager#finalizeData method.
When entering that method, the internal fields allowed and denied have the correct values (allowed being 0 and denied being 32768), but then this method calls PermOverrideManager#setupValues, which changes the value of the allowed field to contain 32768 instead of 0.

@MinnDevelopment
Copy link
Member

I just want to point out how horrible you're using JDA here in general.

Here is the code cleaned up to the core issue:

PermissionOverride po = tc.createPermissionOverride(role).complete();
//wait
po.getManager().grant(Permission.MESSAGE_ATTACH_FILES.getRawValue()).complete();
//wait
po.getManager().deny(Permission.MESSAGE_ATTACH_FILES.getRawValue()).complete();

Simplest fix would probably changing the manager to always set set |= PERMISSIONS instead of set |= ALLOWED and set |= DENIED.

Feel free to fork, apply the change, test, and pull request yourself.

@schnapster
Copy link
Contributor Author

schnapster commented Sep 9, 2018

I just want to point out how horrible you're using JDA here in general.

It's a JS eval to reproduce it, nothing more lol. Let's stay on topic.

Feel free to fork, apply the change, test, and pull request yourself.

If I were able to fully understand the internal logic of the PermOverrideManager or have some tests in place to give me confidence that changes to shared methods that might fix this issue won't break anything else, I would have already done so. As things stand, I'll just have to wait for a more competent dev than myself to come along :)

schnapster added a commit to schnapster/JDA that referenced this issue Sep 9, 2018
@MinnDevelopment MinnDevelopment added status: in progress already in the process of being implemented and removed good first issue up for grabs labels Sep 9, 2018
@MinnDevelopment MinnDevelopment added status: completed has been completed but is not yet released and removed status: in progress already in the process of being implemented labels Sep 10, 2018
MinnDevelopment added a commit that referenced this issue Sep 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug level: novice is doable as a java/jda novice priority: low status: completed has been completed but is not yet released
Projects
None yet
Development

No branches or pull requests

2 participants