From 9f1e74fa7c5ce8226d92a9ab02aa383111c6c461 Mon Sep 17 00:00:00 2001 From: Luck Date: Sun, 18 Jun 2023 21:38:19 +0100 Subject: [PATCH] Fix user deletion not being correctly processed on SQL backends (#3664) --- .../common/model/nodemap/RecordedNodeMap.java | 8 +++ .../implementation/sql/SqlStorage.java | 7 +++ .../common/storage/SqlStorageTest.java | 61 +++++++++++++++++++ .../standalone/StorageIntegrationTest.java | 41 ++++++++++++- 4 files changed, 114 insertions(+), 3 deletions(-) diff --git a/common/src/main/java/me/lucko/luckperms/common/model/nodemap/RecordedNodeMap.java b/common/src/main/java/me/lucko/luckperms/common/model/nodemap/RecordedNodeMap.java index e655c2ccf..40a12aaf4 100644 --- a/common/src/main/java/me/lucko/luckperms/common/model/nodemap/RecordedNodeMap.java +++ b/common/src/main/java/me/lucko/luckperms/common/model/nodemap/RecordedNodeMap.java @@ -27,6 +27,8 @@ package me.lucko.luckperms.common.model.nodemap; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableSet; +import me.lucko.luckperms.common.model.manager.group.GroupManager; +import me.lucko.luckperms.common.node.types.Inheritance; import me.lucko.luckperms.common.util.Difference; import net.luckperms.api.context.ContextSet; import net.luckperms.api.context.ImmutableContextSet; @@ -86,6 +88,12 @@ public class RecordedNodeMap implements NodeMap { } } + public Difference addDefaultNodeToChangeSet() { + Difference diff = new Difference<>(); + diff.recordChange(Difference.ChangeType.ADD, Inheritance.builder(GroupManager.DEFAULT_GROUP_NAME).build()); + return record(diff); + } + private Difference record(Difference result) { this.lock.lock(); try { diff --git a/common/src/main/java/me/lucko/luckperms/common/storage/implementation/sql/SqlStorage.java b/common/src/main/java/me/lucko/luckperms/common/storage/implementation/sql/SqlStorage.java index 81409aa9c..ee68fb342 100644 --- a/common/src/main/java/me/lucko/luckperms/common/storage/implementation/sql/SqlStorage.java +++ b/common/src/main/java/me/lucko/luckperms/common/storage/implementation/sql/SqlStorage.java @@ -399,6 +399,13 @@ public class SqlStorage implements StorageImplementation { return true; }); + // if the user only has the default group, delete their data + boolean isDefaultUser = !this.plugin.getUserManager().isNonDefaultUser(user); + if (changes != null && isDefaultUser) { + user.normalData().addDefaultNodeToChangeSet(); + changes = null; + } + if (changes == null) { try (Connection c = this.connectionFactory.getConnection()) { deleteUser(c, user.getUniqueId()); diff --git a/common/src/test/java/me/lucko/luckperms/common/storage/SqlStorageTest.java b/common/src/test/java/me/lucko/luckperms/common/storage/SqlStorageTest.java index bb9836882..bcd45e79e 100644 --- a/common/src/test/java/me/lucko/luckperms/common/storage/SqlStorageTest.java +++ b/common/src/test/java/me/lucko/luckperms/common/storage/SqlStorageTest.java @@ -28,20 +28,31 @@ package me.lucko.luckperms.common.storage; import com.google.common.collect.ImmutableSet; import me.lucko.luckperms.common.actionlog.Log; import me.lucko.luckperms.common.actionlog.LoggedAction; +import me.lucko.luckperms.common.config.ConfigKeys; +import me.lucko.luckperms.common.config.LuckPermsConfiguration; import me.lucko.luckperms.common.event.EventDispatcher; import me.lucko.luckperms.common.model.Group; +import me.lucko.luckperms.common.model.PrimaryGroupHolder; +import me.lucko.luckperms.common.model.User; import me.lucko.luckperms.common.model.manager.group.GroupManager; import me.lucko.luckperms.common.model.manager.group.StandardGroupManager; +import me.lucko.luckperms.common.model.manager.user.StandardUserManager; +import me.lucko.luckperms.common.model.manager.user.UserManager; +import me.lucko.luckperms.common.node.types.Inheritance; import me.lucko.luckperms.common.node.types.Permission; import me.lucko.luckperms.common.plugin.LuckPermsPlugin; import me.lucko.luckperms.common.plugin.bootstrap.LuckPermsBootstrap; +import me.lucko.luckperms.common.plugin.scheduler.SchedulerAdapter; import me.lucko.luckperms.common.storage.implementation.sql.SqlStorage; import me.lucko.luckperms.common.storage.implementation.sql.connection.ConnectionFactory; import me.lucko.luckperms.common.storage.implementation.sql.connection.file.NonClosableConnection; import net.luckperms.api.actionlog.Action; import net.luckperms.api.model.PlayerSaveResult; import net.luckperms.api.model.PlayerSaveResult.Outcome; +import net.luckperms.api.model.data.DataType; import net.luckperms.api.node.Node; +import net.luckperms.api.node.types.InheritanceNode; +import net.luckperms.api.node.types.PermissionNode; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -58,6 +69,7 @@ import java.util.concurrent.TimeUnit; import java.util.function.Function; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; @@ -66,18 +78,25 @@ import static org.mockito.AdditionalAnswers.answer; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) public class SqlStorageTest { @Mock private LuckPermsPlugin plugin; @Mock private LuckPermsBootstrap bootstrap; + @Mock private LuckPermsConfiguration configuration; private SqlStorage storage; @BeforeEach public void setupMocksAndDatabase() throws Exception { lenient().when(this.plugin.getBootstrap()).thenReturn(this.bootstrap); + lenient().when(this.plugin.getConfiguration()).thenReturn(this.configuration); + lenient().when(this.plugin.getEventDispatcher()).thenReturn(mock(EventDispatcher.class)); + lenient().when(this.bootstrap.getScheduler()).thenReturn(mock(SchedulerAdapter.class)); + lenient().when(this.configuration.get(ConfigKeys.PRIMARY_GROUP_CALCULATION)).thenReturn(PrimaryGroupHolder.AllParentsByWeight::new); + lenient().when(this.configuration.get(ConfigKeys.PRIMARY_GROUP_CALCULATION_METHOD)).thenReturn("parents-by-weight"); lenient().when(this.bootstrap.getResourceStream(anyString())) .then(answer((String path) -> SqlStorageTest.class.getClassLoader().getResourceAsStream(path))); lenient().when(this.plugin.getEventDispatcher()).thenReturn(mock(EventDispatcher.class)); @@ -197,6 +216,48 @@ public class SqlStorageTest { assertEquals(nodes, loaded.normalData().asSet()); } + @Test + public void testSaveAndDeleteUser() throws SQLException { + StandardUserManager userManager = new StandardUserManager(this.plugin); + + //noinspection unchecked,rawtypes + when(this.plugin.getUserManager()).thenReturn((UserManager) userManager); + + UUID exampleUniqueId = UUID.fromString("069a79f4-44e9-4726-a5be-fca90e38aaf5"); + String exampleUsername = "Notch"; + PermissionNode examplePermission = Permission.builder() + .permission("test.1") + .withContext("server", "test") + .build(); + InheritanceNode defaultGroupNode = Inheritance.builder(GroupManager.DEFAULT_GROUP_NAME).build(); + + // create a default user, assert that is doesn't appear in unique users list + this.storage.savePlayerData(exampleUniqueId, exampleUsername); + assertFalse(this.storage.getUniqueUsers().contains(exampleUniqueId)); + + // give the user a node, assert that it does appear in unique users list + User user = this.storage.loadUser(exampleUniqueId, exampleUsername); + user.setNode(DataType.NORMAL, examplePermission, true); + this.storage.saveUser(user); + assertTrue(this.storage.getUniqueUsers().contains(exampleUniqueId)); + + // clear all nodes (reset to default) and assert that it does not appear in unique users list + user.clearNodes(DataType.NORMAL, null, true); + this.storage.saveUser(user); + assertFalse(this.storage.getUniqueUsers().contains(exampleUniqueId)); + assertEquals(ImmutableSet.of(defaultGroupNode), user.normalData().asSet()); + + // give it a node again, assert that it shows as a unique user + user.setNode(DataType.NORMAL, examplePermission, true); + this.storage.saveUser(user); + assertTrue(this.storage.getUniqueUsers().contains(exampleUniqueId)); + assertEquals(ImmutableSet.of(defaultGroupNode, examplePermission), user.normalData().asSet()); + + // reload user data from the db and assert that it is unchanged + user = this.storage.loadUser(exampleUniqueId, exampleUsername); + assertEquals(ImmutableSet.of(defaultGroupNode, examplePermission), user.normalData().asSet()); + } + private static class TestH2ConnectionFactory implements ConnectionFactory { private final NonClosableConnection connection; diff --git a/standalone/src/test/java/me/lucko/luckperms/standalone/StorageIntegrationTest.java b/standalone/src/test/java/me/lucko/luckperms/standalone/StorageIntegrationTest.java index da8bb281b..9a198c7ff 100644 --- a/standalone/src/test/java/me/lucko/luckperms/standalone/StorageIntegrationTest.java +++ b/standalone/src/test/java/me/lucko/luckperms/standalone/StorageIntegrationTest.java @@ -62,7 +62,11 @@ import java.time.ZoneOffset; import java.util.Map; import java.util.UUID; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; @Testcontainers public class StorageIntegrationTest { @@ -119,8 +123,10 @@ public class StorageIntegrationTest { // try to create / save a user UUID exampleUniqueId = UUID.fromString("c1d60c50-70b5-4722-8057-87767557e50d"); - plugin.getStorage().savePlayerData(exampleUniqueId, "Luck").join(); - User user = plugin.getStorage().loadUser(exampleUniqueId, "Luck").join(); + String exampleUsername = "Luck"; + + plugin.getStorage().savePlayerData(exampleUniqueId, exampleUsername).join(); + User user = plugin.getStorage().loadUser(exampleUniqueId, exampleUsername).join(); user.setNode(DataType.NORMAL, TEST_PERMISSION_1, true); user.setNode(DataType.NORMAL, TEST_PERMISSION_2, true); user.setNode(DataType.NORMAL, TEST_GROUP, true); @@ -137,6 +143,35 @@ public class StorageIntegrationTest { User testUser = plugin.getStorage().loadUser(exampleUniqueId, null).join(); assertNotNull(testUser); assertEquals(ImmutableSet.of(Inheritance.builder("default").build(), TEST_PERMISSION_1, TEST_PERMISSION_2, TEST_GROUP, TEST_PREFIX, TEST_META), testUser.normalData().asSet()); + assertTrue(exampleUsername.equalsIgnoreCase(testUser.getUsername().orElse("unknown"))); + + + // create another user + UUID otherExampleUniqueId = UUID.fromString("069a79f4-44e9-4726-a5be-fca90e38aaf5"); + String otherExampleUsername = "Notch"; + + plugin.getStorage().savePlayerData(otherExampleUniqueId, otherExampleUsername).join(); + assertFalse(plugin.getStorage().getUniqueUsers().join().contains(otherExampleUniqueId)); + + User otherUser = plugin.getStorage().loadUser(otherExampleUniqueId, otherExampleUsername).join(); + otherUser.setNode(DataType.NORMAL, TEST_PERMISSION_1, true); + plugin.getStorage().saveUser(otherUser).join(); + assertTrue(plugin.getStorage().getUniqueUsers().join().contains(otherExampleUniqueId)); + + otherUser.clearNodes(DataType.NORMAL, null, true); + plugin.getStorage().saveUser(otherUser).join(); + assertFalse(plugin.getStorage().getUniqueUsers().join().contains(otherExampleUniqueId)); + + + // test uuid/username lookup + assertEquals(otherExampleUniqueId, plugin.getStorage().getPlayerUniqueId(otherExampleUsername).join()); + assertTrue(otherExampleUsername.equalsIgnoreCase(plugin.getStorage().getPlayerName(otherExampleUniqueId).join())); + + plugin.getStorage().deletePlayerData(otherExampleUniqueId).join(); + assertNull(plugin.getStorage().getPlayerUniqueId(otherExampleUsername).join()); + assertNull(plugin.getStorage().getPlayerName(otherExampleUniqueId).join()); + assertNull(plugin.getStorage().getPlayerUniqueId("example").join()); + assertNull(plugin.getStorage().getPlayerName(UUID.randomUUID()).join()); } @Nested