Prevent chests connecting on claim boundary

[FrankHeijden]

If a chest is placed on the claim boundary, thieves can place a chest connecting to the chest in the claim and thus loot the contents. This PR fixes that bug.

[Jikoo]

I was under the impression that chests must actually be placed directly on the chest they are connecting to as of 1.15. Wouldn't it be easier to check if the block placed against is of the same material instead of looping around to potentially unrelated locations? I think knowing if it's horizontal is a nice quick check though.

[FrankHeijden]

I was under the impression that chests must actually be placed directly on the chest they are connecting to as of 1.15. Wouldn't it be easier to check if the block placed against is of the same material instead of looping around to potentially unrelated locations? I think knowing if it's horizontal is a nice quick check though.

As far as I know no, I was able to connect chests when just placing them on the ground.

[Jikoo]

Huh, okay. What about just checking the direction based on Chest half? I know it provides an enum that's something like LEFT, RIGHT, and SELF

[FrankHeijden]

This approach works fine though?

[Jikoo]

It works, but it checks as many as 4 unnecessary locations depending on chest configuration.

[FrankHeijden]

Block lookups are basically 0 cost when the chunk is loaded.

[Jikoo]

While I don't disagree that block lookups are pretty inexpensive, I don't see why you'd want to use a method that is strictly less efficient. Each iteration requires several operations that are completely unnecessary since a potential chest pair can be directly identified.

[FrankHeijden]

While I don't disagree that block lookups are pretty inexpensive, I don't see why you'd want to use a method that is strictly less efficient. Each iteration requires several operations that are completely unnecessary since a potential chest pair can be directly identified.

There is no easy way to get the connected block like that. This is by far the easiest and most readable code, without messing with the direction the chest is facing and what side the chest is (to rotate the direction BlockFace by 90 degrees depending on the side of the chest). This doesn't improve code readability, and there's no nice way to rotate by 90 degrees clock/counter clockwise without hardcoding extra methods to achieve this.

[RoboMWM]

Has this "exploit" always been possible?

[RoboMWM]

This will also break when crossing boundaries of any two claims, regardless of build trust/owned by the same player

[FrankHeijden]

Has this "exploit" always been possible?

As far as I know, I didn't see any traces of it being prevented in GP earlier.

[FrankHeijden]

This will also break when crossing boundaries of any two claims, regardless of build trust/owned by the same player

