Unset removed permissions and retire empty attachments

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();
+				}
+			}
 		}
 	}
 

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);

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<Permissible> 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());
+    }
+}

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> 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());
+    }
+}