diff --git a/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java b/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java index 4b493ab8..d041f998 100644 --- a/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java +++ b/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java @@ -36,7 +36,9 @@ import java.time.Duration; import java.util.Objects; +import java.util.Set; import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; @@ -66,6 +68,11 @@ final class AgentPlayerActions * NMS-backed synthetic player implementation. */ private final IBotPlayerNmsAdapter botPlayerNmsAdapter; + /** + * UUIDs of synthetic players that joined through the FULL_LOGIN pipeline and therefore own a real TCP + * connection; their removal must run the real disconnect flow instead of the legacy player-list removal. + */ + private final Set fullLoginPlayerIds = ConcurrentHashMap.newKeySet(); /** * @param plugin @@ -368,6 +375,7 @@ private void registerJoinedPlayer(Player player, CreatePlayer.Command command) player.teleport(target); playerStore.registerSyntheticPlayer(uuid, player); + fullLoginPlayerIds.add(uuid); if (health != null) player.setHealth(Math.min(player.getMaxHealth(), health)); if (permissionsCsv != null && !permissionsCsv.isBlank()) @@ -393,9 +401,7 @@ RemovePlayer.Response handleRemovePlayer(RemovePlayer.Command command) mainThreadExecutor.callOnMainThread(() -> { final Player player = playerStore.getRequiredPlayer(uuid); - playerStore.removePermissionAttachment(uuid, player); - botPlayerNmsAdapter.removePlayer(player); - playerStore.removeSyntheticPlayer(uuid); + removeRegisteredPlayer(uuid, player); plugin.getLogger().info( "LK_AGENT: Removed synthetic player '%s' (%s)." .formatted(player.getName(), player.getUniqueId()) @@ -406,6 +412,50 @@ RemovePlayer.Response handleRemovePlayer(RemovePlayer.Command command) return new RemovePlayer.Response(); } + /** + * Removes a registered synthetic player through the path matching how it joined (main thread only). + * + *

A {@code FULL_LOGIN} bot owns a real TCP connection, so it must leave through the real disconnect + * flow ({@code Player#kickPlayer}): the play-phase disconnect packet reaches the login driver, the channel + * closes, and {@code PlayerQuitEvent} fires on the regular quit path. Removing such a bot through the + * legacy player-list removal would leave its connection open — the server times it out ~30 seconds later + * and runs the removal a second time, which crashes Paper ({@code EntityScheduler}: "Already retired"). + * Legacy-spawn bots have no real connection and keep the reflective removal path. + * + *

{@code PlayerKickEvent} is cancellable on both distros, so a vetoing plugin would silently turn the + * kick into a no-op while the agent reports success — a permanent ghost bot whose driver keeps answering + * keep-alives. The disconnect completes synchronously inside {@code kickPlayer} on both jars, so the + * {@code isOnline()} probe below reliably reflects the outcome; a vetoed kick fails loud BEFORE any agent + * state is mutated, keeping the removal cleanly retryable. + * + * @param uuid + * UUID of the player to remove. + * @param player + * The registered player instance. + * @throws AgentProtocolException + * If a plugin cancelled the {@code PlayerKickEvent} of a full-login bot's removal. + */ + private void removeRegisteredPlayer(UUID uuid, Player player) + { + if (fullLoginPlayerIds.contains(uuid)) + { + player.kickPlayer("LightKeeper bot removed."); + if (player.isOnline()) + throw new AgentProtocolException( + AgentErrorCode.REQUEST_FAILED, + ("PlayerKickEvent was cancelled by a plugin: full-login bot '%s' (%s) is still online and " + + "was NOT removed.").formatted(player.getName(), uuid)); + fullLoginPlayerIds.remove(uuid); + playerStore.removePermissionAttachment(uuid, player); + } + else + { + playerStore.removePermissionAttachment(uuid, player); + botPlayerNmsAdapter.removePlayer(player); + } + playerStore.removeSyntheticPlayer(uuid); + } + /** * Handles {@code EXECUTE_PLAYER_COMMAND} by dispatching the command in the player's execution context. * @@ -638,9 +688,7 @@ void cleanupSyntheticPlayers() try { final Player player = playerStore.getRequiredPlayer(uuid); - playerStore.removePermissionAttachment(uuid, player); - botPlayerNmsAdapter.removePlayer(player); - playerStore.removeSyntheticPlayer(uuid); + removeRegisteredPlayer(uuid, player); } catch (Exception exception) { diff --git a/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java b/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java index a01698c9..56e2f36e 100644 --- a/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java +++ b/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java @@ -20,6 +20,7 @@ import nl.pim16aap2.lightkeeper.protocol.MutatePlayerPermission; import nl.pim16aap2.lightkeeper.protocol.PlacePlayerBlock; import nl.pim16aap2.lightkeeper.protocol.PlayerChat; +import nl.pim16aap2.lightkeeper.protocol.RemovePlayer; import nl.pim16aap2.lightkeeper.protocol.RightClickBlock; import nl.pim16aap2.lightkeeper.protocol.TeleportPlayer; import org.bukkit.Bukkit; @@ -28,12 +29,15 @@ import org.bukkit.block.Block; import org.bukkit.block.BlockFace; import org.bukkit.entity.Player; +import org.bukkit.event.Listener; import org.bukkit.event.block.Action; import org.bukkit.event.player.PlayerDropItemEvent; import org.bukkit.event.player.PlayerInteractEvent; +import org.bukkit.event.player.PlayerJoinEvent; import org.bukkit.inventory.ItemStack; import org.bukkit.inventory.PlayerInventory; import org.bukkit.permissions.PermissionAttachment; +import org.bukkit.plugin.EventExecutor; import org.bukkit.plugin.PluginManager; import org.bukkit.plugin.java.JavaPlugin; import org.bukkit.scheduler.BukkitScheduler; @@ -672,17 +676,117 @@ void handleCreatePlayer_shouldSurfaceMissingJoinEventAsTypedTimeout() } } + @Test + void handleRemovePlayer_shouldKickFullLoginBotInsteadOfLegacyRemoval() + throws Exception + { + // setup — drive a successful FULL_LOGIN join (the stubbed plugin manager fires PlayerJoinEvent on + // registration) so the bot is tracked as owning a real connection. + final PlayerActionsFixture fixture = createPlayerActionsFixture(); + final IBotLoginDriver loginDriver = mock(); + when(fixture.nmsAdapter().loginDriver()).thenReturn(loginDriver); + when(loginDriver.login(any())).thenReturn(new IBotLoginOutcome.Joined("fullbot")); + final UUID uuid = UUID.randomUUID(); + final Player player = mock(); + when(player.getUniqueId()).thenReturn(uuid); + when(player.getName()).thenReturn("fullbot"); + final CreatePlayer.Command createCommand = new CreatePlayer.Command( + "request-full-rm", "fullbot", null, "world", null, null, null, null, null, JoinMode.FULL_LOGIN, null); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) + { + stubFullLoginBukkitStatics(bukkit, joinEventFiringPluginManager(player)); + final CreatePlayer.Response created = fixture.playerActions().handleCreatePlayer(createCommand); + assertThat(created.uuid()).isEqualTo(uuid); + + // execute + fixture.playerActions().handleRemovePlayer(new RemovePlayer.Command("request-remove-full", uuid)); + + // verify — the bot leaves through the real disconnect flow (kick), never the legacy player-list + // removal that would leave its TCP connection open (Paper crashes on the second removal when the + // server times that connection out). + verify(player).kickPlayer(anyString()); + verify(fixture.nmsAdapter(), never()).removePlayer(any()); + } + } + + @Test + void handleRemovePlayer_shouldFailLoudWhenKickOfFullLoginBotIsCancelled() + throws Exception + { + // setup — a kick-cancelling plugin (PlayerKickEvent is cancellable on both distros) leaves the bot + // online after kickPlayer returns; the agent must fail loud instead of reporting a successful removal. + final PlayerActionsFixture fixture = createPlayerActionsFixture(); + final IBotLoginDriver loginDriver = mock(); + when(fixture.nmsAdapter().loginDriver()).thenReturn(loginDriver); + when(loginDriver.login(any())).thenReturn(new IBotLoginOutcome.Joined("fullbot")); + final UUID uuid = UUID.randomUUID(); + final Player player = mock(); + when(player.getUniqueId()).thenReturn(uuid); + when(player.getName()).thenReturn("fullbot"); + when(player.isOnline()).thenReturn(true); + final CreatePlayer.Command createCommand = new CreatePlayer.Command( + "request-full-veto", "fullbot", null, "world", null, null, null, null, null, JoinMode.FULL_LOGIN, + null); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) + { + stubFullLoginBukkitStatics(bukkit, joinEventFiringPluginManager(player)); + fixture.playerActions().handleCreatePlayer(createCommand); + + // execute + final Throwable thrown = catchThrowable(() -> fixture.playerActions() + .handleRemovePlayer(new RemovePlayer.Command("request-remove-veto", uuid))); + + // verify — typed failure naming the veto, and no agent state mutated: the bot stays registered + // (the removal is retryable) and the legacy removal path is never used as a fallback. + assertThat(thrown) + .isInstanceOf(AgentProtocolException.class) + .satisfies(failure -> assertThat(((AgentProtocolException) failure).errorCode()) + .isEqualTo(AgentErrorCode.REQUEST_FAILED)) + .hasMessageContaining("PlayerKickEvent was cancelled"); + verify(player).kickPlayer(anyString()); + verify(fixture.nmsAdapter(), never()).removePlayer(any()); + assertThat(fixture.playerStore().getRequiredPlayer(uuid)).isSameAs(player); + } + } + + /** + * Creates a plugin-manager mock that immediately fires a {@code PlayerJoinEvent} for the given player + * whenever an event executor is registered, completing a FULL_LOGIN join's join-latch synchronously. + */ + private static PluginManager joinEventFiringPluginManager(Player player) + { + final PluginManager pluginManager = mock(); + doAnswer(invocation -> + { + final Listener listener = invocation.getArgument(1); + final EventExecutor eventExecutor = invocation.getArgument(3); + eventExecutor.execute(listener, new PlayerJoinEvent(player, "joined")); + return null; + }).when(pluginManager).registerEvent(eq(PlayerJoinEvent.class), any(), any(), any(), any()); + return pluginManager; + } + /** * Stubs the Bukkit statics a FULL_LOGIN join touches: NOT the primary thread (the handler rejects * main-thread execution), a server exposing a port, a known world named {@code "world"}, a no-op plugin * manager, and a scheduler that runs main-thread callables inline. */ private static void stubFullLoginBukkitStatics(MockedStatic bukkit) + { + stubFullLoginBukkitStatics(bukkit, mock(PluginManager.class)); + } + + /** + * Variant of {@link #stubFullLoginBukkitStatics(MockedStatic)} with a caller-supplied plugin manager, for + * tests that need event registrations to actually fire events. + */ + private static void stubFullLoginBukkitStatics(MockedStatic bukkit, PluginManager pluginManager) { final org.bukkit.Server server = mock(); when(server.getPort()).thenReturn(25_565); final World world = mock(); - final PluginManager pluginManager = mock(); final BukkitScheduler scheduler = mock(); when(scheduler.callSyncMethod(any(), any())).thenAnswer(invocation -> { diff --git a/lightkeeper-integration-tests/pom.xml b/lightkeeper-integration-tests/pom.xml index 177786f0..36ed8213 100644 --- a/lightkeeper-integration-tests/pom.xml +++ b/lightkeeper-integration-tests/pom.xml @@ -108,6 +108,15 @@ ${project.version} lightkeeper-spigot-test-plugin.jar + + + modrinth + luckperms + v5.5.53-bukkit + ${lightkeeper.minecraft-version} + LuckPerms.jar + diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java new file mode 100644 index 00000000..e5b714e9 --- /dev/null +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java @@ -0,0 +1,89 @@ +package nl.pim16aap2.lightkeeper.maven.test; + +import nl.pim16aap2.lightkeeper.framework.CapturedEventSnapshot; +import nl.pim16aap2.lightkeeper.framework.EntitySnapshot; +import nl.pim16aap2.lightkeeper.framework.Vec3; +import nl.pim16aap2.lightkeeper.framework.WorldHandle; +import nl.pim16aap2.lightkeeper.protocol.IProtocolValue; + +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Optional; +import java.util.UUID; + +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.assertThat; +import static org.assertj.core.api.Assertions.within; + +/** + * Shared assertions and lookups for the FULL_LOGIN validation-matrix integration tests. + * + *

Kept as plain static helpers so both matrix classes ({@code LightkeeperFullLoginValidationIT} and + * {@code LightkeeperFullLoginSessionIT}) assert identities, positions, and captured events the same way. + */ +final class FullLoginItSupport +{ + private FullLoginItSupport() + { + } + + /** + * Computes the offline-mode UUID the server derives from a player name during a {@code FULL_LOGIN} join. + * + * @param name + * The player name. + * @return The server-derived offline UUID. + */ + static UUID offlineUuid(String name) + { + return UUID.nameUUIDFromBytes(("OfflinePlayer:" + name).getBytes(StandardCharsets.UTF_8)); + } + + /** + * Looks up a player's current position through the live entity query. + * + * @param world + * World to query. + * @param playerId + * UUID of the player to find. + * @return The player's position, or empty when the player is not online in the world. + */ + static Optional playerPosition(WorldHandle world, UUID playerId) + { + return world.entities().ofType("minecraft:player").snapshot().stream() + .filter(snapshot -> snapshot.uuid().equals(playerId)) + .map(EntitySnapshot::position) + .findFirst(); + } + + /** + * Asserts that a position matches the expected coordinates within half a block on every axis. + * + * @param actual + * Observed position. + * @param expected + * Expected position. + */ + static void assertPositionCloseTo(Vec3 actual, Vec3 expected) + { + assertThat(actual.x()).isCloseTo(expected.x(), within(0.5)); + assertThat(actual.y()).isCloseTo(expected.y(), within(0.5)); + assertThat(actual.z()).isCloseTo(expected.z(), within(0.5)); + } + + /** + * Filters captured events down to those whose {@code getPlayer} value references the given player. + * + * @param events + * Captured event snapshots. + * @param playerId + * UUID of the acting player. + * @return The events attributed to the player, in capture order. + */ + static List eventsWithPlayerRef(List events, UUID playerId) + { + return events.stream() + .filter(event -> event.value("getPlayer") instanceof IProtocolValue.PRef playerRef + && playerRef.id().equals(playerId.toString())) + .toList(); + } +} diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java new file mode 100644 index 00000000..43fc1f3e --- /dev/null +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java @@ -0,0 +1,280 @@ +package nl.pim16aap2.lightkeeper.maven.test; + +import nl.pim16aap2.lightkeeper.framework.CapturedEventSnapshot; +import nl.pim16aap2.lightkeeper.framework.CommandResult; +import nl.pim16aap2.lightkeeper.framework.EntitySnapshot; +import nl.pim16aap2.lightkeeper.framework.ILightkeeperFramework; +import nl.pim16aap2.lightkeeper.framework.LightkeeperExtension; +import nl.pim16aap2.lightkeeper.framework.PlayerHandle; +import nl.pim16aap2.lightkeeper.framework.Vec3; +import nl.pim16aap2.lightkeeper.protocol.CommandSource; +import nl.pim16aap2.lightkeeper.protocol.IProtocolValue; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.extension.ExtendWith; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.UUID; +import java.util.stream.Collectors; + +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.assertThat; +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.eventually; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.assertPositionCloseTo; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.eventsWithPlayerRef; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.offlineUuid; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.playerPosition; + +/** + * FULL_LOGIN validation matrix, part 2: in-session behaviors of fully logged-in bots. + * + *

Covers the driver's teleport acknowledgement, login under the (default-on) compression threshold, + * multiple simultaneous sessions, the configuration-phase locale, chat parity with the S4 event path, and the + * LuckPerms Permissible-injection survival that motivated the full-login pipeline in the first place. Part 1 + * ({@link LightkeeperFullLoginValidationIT}) covers the login-event contract and the denial matrix. + */ +@ExtendWith(LightkeeperExtension.class) +class LightkeeperFullLoginSessionIT +{ + private static final String CHAT_EVENT = "org.bukkit.event.player.AsyncPlayerChatEvent"; + private static final String QUIT_EVENT = "org.bukkit.event.player.PlayerQuitEvent"; + + @Test + @Timeout(180) + void fullLogin_shouldAckTeleportAfterJoin(ILightkeeperFramework framework) + { + // setup — watch for kicks: a driver that failed to acknowledge the teleport would desync the session. + final var world = framework.worlds().main(); + final String name = "lktpack"; + final Vec3 target = new Vec3(33.0, 120.0, 33.0); + + try (var quitCapture = framework.events().capture(QUIT_EVENT)) + { + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + + // execute — the teleport makes the server send a position packet that the driver must answer with + // the teleport-accept packet for the session to stay in sync. + player.teleport(world, target); + + // verify — the position is applied server-side and the session survives the ack round trip. + eventually(Duration.ofSeconds(10), () -> + assertThat(playerPosition(world, player.uniqueId())).hasValueSatisfying(position -> + assertPositionCloseTo(position, target))); + player.andWaitTicks(20); + assertThat(eventsWithPlayerRef(quitCapture.getCapturedEvents(), player.uniqueId())).isEmpty(); + + player.remove(); + } + } + + @Test + @Timeout(180) + void fullLogin_shouldNegotiateCompressionWhenEnabled(ILightkeeperFramework framework) + throws IOException + { + // setup — the provisioner leaves the vanilla default network-compression-threshold=256 in place, so + // compression is negotiated on every login. Assert that explicitly: if provisioning ever flipped it + // to -1 (disabled), this test must fail instead of silently no longer covering the driver's + // compression path. A per-test server.properties override is not possible today: the config overlay + // is copied wholesale after the runtime-port rewrite, so overlaying server.properties would clobber + // the reserved port. + final Path serverProperties = framework.server().directory().resolve("server.properties"); + final int compressionThreshold = readCompressionThreshold(serverProperties); + assertThat(compressionThreshold) + .as("network-compression-threshold in %s (>= 0 means compression is enabled)", serverProperties) + .isGreaterThanOrEqualTo(0); + + // execute — the join must handle the login-phase compression packet; everything after it (the whole + // configuration phase, the join, and the chat below) rides the compressed framing. + final var world = framework.worlds().main(); + final String message = "compressed chat roundtrip"; + try (var chatCapture = framework.events().capture(CHAT_EVENT)) + { + final var player = framework.bots().builder() + .withName("lkcompress") + .atSpawn(world) + .fullLogin() + .build(); + player.chat(message); + + // verify + eventually(Duration.ofSeconds(10), () -> + assertThat(chatCapture.getCapturedEvents()).anySatisfy(event -> + assertThat(event.value("getMessage")).isEqualTo(new IProtocolValue.PString(message)))); + + player.remove(); + } + } + + @Test + @Timeout(180) + void fullLogin_shouldSustainFiveSimultaneousSessions(ILightkeeperFramework framework) + { + // setup — the framework transport (UdsAgentTransport.send) is synchronized: exactly one request may + // be in flight per agent connection, so wire-level concurrent joins are not deliverable through this + // framework today. The five bots therefore join back-to-back, and the assertions target what full + // logins must guarantee regardless: five simultaneously online sessions with distinct server-derived + // identities and no cross-talk between their driver sessions. + final var world = framework.worlds().main(); + final List bots = new ArrayList<>(); + + try + { + // execute + for (int index = 1; index <= 5; ++index) + { + bots.add(framework.bots().builder() + .withName("lkconc" + index) + .atSpawn(world) + .fullLogin() + .build()); + } + + // verify — five distinct offline UUIDs, each derived from its own name... + final Set uniqueIds = bots.stream().map(PlayerHandle::uniqueId).collect(Collectors.toSet()); + assertThat(uniqueIds).hasSize(5); + for (final PlayerHandle bot : bots) + assertThat(bot.uniqueId()).isEqualTo(offlineUuid(bot.name())); + + // ...all simultaneously online, observed in one internally consistent entity-query burst. + eventually(Duration.ofSeconds(10), () -> + { + final Set onlineIds = world.entities().ofType("minecraft:player").snapshot().stream() + .map(EntitySnapshot::uuid) + .collect(Collectors.toSet()); + assertThat(onlineIds).containsAll(uniqueIds); + }); + } + finally + { + for (final PlayerHandle bot : bots) + bot.remove(); + } + } + + @Test + @Timeout(180) + void fullLogin_shouldApplyRequestedLocale(ILightkeeperFramework framework) + { + // setup — the driver sends the locale inside ClientInformation during the configuration phase; the + // test plugin's /lktestlocale logs Player#getLocale, so the server-side view of the locale is + // asserted from the captured console output. + final var world = framework.worlds().main(); + final String name = "lklocale"; + + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .withLocale("de_de") + .fullLogin() + .build(); + + // execute + player.executeCommand("lktestlocale"); + + // verify + eventually(Duration.ofSeconds(10), () -> + assertThat(framework.server().output()) + .anyMatch(line -> line.contains("LK_LOCALE name=%s locale=de_de".formatted(name)))); + + player.remove(); + } + + @Test + @Timeout(180) + void fullLogin_shouldChatLikeARealClient(ILightkeeperFramework framework) + { + // setup + final var world = framework.worlds().main(); + final String name = "lkfullchat"; + final String message = "hello from a real login"; + + try (var chatCapture = framework.events().capture(CHAT_EVENT)) + { + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + + // execute — chat parity with the S4 path, now from a fully logged-in session. + player.chat(message); + + // verify — the real AsyncPlayerChatEvent fires, attributed to the full-login bot. + eventually(Duration.ofSeconds(10), () -> + assertThat(chatCapture.getCapturedEvents()).isNotEmpty()); + final CapturedEventSnapshot chatEvent = chatCapture.getCapturedEvents().getFirst(); + assertThat(chatEvent.value("getMessage")).isEqualTo(new IProtocolValue.PString(message)); + assertThat(chatEvent.value("getPlayer")) + .isInstanceOfSatisfying(IProtocolValue.PRef.class, playerRef -> + assertThat(playerRef.id()).isEqualTo(player.uniqueId().toString())); + + player.remove(); + } + } + + @Test + @Timeout(180) + void fullLogin_shouldRetainLuckPermsPermissionInjectedAtLogin(ILightkeeperFramework framework) + { + // setup — LuckPerms is provisioned by this module's pom (Modrinth, pinned version); fail fast with a + // clear reason when that wiring is missing rather than timing out on the permission check below. + assertThat(framework.server().directory().resolve("plugins").resolve("LuckPerms.jar")) + .as("LuckPerms must be provisioned for the Permissible-injection survival check") + .isRegularFile(); + + final var world = framework.worlds().main(); + final String name = "lklperm"; + final String node = "lightkeeper.test.lpnode"; + + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + assertThat(player.permissions().has(node)).isFalse(); + + try + { + // execute — grant through LuckPerms itself (console), NOT LightKeeper's own attachment. + final CommandResult grantResult = framework.server().executeCommand( + CommandSource.CONSOLE, "lp user %s permission set %s true".formatted(name, node)); + assertThat(grantResult.success()).isTrue(); + + // verify — the LIVE hasPermission resolves through the Permissible LuckPerms injected at + // PlayerLoginEvent. Had the injection not survived onto the joined player instance, the + // LuckPerms-stored node would keep reading false here. LuckPerms applies console edits + // asynchronously, hence the retry window. + eventually(Duration.ofSeconds(20), () -> + assertThat(player.permissions().has(node)).isTrue()); + } + finally + { + framework.server().executeCommand( + CommandSource.CONSOLE, "lp user %s permission unset %s".formatted(name, node)); + player.remove(); + } + } + + private static int readCompressionThreshold(Path serverProperties) + throws IOException + { + for (final String line : Files.readAllLines(serverProperties, StandardCharsets.UTF_8)) + { + final String trimmed = line.trim(); + if (trimmed.startsWith("network-compression-threshold=")) + return Integer.parseInt(trimmed.substring("network-compression-threshold=".length()).trim()); + } + throw new AssertionError("No network-compression-threshold entry in " + serverProperties); + } +} diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java new file mode 100644 index 00000000..fa60d02c --- /dev/null +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java @@ -0,0 +1,241 @@ +package nl.pim16aap2.lightkeeper.maven.test; + +import nl.pim16aap2.lightkeeper.framework.BotJoinDeniedException; +import nl.pim16aap2.lightkeeper.framework.CapturedEventSnapshot; +import nl.pim16aap2.lightkeeper.framework.CommandResult; +import nl.pim16aap2.lightkeeper.framework.ILightkeeperFramework; +import nl.pim16aap2.lightkeeper.framework.LightkeeperExtension; +import nl.pim16aap2.lightkeeper.protocol.CommandSource; +import nl.pim16aap2.lightkeeper.protocol.IProtocolValue; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.extension.ExtendWith; + +import java.time.Duration; +import java.util.List; +import java.util.Locale; +import java.util.UUID; + +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.assertThat; +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.catchThrowable; +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.eventually; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.eventsWithPlayerRef; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.offlineUuid; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.playerPosition; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * FULL_LOGIN validation matrix, part 1: login-event contract, denial matrix, and quit/rejoin identity. + * + *

These tests pin the behaviors only a real login pipeline can deliver — the documented login-event order + * on both distros, whitelist/ban denials surfaced as typed errors, and the persisted offline identity across a + * real quit/rejoin cycle. Part 2 ({@link LightkeeperFullLoginSessionIT}) covers the in-session behaviors. + */ +@ExtendWith(LightkeeperExtension.class) +class LightkeeperFullLoginValidationIT +{ + private static final String PRE_LOGIN_EVENT = "org.bukkit.event.player.AsyncPlayerPreLoginEvent"; + private static final String LOGIN_EVENT = "org.bukkit.event.player.PlayerLoginEvent"; + private static final String JOIN_EVENT = "org.bukkit.event.player.PlayerJoinEvent"; + private static final String QUIT_EVENT = "org.bukkit.event.player.PlayerQuitEvent"; + + @Test + @Timeout(180) + void fullLogin_shouldFireLoginEventsInDocumentedOrderPerDistro(ILightkeeperFramework framework) + { + // setup — register all three captures BEFORE joining so no login event can be missed. + final var world = framework.worlds().main(); + final String name = "lkevorder"; + final UUID expectedUuid = offlineUuid(name); + + try (var preLoginCapture = framework.events().capture(PRE_LOGIN_EVENT); + var loginCapture = framework.events().capture(LOGIN_EVENT); + var joinCapture = framework.events().capture(JOIN_EVENT)) + { + // execute + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + + // verify — all three login events fired exactly once for this bot. + eventually(Duration.ofSeconds(10), () -> + assertThat(eventsWithPlayerRef(joinCapture.getCapturedEvents(), expectedUuid)).hasSize(1)); + + final List preLoginEvents = preLoginCapture.getCapturedEvents().stream() + .filter(event -> new IProtocolValue.PString(name).equals(event.value("getName"))) + .toList(); + final List loginEvents = + eventsWithPlayerRef(loginCapture.getCapturedEvents(), expectedUuid); + final List joinEvents = + eventsWithPlayerRef(joinCapture.getCapturedEvents(), expectedUuid); + assertThat(preLoginEvents).hasSize(1); + assertThat(loginEvents).hasSize(1); + assertThat(joinEvents).hasSize(1); + + // The async pre-login carries the server-derived offline UUID; the login result is ALLOWED. + assertThat(preLoginEvents.getFirst().value("getUniqueId")) + .isEqualTo(new IProtocolValue.PUuid(expectedUuid)); + assertThat(loginEvents.getFirst().value("getResult")) + .isInstanceOfSatisfying(IProtocolValue.PEnum.class, result -> + assertThat(result.name()).isEqualTo("ALLOWED")); + + // Documented order — AsyncPlayerPreLoginEvent, then PlayerLoginEvent, then PlayerJoinEvent — + // asserted via tick stamps. This IS the per-distro expectation: the known Paper/Spigot divergence + // (Paper fires PlayerLoginEvent at configuration-finish, Spigot during the login phase) moves the + // login event relative to the configuration phase only, never behind the join, so the same + // ordering holds on both lanes without weakening to an unordered check. + final long preLoginTick = preLoginEvents.getFirst().tick(); + final long loginTick = loginEvents.getFirst().tick(); + final long joinTick = joinEvents.getFirst().tick(); + assertThat(preLoginTick).isLessThanOrEqualTo(loginTick); + assertThat(loginTick).isLessThanOrEqualTo(joinTick); + + player.remove(); + } + } + + @Test + @Timeout(180) + void fullLogin_shouldRejectBannedPlayerWithTypedDenial(ILightkeeperFramework framework) + { + // setup — ban the name BEFORE it ever joins; the console resolves the offline profile by name. + final var world = framework.worlds().main(); + final String name = "lkbanned"; + final String banReason = "lk-ban-reason-e2e"; + final CommandResult banResult = + framework.server().executeCommand(CommandSource.CONSOLE, "ban %s %s".formatted(name, banReason)); + assertThat(banResult.success()).isTrue(); + + try + { + // execute + verify — the join is denied as a typed error carrying the ban reason. + assertThatThrownBy(() -> framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build()) + .isInstanceOf(BotJoinDeniedException.class) + .hasMessageContaining(banReason); + } + finally + { + framework.server().executeCommand(CommandSource.CONSOLE, "pardon " + name); + } + } + + @Test + @Timeout(180) + void fullLogin_shouldRejectWhenWhitelistExcludesPlayer(ILightkeeperFramework framework) + { + // setup + final var world = framework.worlds().main(); + final String name = "lkwhite"; + assertThat(framework.server().executeCommand(CommandSource.CONSOLE, "whitelist on").success()).isTrue(); + + try + { + // execute + verify — the excluded bot is denied as a typed error. Fail-fast semantics are proven + // by the TYPE of the failure: a BotJoinDeniedException (the server's login-phase kick) instead of + // a BotJoinTimeoutException. Deliberately no wall-clock bound: elapsed-time assertions flake on + // starved CI runners. + final Throwable denial = catchThrowable(() -> framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build()); + assertThat(denial).isInstanceOf(BotJoinDeniedException.class); + // Normalized check: Spigot/Paper say "whitelisted", older vanilla strings say "white-listed". + assertThat(denial.getMessage().toLowerCase(Locale.ROOT).replace("-", "")) + .as("denial reason should name the whitelist: %s", denial.getMessage()) + .contains("whitelist"); + + // execute + verify — whitelisting the name turns the same join into a success. + framework.server().executeCommand(CommandSource.CONSOLE, "whitelist add " + name); + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + assertThat(player.uniqueId()).isEqualTo(offlineUuid(name)); + player.remove(); + } + finally + { + framework.server().executeCommand(CommandSource.CONSOLE, "whitelist remove " + name); + framework.server().executeCommand(CommandSource.CONSOLE, "whitelist off"); + } + } + + @Test + @Timeout(180) + void fullLogin_shouldRejoinAfterQuitWithSamePersistedIdentity(ILightkeeperFramework framework) + { + // setup — join, then give the bot a distinctive item that must survive the quit/rejoin cycle. The + // persistence observable is the inventory (saved to player data on the real quit path) rather than the + // position, because FULL_LOGIN honors the requested placement: the agent teleports a (re)joining bot to + // the requested location/world spawn post-join, overriding the position loaded from player data. + final var world = framework.worlds().main(); + final String name = "lkrejoin"; + final String persistedItem = "minecraft:diamond"; + + final var firstJoin = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + final UUID firstUuid = firstJoin.uniqueId(); + assertThat(framework.server() + .executeCommand(CommandSource.CONSOLE, "give %s %s 3".formatted(name, persistedItem)) + .success()).isTrue(); + eventually(Duration.ofSeconds(10), () -> + assertThat(firstJoin.inventory().findItem(persistedItem)).isNotNull()); + + // execute — quit (real quit path saves player data), then rejoin under the same name. + firstJoin.remove(); + eventually(Duration.ofSeconds(10), () -> + assertThat(playerPosition(world, firstUuid)).isEmpty()); + final var rejoined = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + + // verify — same server-derived offline identity, and the inventory was restored from player data. + assertThat(rejoined.uniqueId()).isEqualTo(firstUuid).isEqualTo(offlineUuid(name)); + eventually(Duration.ofSeconds(10), () -> + assertThat(rejoined.inventory().findItem(persistedItem)).isNotNull()); + + rejoined.remove(); + } + + @Test + @Timeout(180) + void fullLogin_shouldFireRealQuitEventOnRemoval(ILightkeeperFramework framework) + { + // setup — capture quit events BEFORE removal so the event cannot be missed. + final var world = framework.worlds().main(); + final String name = "lkquit"; + + try (var quitCapture = framework.events().capture(QUIT_EVENT)) + { + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + final UUID playerId = player.uniqueId(); + assertThat(eventsWithPlayerRef(quitCapture.getCapturedEvents(), playerId)).isEmpty(); + + // execute + player.remove(); + + // verify — removal traverses the real quit path: PlayerQuitEvent fires and the player is gone. + eventually(Duration.ofSeconds(10), () -> + assertThat(eventsWithPlayerRef(quitCapture.getCapturedEvents(), playerId)).hasSize(1)); + eventually(Duration.ofSeconds(10), () -> + assertThat(playerPosition(world, playerId)).isEmpty()); + } + } +} diff --git a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java index 9c991f1a..4dac0141 100644 --- a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java +++ b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java @@ -2,6 +2,7 @@ import lombok.AllArgsConstructor; import lombok.NoArgsConstructor; +import nl.pim16aap2.lightkeeper.maven.provisioning.LoopbackLoginGuard; import nl.pim16aap2.lightkeeper.maven.provisioning.PluginArtifactSpec; import nl.pim16aap2.lightkeeper.maven.provisioning.ResolvedPluginArtifact; import nl.pim16aap2.lightkeeper.maven.provisioning.ServerAssetInstaller; @@ -209,6 +210,9 @@ public void execute() final Path targetServerDirectory = serverProvider.targetServerDirectoryPath(); installServerAssets(targetServerDirectory, executionContext, executionContext.pluginArtifactSpecs()); + // Runs after ALL provisioning mutations (base copy, plugins, overlay): a config overlay is the one + // vector through which online-mode/proxy-forwarding could enter and silently break FULL_LOGIN joins. + LoopbackLoginGuard.validate(targetServerDirectory, getLog()); final RuntimeManifest runtimeManifest = createRuntimeManifest( executionContext.normalizedServerType(), diff --git a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java new file mode 100644 index 00000000..ec7bec00 --- /dev/null +++ b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java @@ -0,0 +1,179 @@ +package nl.pim16aap2.lightkeeper.maven.provisioning; + +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugin.logging.Log; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Locale; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Fail-loud guard against server configurations that would break {@code FULL_LOGIN} loopback logins. + * + *

LightKeeper's full-login bots complete a real offline-mode login over loopback TCP against the provisioned + * server. Three configuration switches silently break that pipeline for every bot: {@code online-mode=true} + * ({@code server.properties}) makes the server demand Mojang session authentication, and + * {@code settings.bungeecord: true} ({@code spigot.yml}) or {@code proxies.velocity.enabled: true} + * ({@code config/paper-global.yml}) make the server expect proxy-forwarded handshakes that a direct loopback + * client never sends. LightKeeper's own provisioning never writes any of these, but a user config overlay could + * flip them, so this guard validates the fully materialized target server directory — after all overlays are + * applied — and fails the build with an actionable message instead of letting every full-login join time out. + * + *

The checks are deliberate line-level scans, not a YAML parse: the goal is failing loudly on the known-bad + * switches in the standard configuration shapes those files ship with, with zero new dependencies. Commented-out + * lines are ignored; files that do not exist are skipped. + */ +public final class LoopbackLoginGuard +{ + /** + * Matches an active {@code bungeecord: true} entry in {@code spigot.yml} (the key only exists under the + * top-level {@code settings} section in stock Spigot configurations), with an optional trailing comment. + * The boolean is matched case-insensitively ({@code True}/{@code TRUE} are legal YAML booleans). + */ + private static final Pattern SPIGOT_BUNGEECORD_ENABLED_PATTERN = + Pattern.compile("^\\s*bungeecord:\\s*(?i:true)\\s*(?:#.*)?$"); + + /** + * Matches the opening of the {@code velocity} section in {@code config/paper-global.yml}, capturing its + * indentation so the guard can tell when the section ends. + */ + private static final Pattern VELOCITY_SECTION_PATTERN = Pattern.compile("^(\\s*)velocity:\\s*(?:#.*)?$"); + + /** + * Matches an active {@code enabled: true} entry (inside the {@code velocity} section), with an optional + * trailing comment. The boolean is matched case-insensitively ({@code True}/{@code TRUE} are legal YAML + * booleans). + */ + private static final Pattern VELOCITY_ENABLED_PATTERN = + Pattern.compile("^\\s*enabled:\\s*(?i:true)\\s*(?:#.*)?$"); + + private LoopbackLoginGuard() + { + } + + /** + * Validates a fully materialized target server directory for {@code FULL_LOGIN} compatibility. + * + *

Must run after every provisioning mutation (base-server copy, plugin installs, config overlay), because + * the overlay is the mechanism through which the forbidden settings could enter. + * + * @param targetServerDirectory + * The materialized server directory to validate. + * @param log + * Maven log sink for the pass confirmation. + * @throws MojoExecutionException + * If a configuration file enables online mode or proxy forwarding, or a present file cannot be read. + */ + public static void validate(Path targetServerDirectory, Log log) + throws MojoExecutionException + { + validateServerProperties(targetServerDirectory.resolve("server.properties")); + validateSpigotConfiguration(targetServerDirectory.resolve("spigot.yml")); + validatePaperGlobalConfiguration(targetServerDirectory.resolve("config").resolve("paper-global.yml")); + log.info("LK_GUARD: Loopback-login guard passed: offline mode, no proxy forwarding."); + } + + private static void validateServerProperties(Path serverPropertiesFile) + throws MojoExecutionException + { + for (final String line : readLinesIfPresent(serverPropertiesFile)) + { + final String trimmed = line.trim(); + // Java Properties comments start with '#' OR '!' (java.util.Properties spec). + if (trimmed.startsWith("#") || trimmed.startsWith("!")) + continue; + + // Split at the first '=' so the legal properties spacing 'online-mode = true' is handled too. + final int separatorIndex = trimmed.indexOf('='); + if (separatorIndex < 0 || !"online-mode".equals(trimmed.substring(0, separatorIndex).trim())) + continue; + + final String value = trimmed.substring(separatorIndex + 1).trim().toLowerCase(Locale.ROOT); + if ("true".equals(value)) + { + throw new MojoExecutionException( + ("Provisioned server configuration '%s' sets 'online-mode=true'. FULL_LOGIN bots complete a " + + "real offline-mode login over loopback TCP; online mode makes the server demand Mojang " + + "session authentication and breaks every full-login join. Remove the override " + + "(LightKeeper provisions 'online-mode=false').") + .formatted(serverPropertiesFile) + ); + } + } + } + + private static void validateSpigotConfiguration(Path spigotConfigurationFile) + throws MojoExecutionException + { + for (final String line : readLinesIfPresent(spigotConfigurationFile)) + { + if (SPIGOT_BUNGEECORD_ENABLED_PATTERN.matcher(line).matches()) + { + throw new MojoExecutionException( + ("Provisioned server configuration '%s' enables BungeeCord proxy forwarding " + + "('bungeecord: true'). The server would expect proxy-forwarded handshakes, which breaks " + + "FULL_LOGIN loopback logins. Remove the setting from the config overlay.") + .formatted(spigotConfigurationFile) + ); + } + } + } + + private static void validatePaperGlobalConfiguration(Path paperGlobalConfigurationFile) + throws MojoExecutionException + { + int velocitySectionIndent = -1; + for (final String line : readLinesIfPresent(paperGlobalConfigurationFile)) + { + final String trimmed = line.trim(); + if (trimmed.isEmpty() || trimmed.startsWith("#")) + continue; + + final int indent = line.length() - line.stripLeading().length(); + if (velocitySectionIndent >= 0 && indent <= velocitySectionIndent) + velocitySectionIndent = -1; + + final Matcher velocitySectionMatcher = VELOCITY_SECTION_PATTERN.matcher(line); + if (velocitySectionMatcher.matches()) + { + velocitySectionIndent = velocitySectionMatcher.group(1).length(); + continue; + } + + if (velocitySectionIndent >= 0 && VELOCITY_ENABLED_PATTERN.matcher(line).matches()) + { + throw new MojoExecutionException( + ("Provisioned server configuration '%s' enables Velocity proxy forwarding " + + "('proxies.velocity.enabled: true'). The server would expect proxy-forwarded handshakes, " + + "which breaks FULL_LOGIN loopback logins. Remove the setting from the config overlay.") + .formatted(paperGlobalConfigurationFile) + ); + } + } + } + + private static List readLinesIfPresent(Path configurationFile) + throws MojoExecutionException + { + if (Files.notExists(configurationFile)) + return List.of(); + + try + { + return Files.readAllLines(configurationFile, StandardCharsets.UTF_8); + } + catch (IOException exception) + { + throw new MojoExecutionException( + "Failed to read provisioned server configuration '%s' for the loopback-login guard." + .formatted(configurationFile), + exception + ); + } + } +} diff --git a/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java b/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java new file mode 100644 index 00000000..5b56f840 --- /dev/null +++ b/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java @@ -0,0 +1,204 @@ +package nl.pim16aap2.lightkeeper.maven.provisioning; + +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugin.logging.SystemStreamLog; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class LoopbackLoginGuardTest +{ + @Test + void validate_shouldPassOnDefaultProvisionedConfiguration(@TempDir Path serverDirectory) + throws Exception + { + // setup + writeServerProperties(serverDirectory, "online-mode=false", "network-compression-threshold=256"); + writeSpigotConfiguration(serverDirectory, "settings:", " bungeecord: false"); + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " enabled: false", + " online-mode: true" + ); + + // execute + verify + assertThatCode(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .doesNotThrowAnyException(); + } + + @Test + void validate_shouldPassWhenOptionalConfigurationFilesAreMissing(@TempDir Path serverDirectory) + { + // setup — an empty directory: nothing to validate must mean nothing to fail on. + + // execute + verify + assertThatCode(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .doesNotThrowAnyException(); + } + + @Test + void validate_shouldFailWhenOnlineModeIsEnabled(@TempDir Path serverDirectory) + throws Exception + { + // setup + writeServerProperties(serverDirectory, "server-port=25565", "online-mode=TRUE"); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("online-mode=true") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenSpigotBungeecordForwardingIsEnabled(@TempDir Path serverDirectory) + throws Exception + { + // setup + writeSpigotConfiguration(serverDirectory, "settings:", " bungeecord: true # enabled by overlay"); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("BungeeCord") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenPaperVelocityForwardingIsEnabled(@TempDir Path serverDirectory) + throws Exception + { + // setup + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " enabled: true", + " secret: hunter2" + ); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("Velocity") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenOnlineModeUsesSpacedAssignment(@TempDir Path serverDirectory) + throws Exception + { + // setup — legal properties spacing around the separator. + writeServerProperties(serverDirectory, "online-mode = true"); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("online-mode=true") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenBungeecordUsesCapitalizedYamlBoolean(@TempDir Path serverDirectory) + throws Exception + { + // setup — 'True' is a legal YAML boolean spelling. + writeSpigotConfiguration(serverDirectory, "settings:", " bungeecord: True"); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("BungeeCord") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenVelocityUsesUppercaseYamlBoolean(@TempDir Path serverDirectory) + throws Exception + { + // setup — 'TRUE' is a legal YAML boolean spelling. + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " enabled: TRUE" + ); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("Velocity") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldIgnoreEnabledFlagsOutsideTheVelocitySection(@TempDir Path serverDirectory) + throws Exception + { + // setup — 'enabled: true' belongs to a sibling section that starts after the velocity block ends. + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " enabled: false", + " bungee-cord:", + " enabled: true", + "watchdog:", + " enabled: true" + ); + + // execute + verify + assertThatCode(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .doesNotThrowAnyException(); + } + + @Test + void validate_shouldIgnoreCommentedOutForwardingSettings(@TempDir Path serverDirectory) + throws Exception + { + // setup + writeServerProperties( + serverDirectory, "# online-mode=true", "! online-mode=true", "online-mode=false"); + writeSpigotConfiguration(serverDirectory, "settings:", " # bungeecord: true"); + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " # enabled: true", + " enabled: false" + ); + + // execute + verify + assertThatCode(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .doesNotThrowAnyException(); + } + + private static void writeServerProperties(Path serverDirectory, String... lines) + throws IOException + { + Files.write(serverDirectory.resolve("server.properties"), List.of(lines)); + } + + private static void writeSpigotConfiguration(Path serverDirectory, String... lines) + throws IOException + { + Files.write(serverDirectory.resolve("spigot.yml"), List.of(lines)); + } + + private static void writePaperGlobalConfiguration(Path serverDirectory, String... lines) + throws IOException + { + final Path configDirectory = serverDirectory.resolve("config"); + Files.createDirectories(configDirectory); + Files.write(configDirectory.resolve("paper-global.yml"), List.of(lines)); + } +} diff --git a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java index 0226bf2a..867c4d5e 100644 --- a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java +++ b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java @@ -22,6 +22,7 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; import java.util.function.Function; /** @@ -40,6 +41,12 @@ final class BotLoginReflection { private static final System.Logger LOG = System.getLogger(BotLoginReflection.class.getName()); + /** + * Upper bound on the wait for a freshly connected channel to become active (see + * {@code awaitChannelActive}); loopback channels activate in well under a millisecond. + */ + private static final long CHANNEL_ACTIVE_TIMEOUT_SECONDS = 5L; + /** * How the session should react to a received clientbound packet, resolved from its class via * {@link #classify(Class)}. @@ -113,6 +120,7 @@ enum PacketKind private final Method setupCompressionMethod; private final Field channelField; private final Method channelCloseMethod; + private final Method channelIsOpenMethod; /** * Paper's per-connection inbound packet-rate counter ({@code Connection.allPacketCounts}), or {@code null} * on distributions without it (Spigot). Paper initializes it from config on EVERY {@code Connection} — @@ -264,6 +272,7 @@ enum PacketKind channelField = NmsReflectionUtils.resolveFieldByNameOrAcceptedType( connectionClass, "channel", channelClass); channelCloseMethod = channelClass.getMethod("close"); + channelIsOpenMethod = channelClass.getMethod("isOpen"); paperPacketLimiterField = resolvePaperPacketLimiterField(connectionClass); channelCloseFutureMethod = channelClass.getMethod("closeFuture"); futureListenerInterface = NmsReflectionUtils.resolveClass( @@ -374,6 +383,15 @@ Object openConnection(int port, Object listenerProxy) final Object connection = connectToServerMethod.invoke(null, address, eventLoopGroupHolder, null); try { + // connectToServer syncs on the Netty CONNECT future, but Connection.channel is only assigned in + // channelActive() on the event loop, so this thread can still observe isConnected() == false here. + // In that window initiateServerboundPlayConnection silently defers the whole protocol/listener + // setup into the connection's pending-actions queue — which nothing ever drains for this untucked + // client-side connection — while send() queues the hello separately. The hello then reaches the + // encoder without a LOGIN protocol or listener ("Sending unknown packet 'serverbound/minecraft: + // hello'") and the join stalls until the server's ReadTimeoutHandler closes it. Waiting for the + // channel to be live makes both calls below take their immediate, in-program-order paths. + awaitChannelActive(connection); disarmPaperPacketLimiter(connection); initiateLoginConnectionMethod.invoke(connection, "127.0.0.1", port, listenerProxy); } @@ -386,6 +404,42 @@ Object openConnection(int port, Object listenerProxy) return connection; } + /** + * Waits (bounded) until the connection's channel is assigned and open, i.e. vanilla's + * {@code isConnected()} contract holds for callers on this thread. + * + * @param connection + * The connection returned by {@code connectToServer}. + * @throws ReflectiveOperationException + * When the reflective channel read fails. + */ + private void awaitChannelActive(Object connection) + throws ReflectiveOperationException + { + final long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(CHANNEL_ACTIVE_TIMEOUT_SECONDS); + while (System.nanoTime() - deadlineNanos < 0) + { + // Non-volatile cross-thread read by design: vanilla's own isConnected() reads this same field + // cross-thread, and the reflective Field.get defeats any hoisting of the read out of the loop. + final Object channel = channelField.get(connection); + if (channel != null && Boolean.TRUE.equals(channelIsOpenMethod.invoke(channel))) + return; + try + { + Thread.sleep(1L); + } + catch (InterruptedException exception) + { + Thread.currentThread().interrupt(); + throw new IllegalStateException( + "Interrupted while waiting for the bot connection channel to become active.", exception); + } + } + throw new IllegalStateException( + "Bot connection channel did not become active within %d seconds." + .formatted(CHANNEL_ACTIVE_TIMEOUT_SECONDS)); + } + /** * Invokes {@code callback} as soon as the connection's channel closes — locally or remotely. * diff --git a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java index 1d0cf204..ab6b1f67 100644 --- a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java +++ b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java @@ -256,13 +256,15 @@ private void dispatchPacket(@Nullable Object packet) catch (ReflectiveOperationException | RuntimeException exception) { // Any servicing failure (reflective or runtime, e.g. a missing structural accessor) means the - // driver can no longer run this connection reliably: stop servicing, close the channel, and make - // sure the outcome can never be left pending. - terminate(); + // driver can no longer run this connection reliably: resolve the outcome so it can never be left + // pending, then stop servicing and close the channel. Completion MUST precede terminate(): + // closing the channel fires the close listener inline, whose generic transport-close denial + // would otherwise win the completion race and mask the real failure. final IllegalStateException failure = new IllegalStateException( "Failed to handle %s packet %s.".formatted(phase, packetClass.getName()), exception); if (!outcome.completeExceptionally(failure)) LOG.log(System.Logger.Level.WARNING, "Full-login packet servicing failed after join.", failure); + terminate(); } } @@ -275,18 +277,19 @@ private void handleUnknownPacket(Class packetClass) return; } - // Fail-loud: an unhandled login/configuration packet would silently stall the join. - terminate(); + // Fail-loud: an unhandled login/configuration packet would silently stall the join. Completion MUST + // precede terminate() so the close listener's generic denial cannot mask the named error. outcome.completeExceptionally(new IllegalStateException( "Unhandled %s packet from server: %s. Add it to the login driver's packet table." .formatted(phase, packetClass.getName()))); + terminate(); } private void handleTransportDisconnect() { - terminate(); if (!outcome.isDone()) outcome.complete(new IBotLoginOutcome.Denied(phase, "Connection closed during " + phase + ".")); + terminate(); } private void completeJoined() @@ -297,9 +300,13 @@ private void completeJoined() private void completeDenied(Object disconnectPacket) { - terminate(); + // Complete with the packet's kick reason BEFORE terminating: terminate() closes the channel, which + // fires the registered close listener inline (handleTransportDisconnect), and its generic + // "Connection closed" denial would otherwise win the one-shot completion race and discard the + // server's actual denial reason (whitelist/ban kick text). if (!outcome.isDone()) outcome.complete(new IBotLoginOutcome.Denied(phase, reflection.disconnectReason(disconnectPacket))); + terminate(); } private Object connection() diff --git a/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java b/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java index c49de887..bc4e43a3 100644 --- a/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java +++ b/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java @@ -32,6 +32,10 @@ public final class LightkeeperSpigotTestPlugin extends JavaPlugin implements Lis * Command name used to provoke log events for error-capture integration tests. */ private static final String ERROR_COMMAND_NAME = "lktesterror"; + /** + * Command name used to log the invoking player's client locale for full-login locale integration tests. + */ + private static final String LOCALE_COMMAND_NAME = "lktestlocale"; /** * Prefix used for deterministic block interaction messages. */ @@ -57,8 +61,14 @@ public void onEnable() throw new IllegalStateException( "Required command '/%s' is not declared in plugin metadata.".formatted(ERROR_COMMAND_NAME)); + final PluginCommand localeCommand = getCommand(LOCALE_COMMAND_NAME); + if (localeCommand == null) + throw new IllegalStateException( + "Required command '/%s' is not declared in plugin metadata.".formatted(LOCALE_COMMAND_NAME)); + pluginCommand.setExecutor(new LkTestGuiCommandExecutor(guiMenuService)); errorCommand.setExecutor(new LkTestErrorCommandExecutor(getLogger())); + localeCommand.setExecutor(new LkTestLocaleCommandExecutor(getLogger())); Bukkit.getPluginManager().registerEvents(this, this); } diff --git a/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java b/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java new file mode 100644 index 00000000..ec1eb09a --- /dev/null +++ b/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java @@ -0,0 +1,70 @@ +package nl.pim16aap2.lightkeeper.spigot.testplugin; + +import org.bukkit.command.Command; +import org.bukkit.command.CommandExecutor; +import org.bukkit.command.CommandSender; +import org.bukkit.entity.Player; + +import java.util.Objects; +import java.util.logging.Logger; + +/** + * Handles {@code /lktestlocale} command execution for the standalone test plugin. + * + *

Logs the invoking player's client locale (as the server sees it via {@code Player#getLocale()}) in a + * deterministic single-line format. Integration tests use this to assert that the locale a {@code FULL_LOGIN} + * bot sent during the configuration phase actually reached the server, by matching the line in the captured + * console output. + */ +final class LkTestLocaleCommandExecutor implements CommandExecutor +{ + /** + * Marker prefix of the logged locale line so tests can identify it in the console output. + */ + static final String LOCALE_MESSAGE_PREFIX = "LK_LOCALE"; + /** + * Reply sent when the command is not invoked by a player (only players have a client locale). + */ + static final String PLAYERS_ONLY_MESSAGE = "Only players can use /lktestlocale."; + + /** + * Logger the locale line is emitted through. + */ + private final Logger logger; + + /** + * @param logger + * Logger to emit the locale line through (the owning plugin's logger). + */ + LkTestLocaleCommandExecutor(Logger logger) + { + this.logger = Objects.requireNonNull(logger, "logger"); + } + + /** + * Logs the sender's client locale, or replies with a hint for non-player senders. + * + * @param sender + * Sender that invoked the command. + * @param command + * Command metadata. + * @param label + * Used command label. + * @param args + * Command arguments (ignored). + * @return + * Always {@code true} because this executor handles all command outcomes directly. + */ + @Override + public boolean onCommand(CommandSender sender, Command command, String label, String[] args) + { + if (!(sender instanceof Player player)) + { + sender.sendMessage(PLAYERS_ONLY_MESSAGE); + return true; + } + + logger.info("%s name=%s locale=%s".formatted(LOCALE_MESSAGE_PREFIX, player.getName(), player.getLocale())); + return true; + } +} diff --git a/lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml b/lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml index df3e7f1b..d79171b1 100644 --- a/lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml +++ b/lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml @@ -12,6 +12,9 @@ commands: lktesterror: description: Emits a SEVERE or WARNING log event for LightKeeper error-capture integration tests usage: /lktesterror + lktestlocale: + description: Logs the invoking player's client locale for LightKeeper full-login locale integration tests + usage: /lktestlocale permissions: lightkeeper.testplugin.lktestgui: description: Allows opening the LightKeeper test GUI. diff --git a/lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java b/lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java new file mode 100644 index 00000000..36288272 --- /dev/null +++ b/lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java @@ -0,0 +1,89 @@ +package nl.pim16aap2.lightkeeper.spigot.testplugin; + +import org.bukkit.command.Command; +import org.bukkit.command.CommandSender; +import org.bukkit.entity.Player; +import org.junit.jupiter.api.Test; + +import java.util.Objects; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +class LkTestLocaleCommandExecutorTest +{ + @Test + void onCommand_shouldLogPlayerNameAndLocaleForPlayerSender() + { + // setup + final AtomicReference captured = new AtomicReference<>(); + final Logger logger = loggerWithHandler(captured); + final LkTestLocaleCommandExecutor executor = new LkTestLocaleCommandExecutor(logger); + final Player player = mock(); + when(player.getName()).thenReturn("lklocale"); + when(player.getLocale()).thenReturn("de_de"); + final Command command = mock(); + + // execute + final boolean handled = executor.onCommand(player, command, "lktestlocale", new String[0]); + + // verify + assertThat(handled).isTrue(); + final LogRecord record = Objects.requireNonNull(captured.get(), "record"); + assertThat(record.getLevel()).isEqualTo(Level.INFO); + assertThat(record.getMessage()).isEqualTo("LK_LOCALE name=lklocale locale=de_de"); + } + + @Test + void onCommand_shouldSendPlayersOnlyMessageForNonPlayerSender() + { + // setup + final AtomicReference captured = new AtomicReference<>(); + final Logger logger = loggerWithHandler(captured); + final LkTestLocaleCommandExecutor executor = new LkTestLocaleCommandExecutor(logger); + final CommandSender sender = mock(); + final Command command = mock(); + + // execute + final boolean handled = executor.onCommand(sender, command, "lktestlocale", new String[0]); + + // verify + assertThat(handled).isTrue(); + verify(sender).sendMessage(LkTestLocaleCommandExecutor.PLAYERS_ONLY_MESSAGE); + assertThat(captured.get()).isNull(); + } + + private static Logger loggerWithHandler(AtomicReference captured) + { + final Logger logger = + Logger.getLogger(LkTestLocaleCommandExecutorTest.class.getName() + "." + captured.hashCode()); + logger.setUseParentHandlers(false); + logger.setLevel(Level.ALL); + logger.addHandler(new Handler() + { + @Override + public void publish(LogRecord record) + { + captured.set(record); + } + + @Override + public void flush() + { + } + + @Override + public void close() + { + } + }); + return logger; + } +}