chestshop and GriefPrevention bug

[SrMonsterYT]

Observed Behavior

every time I try to buy something inside 1 land protected by GriefPrevention it appears that I cannot build, and this only happens in the new versions of the paper to whom should I report?

Expected Behavior

you can buy

Reproduction steps

this only happens with the updated versions of paper and spigot, players can no longer buy items from the chestshop plugin, it appears that they can not build

Stack trace or error log

No response

Server version

paper-1.20.1-115

GriefPrevention version

version: 'de1e72d'

Configuration

config: https://mclo.gs/IGUjpAs

Plugin list

GriefPrevention, ChestShop, Essentials and Vault

Running without GriefPrevention

• I attempted running the server without GriefPrevention installed.
• The problem does not occur when GriefPrevention is removed from the server.

Running with only GriefPrevention

• I attempted running only GriefPrevention on the server.
• The issue still occurs when GriefPrevention is the only plugin running.

Running on a fresh, clean server installation

• I attempted testing for the issue on a new server.
• The issue still occurs on a new server.

Using unmodified client

• I attempted testing for the issue with the vanilla client.
• The issue still occurs when using the vanilla client.

We appreciate you taking the time to fill out a bug report!

• I searched for similar issues before submitting this bug report.

[Bobcat00]

What message do you get when you try to place a block in the claim?

[SrMonsterYT]

https://prnt.sc/iwMpdH5jQqM-

bug is between chestshop and GriefPrevention

in the early versions of paper 1.20.1 does not happen this only in the new versions

[RoboMWM]

Ah, probably due to signs and waxing and nonsense. Not sure if a new handler was added to GP for this or an existing PlayerInteractEvent is being used, since the SignChangeEvent apparently does not cover all instances of a sign being changed - though this may have changed in later versions of spigot?

Possible solutions:

1. See if PlayerInteractEvent is still needed
2. If so, will changing the priority help?

[Bobcat00]

There's this:

// Only block interaction with un-editable signs to allow command signs to function.
// TODO: When we are required to update Spigot API to 1.20 to support a change, swap to Sign#isWaxed
Tag.SIGNS.isTagged(clickedBlockType) && clickedBlock.getState() instanceof Sign sign && sign.isEditable()

https://github.com/TechFortress/GriefPrevention/blob/de1e72d99353fec63e14a22c377890d406f1de4b/src/main/java/me/ryanhamshire/GriefPrevention/PlayerEventHandler.java#L1815

[Jikoo]

The comment is wrong, should read "editable," but the code itself is correct - interacting with editable signs should be cancelled because players should not be allowed to edit them.

Imo shop plugins should make their signs un-editable, so this isn't really GP's issue to fix. Most already did because they add color codes to specific lines. Only signs with completely vanilla-creatable text are editable. We can work around this by only blocking interaction with specific items and listening to the new PlayerSignOpenEvent, but it increases our maintenance cost.

[rgaminecraft]

Imo shop plugins should make their signs un-editable, so this isn't really GP's issue to fix.

While that is a great opinion, it's easier to make mud than to clean it up. The real issue here is that cancelling the right click is breaking backwards compatibility. This appears to be a cosmetic "fix" to stop the Edit Sign GUI from opening, but GP is already properly handling the update sign event in the event that a player tries to edit a sign. I've tested this by using a GP version made for a lower Minecraft version (1.19) on a 1.20 server, and while I could bring up the Edit Sign GUI, no changes were made when I saved it.

While making all plugin signs waxed could be a great idea, many plugins and authors are slow and/or reluctant to update or make changes (especially if they have to support multiple server versions). I understand there are ways to get around this, for instance in my plugins I have forced my code to run whether the event is cancelled or not. The problem with this is you still get spammed with the "You don't have permission" message.

My suggested fix would be to remove the code that blocks right click on editable signs, and replace it with something that can be toggled via config, defaulted to disabled. The only person who would really benefit from this change would be someone who specifically doesn't want the Edit Sign GUI to pop up. I know there might have been an influx of people worried that players can edit signs for other plugins, but simply blocking the update event from a player who doesn't have permissions (which is already handled properly) is all you really need.

[MacTh3Mac]

I agree that players shouldn't be able to edit signs in claim, however commit #2080 is redundant as the edit GUI displaying isn't the edit event.

At the end of the sign edit the a block place event is already blocked by GP prior to this breaking commit.

This commit spams players with messages saying they cannot build, when they are only in reality interacting with the sign. Many plugins use the interact with signs to control in game features. Examples being ChestShops, Essentialsx, shop signs, economy plugins etc etc.

There is no need for the change in #2080 other than a small cosmetic change in not opening a GUI and the very real effect of breaking many other plugins, including some very large players, who do protect their signs, but GP is actually preventing the event even being passed to them in the first place to enable them to handle it.

I do however agree that the honeycomb should be added to the monitored right click items.

[Jikoo]

