From c767067aa6d97e571f5e2036b4f2574e9c44091f Mon Sep 17 00:00:00 2001 From: Ben Date: Sat, 24 Oct 2026 22:59:05 -0600 Subject: [PATCH] Unset removed permissions and retire empty attachments --- .../api/permissions/PermissionHandler.java | 14 +- .../permissions/PlayerPermissionHandler.java | 3 +- .../ExplicitPermissionRemovalTest.java | 190 ++++++++++++++++++ .../PermissionAttachmentRetirementTest.java | 175 ++++++++++++++++ 4 files changed, 377 insertions(+), 5 deletions(-) create mode 100644 AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/api/permissions/ExplicitPermissionRemovalTest.java create mode 100644 AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/api/permissions/PermissionAttachmentRetirementTest.java diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java index e5502f55e..85f8d6441 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PermissionHandler.java @@ -332,9 +332,17 @@ void removePermission(UUID uuid, PlayerPermissionHandler expected) { void removePermissionIfEmpty(UUID uuid, PlayerPermissionHandler expected, boolean attachmentIsOffline) { synchronized (stateLock(uuid)) { - if (!expected.isHandlerEmpty(attachmentIsOffline)) return; - perms.remove(uuid, expected); - permsToAdd.remove(uuid, expected); + synchronized (expected) { + if (!expected.isHandlerEmpty(attachmentIsOffline)) return; + boolean removedOnline = perms.remove(uuid, expected); + permsToAdd.remove(uuid, expected); + if (removedOnline && !attachmentIsOffline) { + PermissionAttachment attachment = expected.getAttachment(); + expected.setAttachment(null); + // Retire the old owner before Bukkit callbacks can grant a replacement. + if (attachment != null) attachment.remove(); + } + } } } diff --git a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java index 658d7a71b..517030240 100644 --- a/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java +++ b/AdvancedCore/src/main/java/com/bencodez/advancedcore/api/permissions/PlayerPermissionHandler.java @@ -248,8 +248,7 @@ public void removePermission(String perm) { if (permsToAdd != null) permsToAdd.remove(perm); if (attachment != null) { - attachment.setPermission(perm, false); - attachment.getPermissions().remove(perm); + attachment.unsetPermission(perm); } removeHandler = isHandlerEmpty(false); diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/api/permissions/ExplicitPermissionRemovalTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/api/permissions/ExplicitPermissionRemovalTest.java new file mode 100644 index 000000000..2e1ac0379 --- /dev/null +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/api/permissions/ExplicitPermissionRemovalTest.java @@ -0,0 +1,190 @@ +package com.bencodez.advancedcore.tests.api.permissions; + +import static com.bencodez.advancedcore.tests.api.permissions.PermissionAccess.expirePermission; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.bukkit.entity.Player; +import org.bukkit.permissions.Permissible; +import org.bukkit.permissions.PermissionAttachment; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import com.bencodez.advancedcore.AdvancedCorePlugin; +import com.bencodez.advancedcore.api.permissions.PermissionHandler; +import com.bencodez.advancedcore.api.permissions.PlayerPermissionHandler; +import com.bencodez.simpleapi.time.ParsedDuration; + +/** Exercises the public removal route with real Bukkit attachment copies and manager maps. */ +class ExplicitPermissionRemovalTest { + private AdvancedCorePlugin plugin; + private PermissionHandler manager; + private final UUID uuid = UUID.randomUUID(); + private final List permissibles = new ArrayList<>(); + + @BeforeEach + void setUp() { + plugin = mock(AdvancedCorePlugin.class, RETURNS_DEEP_STUBS); + when(plugin.isEnabled()).thenReturn(true); + when(plugin.getServerDataFile().getData()).thenReturn(null); + manager = new PermissionHandler(plugin); + } + + @AfterEach + void close() throws InterruptedException { + manager.getTimer().shutdownNow(); + assertTrue(manager.getTimer().awaitTermination(2, TimeUnit.SECONDS)); + } + + private PermissionAttachment attachment() { + Permissible permissible = mock(Permissible.class); + permissibles.add(permissible); + return new PermissionAttachment(plugin, permissible); + } + + private PlayerPermissionHandler online(PermissionAttachment attachment) { + PlayerPermissionHandler handler = new PlayerPermissionHandler(uuid, attachment, manager); + manager.getPerms().put(uuid, handler); + return handler; + } + + @Test + void removingTheOnlyTimedGrantUnsetsItsLiveNodeAndReclaimsTheHandler() { + assertSoleGrantRemoval(true); + } + + @Test + void removingTheOnlyPermanentGrantUnsetsItsLiveNodeAndReclaimsTheHandler() { + assertSoleGrantRemoval(false); + } + + private void assertSoleGrantRemoval(boolean timed) { + PermissionAttachment attachment = attachment(); + PlayerPermissionHandler handler = online(attachment); + if (timed) handler.addExpiration("example.use", ParsedDuration.ofMillis(60_000)); + else handler.addPerm("example.use"); + clearInvocations(permissibles.getFirst()); + + manager.removePermission(uuid, null, "example.use"); + + assertTrue(attachment.getPermissions().isEmpty()); + assertTrue(handler.getPersistentPermissions().isEmpty()); + assertTrue(handler.getTimedPermissions() == null || handler.getTimedPermissions().isEmpty()); + assertFalse(manager.getPerms().containsKey(uuid)); + assertFalse(manager.getPermsToAdd().containsKey(uuid)); + verify(permissibles.getFirst()).recalculatePermissions(); + } + + @Test + void removingOneGrantPreservesUnrelatedLiveAndTrackedGrants() { + PermissionAttachment attachment = attachment(); + PlayerPermissionHandler handler = online(attachment); + handler.addPerm("removed.use").addPerm("remaining.use"); + + manager.removePermission(uuid, null, "removed.use"); + + assertEquals(Map.of("remaining.use", true), attachment.getPermissions()); + assertEquals(java.util.Set.of("remaining.use"), handler.getPersistentPermissions()); + assertSame(handler, manager.getPerms().get(uuid)); + } + + @Test + void removingUnknownPermissionDoesNotIntroduceADenial() { + PermissionAttachment attachment = attachment(); + PlayerPermissionHandler handler = online(attachment).addPerm("remaining.use"); + + manager.removePermission(uuid, null, "unknown.use"); + + assertEquals(Map.of("remaining.use", true), attachment.getPermissions()); + assertSame(handler, manager.getPerms().get(uuid)); + } + + @Test + void removingPipeSeparatedGrantsReclaimsTheSameHandler() { + PermissionAttachment attachment = attachment(); + online(attachment).addPerm("first.use").addPerm("second.use"); + + manager.removePermission(uuid, "SyntheticPlayer", "first.use|second.use"); + + assertTrue(attachment.getPermissions().isEmpty()); + assertFalse(manager.getPerms().containsKey(uuid)); + } + + @Test + void queuedOfflineRemovalDoesNotResurrectTheGrantOnLogin() { + PlayerPermissionHandler handler = new PlayerPermissionHandler(uuid, null, manager) + .addOfflinePerm("removed.use", ParsedDuration.ofMillis(60_000)) + .addOfflinePerm("remaining.use", ParsedDuration.empty()); + manager.getPermsToAdd().put(uuid, handler); + + manager.removePermission(uuid, null, "removed.use"); + + assertSame(handler, manager.getPermsToAdd().get(uuid)); + PermissionAttachment attachment = attachment(); + handler.setAttachment(attachment); + handler.onLogin(mock(Player.class)); + assertEquals(Map.of("remaining.use", true), attachment.getPermissions()); + assertTrue(handler.getTimedPermissions() == null || handler.getTimedPermissions().isEmpty()); + } + + @Test + void removingTheOnlyQueuedOfflineGrantReclaimsItsManagerEntry() { + PlayerPermissionHandler handler = new PlayerPermissionHandler(uuid, null, manager) + .addOfflinePerm("removed.use", ParsedDuration.empty()); + manager.getPermsToAdd().put(uuid, handler); + + manager.removePermission(uuid, null, "removed.use"); + + assertFalse(manager.getPermsToAdd().containsKey(uuid)); + PermissionAttachment attachment = attachment(); + handler.setAttachment(attachment); + handler.onLogin(mock(Player.class)); + assertTrue(attachment.getPermissions().isEmpty()); + } + + @Test + void staleExpirationAfterRemovalCannotAffectAReplacementGrant() { + PermissionAttachment oldAttachment = attachment(); + PlayerPermissionHandler old = online(oldAttachment) + .addExpiration("example.use", ParsedDuration.ofMillis(60_000)); + long oldExpiry = old.getTimedPermissions().get("example.use"); + manager.removePermission(uuid, null, "example.use"); + PermissionAttachment replacementAttachment = attachment(); + PlayerPermissionHandler replacement = online(replacementAttachment) + .addExpiration("example.use", ParsedDuration.ofMillis(120_000)); + long replacementExpiry = replacement.getTimedPermissions().get("example.use"); + + expirePermission(old, "example.use", oldExpiry, true); + + assertSame(replacement, manager.getPerms().get(uuid)); + assertEquals(replacementExpiry, replacement.getTimedPermissions().get("example.use")); + assertEquals(Map.of("example.use", true), replacementAttachment.getPermissions()); + } + + @Test + void cleanupCannotRemoveAReplacementPublishedDuringAttachmentRecalculation() { + PermissionAttachment oldAttachment = attachment(); + PlayerPermissionHandler old = online(oldAttachment).addPerm("old.use"); + PlayerPermissionHandler replacement = new PlayerPermissionHandler(uuid, attachment(), manager) + .addPerm("replacement.use"); + AtomicBoolean replace = new AtomicBoolean(true); + doAnswer(call -> { + if (replace.compareAndSet(true, false)) manager.getPerms().put(uuid, replacement); + return null; + }).when(permissibles.getFirst()).recalculatePermissions(); + + manager.removePermission(uuid, null, "old.use"); + + assertSame(replacement, manager.getPerms().get(uuid)); + assertTrue(old.getPersistentPermissions().isEmpty()); + assertEquals(Map.of("replacement.use", true), replacement.getAttachment().getPermissions()); + } +} diff --git a/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/api/permissions/PermissionAttachmentRetirementTest.java b/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/api/permissions/PermissionAttachmentRetirementTest.java new file mode 100644 index 000000000..3ae92a145 --- /dev/null +++ b/AdvancedCore/src/test/java/com/bencodez/advancedcore/tests/api/permissions/PermissionAttachmentRetirementTest.java @@ -0,0 +1,175 @@ +package com.bencodez.advancedcore.tests.api.permissions; + +import static com.bencodez.advancedcore.tests.api.permissions.PermissionAccess.*; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.util.List; +import java.util.UUID; +import java.util.concurrent.TimeUnit; + +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.bukkit.entity.Player; +import org.bukkit.permissions.PermissibleBase; +import org.bukkit.permissions.PermissionAttachment; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +import com.bencodez.advancedcore.AdvancedCorePlugin; +import com.bencodez.advancedcore.api.permissions.PermissionHandler; +import com.bencodez.advancedcore.api.permissions.PlayerPermissionHandler; +import com.bencodez.simpleapi.time.ParsedDuration; + +/** Real Bukkit registration catches empty attachments invisible to effective-permission assertions. */ +class PermissionAttachmentRetirementTest { + private final UUID uuid = UUID.randomUUID(); + private MockedStatic bukkit; + private AdvancedCorePlugin plugin; + private PermissionHandler manager; + private PermissibleBase permissible; + private Player player; + + @BeforeEach + void setUp() { + plugin = mock(AdvancedCorePlugin.class, RETURNS_DEEP_STUBS); + when(plugin.isEnabled()).thenReturn(true); + when(plugin.getServerDataFile().getData()).thenReturn(null); + Server server = mock(Server.class, RETURNS_DEEP_STUBS); + player = mock(Player.class); + when(player.getUniqueId()).thenReturn(uuid); + bukkit = mockStatic(Bukkit.class); + bukkit.when(Bukkit::getServer).thenReturn(server); + bukkit.when(() -> Bukkit.getPlayer(uuid)).thenReturn(player); + permissible = new PermissibleBase(null); + when(player.addAttachment(plugin)).thenAnswer(call -> permissible.addAttachment(plugin)); + doAnswer(call -> { permissible.removeAttachment(call.getArgument(0)); return null; }) + .when(player).removeAttachment(any(PermissionAttachment.class)); + manager = new PermissionHandler(plugin); + } + + @AfterEach + void close() throws InterruptedException { + try { + if (manager != null) { + manager.getTimer().shutdownNow(); + assertTrue(manager.getTimer().awaitTermination(2, TimeUnit.SECONDS)); + } + } finally { + if (bukkit != null) bukkit.close(); + } + } + + private List registeredAttachments() throws ReflectiveOperationException { + var field = PermissibleBase.class.getDeclaredField("attachments"); + field.setAccessible(true); + return List.copyOf((List) field.get(permissible)); + } + + @Test + void repeatedPermanentAndTimedPublicRemovalsReleaseRegisteredAttachments() throws Exception { + for (ParsedDuration duration : List.of(ParsedDuration.empty(), ParsedDuration.ofMillis(60_000))) { + for (int cycle = 0; cycle < 3; cycle++) { + manager.addPermission(uuid, "example.use", duration); + PlayerPermissionHandler old = manager.getPerms().get(uuid); + assertEquals(1, registeredAttachments().size()); + assertTrue(permissible.hasPermission("example.use")); + + manager.removePermission(uuid, null, "example.use"); + + assertTrue(registeredAttachments().isEmpty(), "empty attachments must not accumulate"); + assertFalse(permissible.isPermissionSet("example.use")); + assertFalse(manager.getPerms().containsKey(uuid)); + assertNull(old.getAttachment()); + } + } + } + + @Test + void removalPreservesAnotherAttachmentsGrantInsteadOfInstallingADenial() throws Exception { + PermissionAttachment other = permissible.addAttachment(plugin, "example.use", true); + manager.addPermission(uuid, "example.use"); + + manager.removePermission(uuid, null, "example.use"); + + assertEquals(List.of(other), registeredAttachments()); + assertTrue(permissible.hasPermission("example.use")); + } + + @Test + void removingOneOfTwoGrantsKeepsTheRegisteredAttachment() throws Exception { + manager.addPermission(uuid, "removed.use|remaining.use"); + PlayerPermissionHandler current = manager.getPerms().get(uuid); + + manager.removePermission(uuid, null, "removed.use"); + + assertEquals(List.of(current.getAttachment()), registeredAttachments()); + assertSame(current, manager.getPerms().get(uuid)); + assertTrue(permissible.hasPermission("remaining.use")); + assertFalse(permissible.isPermissionSet("removed.use")); + } + + @Test + void owningContextExpirationAlsoReleasesItsEmptyAttachment() throws Exception { + manager.addPermission(uuid, "example.use", ParsedDuration.ofMillis(60_000)); + PlayerPermissionHandler old = manager.getPerms().get(uuid); + long expired = System.currentTimeMillis() - 1; + old.getTimedPermissions().put("example.use", expired); + + expirePermission(old, "example.use", expired, true); + + assertTrue(registeredAttachments().isEmpty()); + assertNull(old.getAttachment()); + assertFalse(manager.getPerms().containsKey(uuid)); + } + + @Test + void offlineExpirationNeverDetachesAnAttachmentFromTheWrongContext() throws Exception { + manager.addPermission(uuid, "example.use", ParsedDuration.ofMillis(60_000)); + PlayerPermissionHandler old = manager.getPerms().remove(uuid); + PermissionAttachment attachment = old.getAttachment(); + manager.getPermsToAdd().put(uuid, old); + long expired = System.currentTimeMillis() - 1; + old.getTimedPermissions().put("example.use", expired); + + expirePermission(old, "example.use", expired, false); + + assertEquals(List.of(attachment), registeredAttachments()); + assertTrue(permissible.hasPermission("example.use")); + assertFalse(manager.getPermsToAdd().containsKey(uuid)); + } + + @Test + void aRemovalCallbackCanGrantThroughANewTrackedAttachment() throws Exception { + manager.addPermission(uuid, "removed.use"); + PlayerPermissionHandler old = manager.getPerms().get(uuid); + old.getAttachment().setRemovalCallback(ignored -> manager.addPermission(uuid, "replacement.use")); + + manager.removePermission(uuid, null, "removed.use"); + + PlayerPermissionHandler replacement = manager.getPerms().get(uuid); + assertNotNull(replacement); + assertNotSame(old, replacement); + assertNull(old.getAttachment()); + assertEquals(List.of(replacement.getAttachment()), registeredAttachments()); + assertTrue(permissible.hasPermission("replacement.use")); + assertFalse(permissible.isPermissionSet("removed.use")); + } + + @Test + void staleHandlerCleanupCannotDetachAReplacementSession() throws Exception { + PlayerPermissionHandler old = new PlayerPermissionHandler(uuid, null, manager); + manager.addPermission(uuid, "replacement.use"); + PlayerPermissionHandler replacement = manager.getPerms().get(uuid); + // Even a stale wrapper retaining the new attachment must not retire its current owner. + old.setAttachment(replacement.getAttachment()); + replacement.getAttachment().unsetPermission("replacement.use"); + + removePermissionIfEmpty(manager, uuid, old, false); + + assertSame(replacement, manager.getPerms().get(uuid)); + assertEquals(List.of(replacement.getAttachment()), registeredAttachments()); + } +}