Added permission check for Allays #1958
Conversation
|
I've been severely inactive in the Minecraft scene for some time, and I'm trying to understand the effects of this. What risk in regards to grief can one perform via giving/taking items from an allay? My only guess is taking things from an allay displayed cosmetically in a claim? |
Some users have come up with ways to use allays in automated farms for item collection and sorting. Others being able to change what item they are holding allows users to disrupt (grief) the intended process. |
There was a problem hiding this comment.
Most major thing is the missing api-version: 1.19 that needs to be included if 1.19 features are used.
I'm semi-reluctant to give this my full seal of approval because while this does fix the easiest way to steal an allay, it does not address luring the allay out by dropping items matching its current, then swapping its item to take ownership. That said, that flaw is somewhat more in line with bees being not completely tracked and protected, users having some level of responsibility to build intelligently.
| @@ -104,6 +104,7 @@ public class GriefPrevention extends JavaPlugin | |||
| public boolean config_claims_protectHorses; //whether horses on a claim should be protected by that claim's rules | |||
| public boolean config_claims_protectDonkeys; //whether donkeys on a claim should be protected by that claim's rules | |||
| public boolean config_claims_protectLlamas; //whether llamas on a claim should be protected by that claim's rules | |||
| public boolean config_claims_protectAllays; //weather allays on a claim should be protected by that claim's rules | |||
|
|
||
| PlayerData playerData = this.dataStore.getPlayerData(player.getUniqueId()); | ||
|
|
||
|
|
||
|
|
| } | ||
| else | ||
| { | ||
| if (entity instanceof Allay) //check if player tried to set item |
There was a problem hiding this comment.
While there aren't currently any Tameable Allay implementations that I'm aware of, it would be better to not have this in an else following the section for Tameable handling. It's similar to how armor stands are handled below.
| Claim claim = this.dataStore.getClaimAt(entity.getLocation(), false, playerData.lastClaim); | ||
| if (claim != null) //make sure user is in an active claim | ||
| { | ||
| Supplier<String> override = () -> |
There was a problem hiding this comment.
This override message is duplicate code and should be removed. It matches the supplier that would be constructed for ClaimPermission.Inventory by default. You only need an override if you want to use a different message, i.e. how interacting with claimed entities generally uses Messages.NoDamageClaimedEntity instead.
See https://github.com/TechFortress/GriefPrevention/blob/058ae98ceea307cf72155bde421564080ac85eb1/src/main/java/me/ryanhamshire/GriefPrevention/Claim.java#L587-L594 and https://github.com/TechFortress/GriefPrevention/blob/058ae98ceea307cf72155bde421564080ac85eb1/src/main/java/me/ryanhamshire/GriefPrevention/ClaimPermission.java#L37
|
Is this pull request still alive? We have a problem on our server with allays, anyone can steal the items from them even in claimed area |
|
The PR owner appears to have not followed up on the review left by Jikoo. |
|
@MrButtersDEV Would it be possible for you to follow up on the review here? 🙂 |
|
PR owner looks inactive for over half a year, not sure if he comes back for this |
Their last GitHub-tracked activity was Mar 28 in a private repo, 3 days ago. I wouldn't be surprised to not see them bother due to the PR age, but they're definitely active. |
Added a permission check similar to pets for setting what item an allay can pick up.
-> Might be a Pedal issue not spigot