While that is a great opinion, it's easier to make mud than to clean it up. The real issue here is that cancelling the right click is breaking backwards compatibility. This appears to be a cosmetic "fix" to stop the Edit Sign GUI from opening, but GP is already properly handling the update sign event in the event that a player tries to edit a sign. I've tested this by using a GP version made for a lower Minecraft version (1.19) on a 1.20 server, and while I could bring up the Edit Sign GUI, no changes were made when I saved it.

I agree that players shouldn't be able to edit signs in claim, however commit #2080 is redundant as the edit GUI displaying isn't the edit event.

The root issue which lead to me choosing that solution was the fact that certain situations were not firing the SignEditEvent. If it isn't fired, GP can't cancel it, so the only way to block modifications is to intercept the edit before it happens, aka when the UI is opened. I think this has been resolved, so GP may be able to safely drop the additional handling.

My suggested fix would be to remove the code that blocks right click on editable signs, and replace it with something that can be toggled via config, defaulted to disabled. The only person who would really benefit from this change would be someone who specifically doesn't want the Edit Sign GUI to pop up. I know there might have been an influx of people worried that players can edit signs for other plugins, but simply blocking the update event from a player who doesn't have permissions (which is already handled properly) is all you really need.

Opening the UI at all if edits will be prevented is bad UX. I agree that GP could do better here, but I disagree that all of the change is bad. I thought I had noted it somewhere, but I do want to swap to using the PlayerSignOpenEvent when GP updates to 1.20.1, which should hopefully make all of us happy. Will have to remember to make sure the in-world block is actually a real sign so as to not block plugins using virtual signs for inputs.

While making all plugin signs waxed could be a great idea, many plugins and authors are slow and/or reluctant to update or make changes (especially if they have to support multiple server versions).

Any plugin offering multiversion support should have a way to enable certain feature flags depending on version. Generic 1.14+ support could be achieved via Sign#setEditable(boolean), though waxing with the mid-1.20 method is better because it contains no risk of booting out the placing user under any circumstances. Realistically what you're saying is that this change is reminding us that all of us in the Bukkit community (myself included) are slow to update. I don't think we should ask GP to hold itself back from thorough protections because of that.

I understand there are ways to get around this, for instance in my plugins I have forced my code to run whether the event is cancelled or not. The problem with this is you still get spammed with the "You don't have permission" message.

