Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -307,13 +307,15 @@ public void onInventoryClick(InventoryClickEvent event) {
if (!(event.getWhoClicked() instanceof Player player)) return;

Inventory inv = event.getInventory();
InventoryHolder holder = inv.getHolder();
// Fires on every inventory click, so avoid block-state snapshots here.
InventoryHolder holder = inv.getHolder(false);

Location location;

if (holder instanceof DoubleChest doubleChest) {
// Always use the left side as the primary location
location = ((Chest) doubleChest.getLeftSide()).getBlock().getLocation();
if (!(doubleChest.getLeftSide(false) instanceof Chest left)) return;
location = left.getBlock().getLocation();
} else if (holder instanceof Container container) {
location = container.getBlock().getLocation();
} else {
Expand All @@ -324,9 +326,10 @@ public void onInventoryClick(InventoryClickEvent event) {
if (event.getClick() == ClickType.LEFT || event.getClick() == ClickType.SHIFT_LEFT) {
if (event.getCurrentItem() == null || event.getCurrentItem().getType().isAir()) return;

ContainerData data = containerDataManager.loadContainerData(location);
// The cached owner skips the disk read for unowned containers.
if (containerDataManager.getOwner(location) == null) return;

if (data.getOwner() == null) return;
ContainerData data = containerDataManager.loadContainerData(location);

if (!data.canAccess(player) && !player.hasPermission("thievery.admin")) {
long now = System.currentTimeMillis();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ private void applySession(Player player, LoadoutSession session) {
@EventHandler
public void onInventoryClick(InventoryClickEvent e) {
if (!(e.getWhoClicked() instanceof Player player)) return;
if (!(e.getView().getTopInventory().getHolder() instanceof LoadoutHolder holder)) return;
if (!(e.getView().getTopInventory().getHolder(false) instanceof LoadoutHolder holder)) return;
if (!holder.getPlayerId().equals(player.getUniqueId())) return;

e.setCancelled(true);
Expand Down Expand Up @@ -235,7 +235,7 @@ public void onInventoryClick(InventoryClickEvent e) {
@EventHandler
public void onInventoryClose(InventoryCloseEvent e) {
if (!(e.getPlayer() instanceof Player player)) return;
if (!(e.getView().getTopInventory().getHolder() instanceof LoadoutHolder holder)) return;
if (!(e.getView().getTopInventory().getHolder(false) instanceof LoadoutHolder holder)) return;
if (!holder.getPlayerId().equals(player.getUniqueId())) return;

sessions.remove(player.getUniqueId());
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/tfminecraft/thievery/steal/StealGui.java
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ public static void updateTitle(Player player, StealGuiHolder holder, String titl
if (player == null || !player.isOnline() || holder == null || title == null) {
return;
}
if (!(player.getOpenInventory().getTopInventory().getHolder() instanceof StealGuiHolder openHolder)) {
if (!(player.getOpenInventory().getTopInventory().getHolder(false) instanceof StealGuiHolder openHolder)) {
return;
}
if (!openHolder.getPlayerId().equals(holder.getPlayerId()) || openHolder.getKind() != holder.getKind()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,9 @@ public static StealGuiHolder getStealGuiHolder(Inventory inventory) {
if (inventory == null) {
return null;
}
InventoryHolder inventoryHolder = inventory.getHolder();
// Runs on every inventory click and close, and every tick per session. getHolder() would copy a block
// inventory (a barrel reloads all of its items) just to answer this.
InventoryHolder inventoryHolder = inventory.getHolder(false);
return inventoryHolder instanceof StealGuiHolder stealGuiHolder ? stealGuiHolder : null;
}

Expand Down
21 changes: 21 additions & 0 deletions src/test/java/net/tfminecraft/thievery/TestInventories.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package net.tfminecraft.thievery;

import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.spy;

import org.bukkit.inventory.Inventory;

/** MockBukkit leaves Paper's non-snapshot getHolder(boolean) unimplemented; the plugin uses it on hot paths. */
public final class TestInventories {

private TestInventories() {
}

/** A spy of {@code inventory} whose getHolder(boolean) answers with the wrapped inventory's holder. */
public static Inventory withHolderLookup(Inventory inventory) {
Inventory spied = spy(inventory);
doAnswer(call -> inventory.getHolder()).when(spied).getHolder(anyBoolean());
return spied;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.*;
import net.tfminecraft.rpcharacters.grave.GraveManager;
import net.tfminecraft.thievery.utils.ToolResolver;
import net.tfminecraft.thievery.TestInventories;
import org.bukkit.*;
import org.bukkit.block.*;
import org.bukkit.entity.*;
Expand Down Expand Up @@ -114,7 +115,7 @@ record DoubleFixture(Block block, Chest left, Chest right, DoubleChest holder, D
}

@Test void clickingPluginMenuDoesNotInspectLocksOrAlertStaff() {
var inventory=Bukkit.createInventory(null,9,"Menu");
var inventory=TestInventories.withHolderLookup(Bukkit.createInventory(null,9,"Menu"));
var event=mock(InventoryClickEvent.class);
when(event.getWhoClicked()).thenReturn(player);
when(event.getInventory()).thenReturn(inventory);
Expand All @@ -128,6 +129,36 @@ record DoubleFixture(Block block, Chest left, Chest right, DoubleChest holder, D
verify(event,never()).setCancelled(true);
}

@Test void takingFromUnownedContainerSkipsTheDataFileAndBlockSnapshot() {
var barrel=block(0,Material.BARREL); var inventory=TestInventories.withHolderLookup(((Container)barrel.getState()).getInventory());
var event=mock(InventoryClickEvent.class);
when(event.getWhoClicked()).thenReturn(player);
when(event.getInventory()).thenReturn(inventory);
when(event.getClick()).thenReturn(ClickType.LEFT);
when(event.getCurrentItem()).thenReturn(new ItemStack(Material.DIAMOND));

manager.onInventoryClick(event);

verify(storage).getOwner(barrel.getLocation());
verify(storage,never()).loadContainerData(any());
verify(inventory,never()).getHolder();
}

@Test void clickingDoubleChestWhoseLeftHalfIsNoLongerAChestIsIgnored() {
var inventory=mock(DoubleChestInventory.class); var leftless=mock(Inventory.class);
when(leftless.getHolder(false)).thenReturn(mock(org.bukkit.inventory.InventoryHolder.class));
when(inventory.getLeftSide()).thenReturn(leftless); when(inventory.getHolder(false)).thenReturn(new DoubleChest(inventory));
var event=mock(InventoryClickEvent.class);
when(event.getWhoClicked()).thenReturn(player);
when(event.getInventory()).thenReturn(inventory);
when(event.getClick()).thenReturn(ClickType.LEFT);
when(event.getCurrentItem()).thenReturn(new ItemStack(Material.DIAMOND));

assertDoesNotThrow(()->manager.onInventoryClick(event));

verifyNoInteractions(storage);
}

@Test void accessToLeftHalfDoesNotAllowOpeningForeignRightHalfButStaffCanBypassSilently() {
var chest=doubleChest();
var left=lock(chest.block(),player.getUniqueId());
Expand Down Expand Up @@ -278,7 +309,7 @@ DoubleFixture doubleChest() {
data.put(chest.left().getLocation(),new ContainerData(chest.left().getLocation(),UUID.randomUUID())); manager.onInventoryMoveItem(event); verify(event).setCancelled(true);
}
@Test void takingFromForeignContainerAlertsOnlyOptedInStaffAndThrottlesRepeats() {
var barrel=block(0,Material.BARREL); var inventory=((Container)barrel.getState()).getInventory(); var lock=lock(barrel,UUID.randomUUID());
var barrel=block(0,Material.BARREL); var inventory=TestInventories.withHolderLookup(((Container)barrel.getState()).getInventory()); var lock=lock(barrel,UUID.randomUUID());
var staff=mock(Player.class); when(staff.getUniqueId()).thenReturn(UUID.randomUUID()); when(staff.hasPermission("thievery.admin")).thenReturn(true);
var disabled=mock(Player.class); when(disabled.getUniqueId()).thenReturn(UUID.randomUUID()); when(disabled.hasPermission("thievery.admin")).thenReturn(true); var ordinary=mock(Player.class);
manager.enableFeedback(staff);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static org.mockito.Mockito.*;
import java.util.*;
import net.tfminecraft.thievery.Thievery;
import net.tfminecraft.thievery.TestInventories;
import net.tfminecraft.thievery.cache.Cache;
import net.tfminecraft.thievery.category.ItemCategory;
import net.tfminecraft.thievery.loader.CategoryLoader;
Expand All @@ -24,7 +25,7 @@ class InventoryManagerTest {
MockBukkit.mock(); points=Cache.categoryPoints; Cache.categoryPoints=30; categories=new ArrayList<>(); byId=new HashMap<>();
plugins=mockStatic(Thievery.class,RETURNS_DEEP_STUBS); var plugin=mock(Thievery.class); when(plugin.namespace()).thenReturn("thievery"); plugins.when(Thievery::getInstance).thenReturn(plugin);
player=mock(Player.class); when(player.getUniqueId()).thenReturn(UUID.randomUUID()); data=new PlayerData(player.getUniqueId()); when(Thievery.getPlayerManager().get(player)).thenReturn(data);
view=mock(InventoryView.class); when(player.getOpenInventory()).thenReturn(view); doAnswer(call->{inventory=call.getArgument(0); when(view.getTopInventory()).thenReturn(inventory); return view;}).when(player).openInventory(any(Inventory.class));
view=mock(InventoryView.class); when(player.getOpenInventory()).thenReturn(view); doAnswer(call->{inventory=TestInventories.withHolderLookup(call.getArgument(0)); when(view.getTopInventory()).thenReturn(inventory); return view;}).when(player).openInventory(any(Inventory.class));
loader=mockStatic(CategoryLoader.class); loader.when(CategoryLoader::getLoadoutCategories).thenReturn(categories); loader.when(()->CategoryLoader.getById(anyString())).thenAnswer(call->byId.get(call.getArgument(0))); manager=new InventoryManager();
}
@AfterEach void close() { loader.close(); plugins.close(); Cache.categoryPoints=points; MockBukkit.unmock(); }
Expand All @@ -33,7 +34,7 @@ ItemCategory category(String id,int cost) {
when(category.getIconItem(anyBoolean())).thenAnswer(call->{var icon=new ItemStack(Material.PAPER); var meta=icon.getItemMeta(); meta.getPersistentDataContainer().set(Keys.categoryId,PersistentDataType.STRING,id); meta.setDisplayName(id+(call.getArgument(0,Boolean.class)?" active":" inactive")); icon.setItemMeta(meta); return icon;});
categories.add(category); byId.put(id,category); return category;
}
InventoryClickEvent click(int slot) { var event=mock(InventoryClickEvent.class); when(event.getWhoClicked()).thenReturn(player); when(event.getView()).thenReturn(view); when(event.getClickedInventory()).thenReturn(inventory); when(event.getSlot()).thenReturn(slot); if(slot>=0 && slot<inventory.getSize()) when(event.getCurrentItem()).thenReturn(inventory.getItem(slot)); return event; }
InventoryClickEvent click(int slot) { var event=mock(InventoryClickEvent.class); when(event.getWhoClicked()).thenReturn(player); when(event.getView()).thenReturn(view); when(event.getClickedInventory()).thenReturn(inventory); when(event.getSlot()).thenReturn(slot); if(slot>=0 && slot<inventory.getSize()) { var item=inventory.getItem(slot); when(event.getCurrentItem()).thenReturn(item); } return event; }
@Test void confirmChargesOnlyNewSelectionsAndCancelDiscardsDraft() {
category("saved",5); category("new",7); data.setActiveCategories(new ArrayList<>(List.of("saved"))); data.setPoints(10);
manager.openLoadout(player); var holder=(LoadoutHolder)inventory.getHolder(); assertEquals(player.getUniqueId(),holder.getPlayerId()); assertEquals(0,holder.getPage()); assertNull(holder.getInventory()); assertEquals(54,inventory.getSize());
Expand Down Expand Up @@ -62,8 +63,8 @@ ItemCategory category(String id,int cost) {
}
@Test void otherOwnersAndOtherMenusAreIgnoredByClickAndCloseHandlers() {
category("one",1); manager.openLoadout(player); var event=click(0); var close=mock(InventoryCloseEvent.class); when(close.getPlayer()).thenReturn(player); when(close.getView()).thenReturn(view);
var other=MockBukkit.getMock().createInventory(new LoadoutHolder(UUID.randomUUID(),0),54); when(view.getTopInventory()).thenReturn(other); manager.onInventoryClick(event); manager.onInventoryClose(close); verify(event,never()).setCancelled(true);
when(view.getTopInventory()).thenReturn(MockBukkit.getMock().createInventory(null,9)); manager.onInventoryClick(event); manager.onInventoryClose(close); verify(event,never()).setCancelled(true);
var other=TestInventories.withHolderLookup(MockBukkit.getMock().createInventory(new LoadoutHolder(UUID.randomUUID(),0),54)); when(view.getTopInventory()).thenReturn(other); manager.onInventoryClick(event); manager.onInventoryClose(close); verify(event,never()).setCancelled(true);
var menu=TestInventories.withHolderLookup(MockBukkit.getMock().createInventory(null,9)); when(view.getTopInventory()).thenReturn(menu); manager.onInventoryClick(event); manager.onInventoryClose(close); verify(event,never()).setCancelled(true);
when(close.getPlayer()).thenReturn(mock(HumanEntity.class)); manager.onInventoryClose(close);
when(view.getTopInventory()).thenReturn(inventory); manager.onInventoryClick(click(0)); manager.onInventoryClick(click(49)); assertEquals(List.of("one"),data.getActiveCategories());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@ void robberyAlwaysReservesPouchSlot(int count) {
UUID id=UUID.randomUUID(); var holder=new StealGuiHolder(id,StealGuiHolder.Kind.CHEST); var player=mock(Player.class,RETURNS_DEEP_STUBS);
StealGui.updateTitle(null,holder,"new"); StealGui.updateTitle(player,holder,"new"); verify(player,never()).getOpenInventory();
when(player.isOnline()).thenReturn(true); StealGui.updateTitle(player,null,"new"); StealGui.updateTitle(player,holder,null); verify(player,never()).getOpenInventory();
var view=player.getOpenInventory(); var top=view.getTopInventory(); when(top.getHolder()).thenReturn(null); StealGui.updateTitle(player,holder,"new"); verify(view,never()).setTitle(anyString());
when(top.getHolder()).thenReturn(new StealGuiHolder(UUID.randomUUID(),StealGuiHolder.Kind.CHEST)); StealGui.updateTitle(player,holder,"new"); verify(view,never()).setTitle(anyString());
when(top.getHolder()).thenReturn(new StealGuiHolder(id,StealGuiHolder.Kind.ROBBERY)); StealGui.updateTitle(player,holder,"new"); verify(view,never()).setTitle(anyString());
when(top.getHolder()).thenReturn(holder); when(view.getTitle()).thenReturn("new"); StealGui.updateTitle(player,holder,"new"); verify(view,never()).setTitle(anyString());
var view=player.getOpenInventory(); var top=view.getTopInventory(); when(top.getHolder(false)).thenReturn(null); StealGui.updateTitle(player,holder,"new"); verify(view,never()).setTitle(anyString());
when(top.getHolder(false)).thenReturn(new StealGuiHolder(UUID.randomUUID(),StealGuiHolder.Kind.CHEST)); StealGui.updateTitle(player,holder,"new"); verify(view,never()).setTitle(anyString());
when(top.getHolder(false)).thenReturn(new StealGuiHolder(id,StealGuiHolder.Kind.ROBBERY)); StealGui.updateTitle(player,holder,"new"); verify(view,never()).setTitle(anyString());
when(top.getHolder(false)).thenReturn(holder); when(view.getTitle()).thenReturn("new"); StealGui.updateTitle(player,holder,"new"); verify(view,never()).setTitle(anyString());
when(view.getTitle()).thenReturn("old"); StealGui.updateTitle(player,holder,"new"); verify(view).setTitle("new");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,18 @@ class StealManagerTest {
var manager=new StealManager(); UUID id=UUID.randomUUID(); var player=player(id); var reference=reference(id); manager.registerSession(reference);
try(var bukkit=mockStatic(Bukkit.class)) {
manager.tickAll(); verify(reference,never()).tick(any()); bukkit.when(()->Bukkit.getPlayer(id)).thenReturn(player); when(player.isOnline()).thenReturn(false); manager.tickAll(); verify(reference,never()).tick(any()); when(player.isOnline()).thenReturn(true);
var top=player.getOpenInventory().getTopInventory(); when(top.getHolder()).thenReturn(null); manager.tickAll();
when(top.getHolder()).thenReturn(new StealGuiHolder(id,StealGuiHolder.Kind.ROBBERY)); manager.tickAll();
when(top.getHolder()).thenReturn(new StealGuiHolder(UUID.randomUUID(),StealGuiHolder.Kind.CHEST)); manager.tickAll(); verify(reference,never()).tick(any());
when(top.getHolder()).thenReturn(new StealGuiHolder(id,StealGuiHolder.Kind.CHEST)); doAnswer(call->{manager.endSession(id,false); return null;}).when(reference).tick(player); manager.tickAll(); verify(reference).tick(player); assertFalse(manager.hasSession(id));
var top=player.getOpenInventory().getTopInventory(); when(top.getHolder(false)).thenReturn(null); manager.tickAll();
when(top.getHolder(false)).thenReturn(new StealGuiHolder(id,StealGuiHolder.Kind.ROBBERY)); manager.tickAll();
when(top.getHolder(false)).thenReturn(new StealGuiHolder(UUID.randomUUID(),StealGuiHolder.Kind.CHEST)); manager.tickAll(); verify(reference,never()).tick(any());
when(top.getHolder(false)).thenReturn(new StealGuiHolder(id,StealGuiHolder.Kind.CHEST)); doAnswer(call->{manager.endSession(id,false); return null;}).when(reference).tick(player); manager.tickAll(); verify(reference).tick(player); assertFalse(manager.hasSession(id));
}
}
@Test void clickAndCloseEventsDispatchOnlyPlayerTheftInventoriesWithMatchingSessionKind() {
var manager=new StealManager(); UUID id=UUID.randomUUID(); var player=player(id); var reference=reference(id);
var click=mock(InventoryClickEvent.class,RETURNS_DEEP_STUBS); var close=mock(InventoryCloseEvent.class,RETURNS_DEEP_STUBS);
var nonplayer=mock(HumanEntity.class); when(click.getWhoClicked()).thenReturn(nonplayer); when(close.getPlayer()).thenReturn(nonplayer); manager.onInventoryClick(click); manager.onInventoryClose(close);
when(click.getWhoClicked()).thenReturn(player); when(close.getPlayer()).thenReturn(player); when(click.getView().getTopInventory().getHolder()).thenReturn(null); when(close.getView().getTopInventory().getHolder()).thenReturn(null); manager.onInventoryClick(click); manager.onInventoryClose(close);
var holder=new StealGuiHolder(id,StealGuiHolder.Kind.CHEST); when(click.getView().getTopInventory().getHolder()).thenReturn(holder); when(close.getView().getTopInventory().getHolder()).thenReturn(holder); manager.onInventoryClick(click); manager.onInventoryClose(close);
when(click.getWhoClicked()).thenReturn(player); when(close.getPlayer()).thenReturn(player); when(click.getView().getTopInventory().getHolder(false)).thenReturn(null); when(close.getView().getTopInventory().getHolder(false)).thenReturn(null); manager.onInventoryClick(click); manager.onInventoryClose(close);
var holder=new StealGuiHolder(id,StealGuiHolder.Kind.CHEST); when(click.getView().getTopInventory().getHolder(false)).thenReturn(holder); when(close.getView().getTopInventory().getHolder(false)).thenReturn(holder); manager.onInventoryClick(click); manager.onInventoryClose(close);
manager.registerSession(reference); when(reference.getKind()).thenReturn(StealGuiHolder.Kind.ROBBERY); manager.onInventoryClick(click); manager.onInventoryClose(close); verify(reference,never()).handleClick(any(),any()); verify(reference,never()).onClose(any());
when(reference.getKind()).thenReturn(StealGuiHolder.Kind.CHEST); manager.onInventoryClick(click); verify(reference).handleClick(click,player); manager.onInventoryClose(close); verify(reference).onClose(player); assertFalse(manager.hasSession(id));
assertNull(StealManager.getStealGuiHolder(null)); assertSame(holder,StealManager.getStealGuiHolder(click.getView().getTopInventory()));
Expand Down
Loading
Loading