fix: auto detect if the player has permission to query nbt fromserver…

… to prevent sending invalid packets (#2)

* fix: dont query normal blocks

* fix: set `TWEAK_SERVER_ENTITY_DATA_SYNCER` to true because we dont always send packets to query nbt.

* fix: NPE if player turn on TWEAK_SERVER_ENTITY_DATA_SYNCER in game.

* fix

* fix

src/main/java/fi/dy/masa/tweakeroo/config/FeatureToggle.java

@@ -87,7 +87,8 @@ public enum FeatureToggle implements IHotkeyTogglable, IConfigNotifiable<IConfig
     TWEAK_RENDER_LIMIT_ENTITIES     ("tweakRenderLimitEntities",            false, "",    "Enables limiting the number of certain types of entities\nto render per frame. Currently XP Orbs and Item entities\nare supported, see Generic configs for the limits."),
     TWEAK_REPAIR_MODE               ("tweakRepairMode",                     false, "",    "If enabled, then fully repaired items held in hand will\nbe swapped to damaged items that have Mending on them."),
     TWEAK_SCULK_PULSE_LENGTH        ("tweakSculkPulseLength",               false, true, "",    "Allows modifying the Sculk Sensor pulse length. Set the pulse length in Generic -> sculkSensorPulseLength"),
-    TWEAK_SERVER_ENTITY_DATA_SYNCER ("tweakServerEntityDataSyncer",         false, "",    "Allows Tweakeroo to attempt to use the Server Data Syncer\nto obtain various Entity Data, such as for Shulker Boxes\n\n§6NOTE: This feature requires OP privileges to work."),
+    // fixme: is this still necessary? we can detect if the player have permission so it wont send too many requests
+    TWEAK_SERVER_ENTITY_DATA_SYNCER ("tweakServerEntityDataSyncer",         true,  "",    "Allows Tweakeroo to attempt to use the Server Data Syncer\nto obtain various Entity Data, such as for Shulker Boxes\n\n§6NOTE: This feature requires OP privileges to work, Tweakeroo will try to detect if you have enough permission."),
     TWEAK_SHULKERBOX_DISPLAY        ("tweakShulkerBoxDisplay",              false, "",    "Enables the Shulker Box contents display when hovering\nover them in an inventory and holding shift"),
     TWEAK_SIGN_COPY                 ("tweakSignCopy",                       false, "",    "When enabled, placed signs will use the text from\nthe previously placed sign.\nCan be combined with tweakNoSignGui to quickly place copies\nof a sign, by enabling that tweak after making the first sign."),
     TWEAK_SNAP_AIM                  ("tweakSnapAim",                        false, "",    KeybindSettings.INGAME_BOTH, "Enabled a snap aim tweak, to make the player face to pre-set exact yaw rotations"),

src/main/java/fi/dy/masa/tweakeroo/data/ServerDataSyncer.java

@@ -25,28 +25,56 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
 
-@SuppressWarnings("deprecation")
+@SuppressWarnings({"deprecation"})
 public class ServerDataSyncer {
-    public static ServerDataSyncer INSTANCE;
+    private static ServerDataSyncer INSTANCE;
+
+    public static ServerDataSyncer getInstance()
+    {
+        if (INSTANCE == null)
+        {
+            INSTANCE = new ServerDataSyncer(MinecraftClient.getInstance().world);
+        }
+        return INSTANCE;
+    }
+
+    public static void resetInstance()
+    {
+        INSTANCE = null;
+    }
 
     /**
      * key: BlockPos
      * value: data, timestamp
      */
     private final Map<BlockPos, Pair<BlockEntity, Long>> blockCache = new HashMap<>();
     private final Map<Integer, Pair<Entity, Long>> entityCache = new HashMap<>();
-    private final Map<Integer, Either<BlockPos, Integer>> pendingQueries = new HashMap<>();
+    private final Map<Either<BlockPos, Integer>, CompletableFuture<@Nullable NbtCompound>> pendingQueries = new HashMap<>();
+    private final Map<Integer, CompletableFuture<@Nullable NbtCompound>> pendingQueryFutures = new HashMap<>();
     private final ClientWorld clientWorld;
+    /**
+     * if the client can query the server for block/entity data? null if not yet known.
+     */
+    private Optional<Boolean> yesIAmOp = Optional.empty();
 
-    public ServerDataSyncer(ClientWorld world) {
+    public ServerDataSyncer(ClientWorld world)
+    {
         this.clientWorld = Objects.requireNonNull(world);
     }
 
-    private @Nullable BlockEntity getCache(BlockPos pos) {
+    private @Nullable BlockEntity getCache(BlockPos pos)
+    {
         var data = blockCache.get(pos);
-        if (data != null && System.currentTimeMillis() - data.getRight() <= 1000) {
-            if (System.currentTimeMillis() - data.getRight() > 500) {
+
+        if (yesIAmOp.isPresent() && !yesIAmOp.get()) return null;
+
+        if (data != null && System.currentTimeMillis() - data.getRight() <= 1000)
+        {
+            if (System.currentTimeMillis() - data.getRight() > 500)
+            {
                 syncBlockEntity(clientWorld, pos);
             }
             return data.getLeft();
@@ -55,10 +83,16 @@ public ServerDataSyncer(ClientWorld world) {
         return null;
     }
 
-    private @Nullable Entity getCache(int networkId) {
+    private @Nullable Entity getCache(int networkId)
+    {
         var data = entityCache.get(networkId);
-        if (data != null && System.currentTimeMillis() - data.getRight() <= 1000) {
-            if (System.currentTimeMillis() - data.getRight() > 500) {
+
+        if (yesIAmOp.isPresent() && !yesIAmOp.get()) return null;
+
+        if (data != null && System.currentTimeMillis() - data.getRight() <= 1000)
+        {
+            if (System.currentTimeMillis() - data.getRight() > 500)
+            {
                 syncEntity(networkId);
             }
             return data.getLeft();
@@ -69,62 +103,52 @@ public ServerDataSyncer(ClientWorld world) {
 
     public void handleQueryResponse(int transactionId, NbtCompound nbt)
     {
-        Tweakeroo.logger.warn("handleQueryResponse: id [{}] // nbt {}", transactionId, nbt.toString());
+        Tweakeroo.logger.debug("handleQueryResponse: id [{}] // nbt {}", transactionId, nbt);
+        pendingQueryFutures.remove(transactionId).complete(nbt);
 
         if (nbt == null) return;
-        if (pendingQueries.containsKey(transactionId)) {
-            Either<BlockPos, Integer> either = pendingQueries.remove(transactionId);
-            either.ifLeft(pos -> {
-                if (!clientWorld.isChunkLoaded(pos)) return;
-                BlockState state = clientWorld.getBlockState(pos);
-                if (state.getBlock() instanceof BlockEntityProvider provider) {
-                    var be = provider.createBlockEntity(pos, state);
-                    if (be != null) {
-                        be.read(nbt, clientWorld.getRegistryManager());
-                        blockCache.put(pos, new Pair<>(be, System.currentTimeMillis()));
-                    }
-                }
-            }).ifRight(id -> {
-                Entity entity = clientWorld.getEntityById(id).getType().create(clientWorld);
-                if (entity != null) {
-                    entity.readNbt(nbt);
-                    entityCache.put(id, new Pair<>(entity, System.currentTimeMillis()));
-                }
-            });
+        if (pendingQueryFutures.containsKey(transactionId))
+        {
+            yesIAmOp = Optional.of(true);
         }
-        if (blockCache.size() > 30) {
+
+        if (blockCache.size() > 30)
+        {
             blockCache.entrySet().removeIf(entry -> System.currentTimeMillis() - entry.getValue().getRight() > 1000);
         }
-        if (entityCache.size() > 30) {
+        if (entityCache.size() > 30)
+        {
             entityCache.entrySet().removeIf(entry -> System.currentTimeMillis() - entry.getValue().getRight() > 1000);
         }
     }
 
     public Inventory getBlockInventory(World world, BlockPos pos)
     {
-        Tweakeroo.logger.warn("getBlockInventory: pos [{}]", pos.toShortString());
+        if (yesIAmOp.isPresent() && !yesIAmOp.get()) return null;
+        Tweakeroo.logger.debug("getBlockInventory: pos [{}], op status: {}", pos.toShortString(), yesIAmOp);
 
         if (!world.isChunkLoaded(pos)) return null;
-        var data = getCache(pos);
-        if (data instanceof Inventory inv) {
+        if (getCache(pos) instanceof Inventory inv)
+        {
             BlockState state = world.getBlockState(pos);
-            if (state.getBlock() instanceof ChestBlock && data instanceof ChestBlockEntity) {
+            if (state.getBlock() instanceof ChestBlock && inv instanceof ChestBlockEntity)
+            {
                 ChestType type = state.get(ChestBlock.CHEST_TYPE);
 
-                if (type != ChestType.SINGLE) {
+                if (type != ChestType.SINGLE)
+                {
                     BlockPos posAdj = pos.offset(ChestBlock.getFacing(state));
                     if (!world.isChunkLoaded(posAdj)) return null;
                     BlockState stateAdj = world.getBlockState(posAdj);
 
                     var dataAdj = getCache(posAdj);
-                    if (dataAdj == null) {
+                    if (dataAdj == null)
+                    {
                         syncBlockEntity(world, posAdj);
                     }
 
-                    if (stateAdj.getBlock() == state.getBlock() &&
-                            dataAdj instanceof ChestBlockEntity inv2 &&
-                            stateAdj.get(ChestBlock.CHEST_TYPE) != ChestType.SINGLE &&
-                            stateAdj.get(ChestBlock.FACING) == state.get(ChestBlock.FACING)) {
+                    if (stateAdj.getBlock() == state.getBlock() && dataAdj instanceof ChestBlockEntity inv2 && stateAdj.get(ChestBlock.CHEST_TYPE) != ChestType.SINGLE && stateAdj.get(ChestBlock.FACING) == state.get(ChestBlock.FACING))
+                    {
                         Inventory invRight = type == ChestType.RIGHT ? inv : inv2;
                         Inventory invLeft = type == ChestType.RIGHT ? inv2 : inv;
                         inv = new DoubleInventory(invRight, invLeft);
@@ -135,48 +159,138 @@ public Inventory getBlockInventory(World world, BlockPos pos)
         }
 
         syncBlockEntity(world, pos);
+
+        BlockState state = world.getBlockState(pos);
+        if (state.getBlock() instanceof ChestBlock)
+        {
+            ChestType type = state.get(ChestBlock.CHEST_TYPE);
+
+            if (type != ChestType.SINGLE)
+            {
+                BlockPos posAdj = pos.offset(ChestBlock.getFacing(state));
+                if (world.isChunkLoaded(posAdj))
+                {
+                    BlockState stateAdj = world.getBlockState(posAdj);
+                    if (stateAdj.getBlock() instanceof ChestBlock)
+                    {
+                        syncBlockEntity(world, posAdj);
+                    }
+                }
+            }
+        }
         return null;
     }
 
-    public void syncBlockEntity(World world, BlockPos pos)
+    public CompletableFuture<NbtCompound> syncBlockEntity(World world, BlockPos pos)
     {
-        Tweakeroo.logger.warn("syncBlockEntity: pos [{}]", pos.toShortString());
+        Tweakeroo.logger.debug("syncBlockEntity: pos [{}], op status: {}", pos.toShortString(), yesIAmOp);
+        if (yesIAmOp.isPresent() && !yesIAmOp.get()) return CompletableFuture.completedFuture(null);
 
         if (MinecraftClient.getInstance().isIntegratedServerRunning())
         {
             BlockEntity blockEntity = MinecraftClient.getInstance().getServer().getWorld(world.getRegistryKey()).getWorldChunk(pos).getBlockEntity(pos, WorldChunk.CreationType.CHECK);
-            if (blockEntity != null) {
+            if (blockEntity != null)
+            {
                 blockCache.put(pos, new Pair<>(blockEntity, System.currentTimeMillis()));
-                return;
+                return CompletableFuture.completedFuture(blockEntity.createNbt(world.getRegistryManager()));
             }
         }
         Either<BlockPos, Integer> posEither = Either.left(pos);
-        if (MinecraftClient.getInstance().getNetworkHandler() != null && !pendingQueries.containsValue(posEither)) {
+        if (MinecraftClient.getInstance().getNetworkHandler() != null)
+        {
+            if (pendingQueries.containsKey(posEither))
+            {
+                return pendingQueries.get(posEither);
+            }
+
+            CompletableFuture<NbtCompound> future = new CompletableFuture<>();
             DataQueryHandler handler = MinecraftClient.getInstance().getNetworkHandler().getDataQueryHandler();
-            handler.queryBlockNbt(pos, it -> {});
-            pendingQueries.put(((IMixinDataQueryHandler) handler).currentTransactionId(), posEither);
+            handler.queryBlockNbt(pos, it -> {
+            });
+            pendingQueries.put(posEither, future);
+            pendingQueryFutures.put(((IMixinDataQueryHandler) handler).currentTransactionId(), future);
+            future.thenAccept(nbt -> {
+                if (!clientWorld.isChunkLoaded(pos) || nbt == null) return;
+                BlockState state = clientWorld.getBlockState(pos);
+
+                if (state.getBlock() instanceof BlockEntityProvider provider)
+                {
+                    var be = provider.createBlockEntity(pos, state);
+                    if (be != null)
+                    {
+                        be.read(nbt, clientWorld.getRegistryManager());
+                        blockCache.put(pos, new Pair<>(be, System.currentTimeMillis()));
+                    }
+                }
+            });
+            if (yesIAmOp.isEmpty())
+            {
+                yesIAmOp = Optional.of(false);
+            }
+            return future;
+        }
+        else
+        {
+            throw new IllegalStateException("Not connected to a server");
         }
     }
 
-    public void syncEntity(int networkId)
+    public CompletableFuture<NbtCompound> syncEntity(int networkId)
     {
-        Tweakeroo.logger.warn("syncEntity: pos [{}]", networkId);
+        Tweakeroo.logger.debug("syncEntity: pos [{}], op status: {}", networkId, yesIAmOp);
+        if (yesIAmOp.isPresent() && !yesIAmOp.get()) return CompletableFuture.completedFuture(null);
 
         Either<BlockPos, Integer> idEither = Either.right(networkId);
-        if (MinecraftClient.getInstance().getNetworkHandler() != null && !pendingQueries.containsValue(idEither)) {
+        if (MinecraftClient.getInstance().getNetworkHandler() != null)
+        {
+            if (pendingQueries.containsKey(idEither))
+            {
+                return pendingQueries.get(idEither);
+            }
+
+            CompletableFuture<NbtCompound> future = new CompletableFuture<>();
             DataQueryHandler handler = MinecraftClient.getInstance().getNetworkHandler().getDataQueryHandler();
-            handler.queryEntityNbt(networkId, it -> {});
-            pendingQueries.put(((IMixinDataQueryHandler) handler).currentTransactionId(), idEither);
+            handler.queryEntityNbt(networkId, it -> {
+            });
+            pendingQueries.put(idEither, future);
+            pendingQueryFutures.put(((IMixinDataQueryHandler) handler).currentTransactionId(), future);
+            future.thenAccept(nbt -> {
+                if (nbt == null) return;
+                if (clientWorld.getEntityById(networkId) != null)
+                {
+                    Entity entity = clientWorld.getEntityById(networkId).getType().create(clientWorld);
+                    if (entity != null)
+                    {
+                        entity.readNbt(nbt);
+                        entityCache.put(networkId, new Pair<>(entity, System.currentTimeMillis()));
+                    }
+                }
+            });
+            if (yesIAmOp.isEmpty())
+            {
+                yesIAmOp = Optional.of(false);
+            }
+            return future;
+        }
+        else
+        {
+            throw new IllegalStateException("Not connected to a server");
         }
     }
 
     public @Nullable Entity getServerEntity(Entity entity)
     {
         Entity serverEntity = getCache(entity.getId());
-        if (serverEntity == null) {
+        if (serverEntity == null)
+        {
             syncEntity(entity.getId());
             return null;
         }
         return serverEntity;
     }
+
+    public void recheckOpStatus()
+    {
+        yesIAmOp = Optional.empty();
+    }
 }

src/main/java/fi/dy/masa/tweakeroo/mixin/MixinClientPlayNetworkHandler.java

@@ -1,5 +1,6 @@
 package fi.dy.masa.tweakeroo.mixin;
 
+import fi.dy.masa.tweakeroo.data.ServerDataSyncer;
 import org.spongepowered.asm.mixin.Mixin;
 import org.spongepowered.asm.mixin.injection.At;
 import org.spongepowered.asm.mixin.injection.Inject;
@@ -38,4 +39,17 @@ private void onPlayerDeath(DeathMessageS2CPacket packetIn, CallbackInfo ci)
             MiscUtils.printDeathCoordinates(mc);
         }
     }
+
+    @Inject(
+            method = "onCommandTree",
+            at = @At("RETURN")
+    )
+    private void onCommandTree(CallbackInfo ci)
+    {
+        if (FeatureToggle.TWEAK_SERVER_ENTITY_DATA_SYNCER.getBooleanValue())
+        {
+            // when the player becomes OP, the server sends the command tree to the client
+            ServerDataSyncer.getInstance().recheckOpStatus();
+        }
+    }
 }

src/main/java/fi/dy/masa/tweakeroo/mixin/MixinDataQueryHandler.java

@@ -3,16 +3,21 @@
 import fi.dy.masa.tweakeroo.Tweakeroo;
 import fi.dy.masa.tweakeroo.config.FeatureToggle;
 import fi.dy.masa.tweakeroo.data.ServerDataSyncer;
+import net.minecraft.client.network.ClientPlayNetworkHandler;
 import net.minecraft.client.network.DataQueryHandler;
 import net.minecraft.nbt.NbtCompound;
+import org.spongepowered.asm.mixin.Final;
 import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.Shadow;
 import org.spongepowered.asm.mixin.injection.At;
 import org.spongepowered.asm.mixin.injection.Inject;
 import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
 
 @Mixin(DataQueryHandler.class)
 public class MixinDataQueryHandler
 {
+    @Shadow @Final private ClientPlayNetworkHandler networkHandler;
+
     @Inject(
             method = "handleQueryResponse",
             at = @At("HEAD")
@@ -23,7 +28,7 @@ private void queryResponse(int transactionId, NbtCompound nbt, CallbackInfoRetur
 
         if (FeatureToggle.TWEAK_SERVER_ENTITY_DATA_SYNCER.getBooleanValue())
         {
-            ServerDataSyncer.INSTANCE.handleQueryResponse(transactionId, nbt);
+            ServerDataSyncer.getInstance().handleQueryResponse(transactionId, nbt);
         }
     }
 }