While it is not a good solution (the ol' "lol depend on the code if you don't like it" is a super crappy line to take and I feel bad even expressing it since I do think GP can do better here with the new 1.20 event) you can listen to GP's ClaimPermissionCheckEvent and remove the message yourself as a stopgap.
Alternately, in the case of GP's current handling, you can make signs specific to your plugin not editable as part of the interaction process, which would be a much healthier solution. This has the bonus of not allowing users to edit signs improperly if your plugin ever fails to load, for example. Depending on listener priority, you'd then see the message a maximum of once per sign. You should also consider lowering your priority to LOWEST so you handle events before protection plugins - what happens with an unwaxed shop that is buying beeswax? GP and every other protection plugin should absolutely be blocking that interaction.

My major reservation about changing to explicitly blocking waxing signs is that it doesn't help GP move away from having massive lists of stuff that has particular behaviors and needs to interact in a specific way, which was one of the really nice side benefits here.

[Bobcat00]

But GriefPrevention works with EssentialsX signs, like Buy signs. The edit GUI doesn't appear. What is EssentialsX doing differently from other plugins?

[MacTh3Mac]

But GriefPrevention works with EssentialsX signs, like Buy signs. The edit GUI doesn't appear. What is EssentialsX doing differently from other plugins?

GP doesn't work with EssentialsX signs after #2080 it warns in chat that you dont have xxx permission to build here.

EssentialsX has run their priority at LOWEST so their interact event is captured regardless.

So whilst the EssentialsX sign functions, it does so in spite of GP not because of a well implemented patch.

My view is that this patch still needs correcting to not blanket block sign interacts as it currently does on 1.20.

The reason I have added to this issue is I would rather not have to fork GP and remove this line on all future updates just. to confirm sign interact compatibility with almost all other sign using plugins.

[Jikoo]

My view is that this patch still needs correcting to not blanket block sign interacts as it currently does on 1.20.

The reason I have added to this issue is I would rather not have to fork GP and remove this line on all future updates just. to confirm sign interact compatibility with almost all other sign using plugins.

Bear in mind that I'm not the author of GP, and my opinion is just an opinion. You could submit a PR, or honestly, if you can convince Robo, I'll make the PR myself. I'm perfectly willing to clean up my own messes, I just am currently of the opinion that this is good practice for modern protections. "I will have to update my plugin to use modern API methods on modern versions" falls a bit flat when that is exactly what the PR was an attempt to do for GP.

That all said, afaik the specific bug (beyond just waxing) that that PR was seeking to address appears to no longer be in Spigot (sign edits logged in my own testing on more recent versions), so if it needs to come out, I don't think there are any problems. The only thing that I'm attached to about it is the UX of not having a sign editor pop up only to be denied on submission, because that's always annoying, even if it is just a couple words. That also can be reimplemented in a different way with an update to 1.20.

Thinking through this some more, there's a future state where we have a listing of materials where usage is denied but the whole interaction is not cancelled, which would resolve the example I mentioned where selling wax would not be possible without workarounds for GP/other protection plugins treating it like the block change it would be in a vanilla environment.

Also of note: Egg on my face, I assumed GP had actually fixed the years-old bug where it was registering its listeners on LOWEST. Double checked, and I was very wrong. Guess that's on the list of easy PRs I should submit the next time I'm feeling a batch of refactors.

[Bobcat00]

So EssentialsX and GP both use LOWEST, so it's just random chance on whether Ess signs work or not?

[rgaminecraft]

... I just am currently of the opinion that this is good practice for modern protections. "I will have to update my plugin to use modern API methods on modern versions" falls a bit flat when that is exactly what the PR was an attempt to do for GP.

While I understand your comment about the UX being important, I don't believe an opinion should dictate how others should have to work going forward. If a plugin author doesn't want their signs to be edited, they should already have measures in place to detect changes and/or interactions and handle them appropriately. If they do not have protections in place, then it is on the plugin author to fix. However, what you're change does is cause the UX of other plugins to be broken or degraded unless they choose to conform to a new standard you are trying to implement, which isn't the easiest thing to sell to others.

There are clearly many ways to fix this, and they all have their own downfalls. Let's look at a few examples:

1. Leave the existing code and force plugin authors to ignore the isCancelled and perform their actions anyways, leading to messages in chat confusing players. (requires code change by plugin author)

2. Leave the existing code and force plugin developers to change their signs to waxed signs, which would require them to possibly make major changes depending on which MC versions they need to support. (requires code changes by plugin author)

3. Leave the existing code and modify GP to run last, and only execute that code if the event was not cancelled by another plugin. This would require all plugins to run their code at a lower priority and cancel the event, which may not work with the way the plugin needs to execute based on priority. (requires code changes from GP and plugin author)

4. Remove the offending code section and allow the UX to be slightly degraded with GP, but still allowing full compatibility with other plugins. As mentioned prior you could add a configuration section to block sign right click events on claimed land, so that if someone REALLY doesn't like it they can. (requires code change by GP)

That all said, afaik the specific bug (beyond just waxing) that that PR was seeking to address appears to no longer be in Spigot (sign edits logged in my own testing on more recent versions), so if it needs to come out, I don't think there are any problems.

It sounds like you coded this in an attempt to fix a bug that allowed signs to be updated, which I 100% stand behind hot patches like this. However, once the initial bug is gone, it should be removed since the patch is no longer needed.

The only thing that I'm attached to about it is the UX of not having a sign editor pop up only to be denied on submission, because that's always annoying, even if it is just a couple words. That also can be reimplemented in a different way with an update to 1.20.

The problem with this statement here is that it's possible a plugin might want to use that GUI, and by blanket cancelling the event you would make that impossible. For instance, if a plugin had a sign [LOCK] that when users right clicked it would allow them to enter codes on each line, and when submitted the answer was validated and might [UNLOCK] access to something.

The example above is quite bad, but my point is by having this in place you are potentially blocking other plugins from functioning simply because you do not agree with it. I know there are way around everything, but keeping things as vanilla as possible limits problems like the ones you are facing now.

Thinking through this some more, there's a future state where we have a listing of materials where usage is denied but the whole interaction is not cancelled, which would resolve the example I mentioned where selling wax would not be possible without workarounds for GP/other protection plugins treating it like the block change it would be in a vanilla environment.

There is no "one case suits all" fix, and as long as your plugin offers protection from day 1 issues, you can always refactor things going forward. My biggest thing is that you should always maintain standard functionality and provide full backwards compatibility, that way server owners aren't inundated with problems when they do a simple update.

[Jikoo]

However, what you're change does is cause the UX of other plugins to be broken or degraded unless they choose to conform to a new standard you are trying to implement, which isn't the easiest thing to sell to others.

Very fair, honestly. All right, I'll accelerate my timeline and submit the PR for the 1.20 state - I had been poking at it since this opened, but I didn't bother to test because I figured it was a semi-required patch to keep GP working, and I actually finally made the commits a few minutes ago.

The only thing that I'm attached to about it is the UX of not having a sign editor pop up only to be denied on submission, because that's always annoying, even if it is just a couple words. That also can be reimplemented in a different way with an update to 1.20.

The problem with this statement here is that it's possible a plugin might want to use that GUI, and by blanket cancelling the event you would make that impossible. For instance, if a plugin had a sign [LOCK] that when users right clicked it would allow them to enter codes on each line, and when submitted the answer was validated and might [UNLOCK] access to something.

The example above is quite bad, but my point is by having this in place you are potentially blocking other plugins from functioning simply because you do not agree with it. I know there are way around everything, but keeping things as vanilla as possible limits problems like the ones you are facing now.

I think I disagree with this, but it gets a little too deep into my issues with Bukkit's event system and plugin interoperability out of the box. Too hard to determine exactly which one of any number of ways a plugin could choose to do the same action, so a best effort is all GP can do. I do definitely agree that my initial patch here to cover a server bug was not the best GP can do.