The same holds for waterflow (can't pass over subclaims / claims with same owner either) or piston subtractions.

[Jikoo]

Robo makes a very good point - moving claim boundaries could yield unexpected results. Should also probably check DoubleChest sides on InventoryOpenEvent to ensure both are accessible in case of clame boundary movements/silly claiming. With that at least both sides have distinct locations.

[FrankHeijden]

Robo makes a very good point - moving claim boundaries could yield unexpected results. Should also probably check DoubleChest sides on InventoryOpenEvent to ensure both are accessible in case of clame boundary movements/silly claiming. With that at least both sides have distinct locations.

There's not really a way to prevent that from happening unfortunately. We could indeed listen for the InventoryOpenEvent and cancel that in case the player doesn't have permissions to access both locations; however, then another problem arises: A player could then claim the chest part that is sticking out of the claim, and thus access the contents of the chest then.

[Jikoo]

There's not really a way to prevent that from happening unfortunately. We could indeed listen for the InventoryOpenEvent and cancel that in case the player doesn't have permissions to access both locations; however, then another problem arises: A player could then claim the chest part that is sticking out of the claim, and thus access the contents of the chest then.

Actually, the DoubleChestInventory allows you to access each half individually. It's very possible to treat a double chest as 2 single chests on inventory open by cancelling the original event and then opening the allowed half.

[FrankHeijden]

There's not really a way to prevent that from happening unfortunately. We could indeed listen for the InventoryOpenEvent and cancel that in case the player doesn't have permissions to access both locations; however, then another problem arises: A player could then claim the chest part that is sticking out of the claim, and thus access the contents of the chest then.

Actually, the DoubleChestInventory allows you to access each half individually. It's very possible to treat a double chest as 2 single chests on inventory open by cancelling the original event and then opening the allowed half.

I am aware, I described another unfixable flaw in the comment which will pop-up if we were to treat chest blocks individually whenever a claim border moves: A player could then claim the chest part that is sticking out of the claim, and thus access the contents of the chest then.

[FrankHeijden]

So yeah, idk if we should care if the user modifies their own claim; this PR is just focussed on fixing stealing from claim boundaries by placing a chest next to a chest in another claim

[FrankHeijden]

I think I will wait on #1056 to get merged, and then use the Claim#getOwnerID method to destinguish between claim boundaries of the same owner (allowing chests to overlap claims with the same claim owner).

[RoboMWM]

I'm still thinking about this. These edge cases really are interesting.

GP's core featureset is primarily the survival side of things (and all future refactoring will take this into account - creative featuresets are a bit outdated, and sometimes unwanted, and due to the much more flexible/varied nature of creative mode, would work better as an addon) - so waterflow and etc. are not considered in the survival mode.

I'm thinking of treating this similar to the "redstone traversing claim boundaries" sort of thing. Now, I get it, this is easier to prevent because it only happens after onPlace, and as such the performance impact is reduced since most players aren't placing chests all the time. I know I've done something for detecting doublechest placements in my PrettySimpleShop plugin, but I can't recall how performant it is.

[FrankHeijden]

This should be good to go now

[PaulBGD]

Ternary

[RoboMWM]

So there are issues with "edge" checks (this, block growth from trees, liquids, etc.) and different ways to go about performing said checks. I'm still debating if I should leave these to an addon or built-in to the plugin (separated via package and able to be disabled - and functioning like addons listening to events and such). I'm thinking of leaning towards the latter.

[FrankHeijden]

I feel like this is something GP should have in their core though. Pushing everything to addons is one thing, but this is a bug (the chest is in one’s claim, it should be protected for those) — therefore it’s not an unnecessary privilege to have it fixed here.

[Jikoo]

I believe that the base protection - preventing placing a chest to convert a chest to a double chest and access its contents - definitely belongs in GP core. I could see the rest being an addon - there are a lot of edge cases and complications that users might want to configure, and frankly if someone is careless enough to unclaim half a double chest I feel like that's on them.

[RoboMWM]

I'm thinking of leaning towards the latter.

[FrankHeijden]

Hmm, any reason why this is being pushed back to milestone 17?

[Jikoo]

Reasoning is explained in this card in the v20 roadmap, though I feel like it's much more aimed at water flow than anything else.

[FrankHeijden]

Reasoning is explained in this card in the v20 roadmap, though I feel like it's much more aimed at water flow than anything else.

But then, why not merge this (it's finished?), open up a new issue, link it to the board for further consideration on edge checks / other functionality?

[Jikoo]

Need Robo for a response to that unfortunately, I just spotted it while doing a bit more housekeeping.

[RoboMWM]

Questions on concerns of performance. Of course idk how expensive it is to perform instanceof so maybe it's ok

[FrankHeijden]

"We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil."

Aside from that: instanceof is efficient.

[Jikoo]

Honestly the most inefficient part is fetching blocks from the world, not checking what they are. I'm all for this being pulled for 16.17 and then revisited. Better to add the safety feature now and refactor it later than not add it now and still have to refactor it later.

I feel like the only places where we really need to be super concerned about microoptimizations are the very heavy hitters like liquid flow and piston stuff.

[RoboMWM]

I'm familiar with premature optimizations, thanks though... I just really don't like how it operates and wish there was a better way to detect "connection." I took a look at how I detected this in my PrettySimpleShop plugin and it looks like I check one tick afterwards to see if it's a DoubleChest. I'll move it out into a separate method and then pull, because I don't like more code just being added into the method directly as there's already more than enough of that.