From bd40931e2fb044d6795cac332c9ac072374ac744 Mon Sep 17 00:00:00 2001 From: "minekube-ai-engineer[bot]" Date: Wed, 21 Oct 2026 18:04:29 +0000 Subject: [PATCH] fix(config): warn when restore-full-profile cannot run login-reassert.restore-full-profile is only read from inside the late pre-login and game-profile handlers, and all of them return early while login-reassert.enabled is false. An operator who flips only the second key sees no change at all: no log line, no error, just the UUID they were trying to replace (endpoint servermc4321, 2026-10-21: offline UUID c54a0640-... through the tunnel vs Mojang 063f9b8a-... on a direct join). - ConfigLoader warns once at startup for enabled: false + restore-full-profile: true; it is the single config path on both proxy platforms and stays silent on Spigot. - proxy-config.yml and docs/login-plugin-integration.md now state that restore-full-profile only has an effect while enabled is true. - Velocity's game-profile re-assert logs Connect's UUID and the UUID the event carries at DEBUG when they differ, so that class of report is diagnosable from the endpoint log alone. No behaviour change: the floor, its ordering, and both defaults are untouched. --- .../minekube/connect/config/ConfigLoader.java | 28 +++++ core/src/main/resources/proxy-config.yml | 4 + .../connect/config/ConfigLoaderTest.java | 106 +++++++++++++++++- docs/login-plugin-integration.md | 4 + velocity/AGENTS.md | 6 + .../VelocityLateReassertListener.java | 8 ++ .../VelocityLateReassertListenerTest.java | 102 ++++++++++++++++- 7 files changed, 251 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/com/minekube/connect/config/ConfigLoader.java b/core/src/main/java/com/minekube/connect/config/ConfigLoader.java index c4bd72f6f..262ad0427 100644 --- a/core/src/main/java/com/minekube/connect/config/ConfigLoader.java +++ b/core/src/main/java/com/minekube/connect/config/ConfigLoader.java @@ -61,6 +61,33 @@ public final class ConfigLoader { private final ConnectLogger logger; + /** + * {@code login-reassert.restore-full-profile} is read only from inside the late pre-login and + * game-profile handlers, and every one of them returns early while + * {@code login-reassert.enabled} is off. An operator who flips only the second key therefore + * keeps the UUID they were trying to replace while every log line still looks healthy. + */ + static final String INERT_RESTORE_FULL_PROFILE_WARNING = + "login-reassert.restore-full-profile has no effect while login-reassert is disabled " + + "(login-reassert.enabled: false). Set login-reassert.enabled: true to restore" + + " the full game profile, or leave restore-full-profile: false if another" + + " plugin is meant to keep changing Connect's login decision."; + + /** + * One startup warning for the combination that looks configured but cannot act on a proxy. + * Spigot has no proxy config at all, so this stays silent there. + */ + private void warnAboutInertLoginReassert(ConnectConfig loaded) { + if (!(loaded instanceof ProxyConnectConfig)) { + return; + } + ProxyConnectConfig.LoginReassertConfig loginReassert = + ((ProxyConnectConfig) loaded).getLoginReassert(); + if (!loginReassert.isEnabled() && loginReassert.isRestoreFullProfile()) { + logger.warn(INERT_RESTORE_FULL_PROFILE_WARNING); + } + } + private static void fixPlaceholderIssue(Path path) throws IOException { Charset charset = StandardCharsets.UTF_8; String content = new String(Files.readAllBytes(path), charset) @@ -122,6 +149,7 @@ public T load() { if (useMinekubeBedrockIdentityDefaults) { loaded.getBedrockIdentity().useMinekubeDefaults(); } + warnAboutInertLoginReassert(loaded); return loaded; } catch (Throwable throwable) { throw new RuntimeException( diff --git a/core/src/main/resources/proxy-config.yml b/core/src/main/resources/proxy-config.yml index 6021c2b0e..533b27705 100644 --- a/core/src/main/resources/proxy-config.yml +++ b/core/src/main/resources/proxy-config.yml @@ -34,6 +34,10 @@ login-reassert: # Also restore Connect's full game profile - the player's Mojang UUID and username - and # not just the skin properties. # + # Only has an effect while `login-reassert.enabled` is true above: with `enabled: false` the + # re-assert never runs, so this option changes nothing. Connect logs a warning at startup when + # both are set that way. + # # PREREQUISITE, read before enabling: only turn this on if every login plugin on this proxy # keys its own database on the player's Mojang UUID. For LibreLogin that means setting # `new-uuid-creator: MOJANG` in LibreLogin's config first. With its default `CRACKED` it diff --git a/core/src/test/java/com/minekube/connect/config/ConfigLoaderTest.java b/core/src/test/java/com/minekube/connect/config/ConfigLoaderTest.java index e102b4de5..12c1a077d 100644 --- a/core/src/test/java/com/minekube/connect/config/ConfigLoaderTest.java +++ b/core/src/test/java/com/minekube/connect/config/ConfigLoaderTest.java @@ -4,6 +4,8 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockingDetails; +import static org.mockito.Mockito.verify; import com.minekube.connect.api.logger.ConnectLogger; import java.nio.file.Files; @@ -201,11 +203,7 @@ void loadsLoginReassertOverrides() throws Exception { */ @Test void shippedProxyTemplateDocumentsTheFullProfilePrerequisite() throws Exception { - String template; - try (java.io.InputStream in = - ConfigLoader.class.getResourceAsStream("/proxy-config.yml")) { - template = new String(in.readAllBytes(), java.nio.charset.StandardCharsets.UTF_8); - } + String template = shippedProxyTemplate(); int optionAt = template.indexOf("restore-full-profile:"); assertTrue(optionAt > 0, "proxy-config.yml must ship the restore-full-profile option"); @@ -214,11 +212,107 @@ void shippedProxyTemplateDocumentsTheFullProfilePrerequisite() throws Exception "the LibreLogin-side prerequisite must be documented next to the option"); } + /** + * Field report 2026-10-21 (endpoint {@code servermc4321}): the operator ran + * {@code login-reassert.enabled: false} together with {@code restore-full-profile: true} and + * expected Mojang UUIDs. The second key is read only from inside the late handlers, which all + * return early while the first is off, so it was silently inert - nothing in either log named + * the combination and the join was diagnosed on both sides for hours. The config loader is the + * single startup path on every proxy platform, so exactly one warning belongs here. + */ + @Test + void warnsWhenRestoreFullProfileCannotRunBecauseLoginReassertIsDisabled() throws Exception { + Files.writeString(tempDir.resolve("config.yml"), + proxyConfigWithLoginReassert(" enabled: false", " restore-full-profile: true")); + ConnectLogger logger = mock(ConnectLogger.class); + + load(ProxyConnectConfig.class, logger); + + assertTrue(ConfigLoader.INERT_RESTORE_FULL_PROFILE_WARNING.contains( + "restore-full-profile has no effect while login-reassert is disabled"), + "the warning must name the exact inert combination an operator has to undo"); + verify(logger).warn(ConfigLoader.INERT_RESTORE_FULL_PROFILE_WARNING); + } + + /** The shipped default (`enabled: true`, `restore-full-profile: false`) must stay quiet. */ + @Test + void doesNotWarnForTheShippedLoginReassertDefaults() throws Exception { + Files.writeString(tempDir.resolve("config.yml"), + proxyConfigWithLoginReassert(" enabled: true", " restore-full-profile: false")); + ConnectLogger logger = mock(ConnectLogger.class); + + load(ProxyConnectConfig.class, logger); + + assertNoWarnings(logger); + } + + /** + * Either key alone is a valid configuration: turning the floor off without asking for the + * full profile is the documented way to let another plugin keep changing the decision. + */ + @Test + void doesNotWarnWhenOnlyOneLoginReassertKeyIsSet() throws Exception { + Files.writeString(tempDir.resolve("config.yml"), + proxyConfigWithLoginReassert(" enabled: false", " restore-full-profile: false")); + ConnectLogger logger = mock(ConnectLogger.class); + + load(ProxyConnectConfig.class, logger); + + assertNoWarnings(logger); + } + + /** + * The template is where an operator reads the option before flipping it, and it used to + * present {@code restore-full-profile} as if it acted on its own. Pin that it states the + * dependency in the option's own comment, not only somewhere else in the file. + */ + @Test + void shippedProxyTemplateMarksRestoreFullProfileAsConditionalOnEnabled() throws Exception { + String documentation = shippedProxyTemplate(); + + int optionAt = documentation.indexOf("restore-full-profile:"); + assertTrue(optionAt > 0, "proxy-config.yml must ship the restore-full-profile option"); + assertTrue(documentation.substring(0, optionAt) + .contains("Only has an effect while `login-reassert.enabled` is true"), + "the option's comment must say it is inert while login-reassert is disabled"); + } + + private static String proxyConfigWithLoginReassert(String enabledLine, String restoreLine) { + return String.join("\n", + "endpoint: codexp2p3", + "allow-offline-mode-players: false", + "login-reassert:", + enabledLine, + restoreLine, + "metrics:", + " disabled: true", + " uuid: 00000000-0000-0000-0000-000000000000", + "config-version: 1", + ""); + } + + private static String shippedProxyTemplate() throws Exception { + try (java.io.InputStream in = + ConfigLoader.class.getResourceAsStream("/proxy-config.yml")) { + return new String(in.readAllBytes(), java.nio.charset.StandardCharsets.UTF_8); + } + } + + private static void assertNoWarnings(ConnectLogger logger) { + boolean warned = mockingDetails(logger).getInvocations().stream() + .anyMatch(invocation -> invocation.getMethod().getName().equals("warn")); + assertFalse(warned, "a valid login-reassert combination must not warn"); + } + private T load(Class configClass) { + return load(configClass, mock(ConnectLogger.class)); + } + + private T load(Class configClass, ConnectLogger logger) { return new ConfigLoader( tempDir, configClass, new ConfigLoader.EndpointNameGenerator(new OkHttpClient()), - mock(ConnectLogger.class)).load(); + logger).load(); } } diff --git a/docs/login-plugin-integration.md b/docs/login-plugin-integration.md index 6e9b8ad90..fcbeabdca 100644 --- a/docs/login-plugin-integration.md +++ b/docs/login-plugin-integration.md @@ -154,6 +154,10 @@ able to change Connect's login decision. Off restores exactly the previous behav restores the player's **Mojang UUID and username**, which is what makes `ConnectApi.isConnectPlayer(uuid)` and `getPlayer(uuid)` resolve for these players. +> **It only has an effect while `enabled` is `true`.** With `login-reassert.enabled: false` the +> re-assert never runs, so `restore-full-profile` changes nothing — players keep the UUID the +> login plugin set. Connect logs one startup warning when both are set that way. + If late-handler registration fails, Connect logs the failure, skips the re-assert floor, and continues with its pre-existing handlers. Ordinary listener registration is unaffected. diff --git a/velocity/AGENTS.md b/velocity/AGENTS.md index d2e9bc7dd..a679e8876 100644 --- a/velocity/AGENTS.md +++ b/velocity/AGENTS.md @@ -23,6 +23,12 @@ The root `AGENTS.md` still applies; this file adds Velocity-specific guidance. - Default profile scope is properties-only (skin) deliberately; restoring Connect's UUID breaks login plugins that key their storage on the proxy UUID, so it is opt-in with its `new-uuid-creator: MOJANG` prerequisite documented next to the option. +- `restore-full-profile` is inert while `login-reassert.enabled` is false, because every late + handler returns before it reads the option. Keep that combination operator-visible: one startup + WARN from `ConfigLoader` (pinned by `ConfigLoaderTest`), the dependency stated in the + `proxy-config.yml` comment, and the Velocity game-profile re-assert logging Connect's UUID vs + the event's UUID at DEBUG so a "right player, wrong UUID - only through the tunnel" report is + diagnosable from the endpoint log alone. - `:velocity:eventOrderTest` is a separate source set running the **real** `VelocityEventManager` and `PluginDependencyUtils` against a Velocity proxy jar pinned by sha256 (ivy repo in `settings.gradle.kts`; PaperMC publishes no proxy artifact to Maven). Separate because that diff --git a/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java index 1deaccf62..5fa795b22 100644 --- a/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java +++ b/velocity/src/main/java/com/minekube/connect/listener/VelocityLateReassertListener.java @@ -176,6 +176,14 @@ void onGameProfileRequestLate(GameProfileRequestEvent event) { } GameProfile current = event.getGameProfile(); GameProfile connectProfile = VelocityGameProfiles.fromConnectPlayer(current, player); + if (!current.getId().equals(connectProfile.getId())) { + // "Same player, wrong UUID - but only through the tunnel" is only diagnosable if the + // endpoint log names both UUIDs; comparing the two sides out of band took hours. + logger.debug("Connect supplied profile UUID {} but the game profile event carries {}" + + " (login-reassert.restore-full-profile: {})", + connectProfile.getId(), current.getId(), + config.getLoginReassert().isRestoreFullProfile()); + } GameProfile wanted = config.getLoginReassert().isRestoreFullProfile() ? connectProfile : current.withProperties(connectProfile.getProperties()); diff --git a/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java b/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java index 7a71ac5c5..57b8f2359 100644 --- a/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java +++ b/velocity/src/test/java/com/minekube/connect/listener/VelocityLateReassertListenerTest.java @@ -24,6 +24,7 @@ import com.velocitypowered.api.util.GameProfile.Property; import java.lang.reflect.Field; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.UUID; @@ -177,6 +178,45 @@ void gameProfileIgnoresConnectionsConnectDidNotAuthenticate() throws Exception { assertSame(before, event.getGameProfile()); } + /** + * The {@code servermc4321} report was "right player, wrong UUID - but only through the + * tunnel": the endpoint log showed a join and never named either UUID, so the two sides had + * to be compared out of band. DEBUG now carries both, which is what makes that class of + * report diagnosable from the proxy's own log. + */ + @Test + void gameProfileLogsConnectAndEventUuidsAtDebugWhenALoginPluginLeftItsOwn() throws Exception { + RecordingLogger logger = new RecordingLogger(); + Fixture fixture = Fixture.connectSession(true, false, logger); + GameProfileRequestEvent event = fixture.gameProfileRequest(); + + fixture.listener.onGameProfileRequestLate(event); + + assertTrue(logger.debugMessages.stream().anyMatch(message -> + message.contains(CONNECT_UUID.toString()) + && message.contains(PROXY_UUID.toString())), + "DEBUG must name Connect's UUID and the UUID the game profile event carried: " + + logger.debugMessages); + } + + /** No mismatch, no line: the normal case with no conflicting plugin installed stays quiet. */ + @Test + void gameProfileDoesNotLogUuidsWhenTheProfileAlreadyMatches() throws Exception { + RecordingLogger logger = new RecordingLogger(); + Fixture fixture = Fixture.connectSession(true, false, logger); + GameProfileRequestEvent event = fixture.gameProfileRequest(); + // What VelocityListener#onGameProfileRequest already did at PostOrder.EARLY. + event.setGameProfile( + VelocityGameProfiles.fromConnectPlayer(event.getGameProfile(), connectPlayer())); + + fixture.listener.onGameProfileRequestLate(event); + + assertFalse(logger.debugMessages.stream().anyMatch(message -> + message.contains(CONNECT_UUID.toString()) + && message.contains(PROXY_UUID.toString())), + "an unchanged profile needs no UUID diagnostic: " + logger.debugMessages); + } + @Test void partialRegistrationRollsBackOnlyTheHandlerThatWasRegistered() { PartialFailureEventManager eventManager = new PartialFailureEventManager(); @@ -204,18 +244,28 @@ private static final class Fixture { private Fixture(boolean enabled, boolean restoreFullProfile, boolean connectSession) throws Exception { + this(enabled, restoreFullProfile, connectSession, mock(ConnectLogger.class)); + } + + private Fixture(boolean enabled, boolean restoreFullProfile, boolean connectSession, + ConnectLogger logger) throws Exception { VelocityConnectPlayers players = new VelocityConnectPlayers(); if (connectSession) { players.remember(connection, connectPlayer()); } listener = new VelocityLateReassertListener( - players, config(enabled, restoreFullProfile), mock(ConnectLogger.class)); + players, config(enabled, restoreFullProfile), logger); } static Fixture connectSession(boolean enabled, boolean restoreFullProfile) throws Exception { return new Fixture(enabled, restoreFullProfile, true); } + static Fixture connectSession(boolean enabled, boolean restoreFullProfile, + ConnectLogger logger) throws Exception { + return new Fixture(enabled, restoreFullProfile, true, logger); + } + static Fixture foreignSession(boolean enabled, boolean restoreFullProfile) throws Exception { return new Fixture(enabled, restoreFullProfile, false); } @@ -304,4 +354,54 @@ public void unregister(Object plugin, EventHandler handler) { unregisteredHandlers.add(handler); } } + + /** + * Captures DEBUG lines as {@code message + args}, so a test can assert on the values a log + * line actually carries instead of on Mockito's varargs matching. + */ + private static final class RecordingLogger implements ConnectLogger { + final List debugMessages = new ArrayList<>(); + + @Override + public void error(String message, Object... args) { + } + + @Override + public void error(String message, Throwable throwable, Object... args) { + } + + @Override + public void warn(String message, Object... args) { + } + + @Override + public void info(String message, Object... args) { + } + + @Override + public void translatedInfo(String message, Object... args) { + } + + @Override + public void debug(String message, Object... args) { + debugMessages.add(message + " " + Arrays.toString(args)); + } + + @Override + public void trace(String message, Object... args) { + } + + @Override + public void enableDebug() { + } + + @Override + public void disableDebug() { + } + + @Override + public boolean isDebug() { + return true; + } + } }